[PATCH 0/4] Introduce bitmap_size()

[PATCH 0/4] Introduce bitmap_size()

[Christophe JAILLET]

This serie introduces bitmap_size() which returns the size, in bytes, of a
bitmap. Such a function is useful to simplify some drivers that use vmalloc() or
other functions to allocate some butmaps.
It also hides some implementation details about how bitmaps are stored (array of
longs)

Before introducing this function in patch 3, patch 1 and 2 rename some functions
with the same name but with different meaning.

Finaly, patch 4 makes use of the new function in bitmap.h.


Other follow-up patches to simplify some drivers will be proposed later if/when
this serie is merged.

Christophe JAILLET (4):
  s390/cio: Rename bitmap_size() as idset_bitmap_size()
  fs/ntfs3: Rename bitmap_size() as ntfs3_bitmap_size()
  bitmap: Introduce bitmap_size()
  bitmap: Use bitmap_size()

 drivers/md/dm-clone-metadata.c |  5 -----
 drivers/s390/cio/idset.c       |  8 ++++----
 fs/ntfs3/bitmap.c              |  4 ++--
 fs/ntfs3/fsntfs.c              |  2 +-
 fs/ntfs3/index.c               |  6 +++---
 fs/ntfs3/ntfs_fs.h             |  2 +-
 fs/ntfs3/super.c               |  2 +-
 include/linux/bitmap.h         | 15 +++++++++------
 lib/math/prime_numbers.c       |  2 --
 9 files changed, 21 insertions(+), 25 deletions(-)

-- 
2.34.1

[PATCH 1/4] s390/cio: Rename bitmap_size() as idset_bitmap_size()

[Christophe JAILLET]

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;
 }
@@ -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

[PATCH 2/4] fs/ntfs3: Rename bitmap_size() as ntfs3_bitmap_size()

[Christophe JAILLET]

In order to introduce a bitmap_size() function in the bitmap API, we have
to rename functions with a similar name.

Add a "ntfs3_" prefix and change bitmap_size() into ntfs3_bitmap_size().

No functional change.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 fs/ntfs3/bitmap.c  | 4 ++--
 fs/ntfs3/fsntfs.c  | 2 +-
 fs/ntfs3/index.c   | 6 +++---
 fs/ntfs3/ntfs_fs.h | 2 +-
 fs/ntfs3/super.c   | 2 +-
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/ntfs3/bitmap.c b/fs/ntfs3/bitmap.c
index e3b5680fd516..f98327453d83 100644
--- a/fs/ntfs3/bitmap.c
+++ b/fs/ntfs3/bitmap.c
@@ -661,7 +661,7 @@ int wnd_init(struct wnd_bitmap *wnd, struct super_block *sb, size_t nbits)
 	wnd->total_zeroes = nbits;
 	wnd->extent_max = MINUS_ONE_T;
 	wnd->zone_bit = wnd->zone_end = 0;
-	wnd->nwnd = bytes_to_block(sb, bitmap_size(nbits));
+	wnd->nwnd = bytes_to_block(sb, ntfs3_bitmap_size(nbits));
 	wnd->bits_last = nbits & (wbits - 1);
 	if (!wnd->bits_last)
 		wnd->bits_last = wbits;
@@ -1323,7 +1323,7 @@ int wnd_extend(struct wnd_bitmap *wnd, size_t new_bits)
 		return -EINVAL;
 
 	/* Align to 8 byte boundary. */
-	new_wnd = bytes_to_block(sb, bitmap_size(new_bits));
+	new_wnd = bytes_to_block(sb, ntfs3_bitmap_size(new_bits));
 	new_last = new_bits & (wbits - 1);
 	if (!new_last)
 		new_last = wbits;
diff --git a/fs/ntfs3/fsntfs.c b/fs/ntfs3/fsntfs.c
index 3de5700a9b83..9c74d88ce0f0 100644
--- a/fs/ntfs3/fsntfs.c
+++ b/fs/ntfs3/fsntfs.c
@@ -493,7 +493,7 @@ static int ntfs_extend_mft(struct ntfs_sb_info *sbi)
 	ni->mi.dirty = true;
 
 	/* Step 2: Resize $MFT::BITMAP. */
-	new_bitmap_bytes = bitmap_size(new_mft_total);
+	new_bitmap_bytes = ntfs3_bitmap_size(new_mft_total);
 
 	err = attr_set_size(ni, ATTR_BITMAP, NULL, 0, &sbi->mft.bitmap.run,
 			    new_bitmap_bytes, &new_bitmap_bytes, true, NULL);
diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c
index 84ccc1409874..5c5ea05a5ef1 100644
--- a/fs/ntfs3/index.c
+++ b/fs/ntfs3/index.c
@@ -1353,7 +1353,7 @@ static int indx_create_allocate(struct ntfs_index *indx, struct ntfs_inode *ni,
 
 	alloc->nres.valid_size = alloc->nres.data_size = cpu_to_le64(data_size);
 
-	err = ni_insert_resident(ni, bitmap_size(1), ATTR_BITMAP, in->name,
+	err = ni_insert_resident(ni, ntfs3_bitmap_size(1), ATTR_BITMAP, in->name,
 				 in->name_len, &bitmap, NULL, NULL);
 	if (err)
 		goto out2;
@@ -1415,7 +1415,7 @@ static int indx_add_allocate(struct ntfs_index *indx, struct ntfs_inode *ni,
 	if (bmp) {
 		/* Increase bitmap. */
 		err = attr_set_size(ni, ATTR_BITMAP, in->name, in->name_len,
-				    &indx->bitmap_run, bitmap_size(bit + 1),
+				    &indx->bitmap_run, ntfs3_bitmap_size(bit + 1),
 				    NULL, true, NULL);
 		if (err)
 			goto out1;
@@ -1973,7 +1973,7 @@ static int indx_shrink(struct ntfs_index *indx, struct ntfs_inode *ni,
 	if (err)
 		return err;
 
-	bpb = bitmap_size(bit);
+	bpb = ntfs3_bitmap_size(bit);
 	if (bpb * 8 == nbits)
 		return 0;
 
diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
index 55d686e3c4ec..85210e610a3a 100644
--- a/fs/ntfs3/ntfs_fs.h
+++ b/fs/ntfs3/ntfs_fs.h
@@ -945,7 +945,7 @@ static inline bool run_is_empty(struct runs_tree *run)
 }
 
 /* NTFS uses quad aligned bitmaps. */
-static inline size_t bitmap_size(size_t bits)
+static inline size_t ntfs3_bitmap_size(size_t bits)
 {
 	return ALIGN((bits + 7) >> 3, 8);
 }
diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index b41d7c824a50..7d48f886ac82 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -1101,7 +1101,7 @@ static int ntfs_fill_super(struct super_block *sb, struct fs_context *fc)
 
 	/* Check bitmap boundary. */
 	tt = sbi->used.bitmap.nbits;
-	if (inode->i_size < bitmap_size(tt)) {
+	if (inode->i_size < ntfs3_bitmap_size(tt)) {
 		err = -EINVAL;
 		goto put_inode_out;
 	}
-- 
2.34.1

[PATCH 3/4] bitmap: Introduce bitmap_size()

[Christophe JAILLET]

The new bitmap_size() function returns the size, in bytes, of a bitmap.

Remove the already existing bitmap_size() functions and macro in some
files.
These files already use the bitmap API and will use the new function
in bitmap.h automatically.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/md/dm-clone-metadata.c | 5 -----
 include/linux/bitmap.h         | 6 ++++++
 lib/math/prime_numbers.c       | 2 --
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-clone-metadata.c b/drivers/md/dm-clone-metadata.c
index c43d55672bce..47c1fa7aad8b 100644
--- a/drivers/md/dm-clone-metadata.c
+++ b/drivers/md/dm-clone-metadata.c
@@ -465,11 +465,6 @@ static void __destroy_persistent_data_structures(struct dm_clone_metadata *cmd)
 
 /*---------------------------------------------------------------------------*/
 
-static size_t bitmap_size(unsigned long nr_bits)
-{
-	return BITS_TO_LONGS(nr_bits) * sizeof(long);
-}
-
 static int __dirty_map_init(struct dirty_map *dmap, unsigned long nr_words,
 			    unsigned long nr_regions)
 {
diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index f091a1664bf1..f66fb98a4126 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -48,6 +48,7 @@ struct device;
  *  bitmap_equal(src1, src2, nbits)             Are *src1 and *src2 equal?
  *  bitmap_intersects(src1, src2, nbits)        Do *src1 and *src2 overlap?
  *  bitmap_subset(src1, src2, nbits)            Is *src1 a subset of *src2?
+ *  bitmap_size(nbits)                          Size, in bytes, of a bitmap
  *  bitmap_empty(src, nbits)                    Are all bits zero in *src?
  *  bitmap_full(src, nbits)                     Are all bits set in *src?
  *  bitmap_weight(src, nbits)                   Hamming Weight: number set bits
@@ -124,6 +125,11 @@ unsigned long *bitmap_alloc_node(unsigned int nbits, gfp_t flags, int node);
 unsigned long *bitmap_zalloc_node(unsigned int nbits, gfp_t flags, int node);
 void bitmap_free(const unsigned long *bitmap);
 
+static __always_inline size_t bitmap_size(unsigned long nbits)
+{
+	return BITS_TO_LONGS(nbits) * sizeof(unsigned long);
+}
+
 /* Managed variants of the above. */
 unsigned long *devm_bitmap_alloc(struct device *dev,
 				 unsigned int nbits, gfp_t flags);
diff --git a/lib/math/prime_numbers.c b/lib/math/prime_numbers.c
index d42cebf7407f..d3b64b10da1c 100644
--- a/lib/math/prime_numbers.c
+++ b/lib/math/prime_numbers.c
@@ -6,8 +6,6 @@
 #include <linux/prime_numbers.h>
 #include <linux/slab.h>
 
-#define bitmap_size(nbits) (BITS_TO_LONGS(nbits) * sizeof(unsigned long))
-
 struct primes {
 	struct rcu_head rcu;
 	unsigned long last, sz;
-- 
2.34.1

[PATCH 4/4] bitmap: Use bitmap_size()

[Christophe JAILLET]

Simplify code and take advantage of the new bitmap_size() function.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 include/linux/bitmap.h | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index f66fb98a4126..668b47c1e5de 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -243,21 +243,18 @@ extern int bitmap_print_list_to_buf(char *buf, const unsigned long *maskp,
 
 static inline void bitmap_zero(unsigned long *dst, unsigned int nbits)
 {
-	unsigned int len = BITS_TO_LONGS(nbits) * sizeof(unsigned long);
-	memset(dst, 0, len);
+	memset(dst, 0, bitmap_size(nbits));
 }
 
 static inline void bitmap_fill(unsigned long *dst, unsigned int nbits)
 {
-	unsigned int len = BITS_TO_LONGS(nbits) * sizeof(unsigned long);
-	memset(dst, 0xff, len);
+	memset(dst, 0xff, bitmap_size(nbits));
 }
 
 static inline void bitmap_copy(unsigned long *dst, const unsigned long *src,
 			unsigned int nbits)
 {
-	unsigned int len = BITS_TO_LONGS(nbits) * sizeof(unsigned long);
-	memcpy(dst, src, len);
+	memcpy(dst, src, bitmap_size(nbits));
 }
 
 /*
-- 
2.34.1

Re: [PATCH 1/4] s390/cio: Rename bitmap_size() as idset_bitmap_size()

[Andy Shevchenko]

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.

...

> -		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() ?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] fs/ntfs3: Rename bitmap_size() as ntfs3_bitmap_size()

[Andy Shevchenko]

On Sat, Jul 02, 2022 at 08:29:27PM +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.

...

>  /* NTFS uses quad aligned bitmaps. */
> -static inline size_t bitmap_size(size_t bits)
> +static inline size_t ntfs3_bitmap_size(size_t bits)
>  {
>  	return ALIGN((bits + 7) >> 3, 8);

It would be easier to understand in a way

	return BITS_TO_BYTES(ALIGN(bits, 64));

>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] bitmap: Introduce bitmap_size()

[Andy Shevchenko]

On Sat, Jul 02, 2022 at 08:29:36PM +0200, Christophe JAILLET wrote:
> The new bitmap_size() function returns the size, in bytes, of a bitmap.
> 
> Remove the already existing bitmap_size() functions and macro in some
> files.
> These files already use the bitmap API and will use the new function
> in bitmap.h automatically.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/md/dm-clone-metadata.c | 5 -----
>  include/linux/bitmap.h         | 6 ++++++
>  lib/math/prime_numbers.c       | 2 --
>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/md/dm-clone-metadata.c b/drivers/md/dm-clone-metadata.c
> index c43d55672bce..47c1fa7aad8b 100644
> --- a/drivers/md/dm-clone-metadata.c
> +++ b/drivers/md/dm-clone-metadata.c
> @@ -465,11 +465,6 @@ static void __destroy_persistent_data_structures(struct dm_clone_metadata *cmd)
>  
>  /*---------------------------------------------------------------------------*/
>  
> -static size_t bitmap_size(unsigned long nr_bits)
> -{
> -	return BITS_TO_LONGS(nr_bits) * sizeof(long);
> -}
> -
>  static int __dirty_map_init(struct dirty_map *dmap, unsigned long nr_words,
>  			    unsigned long nr_regions)
>  {
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index f091a1664bf1..f66fb98a4126 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -48,6 +48,7 @@ struct device;
>   *  bitmap_equal(src1, src2, nbits)             Are *src1 and *src2 equal?
>   *  bitmap_intersects(src1, src2, nbits)        Do *src1 and *src2 overlap?
>   *  bitmap_subset(src1, src2, nbits)            Is *src1 a subset of *src2?
> + *  bitmap_size(nbits)                          Size, in bytes, of a bitmap
>   *  bitmap_empty(src, nbits)                    Are all bits zero in *src?
>   *  bitmap_full(src, nbits)                     Are all bits set in *src?
>   *  bitmap_weight(src, nbits)                   Hamming Weight: number set bits
> @@ -124,6 +125,11 @@ unsigned long *bitmap_alloc_node(unsigned int nbits, gfp_t flags, int node);
>  unsigned long *bitmap_zalloc_node(unsigned int nbits, gfp_t flags, int node);
>  void bitmap_free(const unsigned long *bitmap);
>  
> +static __always_inline size_t bitmap_size(unsigned long nbits)
> +{
> +	return BITS_TO_LONGS(nbits) * sizeof(unsigned long);
> +}
> +
>  /* Managed variants of the above. */
>  unsigned long *devm_bitmap_alloc(struct device *dev,
>  				 unsigned int nbits, gfp_t flags);
> diff --git a/lib/math/prime_numbers.c b/lib/math/prime_numbers.c
> index d42cebf7407f..d3b64b10da1c 100644
> --- a/lib/math/prime_numbers.c
> +++ b/lib/math/prime_numbers.c
> @@ -6,8 +6,6 @@
>  #include <linux/prime_numbers.h>
>  #include <linux/slab.h>
>  
> -#define bitmap_size(nbits) (BITS_TO_LONGS(nbits) * sizeof(unsigned long))
> -
>  struct primes {
>  	struct rcu_head rcu;
>  	unsigned long last, sz;
> -- 
> 2.34.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] s390/cio: Rename bitmap_size() as idset_bitmap_size()

[Christophe JAILLET]

Le 02/07/2022 à 20:54, Andy Shevchenko a écrit :
> 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.
> 
> ...
> 
>> -		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() ?
> 
> 

Hi,

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.

CJ

Re: [PATCH 1/4] s390/cio: Rename bitmap_size() as idset_bitmap_size()

[Andy Shevchenko]

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?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] fs/ntfs3: Rename bitmap_size() as ntfs3_bitmap_size()

[Christophe JAILLET]

Le 02/07/2022 à 20:58, Andy Shevchenko a écrit :
> On Sat, Jul 02, 2022 at 08:29:27PM +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.
> 
> ...
> 
>>   /* NTFS uses quad aligned bitmaps. */
>> -static inline size_t bitmap_size(size_t bits)
>> +static inline size_t ntfs3_bitmap_size(size_t bits)
>>   {
>>   	return ALIGN((bits + 7) >> 3, 8);
> 
> It would be easier to understand in a way
> 
> 	return BITS_TO_BYTES(ALIGN(bits, 64));

This purpose of the patch was only to rename a function, not to modify 
the code itself, even if both version also looks equivalent to me.

So I'll leave it to you or anyone else to change it.

CJ

> 
>>   }
> 

Re: [PATCH 1/4] s390/cio: Rename bitmap_size() as idset_bitmap_size()

[Christophe JAILLET]

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

Re: [PATCH 0/4] Introduce bitmap_size()

[Yury Norov]

On Sat, Jul 02, 2022 at 08:28:53PM +0200, Christophe JAILLET wrote:
> This serie introduces bitmap_size() which returns the size, in bytes, of a
> bitmap. Such a function is useful to simplify some drivers that use vmalloc() or
> other functions to allocate some butmaps.

This generally looks like a step in a wrong direction. Bitmap is by
definition an array of bits. If someone has a reason to handle bitmap
on a per-byte basis, the guy is probably doing something wrong.

We already have quite comprehensive list of functions that help to
allocate, fill, clear, copy etc bitmap without considering it as an
array of bytes or words.

> ... some drivers that use vmalloc() ...

If a driver needs vmalloc() for a bitmap, we should introduce
bitmap_vmalloc(), not bitmap_size().

> It also hides some implementation details about how bitmaps are stored (array of
> longs)

Sorry, I don't understand that. How bitmap_size() helps to hide a fact that
bitmap is an array of unsigned longs? (Except that it makes an impression
that it's an array of bytes.)

> Before introducing this function in patch 3, patch 1 and 2 rename some functions
> with the same name but with different meaning.
> 
> Finaly, patch 4 makes use of the new function in bitmap.h.

You mentioned, you need bitmap_size() to use with vmalloc(), but in
patch 4, there's no such code. 
 
> Other follow-up patches to simplify some drivers will be proposed later if/when
> this serie is merged.

This series doesn't show an example of how you're going to use new
API, and therefore it's hard to judge, do we really need bitmap_size(),
or we just need more helpers around.

As I already said, bitmaps are evolving in 2nd direction, which is the
right approach, I think.

Thanks,
Yury

> Christophe JAILLET (4):
>   s390/cio: Rename bitmap_size() as idset_bitmap_size()
>   fs/ntfs3: Rename bitmap_size() as ntfs3_bitmap_size()
>   bitmap: Introduce bitmap_size()
>   bitmap: Use bitmap_size()
> 
>  drivers/md/dm-clone-metadata.c |  5 -----
>  drivers/s390/cio/idset.c       |  8 ++++----
>  fs/ntfs3/bitmap.c              |  4 ++--
>  fs/ntfs3/fsntfs.c              |  2 +-
>  fs/ntfs3/index.c               |  6 +++---
>  fs/ntfs3/ntfs_fs.h             |  2 +-
>  fs/ntfs3/super.c               |  2 +-
>  include/linux/bitmap.h         | 15 +++++++++------
>  lib/math/prime_numbers.c       |  2 --
>  9 files changed, 21 insertions(+), 25 deletions(-)
> 
> -- 
> 2.34.1

Re: [PATCH 1/4] s390/cio: Rename bitmap_size() as idset_bitmap_size()

[Yury Norov]

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

Re: [PATCH 2/4] fs/ntfs3: Rename bitmap_size() as ntfs3_bitmap_size()

[Yury Norov]

On Sat, Jul 02, 2022 at 08:29:27PM +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 "ntfs3_" prefix and change bitmap_size() into ntfs3_bitmap_size().
> 
> No functional change.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  fs/ntfs3/bitmap.c  | 4 ++--
>  fs/ntfs3/fsntfs.c  | 2 +-
>  fs/ntfs3/index.c   | 6 +++---
>  fs/ntfs3/ntfs_fs.h | 2 +-
>  fs/ntfs3/super.c   | 2 +-
>  5 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ntfs3/bitmap.c b/fs/ntfs3/bitmap.c
> index e3b5680fd516..f98327453d83 100644
> --- a/fs/ntfs3/bitmap.c
> +++ b/fs/ntfs3/bitmap.c
> @@ -661,7 +661,7 @@ int wnd_init(struct wnd_bitmap *wnd, struct super_block *sb, size_t nbits)
>  	wnd->total_zeroes = nbits;
>  	wnd->extent_max = MINUS_ONE_T;
>  	wnd->zone_bit = wnd->zone_end = 0;
> -	wnd->nwnd = bytes_to_block(sb, bitmap_size(nbits));
> +	wnd->nwnd = bytes_to_block(sb, ntfs3_bitmap_size(nbits));
>  	wnd->bits_last = nbits & (wbits - 1);
>  	if (!wnd->bits_last)
>  		wnd->bits_last = wbits;
> @@ -1323,7 +1323,7 @@ int wnd_extend(struct wnd_bitmap *wnd, size_t new_bits)
>  		return -EINVAL;
>  
>  	/* Align to 8 byte boundary. */
> -	new_wnd = bytes_to_block(sb, bitmap_size(new_bits));
> +	new_wnd = bytes_to_block(sb, ntfs3_bitmap_size(new_bits));
>  	new_last = new_bits & (wbits - 1);
>  	if (!new_last)
>  		new_last = wbits;
> diff --git a/fs/ntfs3/fsntfs.c b/fs/ntfs3/fsntfs.c
> index 3de5700a9b83..9c74d88ce0f0 100644
> --- a/fs/ntfs3/fsntfs.c
> +++ b/fs/ntfs3/fsntfs.c
> @@ -493,7 +493,7 @@ static int ntfs_extend_mft(struct ntfs_sb_info *sbi)
>  	ni->mi.dirty = true;
>  
>  	/* Step 2: Resize $MFT::BITMAP. */
> -	new_bitmap_bytes = bitmap_size(new_mft_total);
> +	new_bitmap_bytes = ntfs3_bitmap_size(new_mft_total);
>  
>  	err = attr_set_size(ni, ATTR_BITMAP, NULL, 0, &sbi->mft.bitmap.run,
>  			    new_bitmap_bytes, &new_bitmap_bytes, true, NULL);
> diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c
> index 84ccc1409874..5c5ea05a5ef1 100644
> --- a/fs/ntfs3/index.c
> +++ b/fs/ntfs3/index.c
> @@ -1353,7 +1353,7 @@ static int indx_create_allocate(struct ntfs_index *indx, struct ntfs_inode *ni,
>  
>  	alloc->nres.valid_size = alloc->nres.data_size = cpu_to_le64(data_size);
>  
> -	err = ni_insert_resident(ni, bitmap_size(1), ATTR_BITMAP, in->name,
> +	err = ni_insert_resident(ni, ntfs3_bitmap_size(1), ATTR_BITMAP, in->name,
>  				 in->name_len, &bitmap, NULL, NULL);
>  	if (err)
>  		goto out2;
> @@ -1415,7 +1415,7 @@ static int indx_add_allocate(struct ntfs_index *indx, struct ntfs_inode *ni,
>  	if (bmp) {
>  		/* Increase bitmap. */
>  		err = attr_set_size(ni, ATTR_BITMAP, in->name, in->name_len,
> -				    &indx->bitmap_run, bitmap_size(bit + 1),
> +				    &indx->bitmap_run, ntfs3_bitmap_size(bit + 1),
>  				    NULL, true, NULL);
>  		if (err)
>  			goto out1;
> @@ -1973,7 +1973,7 @@ static int indx_shrink(struct ntfs_index *indx, struct ntfs_inode *ni,
>  	if (err)
>  		return err;
>  
> -	bpb = bitmap_size(bit);
> +	bpb = ntfs3_bitmap_size(bit);
>  	if (bpb * 8 == nbits)
>  		return 0;
>  
> diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
> index 55d686e3c4ec..85210e610a3a 100644
> --- a/fs/ntfs3/ntfs_fs.h
> +++ b/fs/ntfs3/ntfs_fs.h
> @@ -945,7 +945,7 @@ static inline bool run_is_empty(struct runs_tree *run)
>  }
>  
>  /* NTFS uses quad aligned bitmaps. */
> -static inline size_t bitmap_size(size_t bits)
> +static inline size_t ntfs3_bitmap_size(size_t bits)
>  {
>  	return ALIGN((bits + 7) >> 3, 8);
>  }

Here everything looks OK for me. NTFS3 has their own good reasons
to reserve 64-bit words for their bitmaps, and they need their own
functions for this. And the prefix looks OK because it underlines
that this is a local story.

Maybe we can turn it into BITS_TO_LLONGS() and put into 
include/linux/bitops.h... But unless we have another subsystem that
needs it, I'm OK with current approach.

Acked-by: Yury Norov <yury.norov@gmail.com>

> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> index b41d7c824a50..7d48f886ac82 100644
> --- a/fs/ntfs3/super.c
> +++ b/fs/ntfs3/super.c
> @@ -1101,7 +1101,7 @@ static int ntfs_fill_super(struct super_block *sb, struct fs_context *fc)
>  
>  	/* Check bitmap boundary. */
>  	tt = sbi->used.bitmap.nbits;
> -	if (inode->i_size < bitmap_size(tt)) {
> +	if (inode->i_size < ntfs3_bitmap_size(tt)) {
>  		err = -EINVAL;
>  		goto put_inode_out;
>  	}
> -- 
> 2.34.1

Re: [PATCH 3/4] bitmap: Introduce bitmap_size()

[Yury Norov]

On Sat, Jul 02, 2022 at 08:29:36PM +0200, Christophe JAILLET wrote:
> The new bitmap_size() function returns the size, in bytes, of a bitmap.
> 
> Remove the already existing bitmap_size() functions and macro in some
> files.
> These files already use the bitmap API and will use the new function
> in bitmap.h automatically.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/md/dm-clone-metadata.c | 5 -----
>  include/linux/bitmap.h         | 6 ++++++
>  lib/math/prime_numbers.c       | 2 --
>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/md/dm-clone-metadata.c b/drivers/md/dm-clone-metadata.c
> index c43d55672bce..47c1fa7aad8b 100644
> --- a/drivers/md/dm-clone-metadata.c
> +++ b/drivers/md/dm-clone-metadata.c
> @@ -465,11 +465,6 @@ static void __destroy_persistent_data_structures(struct dm_clone_metadata *cmd)
>  
>  /*---------------------------------------------------------------------------*/
>  
> -static size_t bitmap_size(unsigned long nr_bits)
> -{
> -	return BITS_TO_LONGS(nr_bits) * sizeof(long);
> -}
> -
>  static int __dirty_map_init(struct dirty_map *dmap, unsigned long nr_words,
>  			    unsigned long nr_regions)
>  {
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index f091a1664bf1..f66fb98a4126 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -48,6 +48,7 @@ struct device;
>   *  bitmap_equal(src1, src2, nbits)             Are *src1 and *src2 equal?
>   *  bitmap_intersects(src1, src2, nbits)        Do *src1 and *src2 overlap?
>   *  bitmap_subset(src1, src2, nbits)            Is *src1 a subset of *src2?
> + *  bitmap_size(nbits)                          Size, in bytes, of a bitmap
>   *  bitmap_empty(src, nbits)                    Are all bits zero in *src?
>   *  bitmap_full(src, nbits)                     Are all bits set in *src?
>   *  bitmap_weight(src, nbits)                   Hamming Weight: number set bits
> @@ -124,6 +125,11 @@ unsigned long *bitmap_alloc_node(unsigned int nbits, gfp_t flags, int node);
>  unsigned long *bitmap_zalloc_node(unsigned int nbits, gfp_t flags, int node);
>  void bitmap_free(const unsigned long *bitmap);
>  
> +static __always_inline size_t bitmap_size(unsigned long nbits)
> +{
> +	return BITS_TO_LONGS(nbits) * sizeof(unsigned long);
> +}
> +
>  /* Managed variants of the above. */
>  unsigned long *devm_bitmap_alloc(struct device *dev,
>  				 unsigned int nbits, gfp_t flags);
> diff --git a/lib/math/prime_numbers.c b/lib/math/prime_numbers.c
> index d42cebf7407f..d3b64b10da1c 100644
> --- a/lib/math/prime_numbers.c
> +++ b/lib/math/prime_numbers.c
> @@ -6,8 +6,6 @@
>  #include <linux/prime_numbers.h>
>  #include <linux/slab.h>
>  
> -#define bitmap_size(nbits) (BITS_TO_LONGS(nbits) * sizeof(unsigned long))
> -

This should be dropped, for sure, and kmalloc() at line 128 should be
replaced with bitmap_alloc().

For the driver, we need to introduce bitmap_kvmalloc/bitmap_kvfree etc. 

>  struct primes {
>  	struct rcu_head rcu;
>  	unsigned long last, sz;
> -- 
> 2.34.1

Re: [PATCH 3/4] bitmap: Introduce bitmap_size()

[Christophe JAILLET]

Le 02/07/2022 à 23:09, Yury Norov a écrit :
> On Sat, Jul 02, 2022 at 08:29:36PM +0200, Christophe JAILLET wrote:
>> The new bitmap_size() function returns the size, in bytes, of a bitmap.
>>
>> Remove the already existing bitmap_size() functions and macro in some
>> files.
>> These files already use the bitmap API and will use the new function
>> in bitmap.h automatically.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>>   drivers/md/dm-clone-metadata.c | 5 -----
>>   include/linux/bitmap.h         | 6 ++++++
>>   lib/math/prime_numbers.c       | 2 --
>>   3 files changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/md/dm-clone-metadata.c b/drivers/md/dm-clone-metadata.c
>> index c43d55672bce..47c1fa7aad8b 100644
>> --- a/drivers/md/dm-clone-metadata.c
>> +++ b/drivers/md/dm-clone-metadata.c
>> @@ -465,11 +465,6 @@ static void __destroy_persistent_data_structures(struct dm_clone_metadata *cmd)
>>   
>>   /*---------------------------------------------------------------------------*/
>>   
>> -static size_t bitmap_size(unsigned long nr_bits)
>> -{
>> -	return BITS_TO_LONGS(nr_bits) * sizeof(long);
>> -}
>> -
>>   static int __dirty_map_init(struct dirty_map *dmap, unsigned long nr_words,
>>   			    unsigned long nr_regions)
>>   {
>> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
>> index f091a1664bf1..f66fb98a4126 100644
>> --- a/include/linux/bitmap.h
>> +++ b/include/linux/bitmap.h
>> @@ -48,6 +48,7 @@ struct device;
>>    *  bitmap_equal(src1, src2, nbits)             Are *src1 and *src2 equal?
>>    *  bitmap_intersects(src1, src2, nbits)        Do *src1 and *src2 overlap?
>>    *  bitmap_subset(src1, src2, nbits)            Is *src1 a subset of *src2?
>> + *  bitmap_size(nbits)                          Size, in bytes, of a bitmap
>>    *  bitmap_empty(src, nbits)                    Are all bits zero in *src?
>>    *  bitmap_full(src, nbits)                     Are all bits set in *src?
>>    *  bitmap_weight(src, nbits)                   Hamming Weight: number set bits
>> @@ -124,6 +125,11 @@ unsigned long *bitmap_alloc_node(unsigned int nbits, gfp_t flags, int node);
>>   unsigned long *bitmap_zalloc_node(unsigned int nbits, gfp_t flags, int node);
>>   void bitmap_free(const unsigned long *bitmap);
>>   
>> +static __always_inline size_t bitmap_size(unsigned long nbits)
>> +{
>> +	return BITS_TO_LONGS(nbits) * sizeof(unsigned long);
>> +}
>> +
>>   /* Managed variants of the above. */
>>   unsigned long *devm_bitmap_alloc(struct device *dev,
>>   				 unsigned int nbits, gfp_t flags);
>> diff --git a/lib/math/prime_numbers.c b/lib/math/prime_numbers.c
>> index d42cebf7407f..d3b64b10da1c 100644
>> --- a/lib/math/prime_numbers.c
>> +++ b/lib/math/prime_numbers.c
>> @@ -6,8 +6,6 @@
>>   #include <linux/prime_numbers.h>
>>   #include <linux/slab.h>
>>   
>> -#define bitmap_size(nbits) (BITS_TO_LONGS(nbits) * sizeof(unsigned long))
>> -
> 
> This should be dropped, for sure, and kmalloc() at line 128 should be
> replaced with bitmap_alloc().

This kmalloc() is for a structure and a flexible array.

You mean re-arranging the code to allocate the structure alone at first, 
then the bitmap?

CJ

> 
> For the driver, we need to introduce bitmap_kvmalloc/bitmap_kvfree etc.
> 
>>   struct primes {
>>   	struct rcu_head rcu;
>>   	unsigned long last, sz;
>> -- 
>> 2.34.1
> 

Re: [PATCH 3/4] bitmap: Introduce bitmap_size()

[Andy Shevchenko]

On Sun, Jul 03, 2022 at 08:50:19AM +0200, Christophe JAILLET wrote:
> Le 02/07/2022 à 23:09, Yury Norov a écrit :
> > On Sat, Jul 02, 2022 at 08:29:36PM +0200, Christophe JAILLET wrote:

...

> > This should be dropped, for sure, and kmalloc() at line 128 should be
> > replaced with bitmap_alloc().
> 
> This kmalloc() is for a structure and a flexible array.
> 
> You mean re-arranging the code to allocate the structure alone at first,
> then the bitmap?

It's one way, but it will increase fragmentation of memory. The other one
as it seems to me is to name a new API properly, i.e. bitmap_size_to_bytes().

In such case you won't need renames to begin with. And then would be able
to convert driver-by-driver in cases of duplicated code.

I think that's what confused Yuri and I kinda agree that bitmap_size() should
return bits, and not bytes. Also argument for pure bitmap_size() would be
bitmap itself, but we have no way to detect the length of bitmap because we
are using POD and not a specific data structure for it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] bitmap: Introduce bitmap_size()

[Yury Norov]

On Sun, Jul 03, 2022 at 06:20:53PM +0300, Andy Shevchenko wrote:
> On Sun, Jul 03, 2022 at 08:50:19AM +0200, Christophe JAILLET wrote:
> > Le 02/07/2022 à 23:09, Yury Norov a écrit :
> > > On Sat, Jul 02, 2022 at 08:29:36PM +0200, Christophe JAILLET wrote:
> 
> ...
> 
> > > This should be dropped, for sure, and kmalloc() at line 128 should be
> > > replaced with bitmap_alloc().
> > 
> > This kmalloc() is for a structure and a flexible array.
> > 
> > You mean re-arranging the code to allocate the structure alone at first,
> > then the bitmap?

We can change struct primes to:
        struct primes {
                struct rcu_head rcu;
                unsigned long last, sz;
                unsigned long *primes;
        };

And then either allocate twice:
        new = kmalloc(sizeof(struct primes);
        new->primes = bitmap_alloc(sz);

Or keep the same struct primes for all expansions, and just allocate
new bitmap for ->primes when needed. This is what I meant.

This a bit deeper rework, but it addresses Andy's concern about excessive
fragmentation. (Did anyone before complain? Is it measurable?)

> It's one way, but it will increase fragmentation of memory. The other one
> as it seems to me is to name a new API properly, i.e. bitmap_size_to_bytes().
>
> In such case you won't need renames to begin with. And then would be able
> to convert driver-by-driver in cases of duplicated code.
> 
> I think that's what confused Yuri and I kinda agree that bitmap_size() should
> return bits, and not bytes. Also argument for pure bitmap_size() would be
> bitmap itself, but we have no way to detect the length of bitmap because we
> are using POD and not a specific data structure for it.

bitmap_size_to_bytes() sounds better. How many places in the kernel
do we have where we can't simply use bitmap_alloc(), and need this
machinery? If this is the only one, I'd prefer to switch it to
bitmap_alloc() instead.

Thanks,
Yury

Re: [PATCH 3/4] bitmap: Introduce bitmap_size()

[Christophe JAILLET]

Le 03/07/2022 à 21:13, Yury Norov a écrit :
> On Sun, Jul 03, 2022 at 06:20:53PM +0300, Andy Shevchenko wrote:
>> On Sun, Jul 03, 2022 at 08:50:19AM +0200, Christophe JAILLET wrote:
>>> Le 02/07/2022 à 23:09, Yury Norov a écrit :
>>>> On Sat, Jul 02, 2022 at 08:29:36PM +0200, Christophe JAILLET wrote:
>>
>> ...
>>
>>>> This should be dropped, for sure, and kmalloc() at line 128 should be
>>>> replaced with bitmap_alloc().
>>>
>>> This kmalloc() is for a structure and a flexible array.
>>>
>>> You mean re-arranging the code to allocate the structure alone at first,
>>> then the bitmap?
> 
> We can change struct primes to:
>          struct primes {
>                  struct rcu_head rcu;
>                  unsigned long last, sz;
>                  unsigned long *primes;
>          };
> 
> And then either allocate twice:
>          new = kmalloc(sizeof(struct primes);
>          new->primes = bitmap_alloc(sz);
> 
> Or keep the same struct primes for all expansions, and just allocate
> new bitmap for ->primes when needed. This is what I meant.
> 
> This a bit deeper rework, but it addresses Andy's concern about excessive
> fragmentation. (Did anyone before complain? Is it measurable?)
> 
>> It's one way, but it will increase fragmentation of memory. The other one
>> as it seems to me is to name a new API properly, i.e. bitmap_size_to_bytes().
>>
>> In such case you won't need renames to begin with. And then would be able
>> to convert driver-by-driver in cases of duplicated code.
>>
>> I think that's what confused Yuri and I kinda agree that bitmap_size() should
>> return bits, and not bytes. Also argument for pure bitmap_size() would be
>> bitmap itself, but we have no way to detect the length of bitmap because we
>> are using POD and not a specific data structure for it.
> 
> bitmap_size_to_bytes() sounds better. How many places in the kernel
> do we have where we can't simply use bitmap_alloc(), and need this
> machinery? If this is the only one, I'd prefer to switch it to
> bitmap_alloc() instead.

I'll spot some places that would require a bitmap_size_to_bytes().

This way, we'll have some more information to decide if:
    - bitmap_size_to_bytes() makes sense or not
    - other helper functions are better suited
    - these places need some rework to use the existing API

CJ

> 
> Thanks,
> Yury
> 

Re: [PATCH 1/4] s390/cio: Rename bitmap_size() as idset_bitmap_size()

[Vineeth Vijayan]

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
>