On Mon, May 19, 2014 at 04:10:04PM +1200, Joe Stringer wrote:
> This looks much tidier, I like it. Particularly given the elimination of 4
> goto statements :-). Comments inline.
> 
> 
> On 17 May 2014 11:28, Ben Pfaff <b...@nicira.com> wrote:
> >
> > + * Notes
> > + * -----
> > + *
> > + * All error reporting is deferred to the call to
> > dpif_flow_dump_destroy().
> > + *
> > + * Previous versions of this interface allowed to caller to say whether it
> > + * actually needed actions or statistics, to allow for optimizations in
> > cases
> > + * where they were not needed.  This version always provided actions and
> > + * statistics because every existing caller requests them.
> > + */
> >
> 
> The main reason the flow_dumper code asked for actions in the current code
> is that they're practically free most of the time (dpif_flow_dump_next()
> only misses actions if they won't fit in the netlink message, in which case
> dpif_flow_dump_next() sends a separate call to fetch them). We were
> thinking about removing actions from the common case though, so more flows
> could be dumped per batch. I'm not sure if this affects your view on this
> interface, perhaps we'll just cross that bridge when we come to it.

You know, you're right.  I'm going to delete that comment, it's not
really helpful.

> +        for (f = flows; f < &flows[n_dumped]; f++) {
> > +            long long int used = f->stats.used;
> > +            uint32_t hash = hash_bytes(f->key, f->key_len, udpif->secret);
> > +            struct udpif_key *ukey = ukey_lookup(udpif, f->key,
> > f->key_len,
> > +                                                 hash);
> > +            bool mark;
> > +
> > +            if (!used && ukey) {
> > +                bool already_dumped;
> > +
> > +                ovs_mutex_lock(&ukey->mutex);
> > +                already_dumped = ukey->mark || !ukey->flow_exists;
> > +                used = ukey->created;
> > +                ovs_mutex_unlock(&ukey->mutex);
> > +
> > +                if (already_dumped) {
> > +                    /* 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. */
> > +                    COVERAGE_INC(upcall_duplicate_flow);
> > +                    continue;
> >                  }
> >              }
> >
> 
> I'm not sure whether you would want it in this in a separate patch, but it
> appears that we only try to detect "already_dumped" for flows that haven't
> been used before. Flows which have been used before and are duplicated will
> execute through to revalidate_ukey(), and potentially revalidate twice (but
> only if the first revalidation was successful).
> 
> I've been looking at replacing 'ukey->mark' with a seq which would simplify
> detecting already-dumped flows, so I could try to address these concerns
> separately if you like.

I think that's subtle enough that I'd prefer to address it
separately.  This patch is intended to be only a refactoring, without
significant changes to behavior (the insignificant changes should only
be to how batches get formed).

> With this patch, it is much easier to tell how many flows are being handled
> in a batch. Out of curiousity, do you know:
> * How many can we fit inside a buffer of NL_DUMP_BUFSIZE (avg case)?

Looking at the definition of ODPUTIL_FLOW_KEY_BYTES, I'd guess that
the "average" key, when tunneling is going on, is about 150 bytes.
Triple that (just a guess) to account for the mask, actions, stats,
other overhead, etc., and you get about 450 bytes per flow.  The
kernel normally dumps 4096 bytes at a time, and 4096/450 is 9, so I'd
guess about ten or so.  Maybe a few more if there are no tunnels and
few actions.

> * How does that compare to REVALIDATE_MAX_BATCH?

Based on that ballpark estimate, I guess REVALIDATE_MAX_BATCH is
bigger than necessary.  4096/50 would only allow 81 bytes per flow,
which isn't going to happen.

> * Is it worth counting these for future reference?

Off-hand, I don't have a good idea for how to use this information.
Do you?

I'll push this in a few minutes.

Thanks,

Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to