2016-09-20 18:47 GMT-07:00 Joe Stringer <j...@ovn.org>:
> To make more of the core revalidate() functions do just one thing and
> not modify state on the way, refactor them to prepare the xcache then
> defer the ukey modification and stats/side effects execution to the end
> of successful revalidation.
>
> If revalidation causes deletion, then the xcache will be prepared and
> attached to the ukey, but the actual execution will be skipped since it
> will be executed on flow_delete very soon anyway with final stats.
>
> Signed-off-by: Joe Stringer <j...@ovn.org>
> ---
> ofproto/ofproto-dpif-upcall.c | 56 ++++++++++++++++++++++++++++++
> ++-----------
> 1 file changed, 42 insertions(+), 14 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 1caff84cfd38..eaf7c3b4dadc 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1877,16 +1877,41 @@ xlate_key(struct udpif *udpif, const struct nlattr
> *key, unsigned int len,
>
> static int
> xlate_ukey(struct udpif *udpif, const struct udpif_key *ukey,
> - const struct dpif_flow_stats *push, struct reval_context *ctx)
> + uint16_t tcp_flags, struct reval_context *ctx)
> {
> - return xlate_key(udpif, ukey->key, ukey->key_len, push, ctx);
> + struct dpif_flow_stats push = {
> + .tcp_flags = tcp_flags,
> + };
>
+ return xlate_key(udpif, ukey->key, ukey->key_len, &push, ctx);
> +}
> +
> +static int
> +populate_xcache(struct udpif *udpif, struct udpif_key *ukey,
> + uint16_t tcp_flags)
> + OVS_REQUIRES(ukey->mutex)
> +{
> + struct reval_context ctx = {
> + .odp_actions = NULL,
> + .netflow = NULL,
> + .wc = NULL,
> + };
> + int error;
> +
> + ovs_assert(!ukey->xcache);
> + ukey->xcache = ctx.xcache = xlate_cache_new();
> + error = xlate_ukey(udpif, ukey, tcp_flags, &ctx);
> + if (error) {
> + return error;
> + }
> + xlate_out_uninit(&ctx.xout);
> +
> + return 0;
> }
>
> static enum reval_result
> revalidate_ukey__(struct udpif *udpif, struct udpif_key *ukey,
> - const struct dpif_flow_stats *push,
> - struct ofpbuf *odp_actions, uint64_t reval_seq,
> - struct recirc_refs *recircs)
> + uint16_t tcp_flags, struct ofpbuf *odp_actions,
> + uint64_t reval_seq, struct recirc_refs *recircs)
> OVS_REQUIRES(ukey->mutex)
> {
> bool need_revalidate = (ukey->reval_seq != reval_seq);
> @@ -1911,7 +1936,7 @@ revalidate_ukey__(struct udpif *udpif, struct
> udpif_key *ukey,
> ukey->xcache = xlate_cache_new();
> }
> ctx.xcache = ukey->xcache;
> - if (xlate_ukey(udpif, ukey, push, &ctx)) {
> + if (xlate_ukey(udpif, ukey, tcp_flags, &ctx)) {
> goto exit;
> }
> xoutp = &ctx.xout;
> @@ -2008,23 +2033,26 @@ revalidate_ukey(struct udpif *udpif, struct
> udpif_key *ukey,
> }
>
> /* We will push the stats, so update the ukey stats cache. */
>
I guess we should remove the comment too?
> - ukey->stats = *stats;
>
if (!push.n_packets && !need_revalidate) {
> result = UKEY_KEEP;
> goto exit;
> }
>
I think the 4 lines above can be removed, because the condition is covered
by the lines immediately below.
>
> - if (ukey->xcache && !need_revalidate) {
> - xlate_push_stats(ukey->xcache, &push);
> - result = UKEY_KEEP;
> - goto exit;
> + if (!need_revalidate) {
> + if (!push.n_packets || ukey->xcache
> + || !populate_xcache(udpif, ukey, push.tcp_flags)) {
> + result = UKEY_KEEP;
> + goto exit;
> + }
> }
>
> - result = revalidate_ukey__(udpif, ukey, &push, odp_actions, reval_seq,
> - recircs);
> -
> + result = revalidate_ukey__(udpif, ukey, push.tcp_flags, odp_actions,
> + reval_seq, recircs);
> exit:
> + /* Stats for deleted flows will be attributed upon flow deletion.
> Skip. */
> if (result != UKEY_DELETE) {
> + xlate_push_stats(ukey->xcache, &push);
> + ukey->stats = *stats;
> ukey->reval_seq = reval_seq;
> }
> return result;
> --
> 2.9.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
