On Tue, Aug 23, 2016 at 07:18:50PM -0700, Joe Stringer wrote: > If a revalidator dumps/revalidates a flow during the 'dump' phase, > resulting in the deletion of the flow, then ukey->flow_exists is set to > false, and the ukey is kept around until the 'sweep' phase. The ukey is > kept around to ensure that cases like duplicated dumps from the > datapaths do not result in multiple attribution of the same stats. > > However, if an upcall for this flow comes for a handler between the > revalidator 'dump' and 'sweep' phases, the handler will lookup the ukey > and find that the ukey exists, then skip installing a new flow entirely. > As a result, for this period all traffic for the flow is slowpathed. > If there is a lot of traffic hitting this flow, then it will all be > handled in userspace until the 'sweep' phase. Eventually the > revalidators will reach the sweep phase and delete the ukey, and > subsequently the handlers should install a new flow. > > To reduce the slowpathing of this traffic during flow table transitions, > allow the handler to identify this case during miss upcall handling and > replace the existing ukey with a new ukey. The handler will then be able > to install a flow for this traffic, allowing the traffic flow to return > to the fastpath. > > There are three situations where the existing ukey has "flow_exists" set > to false: > > * Firstly, the flow for the ukey might not yet have been installed. In > this case, the other handler that set up this ukey will be holding the > lock for the ukey. So, if we attempt to grab the lock and fail then > another handler is already setting up the flow and we can safely skip > flow install. > * Secondly, a revalidator could be currently deleting the flow. In this > case, the revalidator holds the ukey lock so the handler will fail to > grab it. This is fine, if traffic continues to miss then a subsequent > miss upcall will hit the third case. > * Thirdly, the flow may have been recently deleted by a revalidator > thread. In this case, we can grab the lock. From the handler thread > we swap the original key out for a new one and rcu-defer its deletion. >
Sweeper code gets the ukey lock, looks up flow_exists, then releases the lock. Now try_ukey_replace succeeds at getting the lock. Sweeper is waiting on the umap mutex, as ukey_install_start holds it. Now both will free the old ukey. Is that analysis correct? Thanks. Cascardo. > Signed-off-by: Joe Stringer <j...@ovn.org> > --- > ofproto/ofproto-dpif-upcall.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index 042a50a9f179..5a0ecc2a6fe6 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -50,6 +50,7 @@ COVERAGE_DEFINE(dumped_duplicate_flow); > COVERAGE_DEFINE(dumped_new_flow); > COVERAGE_DEFINE(handler_duplicate_upcall); > COVERAGE_DEFINE(upcall_ukey_contention); > +COVERAGE_DEFINE(upcall_ukey_replace); > COVERAGE_DEFINE(revalidate_missed_dp_flow); > > /* A thread that reads upcalls from dpif, forwards each upcall's packet, > @@ -1569,6 +1570,33 @@ ukey_create_from_dpif_flow(const struct udpif *udpif, > return 0; > } > > +static bool > +try_ukey_replace(struct umap *umap, struct udpif_key *old_ukey, > + struct udpif_key *new_ukey) > + OVS_REQUIRES(umap->mutex) > + OVS_TRY_LOCK(true, new_ukey->mutex) > +{ > + bool replaced = false; > + > + if (!ovs_mutex_trylock(&old_ukey->mutex)) { > + if (!old_ukey->flow_exists) { > + /* The flow was deleted during the current revalidator dump, > + * but its ukey won't be cleaned up until the sweep phase. > + * In the mean time, we are receiving upcalls for this traffic. > + * Expedite the flow install by replacing the ukey. */ > + COVERAGE_INC(upcall_ukey_replace); > + ovs_mutex_lock(&new_ukey->mutex); > + cmap_replace(&umap->cmap, &old_ukey->cmap_node, > + &new_ukey->cmap_node, new_ukey->hash); > + ovsrcu_postpone(ukey_delete__, old_ukey); > + replaced = true; > + } > + ovs_mutex_unlock(&old_ukey->mutex); > + } > + > + return replaced; > +} > + > /* Attempts to insert a ukey into the shared ukey maps. > * > * On success, returns true, installs the ukey and returns it in a locked > @@ -1591,6 +1619,7 @@ ukey_install_start(struct udpif *udpif, struct > udpif_key *new_ukey) > if (old_ukey->key_len == new_ukey->key_len > && !memcmp(old_ukey->key, new_ukey->key, new_ukey->key_len)) { > COVERAGE_INC(handler_duplicate_upcall); > + locked = try_ukey_replace(umap, old_ukey, new_ukey); > } else { > struct ds ds = DS_EMPTY_INITIALIZER; > > -- > 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