[ovs-dev] [PATCH 02/12] flow: Add comments to mf_get_next_in_map()

Fischetti, Antonio antonio.fischetti at intel.com
Sun Oct 9 08:16:10 UTC 2016


Thanks Jarno, will follow your suggestions. That's really a good improvement and will help a lot to explain the function details.

Antonio

> -----Original Message-----
> From: dev [mailto:dev-bounces at openvswitch.org] On Behalf Of Jarno
> Rajahalme
> Sent: Friday, October 7, 2016 10:08 PM
> To: Bodireddy, Bhanuprakash <bhanuprakash.bodireddy at intel.com>
> Cc: dev at openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 02/12] flow: Add comments to
> mf_get_next_in_map()
> 
> 
> > On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy
> <bhanuprakash.bodireddy at intel.com> wrote:
> >
> 
> A brief commit message should be included here. E.g.:
> 
> This patch adds comments to mf)get_next_in_map() to make it more
> comprehensible.
> 
> > Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy at intel.com>
> > Signed-off-by: Antonio Fischetti <antonio.fischetti at intel.com>
> > ---
> > lib/flow.h | 23 +++++++++++++++++++++--
> > 1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/flow.h b/lib/flow.h
> > index ea24e28..4eb19ae 100644
> > --- a/lib/flow.h
> > +++ b/lib/flow.h
> > @@ -570,15 +570,27 @@ struct mf_for_each_in_map_aux {
> >     const uint64_t *values;
> > };
> >
> > +/*
> > + * Parse the bitmap mask of the subtable and output the next value
> > + * from the search-key.
> > + */
> 
> While it is helpful to refer to higher level concepts such as subtable and
> search-key, I’d prefer the comment to rather state what the function does
> in terms of the abstractions at hand and then provide an example of use
> referring to subtables and such. Like this for example:
> 
> /* Get the data from ‘aux->values’ corresponding to the next lowest 1-bit
> in
>  * ‘aux->map’, given that ‘aux->values’ points to an array of 64-bit words
>  * corresponding to the 1-bits in ‘aux->fmap’, starting from the rightmost
> 1-bit.
>  *
>  * Returns ’true’ if the traversal is incomplete, ‘false’ otherwise. ‘aux’
> is prepared
>  * for the next iteration after each call.
>  *
>  * This is used to traverse through, for example, the values in a miniflow
>  * representation of a flow key selected by non-zero 64-bit words in a
>  * corresponding subtable mask. */
> 
> > static inline bool
> > mf_get_next_in_map(struct mf_for_each_in_map_aux *aux,
> >                    uint64_t *value)
> > {
> > -    map_t *map, *fmap;
> > +    map_t *map;    /* map refers to the bitmap mask of the subtable. */
> > +    map_t *fmap;   /* fmap refers to the bitmap of the search-key. */
> 
> Again, given that the example use was referred in the main comment above,
> these comments should stick to the abstractions at hand. Better yet, these
> comments should instead be placed into the aux struct definition above,
> e.g.:
> 
> struct mf_for_each_in_map_aux {
>     size_t unit;                      /* Current 64-bit unit of the
> flowmaps being processed. */
>     struct flowmap fmap;      /* Remaining 1-bits corresponding to the 64-
> bit words in ‘values’
>     struct flowmap map;       /* Remaining 1-bits corresponding to the 64-
> bit words of interest.
>     const uint64_t *values;   /* 64-bit words corresponding to the 1-bits
> in ‘fmap’. */
> };
> 
> >     map_t rm1bit;
> >
> > +    /* The bitmap mask selects which value must be considered from the
> > +     * search-key. We process the corresponding 'unit' of size 64 bits.
> > +     * 'aux->unit' is an index to the current unit of 64 bits. */
> 
> Given the above comments this could be changed to:
> 
> /* Skip empty map units. */
> 
> >     while (OVS_UNLIKELY(!*(map = &aux->map.bits[aux->unit]))) {
> > -        /* Skip remaining data in the previous unit. */
> > +        /* No bits are enabled, so no search-key value will be output.
> > +         * In case some of the corresponding data chunks in the search-
> key
> > +         * bitmap are set, the data chunks must be skipped, as they are
> not
> > +         * considered by this mask. This is handled by incrementing
> aux->values
> > +         * accordingly. */
> 
> This comment is incorrect, as the determination of the iteration
> termination is done later (the ‘false’ return case).
> 
> How about this:
> 
> /* Skip remaining data in the current unit before advancing to the next.
> */
> 
> >         aux->values += count_1bits(aux->fmap.bits[aux->unit]);
> >         if (++aux->unit == FLOWMAP_UNITS) {
> >             return false;
> > @@ -589,7 +601,12 @@ mf_get_next_in_map(struct mf_for_each_in_map_aux
> *aux,
> >     *map -= rm1bit;
> >     fmap = &aux->fmap.bits[aux->unit];
> >
> > +    /* If the 8-byte value referred by 'rm1bit' is present in the
> > +     * search-key return 'value', otherwise return 0. */
> 
> How about this instead:
> 
> /* If the rightmost 1-bit found from the current unit in ‘aux->map’
>  * (‘rm1bit’) is also present in ‘aux->fmap’, store the corresponding
>  * value from ‘aux->values’ to ‘*value', otherwise store 0. */
> 
> >     if (OVS_LIKELY(*fmap & rm1bit)) {
> > +        /* The value is in the search-key, if the search-key contains
> other
> > +         * values preceeding the 'rm1bit' bit, we consider it trash and
> the
> > +         * corresponding data chunks should be skipped accordingly. */
> 
> How about this instead:
> 
> /* Skip all 64-bit words in ‘values’ preceding the one corresponding to
> ‘rm1bit’. */
> 
> >         map_t trash = *fmap & (rm1bit - 1);
> >
> >         *fmap -= trash;
> > @@ -600,6 +617,8 @@ mf_get_next_in_map(struct mf_for_each_in_map_aux
> *aux,
> >
> >         *value = *aux->values;
> >     } else {
> > +        /* The search-key does not contain a value that corresponds to
> > +         * rm1bit. */
> 
> Given the above, I believe this comment can be omitted.
> 
> >         *value = 0;
> >     }
> >     return true;
> > --
> > 2.4.11
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev


More information about the dev mailing list