Justin, I know that you're eager for a full review of this patch, but
it's just not going to happen tonight. I have some initial feedback
here, and I'll finish it up tomorrow morning.
On Wed, Jun 05, 2013 at 03:21:39PM -0700, Justin Pettit wrote:
> Dynamically determines the flow fields that were relevant in
> processing flows based on the OpenFlow flow table and switch
> configuration. The immediate use for this functionality is to
> generate an action translation cache for similar flows. This
> yields a roughly 40% improvement in flow set up rates for a
> complicated flow table. (Another 40% improvement should be
> trivially achievable by more efficiently deleting facets.)
>
> More importantly, these wildcards will be used to determine what to
> wildcard for the forthcoming megaflow patches that will allow
> wildcarding in the kernel, which will provide significant flow
> set up improvements.
>
> The approach to tracking fields and the idea of caching action
> translations was based on an impressive prototype by Ethan Jackson.
>
> Co-authored-by: Ethan Jackson <et...@nicira.com>
"git am" says:
Applying: ofproto-dpif: Track relevant fields for wildcarding and add xout
cache.
/home/blp/ovs/.git/rebase-apply/patch:1342: trailing whitespace.
create_xc(const struct flow_wildcards *initial_wc,
warning: 1 line adds whitespace errors.
general
-------
There's a number of repetitions of this pattern:
union mf_value mask_value;
memset(&mask_value, 0xff, sizeof mask_value);
mf_set_flow_value(move->src.field, &mask_value, &wc->masks);
Perhaps one could invent a helper function that follows this pattern,
e.g. "void mf_set_wildcards_fixed(const struct mf_field *, struct
flow_wildcards *);" or some such. (Maybe there'd need to be one for a
subfield too?)
bond.c
------
bond_choose_output_slave() needs a comment update.
The new logic in choose_output_slave() looks weird to me. It appears
that passing 'wc' in or not actually affects the bond's behavior,
which I didn't expect (I would have thought it would just affect
whether 'wc' gets updated):
case BM_TCP:
+ if (bond->lacp_status == LACP_NEGOTIATED && wc) {
+ flow_mask_hash_fields(wc, NX_HASH_FIELDS_SYMMETRIC_L4);
+ } else {
/* Must have LACP negotiations for TCP balanced bonds. */
return NULL;
}
/* Fall Through. */
learn.c
-------
learn_mask() could use a comment.
ofproto-dpif.c
--------------
I would make this a full-width comment and possibly expand it (maybe
give an example?):
+ struct flow_wildcards wc; /* Relevant wildcards in translation.
+ * Any fields that were used to
+ * calculate the action must be set for
+ * caching and megaflows to work. */
I think that struct xout_cache could use a top-level comment
explaining its purpose and how it generally fits in with the rest of
the system.
What's the relationship between 'cr->match.flow' and 'flow' in
xout_cache? After skimming code very quickly, I think that 'flow' is
some particular flow that caused the cache entry to be created. But
isn't cr->match.flow also such a flow? The meaning of 'flow' probably
warrants a comment.
I usually try to make comments help a little more to figure out how
stuff links together. For example:
/* Membership in the xout cache. */
struct list xc_list_node; /* In owning xout_cache's 'facets' list. */
struct xout_cache *xc; /* Owning xout_cache. */
and in the xout_cache:
struct list facets; /* Contains 'struct facet's by 'xc_list_node'. */
and in struct ofproto_dpif:
/* xlate_out cache. */
struct classifier xout_cache; /* Contains "struct xout_cache"s. */
One property that isn't obvious at first glance is, when does a facet
have a xout_cache entry? Essentially all the time (I can see that it
can be without one at least temporarily after flushing or invalidating
the xout cache and before revalidating), or are there other
exceptions? Might be worth some commentary.
Have you tried doing "make check-valgrind" on at least some tests to
check for memory leaks (and bugs)? It's a little time consuming
(takes several minutes, maybe half an hour? with TESTSUITEFLAGS=-j8)
but it can be useful especially for large changes.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
