| Message ID | 1475857062-55311-3-git-send-email-bhanuprakash.bodireddy@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Daniele Di Proietto |
| Headers | show |
> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy <bhanuprakash.bodireddy@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@intel.com> > Signed-off-by: Antonio Fischetti <antonio.fischetti@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
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@openvswitch.org] On Behalf Of Jarno > Rajahalme > Sent: Friday, October 7, 2016 10:08 PM > To: Bodireddy, Bhanuprakash <bhanuprakash.bodireddy@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.bodireddy@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@intel.com> > > Signed-off-by: Antonio Fischetti <antonio.fischetti@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
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. + */ 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. */ 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. */ 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. */ 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. */ 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. */ 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. */ *value = 0; } return true;