| Message ID | 20240212075646.19114-4-herve.codina@bootlin.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Add support for QMC HDLC | expand |
On Mon, Feb 12, 2024 at 08:56:31AM +0100, Herve Codina wrote: > Currently the bitmap_onto() is available only for CONFIG_NUMA=y case, > while some users may benefit out of it and being independent to NUMA > code. > > Make it available to users by moving out of ifdeffery and exporting for > modules. Wondering if you are trying to have something like https://lore.kernel.org/lkml/20230926052007.3917389-1-andriy.shevchenko@linux.intel.com/
Hi Andy, On Mon, 12 Feb 2024 14:27:16 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Mon, Feb 12, 2024 at 08:56:31AM +0100, Herve Codina wrote: > > Currently the bitmap_onto() is available only for CONFIG_NUMA=y case, > > while some users may benefit out of it and being independent to NUMA > > code. > > > > Make it available to users by moving out of ifdeffery and exporting for > > modules. > > Wondering if you are trying to have something like > https://lore.kernel.org/lkml/20230926052007.3917389-1-andriy.shevchenko@linux.intel.com/ > Yes, it looks like. Can you confirm that your bitmap_scatter() do the same operations as the existing bitmap_onto() ? If so, your bitmap_gather() will match my bitmap_off() (patch 4 in this series). Thanks, Hervé
On Mon, Feb 12, 2024 at 02:37:53PM +0100, Herve Codina wrote: > On Mon, 12 Feb 2024 14:27:16 +0200 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Feb 12, 2024 at 08:56:31AM +0100, Herve Codina wrote: > > > Currently the bitmap_onto() is available only for CONFIG_NUMA=y case, > > > while some users may benefit out of it and being independent to NUMA > > > code. > > > > > > Make it available to users by moving out of ifdeffery and exporting for > > > modules. > > > > Wondering if you are trying to have something like > > https://lore.kernel.org/lkml/20230926052007.3917389-1-andriy.shevchenko@linux.intel.com/ > > Yes, it looks like. > Can you confirm that your bitmap_scatter() do the same operations as the > existing bitmap_onto() ? I have test cases to be 100% sure, but on the first glance, yes it does with the adjustment to the atomicity of the operations (which I do not understand why be atomic in the original bitmap_onto() implementation). This actually gives a question if we should use your approach or mine. At least the help of bitmap_onto() is kinda hard to understand. > If so, your bitmap_gather() will match my bitmap_off() (patch 4 in this > series). Yes.
On Mon, 12 Feb 2024 16:01:38 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Mon, Feb 12, 2024 at 02:37:53PM +0100, Herve Codina wrote: > > On Mon, 12 Feb 2024 14:27:16 +0200 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > On Mon, Feb 12, 2024 at 08:56:31AM +0100, Herve Codina wrote: > > > > Currently the bitmap_onto() is available only for CONFIG_NUMA=y case, > > > > while some users may benefit out of it and being independent to NUMA > > > > code. > > > > > > > > Make it available to users by moving out of ifdeffery and exporting for > > > > modules. > > > > > > Wondering if you are trying to have something like > > > https://lore.kernel.org/lkml/20230926052007.3917389-1-andriy.shevchenko@linux.intel.com/ > > > > Yes, it looks like. > > Can you confirm that your bitmap_scatter() do the same operations as the > > existing bitmap_onto() ? > > I have test cases to be 100% sure, but on the first glance, yes it does with > the adjustment to the atomicity of the operations (which I do not understand > why be atomic in the original bitmap_onto() implementation). > > This actually gives a question if we should use your approach or mine. > At least the help of bitmap_onto() is kinda hard to understand. Agree, the bitmap_onto() code is simpler to understand than its help. I introduced bitmap_off() to be the "reverse" bitmap_onto() operations and I preferred to avoid duplicating function that do the same things. On my side, I initially didn't use the bitmap_*() functions and did the the bits manipulation by hand. During the review, it was suggested to use the bitmap_*() family and I followed this suggestion. I did tests to be sure that bitmap_onto() and bitmap_off() did exactly the same things as my previous code did. > > > If so, your bitmap_gather() will match my bitmap_off() (patch 4 in this > > series). > > Yes. > Regards, Hervé
On Mon, Feb 12, 2024 at 03:20:22PM +0100, Herve Codina wrote: > On Mon, 12 Feb 2024 16:01:38 +0200 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: ... > Agree, the bitmap_onto() code is simpler to understand than its help. > > I introduced bitmap_off() to be the "reverse" bitmap_onto() operations > and I preferred to avoid duplicating function that do the same things. > > On my side, I initially didn't use the bitmap_*() functions and did the the > bits manipulation by hand. > During the review, it was suggested to use the bitmap_*() family and I followed > this suggestion. I also would go this way, the problems I see with the current implementation are: - being related to NUMA (and as Rasmus once pointed out better to be there); - unclear naming, esp. proposed bitmap_off(); - the quite hard to understand help text - atomicity when it's not needed (AFAICT). > I did tests to be sure that bitmap_onto() and bitmap_off() did > exactly the same things as my previous code did. Yuri, what do you think about all this?
On Mon, Feb 12, 2024 at 04:36:36PM +0200, Andy Shevchenko wrote: > On Mon, Feb 12, 2024 at 03:20:22PM +0100, Herve Codina wrote: > > On Mon, 12 Feb 2024 16:01:38 +0200 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > ... > > > Agree, the bitmap_onto() code is simpler to understand than its help. > > > > I introduced bitmap_off() to be the "reverse" bitmap_onto() operations > > and I preferred to avoid duplicating function that do the same things. > > > > On my side, I initially didn't use the bitmap_*() functions and did the the > > bits manipulation by hand. > > During the review, it was suggested to use the bitmap_*() family and I followed > > this suggestion. > > I also would go this way, the problems I see with the current implementation are: Sure, opencoding and duplicating the functionality is always a bad idea. > - being related to NUMA (and as Rasmus once pointed out better to be there); It's 'related to NUMA' for the only reason - it's used by NUMA only. Nothing NUMA-specific in the function itself. Now that we've got a non-NUMA user, the bitmap_onto() is not related to NUMA anymore. > - unclear naming, esp. proposed bitmap_off(); That's I agree. Scatter/gather from your last approach sound better. Do you plan to send a v2? > - the quite hard to understand help text Yes, we need a picture that would illustrate what actually happens > - atomicity when it's not needed (AFAICT). Agree. A series of atomic ops is not atomic. For example if (test_bit(n, map)) set_bit(m, map); is not atomic as a whole. And this is what we do in bitmap_onto/off() in a loop. This must be fixed by using underscoded version. > > I did tests to be sure that bitmap_onto() and bitmap_off() did > > exactly the same things as my previous code did. > > Yuri, what do you think about all this? I think your scatter/gather is better then this onto/off by naming and implementation. If you'll send a v2, and it would work for Herve, I'd prefer scatter/gather. But we can live with onto/off as well. Thanks, Yury
Hi Andy, Yury, On Mon, 12 Feb 2024 11:13:13 -0800 Yury Norov <yury.norov@gmail.com> wrote: ... > > That's I agree. Scatter/gather from your last approach sound better. > Do you plan to send a v2? > ... > > I think your scatter/gather is better then this onto/off by naming and > implementation. If you'll send a v2, and it would work for Herve, I'd > prefer scatter/gather. But we can live with onto/off as well. > Andy, I tested your bitmap_{scatter,gather}() in my code. I simply replaced my bitmap_{onto,off}() calls by calls to your helpers and it works perfectly for my use case. I didn't use your whole patch "[PATCH v1 2/5] lib/bitmap: Introduce bitmap_scatter() and bitmap_gather() helpers" because it didn't apply on a v6.8-rc1 based branch. I just manually extracted the needed functions for my tests and I didn't look at the lib/test_bitmap.c part. Now what's the plan ? Andy, do you want to send a v2 of this patch or may I get the patch, modify it according to reviews already present in v1 and integrate it in my current series ? Yury, any preferences ? Best regards, Hervé
On Thu, Feb 15, 2024 at 06:46:12PM +0100, Herve Codina wrote: > On Mon, 12 Feb 2024 11:13:13 -0800 > Yury Norov <yury.norov@gmail.com> wrote: ... > > That's I agree. Scatter/gather from your last approach sound better. > > Do you plan to send a v2? See below. ... > > I think your scatter/gather is better then this onto/off by naming and > > implementation. If you'll send a v2, and it would work for Herve, I'd > > prefer scatter/gather. But we can live with onto/off as well. > > Andy, I tested your bitmap_{scatter,gather}() in my code. > I simply replaced my bitmap_{onto,off}() calls by calls to your helpers and > it works perfectly for my use case. > > I didn't use your whole patch > "[PATCH v1 2/5] lib/bitmap: Introduce bitmap_scatter() and bitmap_gather() helpers" > because it didn't apply on a v6.8-rc1 based branch. > I just manually extracted the needed functions for my tests and I didn't look > at the lib/test_bitmap.c part. > > Now what's the plan ? > Andy, do you want to send a v2 of this patch or may I get the patch, modify it > according to reviews already present in v1 and integrate it in my current > series ? I would like to do that, but under pile of different things. I would try my best but if you have enough time and motivation feel free to take over, address the comments and integrate in your series. I dunno what to do with bitmap_onto(), perhaps in a separate patch we can replace it with bitmap_scatter() (IIUC) with explanation that the former 1) uses atomic ops while being non-atomic as a whole, and b) having quite hard to get documentation. At least that's how I see it, I mean that I would like to leave bitmap_onto() alone and address it separately. > Yury, any preferences ?
Hi Andy, Yury, On Thu, 15 Feb 2024 21:17:23 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: [...] > > Now what's the plan ? > > Andy, do you want to send a v2 of this patch or may I get the patch, modify it > > according to reviews already present in v1 and integrate it in my current > > series ? > > I would like to do that, but under pile of different things. > I would try my best but if you have enough time and motivation feel free > to take over, address the comments and integrate in your series. > > I dunno what to do with bitmap_onto(), perhaps in a separate patch we can > replace it with bitmap_scatter() (IIUC) with explanation that the former > 1) uses atomic ops while being non-atomic as a whole, and b) having quite > hard to get documentation. At least that's how I see it, I mean that I would > like to leave bitmap_onto() alone and address it separately. > I will take the Andy's bitmap_{scatter,gather}() patch in my next iteration. And use bitmap_{scatter,gather}() in my code. For bitmap_onto() replacement, nothing will be done in my next iteration as it is out of this series scope. Hervé
On Wed, Feb 21, 2024 at 02:44:31PM +0100, Herve Codina wrote: > On Thu, 15 Feb 2024 21:17:23 +0200 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: [...] > > > Now what's the plan ? > > > Andy, do you want to send a v2 of this patch or may I get the patch, modify it > > > according to reviews already present in v1 and integrate it in my current > > > series ? > > > > I would like to do that, but under pile of different things. > > I would try my best but if you have enough time and motivation feel free > > to take over, address the comments and integrate in your series. > > > > I dunno what to do with bitmap_onto(), perhaps in a separate patch we can > > replace it with bitmap_scatter() (IIUC) with explanation that the former > > 1) uses atomic ops while being non-atomic as a whole, and b) having quite > > hard to get documentation. At least that's how I see it, I mean that I would > > like to leave bitmap_onto() alone and address it separately. > > I will take the Andy's bitmap_{scatter,gather}() patch in my next iteration. > And use bitmap_{scatter,gather}() in my code. Thank you and sorry that I have no time to finish that. I will be happy to help reviewing if you Cc me. > For bitmap_onto() replacement, nothing will be done in my next iteration as > it is out of this series scope. I agree on this. This will be a separate logical change related to NUMA with explanation and replacement of all callers at once.
diff --git a/lib/bitmap.c b/lib/bitmap.c index 09522af227f1..2feccb5047dc 100644 --- a/lib/bitmap.c +++ b/lib/bitmap.c @@ -547,7 +547,6 @@ int bitmap_bitremap(int oldbit, const unsigned long *old, } EXPORT_SYMBOL(bitmap_bitremap); -#ifdef CONFIG_NUMA /** * bitmap_onto - translate one bitmap relative to another * @dst: resulting translated bitmap @@ -681,7 +680,9 @@ void bitmap_onto(unsigned long *dst, const unsigned long *orig, m++; } } +EXPORT_SYMBOL(bitmap_onto); +#ifdef CONFIG_NUMA /** * bitmap_fold - fold larger bitmap into smaller, modulo specified size * @dst: resulting smaller bitmap
Currently the bitmap_onto() is available only for CONFIG_NUMA=y case, while some users may benefit out of it and being independent to NUMA code. Make it available to users by moving out of ifdeffery and exporting for modules. Signed-off-by: Herve Codina <herve.codina@bootlin.com> --- lib/bitmap.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)