Message ID | CAPWQB7GwdaBerP4w73bKdwFwjBRfabPu=5821F_U+0uw_o2Yqg@mail.gmail.com |
---|---|
State | Not Applicable |
Headers | show |
Joe, I just noticed I never sent this out. Sorry, and thanks for the review! Jarno > On Feb 3, 2016, at 2:19 PM, Joe Stringer <joe@ovn.org> wrote: > > On 3 February 2016 at 12:33, Jarno Rajahalme <jarno@ovn.org> wrote: >> From: Ethan Jackson <ethan@nicira.com> >> >> There are certain use cases (such as bond rebalancing) where a >> datapath flow's actions may change, while it's wildcard pattern >> remains the same. Before this patch, revalidators would note the >> change, delete the flow, and wait for the handlers to install an >> updated version. This is inefficient, as many packets could get >> punted to userspace before the new flow is finally installed. >> >> To improve the situation, this patch implements in place modification >> of datapath flows. If the revalidators detect the only change to a >> given ukey is its actions, instead of deleting it, it does a put with >> the MODIFY flag set. >> >> This patch is a backport of commit 43b2f13 to branch-2.3. >> >> Signed-off-by: Ethan J. Jackson <ethan@nicira.com> >> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > > This also squashes in commit c56eba3b7ab0 ("ofproto-dpif-upcall: Don't > delete modified ukeys.”) Thanks, I added this note to the commit message. > . > > <snip> > >> @@ -1543,62 +1611,68 @@ revalidate(struct revalidator *revalidator) >> } >> >> dpif_flow_dump_state_uninit(udpif->dpif, state); >> -} >> - >> -/* Called with exclusive access to 'revalidator' and 'ukey'. */ >> -static bool >> -handle_missed_revalidation(struct revalidator *revalidator, >> - struct udpif_key *ukey) >> - OVS_NO_THREAD_SAFETY_ANALYSIS >> -{ >> - struct udpif *udpif = revalidator->udpif; >> - struct nlattr *mask, *actions; >> - size_t mask_len, actions_len; >> - struct dpif_flow_stats stats; >> - struct ofpbuf *buf; >> - bool keep = false; >> - >> - COVERAGE_INC(revalidate_missed_dp_flow); >> - >> - if (!dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len, &buf, >> - &mask, &mask_len, &actions, &actions_len, &stats)) { >> - keep = revalidate_ukey(udpif, ukey, mask, mask_len, actions, >> - actions_len, &stats); >> - ofpbuf_delete(buf); >> - } >> - >> - return keep; >> + ofpbuf_uninit(&odp_actions); >> } >> >> static void >> revalidator_sweep__(struct revalidator *revalidator, bool purge) >> OVS_NO_THREAD_SAFETY_ANALYSIS >> { >> + uint64_t odp_actions_stub[1024 / 8]; >> + struct ofpbuf odp_actions; >> struct dump_op ops[REVALIDATE_MAX_BATCH]; >> struct udpif_key *ukey, *next; >> size_t n_ops; >> >> n_ops = 0; >> >> + ofpbuf_use_stub(&odp_actions, odp_actions_stub, sizeof odp_actions_stub); >> + >> /* During garbage collection, this revalidator completely owns its ukeys >> * map, and therefore doesn't need to do any locking. */ >> HMAP_FOR_EACH_SAFE (ukey, next, hmap_node, revalidator->ukeys) { >> if (ukey->flow_exists) { >> bool missed_flow = !ukey->mark; >> + enum reval_result result; >> + struct nlattr *mask; >> + size_t mask_len; >> + struct ofpbuf *buf = NULL; >> >> ukey->mark = false; >> - if (purge >> - || (missed_flow >> - && revalidator->udpif->need_revalidate >> - && !handle_missed_revalidation(revalidator, ukey))) { >> - struct dump_op *op = &ops[n_ops++]; >> - >> - dump_op_init(op, ukey->key, ukey->key_len, ukey); >> - if (n_ops == REVALIDATE_MAX_BATCH) { >> - push_dump_ops(revalidator, ops, n_ops); >> - n_ops = 0; >> + result = purge ? UKEY_DELETE : UKEY_KEEP; >> + >> + if (missed_flow && revalidator->udpif->need_revalidate) { >> + struct udpif *udpif = revalidator->udpif; >> + struct nlattr *actions; >> + size_t actions_len; >> + struct dpif_flow_stats stats; >> + >> + COVERAGE_INC(revalidate_missed_dp_flow); >> + >> + if (!dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len, &buf, >> + &mask, &mask_len, &actions, &actions_len, >> + &stats)) { >> + result = revalidate_ukey(udpif, ukey, mask, mask_len, >> + actions, actions_len, &stats, >> + &odp_actions); > > > handle_missed_revalidation() would previously return the equivalent of > UKEY_DELETE if the dpif_flow_get() fails. This would lead on to > attempting flow deletion, which I'd expect is most likely also is > going to fail, but it would also result in the ukey getting deleted > (which we want). I suspect that with the current version above, if one > runs 'ovs-dpctl del-flow...' then the corresponding ukeys will never > be deleted. > > Simplest (same as current) behaviour is probably this incremental: > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index 03b4afa7d2c5..ba0cd2f9b2d3 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -1649,9 +1649,11 @@ revalidator_sweep__(struct revalidator > *revalidator, bool purge) > > COVERAGE_INC(revalidate_missed_dp_flow); > > - if (!dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len, &buf, > - &mask, &mask_len, &actions, &actions_len, > - &stats)) { > + if (dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len, &buf, > + &mask, &mask_len, &actions, &actions_len, > + &stats)) { > + result = UKEY_DELETE; > + } else { > result = revalidate_ukey(udpif, ukey, mask, mask_len, > actions, actions_len, &stats, > &odp_actions); > > Thanks, folded in for v2. > Separately from this patch, I noticed a couple of other things: > > - I couldn't compile branch-2.3 without at least this patch for > lib/type-props.h: > 0f395fd366413208aac7072ef81b5aeda6a9e307 > (which depends on c002791a30818c2458599f993d1711e03566e7cc) I cherry-picked these two for the v2. > - I wonder if commit e83c93573b10 ("ofproto-dpif-upcall: Do not > attribute stats when flow_del returns error.") is also a candidate for > backport, but that's more of a statistics issue rather than > reordering/correctness. Done.
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 03b4afa7d2c5..ba0cd2f9b2d3 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -1649,9 +1649,11 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge) COVERAGE_INC(revalidate_missed_dp_flow); - if (!dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len, &buf, - &mask, &mask_len, &actions, &actions_len, - &stats)) { + if (dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len, &buf, + &mask, &mask_len, &actions, &actions_len, + &stats)) { + result = UKEY_DELETE; + } else { result = revalidate_ukey(udpif, ukey, mask, mask_len, actions, actions_len, &stats,