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
