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

Joe Stringer joestringer at nicira.com
Wed May 28 02:33:21 UTC 2014


Yeah, I'd like to get a gauge of this as well. I'll follow up on this.


On 28 May 2014 14:28, Ethan Jackson <ethan at nicira.com> wrote:

> Could we measure if this actually causes a performance degradation?
> I'd honestly be surprised if it does . . . My guess is our bottlenecks
> are elsewhere, but perhaps they aren't.
>
> Ethan
>
> On Tue, May 27, 2014 at 7:13 PM, Joe Stringer <joestringer at nicira.com>
> wrote:
> > 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
> >>>
> >>
> >
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140528/6ac08655/attachment-0005.html>


More information about the dev mailing list