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

Justin Pettit jpettit at nicira.com
Thu Jun 6 22:34:08 UTC 2013


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





More information about the dev mailing list