Message-ID: <ZRpBwFznhl+pci/a@yury-ThinkPad>
Date:   Sun, 1 Oct 2023 21:06:24 -0700
From:   Yury Norov <yury.norov@...il.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
        linux-gpio@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org,
        Shubhrajyoti Datta <shubhrajyoti.datta@....com>,
        Srinivas Neeli <srinivas.neeli@....com>,
        Michal Simek <michal.simek@....com>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Marek Behún <kabel@...nel.org>
Subject: Re: [PATCH v1 2/5] lib/bitmap: Introduce bitmap_scatter() and
 bitmap_gather() helpers

On Wed, Sep 27, 2023 at 03:02:34PM +0300, Andy Shevchenko wrote:

[...]

> > It looks like those are designed complement to each other. Is that
> > true? If so, can you make your example showing that
> >         scatter -> gather -> scatter
> > would restore the original bitmap?
> 
> It looks like you stopped reading documentation somewhere on the middle.

What a wonderful week of strong statements... Whatever...

> The two APIs are documented with the same example which makes it clear
> that they are data-loss transformations.
> 
> Do you need something like this to be added (in both documentations):
> 
>   The bitmap_scatter(), when executed over the @dst bitmap, will
>   restore the @src one if the @mask is kept the same, see the example
>   in the function description.
> 
> ?
> 
> > If I'm wrong, can you please underline that they are not complement,
> > and why?
> 
> No, you are not.

I should be confused even more. You're saying that I'm not wrong here, and few
lines above you're saying it's a data loss...

I don't mind this new 3-liners, but I'd like you to have a well better
wording and testing around them because those bitmap_scatter/gather are
all about performance, readability and usability.

To begin with, the whole name of the series: "get rid of bitmap_remap() and
bitmap_biremap() uses" is wrong because the functions are still here, and are
still used.

Even worse, instead of getting rid of some useless code, you're
bloating the kernel with something that duplicates existing
functionality.

This is an example of a series that 'gets rid of' something for true:

https://yhbt.net/lore/all/20230925023817.782509-7-yury.norov@gmail.com/T/

(And unfortunately it's still unreviewed.)

But I understand your motivation, and as I already said, I like this
series in general. So let's please figure out a better wording before
moving forward?

Below are some my of thought.

1. Stop saying that you're getting rid of something when you're not.
   I'd suggest something like: "add simple and verbose alternatives to
   bitmap_remap(), and use them where appropriate".

2. Don't blame people considering a parameter named 'mask' as a mask.
   I mean this sentence: 

   > You should get the mask meaning. It's not the bit provider, it's a bit
   > positions provider.

   If it's a 'bit position provider', please give it a proper name,
   for example 'pos'. I'd suggest something like:
        unsigned long bitmap_scatter(unsigned long *dst, unsigned long *pos,
                                                          unsigned long *val)

3. If you're saying that new API is a simplified replacement for
   something, I'd like to see the full contract, i.e. explicit list of all
   simplifications and limitations implied:
   - val == dst is not handled;
   - when 'pos' is empty, val is not copied to dst;
   - new API doesn't wrap around 0, like bitmap_remap() does;
   - set bits in 'val' are not copied to 'dst' when not in 'pos' (?)'
   - anything else else?

4. Similarly to previous item, I'd like to have explicit understanding
   and examples where and how bitmap_remap may be replaced. You're
   only saying that it is possible when either 'new' or 'old' are
   dense. Is that the only case? Can you add a test that explicitly
   checks that bitmap_remap and bitmap_scatter/gather produce the same
   output. Something like this:
        bitmap_remap(dst1, val, dense_map, pos, nbits);
        bitmap_scatter(dst2, val, pos, nbits);
        check_eq_bitmap(dst1, dst2, nbits);

5. Can you add a picture like this to explain the algorithm even more:

        mask:    ...v..vv..v...vv
        bits:    0000001100000010
        1.          ^  ^^  ^    0
        2.          |  ||  |   10
        3.          |  ||  +> 010
        4.          |  |+--> 1010
        5.          |  +--> 11010
        6.          +----> 011010
        gather:  ..........011010

5. Regarding my confusion, I had to draw the picture above to realise
   how it's possible that scatter/gather are inverse and data-loss
   (i.e. not inverse) at the same time. Can you explain it with a
   wording like this: "For bits selected by 'pos' bitmap, gathering a
   'val' bitmap with the following scattering restores the original map.
   All other bits values are lost and replaced with zeros." Similarly
   for gathering. And please add a test case.

6. Regarding performance. I think it's wrong to say that that your
   code is better optimized then some other, and then ask your
   reviewers to figure out how to measure the difference. If you make
   such statement, you should already have some test or real-life
   measurement.

   However, if you ask me, I can suggest you to pull this patch:
   https://lore.kernel.org/lkml/20230828184353.5145-4-yury.norov@gmail.com/

   and modify/extend it in a way that both bitmap_remap and
   bitmap_scatter/gather take the same 'val' and 'pos' bitmaps,
   produce the same output, and then see which code is faster.

   Worth to mention that since all current users of your API are working
   on 64-bit maps, performance is doubty an issue. So you can drop the
   'optimization' part of your wording, and don't add performance test.

Thanks,
Yury

Powered by blists - more mailing lists