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 <b...@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 <b...@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 <gy...@nicira.com>
> > > > > Signed-off-by: Andy Zhou <az...@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);
> > > >
> >
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
