[ovs-dev] [xc 4/4] ofproto-dpif: Track relevant fields for wildcarding and add xout cache.

[Ben Pfaff]

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 <blp at 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
> 
>