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.
+Ethan any thoughts on this?
+ 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.
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)?
* How does that compare to REVALIDATE_MAX_BATCH?
* Is it worth counting these for future reference?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
