| Message ID | 20180618131003.88110-4-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
On Mon, Jun 18, 2018 at 2:14 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Mon, 18 Jun 2018 16:10:01 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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...
On Tue, Jun 19, 2018 at 1:01 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Mon, Jun 18, 2018 at 2:14 PM Andrew Morton <akpm@linux-foundation.org> wrote: >> >> On Mon, 18 Jun 2018 16:10:01 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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?
On Tue, Jun 19, 2018 at 2:10 AM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Mon, 18 Jun 2018 15:01:43 -0700 Dmitry Torokhov <dmitry.torokhov@gmail.com> 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.
On Tue, Jun 19, 2018 at 2:10 AM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Mon, 18 Jun 2018 15:01:43 -0700 Dmitry Torokhov <dmitry.torokhov@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.
On Thu, Jun 21, 2018 at 05:13:39AM +0300, Andy Shevchenko wrote: > On Tue, Jun 19, 2018 at 2:10 AM, Andrew Morton > <akpm@linux-foundation.org> wrote: > > On Mon, 18 Jun 2018 15:01:43 -0700 Dmitry Torokhov <dmitry.torokhov@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. Just my 2 cents...
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 > > <akpm@linux-foundation.org> wrote: > > > On Mon, 18 Jun 2018 15:01:43 -0700 Dmitry Torokhov <dmitry.torokho > > > 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!
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 <linux/bitops.h> #include <linux/bug.h> #include <linux/kernel.h> +#include <linux/slab.h> #include <linux/string.h> #include <linux/uaccess.h> @@ -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
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 <andriy.shevchenko@linux.intel.com> --- include/linux/bitmap.h | 8 ++++++++ lib/bitmap.c | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+)