Thanks for all the improvements! They sound good to me.
On Thu, Jun 06, 2013 at 03:34:08PM -0700, Justin Pettit wrote:
> 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
