Re: [PATCH 3/6] bitops: bitmap helper to set variable length values

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Wed, Jul 13, 2022 at 01:42:17PM -0700, Yury Norov wrote:
> On Wed, Jul 13, 2022 at 10:14:24PM +0200, Andy Shevchenko wrote:
> > On Wed, Jul 13, 2022 at 9:44 PM Yury Norov <yury.norov@gmail.com> wrote:
> > > On Wed, Jul 13, 2022 at 09:10:33PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Jul 13, 2022 at 8:56 PM Yury Norov <yury.norov@gmail.com> wrote:
> > > > > On Wed, Jul 13, 2022 at 06:31:59PM +0200, Sebastian Fricke wrote:

...

> > > > > I'd suggest you to try implementing
> > > > >         bitmap_copy_from(dst, src, dst_off, len)
> > > > > or even
> > > > >         bitmap_copy_from(dst, dst_off, src, src_off, len)
> > > > > if you expect that you'll need more flexibility in the future.
> > > >
> > > > Do you think it would be useful?
> > > >
> > > > We have bitmap_replace() & bitmap_remap(). Wouldn't that be enough?
> > >
> > > bitmap_replace and bitmap_remap have no an 'offset' parameter.
> > 
> > True.
> > 
> > But then it's a bit too generic to have this src_off, no?
> 
> That's why I said:
> 
> > > > > if you expect that you'll need more flexibility in the future.
> 
> My preferred option is bitmap_copy_from(dst, src, dst_off, len).
> 
> > I would rather expect for asymmetrical bitmaps that the other side
> > will be either one of the fixed width types (it makes sense to have
> > for 32- or 64-bit arguments.
> 
> Look at patch #6 - it copies 1,4,5,9,10,32,37... - pretty much a random
> number number of bits.

It's too poor randomness, as u64 covers all what in patch 6.

> > When you have a source bitmap of x bits and  you would like to copy it
> > into a y-bit one, I would think that either you have a small amount of
> > bits in x anyway, or x is a full-sized bitmap (same order as y).
> 
> It sounds like a speculation to me. Why shouldn't we let people to
> copy with an offset any number of bits? 

Because it's a common case. You have a value in the register / variable, which
naturally is one of the POD types. Now you want to inject this into bitmap at
the arbitrary offset. Value itself also needs to be variadic size in bits.

Basically what he is trying to achieve is something like bitfield.h API over
bitmaps. Dunno, if actually bitfield.h in the certain driver wouldn't be
enough.

> > Also
> > keep in mind that granularity is long, so less than long it makes no
> > sense.
> > 
> >   bitmap_copy_from_T(unsigned long *map, start, len, T src),
> > 
> > where T is type, start is the offset in map, len is the amount of bits
> > from src starting from 0. That's what is required in most of the cases
> > I believe.
> 
> But not in Sebastian's case, according to patch #6.

I think it's a case, see above.

-- 
With Best Regards,
Andy Shevchenko