Re: [net-next PATCH 04/10] bitfield.h: add FIELD_WIDTH()

[David Laight]

On Thu, 2 Apr 2026 09:52:49 -0400
Yury Norov <ynorov@xxxxxxxxxx> wrote:

> On Thu, Apr 02, 2026 at 10:27:17AM +0100, David Laight wrote:
> > On Thu, 2 Apr 2026 01:00:20 -0300
> > Luiz Angelo Daros de Luca <luizluca@xxxxxxxxx> wrote:
> >   
> > > > > +/**
> > > > > + * FIELD_WIDTH() - return the width of a bitfield
> > > > > + * @_mask: shifted mask defining the field's length and position
> > > > > + *
> > > > > + * Returns the number of contiguous bits covered by @_mask.
> > > > > + * This corresponds to the bit width of FIELD_MAX(@_mask).
> > > > > + */
> > > > > +#define FIELD_WIDTH(_mask)                                           \    
> > > >
> > > > Please no underscored names unless necessary.    
> > > 
> > > I used _mask to maintain consistency with the existing public macros
> > > in this file, such as FIELD_GET, FIELD_PREP, FIELD_MAX, and FIELD_FIT.
> > > All of them use the underscore prefix for parameters. Should I diverge
> > > from them?
> > >   
> > > > > +     ({                                                              \
> > > > > +             __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_WIDTH: ");   \
> > > > > +             __bf_shf(~FIELD_MAX(_mask));                            \
> > > > > +     })    
> > > >
> > > > I believe, this should be:
> > > >
> > > >   #define FIELD_WIDTH(mask) ({                                  \
> > > >         __BF_FIELD_CHECK_MASK(mask, 0ULL, "FIELD_WIDTH: ");     \
> > > >         HWEIGHT(mask);                                          \
> > > >   })    
> > > 
> > > HWEIGHT() is indeed much cleaner. However, to keep bitfield.h
> > > self-contained and avoid adding more includes, I'll try
> > > __builtin_popcountll() in a similar way __builtin_ffsll is already
> > > used.
> > > 
> > > I also noticed the suggestion to use __BF_FIELD_CHECK_MASK instead of
> > > __BF_FIELD_CHECK. Both FIELD_MAX and FIELD_FIT currently use the full
> > > __BF_FIELD_CHECK with 0ULL as a dummy register to ensure the mask fits
> > > within the header's supported limits. If __BF_FIELD_CHECK_MASK is
> > > preferred for FIELD_WIDTH, I can certainly use it, but should we also
> > > update FIELD_MAX and FIELD_FIT for consistency?  
> > 
> > All of the calls with the 0ULL placeholder (especially for the register)
> > should really be removed.
> > Last time I looked there where some calls that only had placeholders.
> > They just bloat the pre-processor output and slow down compilation.  
> 
> Have you any numbers? Can you send a patch?

It was all in a patch I send a while back.
Basically GENMASK() is a couple of hundred bytes and FIELD_PREP(GENMASK())
hits a few thousand due to the number of times mask (in particular)
get expanded.

The problem was bit-fields...

> 
> > > I intend to send a v2 with the following implementation:
> > > 
> > > #define __bf_shf(x) (__builtin_ffsll(x) - 1)
> > > +#define __bf_hweight(x) __builtin_popcountll(x)
> > > 
> > > #define __scalar_type_to_unsigned_cases(type)                          \
> > >                unsigned type:  (unsigned type)0,                       \
> > > @@ -111,6 +112,19 @@
> > >                (typeof(_mask))((_mask) >> __bf_shf(_mask));            \
> > >        })
> > > 
> > > +/**
> > > + * FIELD_WIDTH() - return the width of a bitfield
> > > + * @_mask: shifted mask defining the field's length and position
> > > + *
> > > + * Returns the number of contiguous bits covered by @_mask.
> > > + * This corresponds to the bit width of FIELD_MAX(@_mask).
> > > + */
> > > +#define FIELD_WIDTH(_mask)                                             \
> > > +       ({                                                              \  
> > 
> > You ought to have:
> > 		auto _fw_mask = mask;
> > here. While _mask has to be a constant, if it comes from GENMASK()
> > it is very long.  
> 
> Yes, but what this _fw means? Here it could be just auto __mask = mask.
> That is what the underscores are used.

The problems arise when the expansions get nested.
Using unique names is compex (and unreadable).
Provided only different defines are nested it is enough to use
different names in each one.

> 
> > > +               __BF_FIELD_CHECK_MASK(_mask, 0ULL, "FIELD_WIDTH: ");   \
> > > +               (typeof(_mask))__bf_hweight(_mask);                     \  
> > 
> > Why the cast of the result?
> > They are everywhere in that file, and many are pointless.
> > But there is no point adding another one.
> > 
> > I'm not even sure you need the extra define.
> > Just use __builtin_popcountll().  
> 
> For __BF_FIELD_CHECK_MASK() check, I guess.

I meant why have __bf_hweight().

	David