| Message ID | 3f2ad7fb91948525f6c52e0d36ec223cd3049c88.1656785856.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | Introduce bitmap_size() | expand |
Le 02/07/2022 à 21:32, Andy Shevchenko a écrit : > On Sat, Jul 02, 2022 at 09:24:24PM +0200, Christophe JAILLET wrote: >> Le 02/07/2022 à 20:54, Andy Shevchenko a écrit : >>> On Sat, Jul 02, 2022 at 08:29:09PM +0200, Christophe JAILLET wrote: > > ... > >>>> - memset(set->bitmap, 0, bitmap_size(num_ssid, num_id)); >>>> + memset(set->bitmap, 0, idset_bitmap_size(num_ssid, num_id)); >>> >>> Why not to use bitmap_zero()? > > ... > >>>> - memset(set->bitmap, 0xff, bitmap_size(set->num_ssid, set->num_id)); >>>> + memset(set->bitmap, 0xff, idset_bitmap_size(set->num_ssid, set->num_id)); >>> >>> Why not to use bitmap_fill() ? > >> For this initial step, I wanted to keep changes as minimal as possible (i.e >> just function renaming) >> >> In fact, I plan to send a follow-up patch on this file. >> This would remove the newly renamed idset_bitmap_size() function, use the >> bitmap API directly (as you pointed-out) with >> "set->num_ssid * set->num_id" as size. >> >> It is already done this way in idset_is_empty(), so it would be more >> consistent. >> >> If the serie needs a v2 (or if required), I can add an additional 5th patch >> for it. Otherwise it will send separatly later. > > If you use bitmap APIs as I suggested above as the first patch, the rest will > have less unneeded churn, no? > > Makes sense. I'll wait for some other potential comments 1 day or 2 and send a v2 with the simplification you propose as an initial step. Thanks for your feed-back. CJ -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Sat, Jul 02, 2022 at 08:29:09PM +0200, Christophe JAILLET wrote: > In order to introduce a bitmap_size() function in the bitmap API, we have > to rename functions with a similar name. > > Add a "idset_" prefix and change bitmap_size() into idset_bitmap_size(). > > No functional change. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > drivers/s390/cio/idset.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/s390/cio/idset.c b/drivers/s390/cio/idset.c > index 45f9c0736be4..e1e77fe080bf 100644 > --- a/drivers/s390/cio/idset.c > +++ b/drivers/s390/cio/idset.c > @@ -16,7 +16,7 @@ struct idset { > unsigned long bitmap[]; > }; > > -static inline unsigned long bitmap_size(int num_ssid, int num_id) > +static inline unsigned long idset_bitmap_size(int num_ssid, int num_id) > { > return BITS_TO_LONGS(num_ssid * num_id) * sizeof(unsigned long); > } > @@ -25,11 +25,11 @@ static struct idset *idset_new(int num_ssid, int num_id) > { > struct idset *set; > > - set = vmalloc(sizeof(struct idset) + bitmap_size(num_ssid, num_id)); > + set = vmalloc(sizeof(struct idset) + idset_bitmap_size(num_ssid, num_id)); > if (set) { > set->num_ssid = num_ssid; > set->num_id = num_id; > - memset(set->bitmap, 0, bitmap_size(num_ssid, num_id)); > + memset(set->bitmap, 0, idset_bitmap_size(num_ssid, num_id)); We don't need bitmap_size() here, we need to replace memset() with bitmap_zero(). > } > return set; > } > @@ -41,7 +41,7 @@ void idset_free(struct idset *set) > > void idset_fill(struct idset *set) > { > - memset(set->bitmap, 0xff, bitmap_size(set->num_ssid, set->num_id)); > + memset(set->bitmap, 0xff, idset_bitmap_size(set->num_ssid, set->num_id)); Same, but bitmap_fill(). > } > > static inline void idset_add(struct idset *set, int ssid, int id) > -- > 2.34.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Sat, Jul 02, 2022 at 08:29:09PM +0200, Christophe JAILLET wrote: > In order to introduce a bitmap_size() function in the bitmap API, we have > to rename functions with a similar name. > > Add a "idset_" prefix and change bitmap_size() into idset_bitmap_size(). > > No functional change. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > drivers/s390/cio/idset.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/s390/cio/idset.c b/drivers/s390/cio/idset.c > index 45f9c0736be4..e1e77fe080bf 100644 > --- a/drivers/s390/cio/idset.c > +++ b/drivers/s390/cio/idset.c > @@ -16,7 +16,7 @@ struct idset { > unsigned long bitmap[]; > }; > > -static inline unsigned long bitmap_size(int num_ssid, int num_id) > +static inline unsigned long idset_bitmap_size(int num_ssid, int num_id) > { > return BITS_TO_LONGS(num_ssid * num_id) * sizeof(unsigned long); > } > @@ -25,11 +25,11 @@ static struct idset *idset_new(int num_ssid, int num_id) > { > struct idset *set; > > - set = vmalloc(sizeof(struct idset) + bitmap_size(num_ssid, num_id)); > + set = vmalloc(sizeof(struct idset) + idset_bitmap_size(num_ssid, num_id)); > if (set) { > set->num_ssid = num_ssid; > set->num_id = num_id; > - memset(set->bitmap, 0, bitmap_size(num_ssid, num_id)); > + memset(set->bitmap, 0, idset_bitmap_size(num_ssid, num_id)); > } > return set; > } Thank you CJ for this patchset. As pointed out by others, it would be great to directly use tthe bitmap-APIs instead of having an intermediate patch, which renames the functions. As you mentioned, it is minimal, but the otherway would be better for maintainability. > @@ -41,7 +41,7 @@ void idset_free(struct idset *set) > > void idset_fill(struct idset *set) > { > - memset(set->bitmap, 0xff, bitmap_size(set->num_ssid, set->num_id)); > + memset(set->bitmap, 0xff, idset_bitmap_size(set->num_ssid, set->num_id)); > } > > static inline void idset_add(struct idset *set, int ssid, int id) > -- > 2.34.1 > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/s390/cio/idset.c b/drivers/s390/cio/idset.c index 45f9c0736be4..e1e77fe080bf 100644 --- a/drivers/s390/cio/idset.c +++ b/drivers/s390/cio/idset.c @@ -16,7 +16,7 @@ struct idset { unsigned long bitmap[]; }; -static inline unsigned long bitmap_size(int num_ssid, int num_id) +static inline unsigned long idset_bitmap_size(int num_ssid, int num_id) { return BITS_TO_LONGS(num_ssid * num_id) * sizeof(unsigned long); } @@ -25,11 +25,11 @@ static struct idset *idset_new(int num_ssid, int num_id) { struct idset *set; - set = vmalloc(sizeof(struct idset) + bitmap_size(num_ssid, num_id)); + set = vmalloc(sizeof(struct idset) + idset_bitmap_size(num_ssid, num_id)); if (set) { set->num_ssid = num_ssid; set->num_id = num_id; - memset(set->bitmap, 0, bitmap_size(num_ssid, num_id)); + memset(set->bitmap, 0, idset_bitmap_size(num_ssid, num_id)); } return set; } @@ -41,7 +41,7 @@ void idset_free(struct idset *set) void idset_fill(struct idset *set) { - memset(set->bitmap, 0xff, bitmap_size(set->num_ssid, set->num_id)); + memset(set->bitmap, 0xff, idset_bitmap_size(set->num_ssid, set->num_id)); } static inline void idset_add(struct idset *set, int ssid, int id)
In order to introduce a bitmap_size() function in the bitmap API, we have to rename functions with a similar name. Add a "idset_" prefix and change bitmap_size() into idset_bitmap_size(). No functional change. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/s390/cio/idset.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)