| Message ID | 20160921014735.25191-6-joe@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
2016-09-20 18:47 GMT-07:00 Joe Stringer <joe@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 <joe@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 >
On 28 September 2016 at 14:22, Daniele Di Proietto <diproiettod@ovn.org> wrote: > > > 2016-09-20 18:47 GMT-07:00 Joe Stringer <joe@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 <joe@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. Good catches, yes I missed these. Thanks!
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. */ - ukey->stats = *stats; if (!push.n_packets && !need_revalidate) { result = UKEY_KEEP; goto exit; } - 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;
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 <joe@ovn.org> --- ofproto/ofproto-dpif-upcall.c | 56 ++++++++++++++++++++++++++++++++----------- 1 file changed, 42 insertions(+), 14 deletions(-)