[v3,RESEND,3/6] bitmap: Make bitmap_onto() available to users

  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(-)
  

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
  

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.