[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