On 24/06/2024 21:26, Usama Arif wrote:
On 24/06/2024 20:31, Yosry Ahmed wrote:
On Mon, Jun 24, 2024 at 10:26 AM Usama Arif <usamaarif642@xxxxxxxxx>
wrote:
On 24/06/2024 19:56, Yosry Ahmed wrote:
[..]
- p->zeromap = bitmap_zalloc(maxpages, GFP_KERNEL);
+ p->zeromap = kvzalloc(DIV_ROUND_UP(maxpages, 8),
GFP_KERNEL);
No, 8 is not right for 32-bit kernels. I think you want
p->zeromap = kvzalloc(BITS_TO_LONGS(maxpages), GFP_KERNEL);
but please check it carefully, I'm easily confused by such
conversions.
Hugh
Ah yes, didnt take into account 32-bit kernel. I think its
supposed to be
p->zeromap = kvzalloc(BITS_TO_LONGS(maxpages) *
sizeof(unsigned long),
GFP_KERNEL);
You can do something similar to bitmap_zalloc() and use:
kvmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long), GFP_KERNEL
| __GFP_ZERO)
I don't see a kvzalloc_array() variant to use directly, but it should
be trivial to add it. I can see other users of kvmalloc_array() that
pass in __GFP_ZERO (e.g. fs/ntfs3/bitmap.c).
, or you could take it a step further and add bitmap_kvzalloc(),
assuming the maintainers are open to that.
Thanks! bitmap_kvzalloc makes most sense to me. It doesnt make sense
that bitmap should only be limited to MAX_PAGE_ORDER size. I can add
this patch below at the start of the series and use it in the patch for
zeropage swap optimization.
bitmap: add support for virtually contiguous bitmap
The current bitmap_zalloc API limits the allocation to
MAX_PAGE_ORDER,
which prevents larger order bitmap allocations. Introduce
bitmap_kvzalloc that will allow larger allocations of bitmap.
kvmalloc_array still attempts to allocate physically
contiguous memory,
but upon failure, falls back to non-contiguous (vmalloc)
allocation.
Suggested-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx>
Signed-off-by: Usama Arif <usamaarif642@xxxxxxxxx>
LGTM with a small fix below.
diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 8c4768c44a01..881c2ff2e834 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -131,9 +131,11 @@ struct device;
*/
unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags);
unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags);
+unsigned long *bitmap_kvzalloc(unsigned int nbits, gfp_t flags);
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);
+void bitmap_kvfree(const unsigned long *bitmap);
DEFINE_FREE(bitmap, unsigned long *, if (_T) bitmap_free(_T))
diff --git a/lib/bitmap.c b/lib/bitmap.c
index b97692854966..eabbfb85fb45 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -727,6 +727,13 @@ unsigned long *bitmap_zalloc(unsigned int nbits,
gfp_t flags)
}
EXPORT_SYMBOL(bitmap_zalloc);
+unsigned long *bitmap_kvzalloc(unsigned int nbits, gfp_t flags)
+{
+ return kvmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned
long),
+ flags | __GFP_ZERO);
+}
+EXPORT_SYMBOL(bitmap_zalloc);
EXPORT_SYMBOL(bitmap_kvzalloc)*
Actually, does it make more sense to change the behaviour of the
current APIs like below instead of above patch? Or is there an
expectation that the current bitmap API is supposed to work only on
physically contiguous bits?
I believe in the kernel if the allocation/free starts with 'k' its
physically contiguous and with "kv" its physically contiguous if
possible, otherwise virtually contiguous. The bitmap functions dont
have either, so we could change the current implementation. I believe
it would not impact the current users of the functions as the first
attempt is physically contiguous which is how it works currently, and
only upon failure it would be virtual and it would increase the use of
current bitmap API to greater than MAX_PAGE_ORDER size allocations.
Yury Norov and Rasmus Villemoes, any views on this?
Thanks
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 7247e217e21b..ad771dc81afa 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -804,6 +804,7 @@ kvmalloc_array_node_noprof(size_t n, size_t size,
gfp_t flags, int node)
#define kvcalloc_node_noprof(_n,_s,_f,_node)
kvmalloc_array_node_noprof(_n,_s,(_f)|__GFP_ZERO,_node)
#define kvcalloc_noprof(...) kvcalloc_node_noprof(__VA_ARGS__,
NUMA_NO_NODE)
+#define kvmalloc_array_node(...)
alloc_hooks(kvmalloc_array_node_noprof(__VA_ARGS__))
#define kvmalloc_array(...)
alloc_hooks(kvmalloc_array_noprof(__VA_ARGS__))
#define kvcalloc_node(...)
alloc_hooks(kvcalloc_node_noprof(__VA_ARGS__))
#define kvcalloc(...) alloc_hooks(kvcalloc_noprof(__VA_ARGS__))
diff --git a/lib/bitmap.c b/lib/bitmap.c
index b97692854966..272164dcbef1 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -716,7 +716,7 @@ void bitmap_fold(unsigned long *dst, const
unsigned long *orig,
unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
{
- return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long),
+ return kvmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned
long),
flags);
}
EXPORT_SYMBOL(bitmap_alloc);
@@ -729,7 +729,7 @@ EXPORT_SYMBOL(bitmap_zalloc);
unsigned long *bitmap_alloc_node(unsigned int nbits, gfp_t flags, int
node)
{
- return kmalloc_array_node(BITS_TO_LONGS(nbits),
sizeof(unsigned long),
+ return kvmalloc_array_node(BITS_TO_LONGS(nbits),
sizeof(unsigned long),
flags, node);
}
EXPORT_SYMBOL(bitmap_alloc_node);
@@ -742,7 +742,7 @@ EXPORT_SYMBOL(bitmap_zalloc_node);
void bitmap_free(const unsigned long *bitmap)
{
- kfree(bitmap);
+ kvfree(bitmap);
}
EXPORT_SYMBOL(bitmap_free);
I decided to go with just using simple kvmalloc_array for v7 [1] with
__GFP_ZERO instead of adding a new API to bitmap or changing the
existing API to kvmalloc/kvfree as I didnt want to make this series
dependent of bitmap API changes and there are other places where its
done using kvmalloc_array like ceph and ntfs3. I am happy to send a
follow up patch after this series that changes the existing API to be kv
if thats something the bitmap maintainers think makes sense.
[1]
https://lore.kernel.org/all/20240627105730.3110705-1-usamaarif642@xxxxxxxxx/
