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