Re: [PATCH 04/13] lib: introduce BITS_{FIRST,LAST} macro
On 16/03/2021 02.54, Yury Norov wrote:
> BITMAP_{LAST,FIRST}_WORD_MASK() in linux/bitmap.h duplicates the
> functionality of GENMASK(). The scope of BITMAP* macros is wider
> than just bitmaps. This patch defines 4 new macros: BITS_FIRST(),
> BITS_LAST(), BITS_FIRST_MASK() and BITS_LAST_MASK() in linux/bits.h
> on top of GENMASK() and replaces BITMAP_{LAST,FIRST}_WORD_MASK()
> to avoid duplication and increase the scope of the macros.
>
> This change doesn't affect code generation. On ARM64:
> scripts/bloat-o-meter vmlinux.before vmlinux
> add/remove: 1/2 grow/shrink: 2/0 up/down: 17/-16 (1)
> Function old new delta
> ethtool_get_drvinfo 900 908 +8
> e843419@0cf2_0001309d_7f0 - 8 +8
> vermagic 48 49 +1
> e843419@0d45_000138bb_f68 8 - -8
> e843419@0cc9_00012bce_198c 8 - -8
[what on earth are those weird symbols?]
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 7f475d59a097..8c191c29506e 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -37,6 +37,12 @@
> #define GENMASK(h, l) \
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>
> +#define BITS_FIRST(nr) GENMASK((nr), 0)
> +#define BITS_LAST(nr)GENMASK(BITS_PER_LONG - 1, (nr))
> +
> +#define BITS_FIRST_MASK(nr) BITS_FIRST((nr) % BITS_PER_LONG)
> +#define BITS_LAST_MASK(nr) BITS_LAST((nr) % BITS_PER_LONG)
I don't think it's a good idea to propagate the unusual closed-range
semantics of GENMASK to those wrappers. Almost all C and kernel code
uses the 'inclusive lower bound, exclusive upper bound', and I'd expect
BITS_FIRST(5) to result in a word with five bits set, not six. So I
think these changes as-is make the code much harder to read and understand.
Regardless, please add some comments on the valid input ranges to the
macros, whether that ends up being 0 <= nr < BITS_PER_LONG or 0 < nr <=
BITS_PER_LONG or whatnot.
It would also be much easier to review if you just redefined the
BITMAP_LAST_WORD_MASK macros etc. in terms of these new things, so you
wouldn't have to do a lot of mechanical changes at the same time as
introducing the new ones - especially when those mechanical changes
involve adding a "minus 1" everywhere.
Rasmus
Re: [PATCH 04/13] lib: introduce BITS_{FIRST,LAST} macro
On Tue, Mar 16, 2021 at 09:35:35AM +0100, Rasmus Villemoes wrote:
> On 16/03/2021 02.54, Yury Norov wrote:
> > BITMAP_{LAST,FIRST}_WORD_MASK() in linux/bitmap.h duplicates the
> > functionality of GENMASK(). The scope of BITMAP* macros is wider
> > than just bitmaps. This patch defines 4 new macros: BITS_FIRST(),
> > BITS_LAST(), BITS_FIRST_MASK() and BITS_LAST_MASK() in linux/bits.h
> > on top of GENMASK() and replaces BITMAP_{LAST,FIRST}_WORD_MASK()
> > to avoid duplication and increase the scope of the macros.
> >
> > This change doesn't affect code generation. On ARM64:
> > scripts/bloat-o-meter vmlinux.before vmlinux
> > add/remove: 1/2 grow/shrink: 2/0 up/down: 17/-16 (1)
> > Function old new delta
> > ethtool_get_drvinfo 900 908 +8
> > e843419@0cf2_0001309d_7f0 - 8 +8
> > vermagic 48 49 +1
> > e843419@0d45_000138bb_f68 8 - -8
> > e843419@0cc9_00012bce_198c 8 - -8
>
> [what on earth are those weird symbols?]
>
>
> > diff --git a/include/linux/bits.h b/include/linux/bits.h
> > index 7f475d59a097..8c191c29506e 100644
> > --- a/include/linux/bits.h
> > +++ b/include/linux/bits.h
> > @@ -37,6 +37,12 @@
> > #define GENMASK(h, l) \
> > (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> >
> > +#define BITS_FIRST(nr) GENMASK((nr), 0)
> > +#define BITS_LAST(nr) GENMASK(BITS_PER_LONG - 1, (nr))
> > +
> > +#define BITS_FIRST_MASK(nr)BITS_FIRST((nr) % BITS_PER_LONG)
> > +#define BITS_LAST_MASK(nr) BITS_LAST((nr) % BITS_PER_LONG)
>
> I don't think it's a good idea to propagate the unusual closed-range
> semantics of GENMASK to those wrappers. Almost all C and kernel code
> uses the 'inclusive lower bound, exclusive upper bound', and I'd expect
> BITS_FIRST(5) to result in a word with five bits set, not six. So I
> think these changes as-is make the code much harder to read and understand.
>
> Regardless, please add some comments on the valid input ranges to the
> macros, whether that ends up being 0 <= nr < BITS_PER_LONG or 0 < nr <=
> BITS_PER_LONG or whatnot.
>
> It would also be much easier to review if you just redefined the
> BITMAP_LAST_WORD_MASK macros etc. in terms of these new things, so you
> wouldn't have to do a lot of mechanical changes at the same time as
> introducing the new ones - especially when those mechanical changes
> involve adding a "minus 1" everywhere.
I tend to agree with Rasmus here.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 04/13] lib: introduce BITS_{FIRST,LAST} macro
On Tue, Mar 16, 2021 at 01:42:45PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 16, 2021 at 09:35:35AM +0100, Rasmus Villemoes wrote:
> > On 16/03/2021 02.54, Yury Norov wrote:
> > > BITMAP_{LAST,FIRST}_WORD_MASK() in linux/bitmap.h duplicates the
> > > functionality of GENMASK(). The scope of BITMAP* macros is wider
> > > than just bitmaps. This patch defines 4 new macros: BITS_FIRST(),
> > > BITS_LAST(), BITS_FIRST_MASK() and BITS_LAST_MASK() in linux/bits.h
> > > on top of GENMASK() and replaces BITMAP_{LAST,FIRST}_WORD_MASK()
> > > to avoid duplication and increase the scope of the macros.
> > >
> > > This change doesn't affect code generation. On ARM64:
> > > scripts/bloat-o-meter vmlinux.before vmlinux
> > > add/remove: 1/2 grow/shrink: 2/0 up/down: 17/-16 (1)
> > > Function old new delta
> > > ethtool_get_drvinfo 900 908 +8
> > > e843419@0cf2_0001309d_7f0 - 8 +8
> > > vermagic 48 49 +1
> > > e843419@0d45_000138bb_f68 8 - -8
> > > e843419@0cc9_00012bce_198c 8 - -8
> >
> > [what on earth are those weird symbols?]
> >
> >
> > > diff --git a/include/linux/bits.h b/include/linux/bits.h
> > > index 7f475d59a097..8c191c29506e 100644
> > > --- a/include/linux/bits.h
> > > +++ b/include/linux/bits.h
> > > @@ -37,6 +37,12 @@
> > > #define GENMASK(h, l) \
> > > (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > >
> > > +#define BITS_FIRST(nr) GENMASK((nr), 0)
> > > +#define BITS_LAST(nr)GENMASK(BITS_PER_LONG - 1, (nr))
> > > +
> > > +#define BITS_FIRST_MASK(nr) BITS_FIRST((nr) % BITS_PER_LONG)
> > > +#define BITS_LAST_MASK(nr) BITS_LAST((nr) % BITS_PER_LONG)
> >
> > I don't think it's a good idea to propagate the unusual closed-range
> > semantics of GENMASK to those wrappers. Almost all C and kernel code
> > uses the 'inclusive lower bound, exclusive upper bound', and I'd expect
> > BITS_FIRST(5) to result in a word with five bits set, not six. So I
> > think these changes as-is make the code much harder to read and understand.
> >
> > Regardless, please add some comments on the valid input ranges to the
> > macros, whether that ends up being 0 <= nr < BITS_PER_LONG or 0 < nr <=
> > BITS_PER_LONG or whatnot.
> >
> > It would also be much easier to review if you just redefined the
> > BITMAP_LAST_WORD_MASK macros etc. in terms of these new things, so you
> > wouldn't have to do a lot of mechanical changes at the same time as
> > introducing the new ones - especially when those mechanical changes
> > involve adding a "minus 1" everywhere.
>
> I tend to agree with Rasmus here.
OK. All this plus terrible GENMASK(high, low) design, when high goes
first, makes me feel like we need to deprecate GENMASK and propose a
new interface.
What do you think about this:
BITS_FIRST(bitnum) -> [0, bitnum)
BITS_LAST(bitnum) -> [bitnum, BITS_PER_LONG)
BITS_RANGE(begin, end) -> [begin, end)
We can pick BITS_{LAST,FIRST} implementation from existing BITMAP_*_WORD_MASK
analogues, and make the BITS_RANGE like:
#define BITS_RANGE(begin, end) BITS_FIRST(end) & BITS_LAST(begin)
Regarding BITMAP_*_WORD_MASK, I can save them in bitmap.h as aliases
to BITS_{LAST,FIRST} to avoid massive renaming. (Should I?)
Would this all work for you?
Re: [PATCH 04/13] lib: introduce BITS_{FIRST,LAST} macro
On 17/03/2021 06.40, Yury Norov wrote:
> On Tue, Mar 16, 2021 at 01:42:45PM +0200, Andy Shevchenko wrote:
>>> It would also be much easier to review if you just redefined the
>>> BITMAP_LAST_WORD_MASK macros etc. in terms of these new things, so you
>>> wouldn't have to do a lot of mechanical changes at the same time as
>>> introducing the new ones - especially when those mechanical changes
>>> involve adding a "minus 1" everywhere.
>>
>> I tend to agree with Rasmus here.
>
> OK. All this plus terrible GENMASK(high, low) design, when high goes
> first, makes me feel like we need to deprecate GENMASK and propose a
> new interface.
>
> What do you think about this:
> BITS_FIRST(bitnum) -> [0, bitnum)
> BITS_LAST(bitnum) -> [bitnum, BITS_PER_LONG)
> BITS_RANGE(begin, end) -> [begin, end)
Better, though I'm not too happy about BITS_LAST(n) not producing a word
with the n highest bits set. I dunno, I don't have better names.
BITS_FROM/BITS_UPTO perhaps, but not really (and upto sounds like it is
inclusive). BITS_LOW/BITS_HIGH have the same problem as BITS_LAST.
Also, be careful to document what one can expect from the boundary
values 0/BITS_PER_LONG. Is BITS_FIRST(0) a valid invocation? Does it
yield 0UL? How about BITS_FIRST(BITS_PER_LONG), does that give ~0UL?
Note that BITMAP_{FIRST,LAST}_WORD_MASK never produce 0, they're never
used except with a word we know to be part of the bitmap.
> We can pick BITS_{LAST,FIRST} implementation from existing BITMAP_*_WORD_MASK
> analogues, and make the BITS_RANGE like:
> #define BITS_RANGE(begin, end) BITS_FIRST(end) & BITS_LAST(begin)
>
> Regarding BITMAP_*_WORD_MASK, I can save them in bitmap.h as aliases
> to BITS_{LAST,FIRST} to avoid massive renaming. (Should I?)
Yes, now that I read these again, I definitely think the
BITMAP_{FIRST,LAST}_WORD_MASK should stay (whether their implementation
change I don't care). Their names document what they do much better than
if you replace them with their potential new implementations:
BITMAP_FIRST_WORD_MASK(start) is obviously about having to mask off some
low bits of the first word we're looking at because we're looking at an
offset into the bitmap, and similarly BITMAP_LAST_WORD_MASK(nbits)
explains itself: nbits is such that the last word needs some masking.
But their replacements would be BITS_LAST(start) and BITS_FIRST(nbits)
respectively (possibly with those arguments reduced mod N), which is
quite confusing.
> Would this all work for you?
Maybe, I think I'd have to see the implementation and how those new
macros get used.
Thanks,
Rasmus
Re: [PATCH 04/13] lib: introduce BITS_{FIRST,LAST} macro
On Wed, Mar 17, 2021 at 08:58:04PM +0100, Rasmus Villemoes wrote:
> On 17/03/2021 06.40, Yury Norov wrote:
> > On Tue, Mar 16, 2021 at 01:42:45PM +0200, Andy Shevchenko wrote:
>
> >>> It would also be much easier to review if you just redefined the
> >>> BITMAP_LAST_WORD_MASK macros etc. in terms of these new things, so you
> >>> wouldn't have to do a lot of mechanical changes at the same time as
> >>> introducing the new ones - especially when those mechanical changes
> >>> involve adding a "minus 1" everywhere.
> >>
> >> I tend to agree with Rasmus here.
> >
> > OK. All this plus terrible GENMASK(high, low) design, when high goes
> > first, makes me feel like we need to deprecate GENMASK and propose a
> > new interface.
> >
> > What do you think about this:
> > BITS_FIRST(bitnum) -> [0, bitnum)
> > BITS_LAST(bitnum) -> [bitnum, BITS_PER_LONG)
> > BITS_RANGE(begin, end) -> [begin, end)
>
> Better, though I'm not too happy about BITS_LAST(n) not producing a word
> with the n highest bits set. I dunno, I don't have better names.
> BITS_FROM/BITS_UPTO perhaps, but not really (and upto sounds like it is
> inclusive). BITS_LOW/BITS_HIGH have the same problem as BITS_LAST.
>
> Also, be careful to document what one can expect from the boundary
> values 0/BITS_PER_LONG. Is BITS_FIRST(0) a valid invocation? Does it
> yield 0UL? How about BITS_FIRST(BITS_PER_LONG), does that give ~0UL?
> Note that BITMAP_{FIRST,LAST}_WORD_MASK never produce 0, they're never
> used except with a word we know to be part of the bitmap.
>
> > We can pick BITS_{LAST,FIRST} implementation from existing
> > BITMAP_*_WORD_MASK
> > analogues, and make the BITS_RANGE like:
> > #define BITS_RANGE(begin, end) BITS_FIRST(end) & BITS_LAST(begin)
> >
> > Regarding BITMAP_*_WORD_MASK, I can save them in bitmap.h as aliases
> > to BITS_{LAST,FIRST} to avoid massive renaming. (Should I?)
>
> Yes, now that I read these again, I definitely think the
> BITMAP_{FIRST,LAST}_WORD_MASK should stay (whether their implementation
> change I don't care). Their names document what they do much better than
> if you replace them with their potential new implementations:
> BITMAP_FIRST_WORD_MASK(start) is obviously about having to mask off some
> low bits of the first word we're looking at because we're looking at an
> offset into the bitmap, and similarly BITMAP_LAST_WORD_MASK(nbits)
> explains itself: nbits is such that the last word needs some masking.
> But their replacements would be BITS_LAST(start) and BITS_FIRST(nbits)
> respectively (possibly with those arguments reduced mod N), which is
> quite confusing.
>
> > Would this all work for you?
>
> Maybe, I think I'd have to see the implementation and how those new
> macros get used.
>
> Thanks,
> Rasmus
I looked at this with a fresh eye this morning. All the noise we
discuss I made to just call BITS_FIRST() 3 times in find.h. I think
that for the purpose of this series, in find.h, it's worth to use
GENMASK(size - 1, 0) where needed.
Regarding the general view on this, all the problems come from the
fact that bitmap API is split between linux/bitmap.h and
asm_generic/bitops/find.h. Find.h is naturally a part of bitmaps,
but because of the split, it's hard to use handy bitmap.h macros
in find.h.
Joining the headers is far out of the scope of this series. If no
objections, I'd prefer to drop this patch now, and later carefully
figure out how to join find.h and bitmap.h.
Yury
