Re: [dm-devel] [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
On Fri, 2018-06-22 at 11:46 -0700, Dmitry Torokhov wrote: > On Thu, Jun 21, 2018 at 05:13:39AM +0300, Andy Shevchenko wrote: > > On Tue, Jun 19, 2018 at 2:10 AM, Andrew Morton > > wrote: > > > On Mon, 18 Jun 2018 15:01:43 -0700 Dmitry Torokhov > > v...@gmail.com> wrote: > > > > We can't as we end up including bitmap.h (by the way of > > > > cpumask.h) > > > > form slab.h, so we gen circular dependency. > > > > It's not just so easy. See below. > > > > > That info should have been in the changelog, and probably a code > > > comment. > > > > > > > Maybe if we removed memcg > > > > stuff from slab.h so we do not need to include workqueue.h... > > > > > > Or move the basic slab API stuff out of slab.h into a new > > > header. Or > > > create a new, standalone work_struct.h - that looks pretty simple. > > > > I tried to move out work_struct, it didn't help. There are actually > > several circular dependencies that ends in bitmap.h either way or > > another. > > > > First one is > > > > slab.h -> gfp.h -> mmzone.h -> nodemask.h -> bitmap.h > > > > And so on... > > > > Splitting out kXalloc stuff to a separate header won't help, I > > think, > > because of the above. > > Splitting out struct work_struct is just a tip of an iceberg. > > Splitting out memcg stuff won't help in the similar way. > > > > I'm all ears for (a better) solution. > > I think ultimately we'd want to untangle this, but allocating bitmaps > is > not in any hot paths so having them as non-inlined functions should > not > hurt us that much for time being. Perhaps I can elaborate a bit in a commit message. Thanks for review! -- Andy Shevchenko Intel Finland Oy -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
On Thu, Jun 21, 2018 at 05:13:39AM +0300, Andy Shevchenko wrote: > On Tue, Jun 19, 2018 at 2:10 AM, Andrew Morton > wrote: > > On Mon, 18 Jun 2018 15:01:43 -0700 Dmitry Torokhov > > wrote: > > >> We can't as we end up including bitmap.h (by the way of cpumask.h) > >> form slab.h, so we gen circular dependency. > > > > It's not just so easy. See below. > > > That info should have been in the changelog, and probably a code > > comment. > > > >> Maybe if we removed memcg > >> stuff from slab.h so we do not need to include workqueue.h... > > > > Or move the basic slab API stuff out of slab.h into a new header. Or > > create a new, standalone work_struct.h - that looks pretty simple. > > I tried to move out work_struct, it didn't help. There are actually > several circular dependencies that ends in bitmap.h either way or > another. > > First one is > > slab.h -> gfp.h -> mmzone.h -> nodemask.h -> bitmap.h > > And so on... > > Splitting out kXalloc stuff to a separate header won't help, I think, > because of the above. > Splitting out struct work_struct is just a tip of an iceberg. > Splitting out memcg stuff won't help in the similar way. > > I'm all ears for (a better) solution. I think ultimately we'd want to untangle this, but allocating bitmaps is not in any hot paths so having them as non-inlined functions should not hurt us that much for time being. Just my 2 cents... -- Dmitry -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
On Tue, Jun 19, 2018 at 2:10 AM, Andrew Morton wrote: > On Mon, 18 Jun 2018 15:01:43 -0700 Dmitry Torokhov > wrote: >> We can't as we end up including bitmap.h (by the way of cpumask.h) >> form slab.h, so we gen circular dependency. > It's not just so easy. See below. > That info should have been in the changelog, and probably a code > comment. > >> Maybe if we removed memcg >> stuff from slab.h so we do not need to include workqueue.h... > > Or move the basic slab API stuff out of slab.h into a new header. Or > create a new, standalone work_struct.h - that looks pretty simple. I tried to move out work_struct, it didn't help. There are actually several circular dependencies that ends in bitmap.h either way or another. First one is slab.h -> gfp.h -> mmzone.h -> nodemask.h -> bitmap.h And so on... Splitting out kXalloc stuff to a separate header won't help, I think, because of the above. Splitting out struct work_struct is just a tip of an iceberg. Splitting out memcg stuff won't help in the similar way. I'm all ears for (a better) solution. -- With Best Regards, Andy Shevchenko -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
A lot of code become ugly because of open coding allocations for bitmaps.
Introduce three helpers to allow users be more clear of intention
and keep their code neat.
Signed-off-by: Andy Shevchenko
---
include/linux/bitmap.h | 8
lib/bitmap.c | 19 +++
2 files changed, 27 insertions(+)
diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 1ee46f492267..acf5e8df3504 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -104,6 +104,14 @@
* contain all bit positions from 0 to 'bits' - 1.
*/
+/*
+ * Allocation and deallocation of bitmap.
+ * Provided in lib/bitmap.c to avoid circular dependency.
+ */
+extern unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags);
+extern unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags);
+extern void bitmap_free(const unsigned long *bitmap);
+
/*
* lib/bitmap.c provides these functions:
*/
diff --git a/lib/bitmap.c b/lib/bitmap.c
index 33e95cd359a2..09acf2fd6a35 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
#include
#include
@@ -1125,6 +1126,24 @@ void bitmap_copy_le(unsigned long *dst, const unsigned
long *src, unsigned int n
EXPORT_SYMBOL(bitmap_copy_le);
#endif
+unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
+{
+ return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long),
flags);
+}
+EXPORT_SYMBOL(bitmap_alloc);
+
+unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
+{
+ return bitmap_alloc(nbits, flags | __GFP_ZERO);
+}
+EXPORT_SYMBOL(bitmap_zalloc);
+
+void bitmap_free(const unsigned long *bitmap)
+{
+ kfree(bitmap);
+}
+EXPORT_SYMBOL(bitmap_free);
+
#if BITS_PER_LONG == 64
/**
* bitmap_from_arr32 - copy the contents of u32 array of bits to bitmap
--
2.17.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
On Mon, Jun 18, 2018 at 2:14 PM Andrew Morton wrote:
>
> On Mon, 18 Jun 2018 16:10:01 +0300 Andy Shevchenko
> wrote:
>
> > A lot of code become ugly because of open coding allocations for bitmaps.
> >
> > Introduce three helpers to allow users be more clear of intention
> > and keep their code neat.
> >
> > ...
> >
> > +unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
> > +{
> > + return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long),
> > flags);
> > +}
> > +EXPORT_SYMBOL(bitmap_alloc);
> > +
> > +unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
> > +{
> > + return bitmap_alloc(nbits, flags | __GFP_ZERO);
> > +}
> > +EXPORT_SYMBOL(bitmap_zalloc);
> > +
> > +void bitmap_free(const unsigned long *bitmap)
> > +{
> > + kfree(bitmap);
> > +}
> > +EXPORT_SYMBOL(bitmap_free);
> > +
>
> I suggest these functions are small and simple enough to justify
> inlining them.
>
We can't as we end up including bitmap.h (by the way of cpumask.h)
form slab.h, so we gen circular dependency. Maybe if we removed memcg
stuff from slab.h so we do not need to include workqueue.h...
--
Dmitry
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
On Tue, Jun 19, 2018 at 2:10 AM, Andrew Morton
wrote:
> On Mon, 18 Jun 2018 15:01:43 -0700 Dmitry Torokhov
> wrote:
>
>> > > +unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
>> > > +{
>> > > + return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long),
>> > > flags);
>> > > +}
>> > > +EXPORT_SYMBOL(bitmap_alloc);
>> > > +
>> > > +unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
>> > > +{
>> > > + return bitmap_alloc(nbits, flags | __GFP_ZERO);
>> > > +}
>> > > +EXPORT_SYMBOL(bitmap_zalloc);
>> > > +
>> > > +void bitmap_free(const unsigned long *bitmap)
>> > > +{
>> > > + kfree(bitmap);
>> > > +}
>> > > +EXPORT_SYMBOL(bitmap_free);
>> > > +
>> >
>> > I suggest these functions are small and simple enough to justify
>> > inlining them.
>> >
>>
>> We can't as we end up including bitmap.h (by the way of cpumask.h)
>> form slab.h, so we gen circular dependency.
>
> That info should have been in the changelog,
I put it in cover letter, though it perhaps better to have in commit
message itself.
> and probably a code
> comment.
This is done in header file. You meant C-file?
>> Maybe if we removed memcg
>> stuff from slab.h so we do not need to include workqueue.h...
>
> Or move the basic slab API stuff out of slab.h into a new header. Or
> create a new, standalone work_struct.h - that looks pretty simple.
Latter one seems requires least effort without potentially breaking things.
I take it as your suggestion, though I would still give a glance if it
is possible to split exactly memcg part out of slab.
--
With Best Regards,
Andy Shevchenko
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
On Tue, Jun 19, 2018 at 1:01 AM, Dmitry Torokhov
wrote:
> On Mon, Jun 18, 2018 at 2:14 PM Andrew Morton
> wrote:
>>
>> On Mon, 18 Jun 2018 16:10:01 +0300 Andy Shevchenko
>> wrote:
>>
>> > A lot of code become ugly because of open coding allocations for bitmaps.
>> >
>> > Introduce three helpers to allow users be more clear of intention
>> > and keep their code neat.
>> >
>> > ...
>> >
>> > +unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
>> > +{
>> > + return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long),
>> > flags);
>> > +}
>> > +EXPORT_SYMBOL(bitmap_alloc);
>> > +
>> > +unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
>> > +{
>> > + return bitmap_alloc(nbits, flags | __GFP_ZERO);
>> > +}
>> > +EXPORT_SYMBOL(bitmap_zalloc);
>> > +
>> > +void bitmap_free(const unsigned long *bitmap)
>> > +{
>> > + kfree(bitmap);
>> > +}
>> > +EXPORT_SYMBOL(bitmap_free);
>> > +
>>
>> I suggest these functions are small and simple enough to justify
>> inlining them.
>>
>
> We can't as we end up including bitmap.h (by the way of cpumask.h)
> form slab.h, so we gen circular dependency. Maybe if we removed memcg
> stuff from slab.h so we do not need to include workqueue.h...
I will look at it. It might be doable.
Though I dunno what MM people would say about this. Anyone's name
comes to your mind to ask?
--
With Best Regards,
Andy Shevchenko
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
On Mon, 18 Jun 2018 15:01:43 -0700 Dmitry Torokhov
wrote:
> > > +unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
> > > +{
> > > + return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long),
> > > flags);
> > > +}
> > > +EXPORT_SYMBOL(bitmap_alloc);
> > > +
> > > +unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
> > > +{
> > > + return bitmap_alloc(nbits, flags | __GFP_ZERO);
> > > +}
> > > +EXPORT_SYMBOL(bitmap_zalloc);
> > > +
> > > +void bitmap_free(const unsigned long *bitmap)
> > > +{
> > > + kfree(bitmap);
> > > +}
> > > +EXPORT_SYMBOL(bitmap_free);
> > > +
> >
> > I suggest these functions are small and simple enough to justify
> > inlining them.
> >
>
> We can't as we end up including bitmap.h (by the way of cpumask.h)
> form slab.h, so we gen circular dependency.
That info should have been in the changelog, and probably a code
comment.
> Maybe if we removed memcg
> stuff from slab.h so we do not need to include workqueue.h...
Or move the basic slab API stuff out of slab.h into a new header. Or
create a new, standalone work_struct.h - that looks pretty simple.
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
On Mon, 18 Jun 2018 16:10:01 +0300 Andy Shevchenko
wrote:
> A lot of code become ugly because of open coding allocations for bitmaps.
>
> Introduce three helpers to allow users be more clear of intention
> and keep their code neat.
>
> ...
>
> +unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
> +{
> + return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long),
> flags);
> +}
> +EXPORT_SYMBOL(bitmap_alloc);
> +
> +unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
> +{
> + return bitmap_alloc(nbits, flags | __GFP_ZERO);
> +}
> +EXPORT_SYMBOL(bitmap_zalloc);
> +
> +void bitmap_free(const unsigned long *bitmap)
> +{
> + kfree(bitmap);
> +}
> +EXPORT_SYMBOL(bitmap_free);
> +
I suggest these functions are small and simple enough to justify
inlining them.
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
