| Message ID | 20160831180605.20969-5-joe@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
With a minor question below, Acked-by: Jarno Rajahalme <jarno@ovn.org> > On Aug 31, 2016, at 11:06 AM, Joe Stringer <joe@ovn.org> 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. > > Signed-off-by: Joe Stringer <joe@ovn.org> > --- > v2: Rebase against ukey lifecycle patch. > --- > ofproto/ofproto-dpif-upcall.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index ce5a392caf78..04873cc42fff 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, > @@ -1568,6 +1569,36 @@ 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->state == UKEY_EVICTED) { > + COVERAGE_INC(upcall_ukey_replace); > + > + /* The flow was deleted during the current revalidator dump, > + * but its ukey won't be fully cleaned up until the sweep phase. > + * In the mean time, we are receiving upcalls for this traffic. > + * Expedite the (new) flow install by replacing the ukey. */ > + 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); > + transition_ukey(old_ukey, UKEY_DELETED); > + transition_ukey(new_ukey, UKEY_VISIBLE); > + 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 > @@ -1590,6 +1621,7 @@ ukey_install__(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); Should we do the coverage only if the try_ukey_replace() returns false? > } else { > struct ds ds = DS_EMPTY_INITIALIZER; > > -- > 2.9.3 >
On 31 August 2016 at 13:18, Jarno Rajahalme <jarno@ovn.org> wrote: > With a minor question below, > > Acked-by: Jarno Rajahalme <jarno@ovn.org> > >> On Aug 31, 2016, at 11:06 AM, Joe Stringer <joe@ovn.org> 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. >> >> Signed-off-by: Joe Stringer <joe@ovn.org> >> --- >> v2: Rebase against ukey lifecycle patch. >> --- >> ofproto/ofproto-dpif-upcall.c | 32 ++++++++++++++++++++++++++++++++ >> 1 file changed, 32 insertions(+) >> >> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c >> index ce5a392caf78..04873cc42fff 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, >> @@ -1568,6 +1569,36 @@ 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->state == UKEY_EVICTED) { >> + COVERAGE_INC(upcall_ukey_replace); >> + >> + /* The flow was deleted during the current revalidator dump, >> + * but its ukey won't be fully cleaned up until the sweep phase. >> + * In the mean time, we are receiving upcalls for this traffic. >> + * Expedite the (new) flow install by replacing the ukey. */ >> + 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); >> + transition_ukey(old_ukey, UKEY_DELETED); >> + transition_ukey(new_ukey, UKEY_VISIBLE); >> + 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 >> @@ -1590,6 +1621,7 @@ ukey_install__(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); > > Should we do the coverage only if the try_ukey_replace() returns false? Can do, might be a smidgen clearer.
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index ce5a392caf78..04873cc42fff 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, @@ -1568,6 +1569,36 @@ 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->state == UKEY_EVICTED) { + COVERAGE_INC(upcall_ukey_replace); + + /* The flow was deleted during the current revalidator dump, + * but its ukey won't be fully cleaned up until the sweep phase. + * In the mean time, we are receiving upcalls for this traffic. + * Expedite the (new) flow install by replacing the ukey. */ + 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); + transition_ukey(old_ukey, UKEY_DELETED); + transition_ukey(new_ukey, UKEY_VISIBLE); + 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 @@ -1590,6 +1621,7 @@ ukey_install__(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;
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. Signed-off-by: Joe Stringer <joe@ovn.org> --- v2: Rebase against ukey lifecycle patch. --- ofproto/ofproto-dpif-upcall.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)