[ovs-dev] [PATCH] ofproto-dpif: keep slow path flow time stamp up-to-date

Andy Zhou azhou at nicira.com
Mon Dec 2 22:35:48 UTC 2013


pushed to the master.  Thanks,


On Fri, Nov 22, 2013 at 6:28 PM, Ben Pfaff <blp at nicira.com> wrote:

> That sounds great, thanks a lot.
>
> On Fri, Nov 22, 2013 at 05:06:11PM -0800, Andy Zhou wrote:
> > Sure, I will test it more and wait for a bit before pushing, just in case
> > Guolin has additional feedback.  Thanks for the review and redactor
> > suggestion.
> >
> >
> > On Fri, Nov 22, 2013 at 4:58 PM, Ben Pfaff <blp at nicira.com> wrote:
> >
> > > Do you want to check it in then?
> > >
> > > On Fri, Nov 22, 2013 at 04:53:09PM -0800, Andy Zhou wrote:
> > > > Yes, this patch would eliminate code duplications.  The logic looks
> right
> > > > to me.
> > > >
> > > >
> > > > On Fri, Nov 22, 2013 at 3:57 PM, Ben Pfaff <blp at nicira.com> wrote:
> > > >
> > > > > On Thu, Nov 21, 2013 at 08:45:22PM -0800, Andy Zhou wrote:
> > > > > > Noting updating slow path subfacet's time stamp can cause their
> > > datapath
> > > > > > flows deleted periodically. For example, CFM datapath flow have
> > > usespace
> > > > > > actions that are handled in dpif slow path. They are deleted and
> > > > > > recreated periodically without the fix.
> > > > > >
> > > > > > This bug are not obvious during normal operation. Deleted CFM
> flow
> > > > > > would cause CFM packets to be handled by flow miss handler which
> will
> > > > > > reinstall the flow in the datapath. The only potentially
> observable
> > > > > > behavior is that when the user space is overwhelmed with flow
> miss
> > > > > packets,
> > > > > > the periodic CFM miss packets may get stuck behind other miss
> > > packets,
> > > > > > cause tunnel flapping.
> > > > > >
> > > > > > Reported-by: Guolin Yang <gyang at nicira.com>
> > > > > > Signed-off-by: Andy Zhou <azhou at nicira.com>
> > > > >
> > > > > I agree that this fixes the problem.  However, it seems to
> duplicate
> > > > > some logic.  What do you think about this alternative patch, which
> > > > > instead refactors slightly to avoid code duplication?
> > > > >
> > > > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > > > > index c1c206b..b56202c 100644
> > > > > --- a/ofproto/ofproto-dpif.c
> > > > > +++ b/ofproto/ofproto-dpif.c
> > > > > @@ -213,7 +213,8 @@ struct subfacet {
> > > > >
> > > > >  #define SUBFACET_DESTROY_MAX_BATCH 50
> > > > >
> > > > > -static struct subfacet *subfacet_create(struct facet *, struct
> > > flow_miss
> > > > > *);
> > > > > +static struct subfacet *subfacet_create(struct facet *, struct
> > > flow_miss
> > > > > *,
> > > > > +                                        uint32_t key_hash);
> > > > >  static struct subfacet *subfacet_find(struct dpif_backer *,
> > > > >                                        const struct nlattr *key,
> size_t
> > > > > key_len,
> > > > >                                        uint32_t key_hash);
> > > > > @@ -3222,13 +3223,32 @@ handle_flow_miss_with_facet(struct
> flow_miss
> > > > > *miss, struct facet *facet,
> > > > >  {
> > > > >      enum subfacet_path want_path;
> > > > >      struct subfacet *subfacet;
> > > > > +    uint32_t key_hash;
> > > > >
> > > > > +    /* Update facet stats. */
> > > > >      facet->packet_count += miss->stats.n_packets;
> > > > >      facet->prev_packet_count += miss->stats.n_packets;
> > > > >      facet->byte_count += miss->stats.n_bytes;
> > > > >      facet->prev_byte_count += miss->stats.n_bytes;
> > > > >
> > > > > -    want_path = facet->xout.slow ? SF_SLOW_PATH : SF_FAST_PATH;
> > > > > +    /* Look for an existing subfacet.  If we find one, update its
> used
> > > > > time. */
> > > > > +    key_hash = odp_flow_key_hash(miss->key, miss->key_len);
> > > > > +    if (!list_is_empty(&facet->subfacets)) {
> > > > > +        subfacet = subfacet_find(miss->ofproto->backer,
> > > > > +                                 miss->key, miss->key_len,
> key_hash);
> > > > > +        if (subfacet) {
> > > > > +            if (subfacet->facet == facet) {
> > > > > +                subfacet->used = MAX(subfacet->used,
> > > miss->stats.used);
> > > > > +            } else {
> > > > > +                /* This shouldn't happen. */
> > > > > +                VLOG_ERR_RL(&rl, "subfacet with wrong facet");
> > > > > +                subfacet_destroy(subfacet);
> > > > > +                subfacet = NULL;
> > > > > +            }
> > > > > +        }
> > > > > +    } else {
> > > > > +        subfacet = NULL;
> > > > > +    }
> > > > >
> > > > >      /* Don't install the flow if it's the result of the
> "userspace"
> > > > >       * action for an already installed facet.  This can occur
> when a
> > > > > @@ -3240,7 +3260,13 @@ handle_flow_miss_with_facet(struct flow_miss
> > > *miss,
> > > > > struct facet *facet,
> > > > >          return;
> > > > >      }
> > > > >
> > > > > -    subfacet = subfacet_create(facet, miss);
> > > > > +    /* Create a subfacet, if we don't already have one. */
> > > > > +    if (!subfacet) {
> > > > > +        subfacet = subfacet_create(facet, miss, key_hash);
> > > > > +    }
> > > > > +
> > > > > +    /* Install the subfacet, if it's not already installed. */
> > > > > +    want_path = facet->xout.slow ? SF_SLOW_PATH : SF_FAST_PATH;
> > > > >      if (subfacet->path != want_path) {
> > > > >          struct flow_miss_op *op = &ops[(*n_ops)++];
> > > > >          struct dpif_flow_put *put = &op->dpif_op.u.flow_put;
> > > > > @@ -4382,37 +4408,20 @@ subfacet_find(struct dpif_backer *backer,
> const
> > > > > struct nlattr *key,
> > > > >      return NULL;
> > > > >  }
> > > > >
> > > > > -/* Searches 'facet' (within 'ofproto') for a subfacet with the
> > > specified
> > > > > - * 'key_fitness', 'key', and 'key_len' members in 'miss'.
>  Returns the
> > > > > - * existing subfacet if there is one, otherwise creates and
> returns a
> > > > > - * new subfacet. */
> > > > > +/* Creates and returns a new subfacet within 'facet' for the flow
> in
> > > > > 'miss'.
> > > > > + * 'key_hash' must be a hash over miss->key.  The caller must have
> > > already
> > > > > + * ensured that no subfacet subfacet already exists. */
> > > > >  static struct subfacet *
> > > > > -subfacet_create(struct facet *facet, struct flow_miss *miss)
> > > > > +subfacet_create(struct facet *facet, struct flow_miss *miss,
> uint32_t
> > > > > key_hash)
> > > > >  {
> > > > >      struct dpif_backer *backer = miss->ofproto->backer;
> > > > >      const struct nlattr *key = miss->key;
> > > > >      size_t key_len = miss->key_len;
> > > > > -    uint32_t key_hash;
> > > > >      struct subfacet *subfacet;
> > > > >
> > > > > -    key_hash = odp_flow_key_hash(key, key_len);
> > > > > -
> > > > > -    if (list_is_empty(&facet->subfacets)) {
> > > > > -        subfacet = &facet->one_subfacet;
> > > > > -    } else {
> > > > > -        subfacet = subfacet_find(backer, key, key_len, key_hash);
> > > > > -        if (subfacet) {
> > > > > -            if (subfacet->facet == facet) {
> > > > > -                return subfacet;
> > > > > -            }
> > > > > -
> > > > > -            /* This shouldn't happen. */
> > > > > -            VLOG_ERR_RL(&rl, "subfacet with wrong facet");
> > > > > -            subfacet_destroy(subfacet);
> > > > > -        }
> > > > > -
> > > > > -        subfacet = xmalloc(sizeof *subfacet);
> > > > > -    }
> > > > > +    subfacet = (list_is_empty(&facet->subfacets)
> > > > > +                ? &facet->one_subfacet
> > > > > +                : xmalloc(sizeof *subfacet));
> > > > >
> > > > >      COVERAGE_INC(subfacet_create);
> > > > >      hmap_insert(&backer->subfacets, &subfacet->hmap_node,
> key_hash);
> > > > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20131202/f4e7784e/attachment-0003.html>


More information about the dev mailing list