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-boun...@openvswitch.org] On Behalf Of Jarno > Rajahalme > Sent: Friday, October 7, 2016 10:08 PM > To: Bodireddy, Bhanuprakash <bhanuprakash.bodire...@intel.com> > Cc: dev@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.bodire...@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.bodire...@intel.com> > > Signed-off-by: Antonio Fischetti <antonio.fische...@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@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
