[ovs-dev] [PATCHv2 4/4] upcall: Replace ukeys for deleted flows.

Jarno Rajahalme jarno at ovn.org
Wed Aug 31 20:18:10 UTC 2016


With a minor question below,

Acked-by: Jarno Rajahalme <jarno at ovn.org>

> On Aug 31, 2016, at 11:06 AM, Joe Stringer <joe at 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 at 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
> 




More information about the dev mailing list