[ovs-dev] [PATCH] ofproto-dpif-upcall: Replace ukeys for deleted flows.

[Thadeu Lima de Souza Cascardo]

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 <joe at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev