With a minor question below,
Acked-by: Jarno Rajahalme <ja...@ovn.org>
> On Aug 31, 2016, at 11:06 AM, Joe Stringer <j...@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 <j...@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
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
