| Message ID | 98f5d3d855a9c687ccc035edf62016b02a6876b7.1656785856.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | Introduce bitmap_size() | expand |
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 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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.
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 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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;
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(-)