On Jun 5, 2013, at 10:43 PM, Ben Pfaff <b...@nicira.com> wrote:
> "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.
Fixed.
> 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?)
As we discussed off-line, we'll leave this as-is.
> bond_choose_output_slave() needs a comment update.
Added the following comment:
* If 'wc' is non-NULL, bitwise-OR's 'wc' with the set of bits that were
* significant in the selection. At some point earlier, 'wc' should
* have been initialized (e.g., by flow_wildcards_init_catchall()).
> 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. */
Ah, crap. You're right. I changed the logic to as follows:
if (bond->lacp_status != LACP_NEGOTIATED) {
/* Must have LACP negotiations for TCP balanced bonds. */
return NULL;
}
if (wc) {
flow_mask_hash_fields(wc, NX_HASH_FIELDS_SYMMETRIC_L4);
}
> learn_mask() could use a comment.
I added the following:
/* Perform a bitwise-OR on 'wc''s fields that are relevant as sources in
* the learn action 'learn'. */
> 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 added the following:
/* Wildcards relevant in translation. Any fields that were used to
* calculate the action must be set for caching and megaflows to
* work. For example, if the flow lookup involved performing the
* "normal" action on IPv4 and ARP packets, 'wc' would have the
* 'in_port' (always set), 'dl_type' (flow match), 'vlan_tci'
* (normal action), and 'dl_dst' (normal action) fields set. */
> 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.
/* A cache of xlate_out (xout) translations. Facets that depend on the
* same flow fields have the same xout entries, so the cache saves on
* needless action translations and redundant copies of xout structures. */
> 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.
It's used to feed into xlate_in_init() in xout_cache_push_stats(). Since we
also store 'initial_vals' for the same purpose, I didn't know if it was worth
translating from 'cr->match.flow's miniflow to a regular flow. However, since
Ethan is planning to remove "initial_vals" everywhere, it probably makes sense
to drop the 'flow' member.
> 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. */
Done. Thanks.
> 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.
This comment now describes the xc definition in facet:
/* Membership in the xout cache.
*
* The facet should only be without a xout_cache for the brief time
* between invalidating old cache entries and a call to
* facet_revalidate() to repopulate it. */
> 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.
As mentioned last night, I fixed the single problem I found when running on
ofproto-dpif.
Thanks!
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
