[ovs-dev] [branch-2.3] revalidator: Eliminate duplicate flow handling.

Joe Stringer joestringer at nicira.com
Wed May 28 02:13:01 UTC 2014


Thanks for the Ack, I pushed this to branch-2.3.

I'd like to send a patch for master anyway, in part so that we ensure to
address this (fix the bug, and make the code more readable), and in part
because it still matters if newer userspace is used with an older kernel.

On 28 May 2014 13:52, Alex Wang <alexw at nicira.com> wrote:

> Acked-by: Alex Wang <alexw at nicira.com>
>
>
> For master, I'm about to refactor the datapath so that we never dump
> duplicated flows.  So, I think we should consider refactoring master after
> that.
>
>
> Thanks,
> Alex Wang,
>
>
> On Tue, May 27, 2014 at 6:19 PM, Joe Stringer <joestringer at nicira.com>wrote:
>
>> A series of bugs have been identified recently that are caused by a
>> combination of the awkward flow dump API, possibility of duplicate flows
>> in a flow dump, and premature optimisation of the revalidator logic.
>> This patch attempts to simplify the revalidator logic by combining
>> multiple critical sections into one, which should make the state more
>> consistent.
>>
>> The new flow of logic is:
>> + Lookup the ukey.
>> + If the ukey doesn't exist, create it.
>> + Insert the ukey into the udpif. If we can't insert it, skip this flow.
>> + Lock the ukey. If we can't lock it, skip it.
>> + Determine if the ukey was already handled. If it has, skip it.
>> + Revalidate.
>> + Update ukey's fields (mark, flow_exists).
>> + Unlock the ukey.
>>
>> Previously, we would attempt process a flow without creating a ukey if
>> it hadn't been dumped before and it was due to be deleted. This patch
>> changes this to always create a ukey, allowing the ukey's
>> mutex to be used as the basis for preventing a flow from being handled
>> twice. This is expected to cause some minor performance regression for
>> cases like TCP_CRR, in favour of correctness and readability.
>>
>> Bug #1252997.
>>
>> Signed-off-by: Joe Stringer <joestringer at nicira.com>
>> ---
>>  ofproto/ofproto-dpif-upcall.c |   64
>> ++++++++++++++++++-----------------------
>>  1 file changed, 28 insertions(+), 36 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index 64444d9..906ccb4 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -1205,6 +1205,7 @@ revalidate_ukey(struct udpif *udpif, struct
>> udpif_key *ukey,
>>                  const struct nlattr *mask, size_t mask_len,
>>                  const struct nlattr *actions, size_t actions_len,
>>                  const struct dpif_flow_stats *stats)
>> +    OVS_REQUIRES(ukey->mutex)
>>  {
>>      uint64_t slow_path_buf[128 / 8];
>>      struct xlate_out xout, *xoutp;
>> @@ -1225,7 +1226,6 @@ revalidate_ukey(struct udpif *udpif, struct
>> udpif_key *ukey,
>>      xoutp = NULL;
>>      netflow = NULL;
>>
>> -    ovs_mutex_lock(&ukey->mutex);
>>      last_used = ukey->stats.used;
>>      push.used = stats->used;
>>      push.tcp_flags = stats->tcp_flags;
>> @@ -1236,11 +1236,6 @@ revalidate_ukey(struct udpif *udpif, struct
>> udpif_key *ukey,
>>          ? stats->n_bytes - ukey->stats.n_bytes
>>          : 0;
>>
>> -    if (!ukey->flow_exists) {
>> -        /* Don't bother revalidating if the flow was already deleted. */
>> -        goto exit;
>> -    }
>> -
>>      if (udpif->need_revalidate && last_used
>>          && !should_revalidate(push.n_packets, last_used)) {
>>          ok = false;
>> @@ -1320,7 +1315,6 @@ revalidate_ukey(struct udpif *udpif, struct
>> udpif_key *ukey,
>>      ok = true;
>>
>>  exit:
>> -    ovs_mutex_unlock(&ukey->mutex);
>>      if (netflow) {
>>          if (!ok) {
>>              netflow_expire(netflow, &flow);
>> @@ -1460,25 +1454,38 @@ revalidate(struct revalidator *revalidator)
>>          ukey = ukey_lookup(udpif, key, key_len, hash);
>>
>>          used = stats->used;
>> -        if (ukey) {
>> -            ovs_mutex_lock(&ukey->mutex);
>> -
>> -            if (ukey->mark || !ukey->flow_exists) {
>> -                /* The flow has already been dumped. This can
>> occasionally
>> -                 * occur if the datapath is changed in the middle of a
>> flow
>> -                 * dump. Rather than perform the same work twice, skip
>> the
>> -                 * flow this time. */
>> -                ovs_mutex_unlock(&ukey->mutex);
>> +        if (!ukey) {
>> +            ukey = ukey_create(key, key_len, used);
>> +            if (!udpif_insert_ukey(udpif, ukey, hash)) {
>> +                /* The same ukey has already been created. This means
>> that
>> +                 * another revalidator is processing this flow
>> +                 * concurrently, so don't bother processing it. */
>>                  COVERAGE_INC(upcall_duplicate_flow);
>> +                ukey_delete(NULL, ukey);
>>                  goto next;
>>              }
>> +        }
>>
>> -            if (!used) {
>> -                used = ukey->created;
>> -            }
>> +        if (ovs_mutex_trylock(&ukey->mutex)) {
>> +            /* The flow has been dumped, and is being handled by another
>> +             * revalidator concurrently. This can occasionally occur if
>> the
>> +             * datapath is changed in the middle of a flow dump. Rather
>> than
>> +             * perform the same work twice, skip the flow this time. */
>> +            COVERAGE_INC(upcall_duplicate_flow);
>> +            goto next;
>> +        }
>> +
>> +        if (ukey->mark || !ukey->flow_exists) {
>> +            /* The flow has already been dumped and handled by another
>> +             * revalidator during this flow dump operation. Skip it. */
>> +            COVERAGE_INC(upcall_duplicate_flow);
>>              ovs_mutex_unlock(&ukey->mutex);
>> +            goto next;
>>          }
>>
>> +        if (!used) {
>> +            used = ukey->created;
>> +        }
>>          n_flows = udpif_get_n_flows(udpif);
>>          max_idle = ofproto_max_idle;
>>          if (n_flows > flow_limit) {
>> @@ -1488,30 +1495,15 @@ revalidate(struct revalidator *revalidator)
>>          if ((used && used < now - max_idle) || n_flows > flow_limit * 2)
>> {
>>              mark = false;
>>          } else {
>> -            if (!ukey) {
>> -                ukey = ukey_create(key, key_len, used);
>> -                if (!udpif_insert_ukey(udpif, ukey, hash)) {
>> -                    /* The same ukey has already been created. This
>> means that
>> -                     * another revalidator is processing this flow
>> -                     * concurrently, so don't bother processing it. */
>> -                    ukey_delete(NULL, ukey);
>> -                    goto next;
>> -                }
>> -            }
>> -
>>              mark = revalidate_ukey(udpif, ukey, mask, mask_len, actions,
>>                                     actions_len, stats);
>>          }
>> -
>> -        if (ukey) {
>> -            ovs_mutex_lock(&ukey->mutex);
>> -            ukey->mark = ukey->flow_exists = mark;
>> -            ovs_mutex_unlock(&ukey->mutex);
>> -        }
>> +        ukey->mark = ukey->flow_exists = mark;
>>
>>          if (!mark) {
>>              dump_op_init(&ops[n_ops++], key, key_len, ukey);
>>          }
>> +        ovs_mutex_unlock(&ukey->mutex);
>>
>>      next:
>>          may_destroy = dpif_flow_dump_next_may_destroy_keys(&udpif->dump,
>> --
>> 1.7.10.4
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140528/ae21fa73/attachment-0005.html>


More information about the dev mailing list