Re: [RFC PATCH 1/2] bits: introduce ffs_val()

[Petr Tesarik <ptesarik@suse.com>]

On Mon, 12 Jan 2026 09:15:41 +0100
Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Hi Petr
> 
> Am 09.01.26 um 17:37 schrieb Petr Tesarik:
> > Introduce a macro that can efficiently extract the least significant
> > non-zero bit from a value.
> >
> > Interestingly, this bit-twiddling trick is open-coded in some places, but
> > it also appears to be little known, leading to various inefficient
> > implementations in other places. Let's make it part of the standard bitops
> > arsenal.
> >
> > Define the macro in a separate header file included from <linux/bitops.h>,
> > to allow using it in very low-level header files that may not want to
> > include all of <linux/bitops.h>.
> >
> > Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> > ---
> >   MAINTAINERS             |  1 +
> >   include/linux/bitops.h  |  1 +
> >   include/linux/ffs_val.h | 21 +++++++++++++++++++++
> >   3 files changed, 23 insertions(+)
> >   create mode 100644 include/linux/ffs_val.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a0dd762f5648b..8f15c76a67ea2 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4466,6 +4466,7 @@ F:	arch/*/lib/bitops.c
> >   F:	include/asm-generic/bitops
> >   F:	include/asm-generic/bitops.h
> >   F:	include/linux/bitops.h
> > +F:	include/linux/ffs_val.h
> >   F:	lib/hweight.c
> >   F:	lib/test_bitops.c
> >   F:	tools/*/bitops*
> > diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> > index ea7898cc59039..209f0c3e07b9e 100644
> > --- a/include/linux/bitops.h
> > +++ b/include/linux/bitops.h
> > @@ -4,6 +4,7 @@
> >   
> >   #include <asm/types.h>
> >   #include <linux/bits.h>
> > +#include <linux/ffs_val.h>
> >   #include <linux/typecheck.h>
> >   
> >   #include <uapi/linux/kernel.h>
> > diff --git a/include/linux/ffs_val.h b/include/linux/ffs_val.h
> > new file mode 100644
> > index 0000000000000..193ec86d2b53b
> > --- /dev/null
> > +++ b/include/linux/ffs_val.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _ASM_LINUX_FFS_VAL_H_
> > +#define _ASM_LINUX_FFS_VAL_H_
> > +
> > +/**
> > + * ffs_val - find the value of the first set bit
> > + * @x: the value to search
> > + *
> > + * Unlike ffs(), which returns a bit position, ffs_val() returns the bit
> > + * value itself.  
> 
> This sentence was confusing me at first, because the individual bit's 
> value is always '1'. Maybe say something more descriptive, such as 
> 'ffs_val returns the value resulting from that bit's position.'

I think I found the best wording only when composing the cover letter:
isolate the least significant set bit. I should have gone back and
adjusted the patches, too.

> > + *
> > + * Returns:
> > + * least significant non-zero bit, 0 if all bits are zero  
> 
> Same here.
> 
> > + */
> > +#define ffs_val(x)			\
> > +({					\
> > +	const typeof(x) val__ = (x);	\
> > +	val__ & -val__;			\  
> 
> Is this construct supposed to work with signed integers and/or negative 
> numbers? I assume that two's complement can be expected nowadays, but 
> for LONG_MIN it returns zero AFAICT. The documentation should mention 
> these cases.

It works fine for all numbers: -LONG_MIN is LONG_MIN, and that is
incidentally the number with only the least significant bit set.

But this is all moot. After reading some other replies, I'm not even
sure, this helper adds any value. I'm going to start over by converting
occurrences of x & -x with the most suitable existing helper, and see
if there's still some value then.

Last but not least, if there's no good self-explanatory name for this
operation, then I'm told that open-coding "x & -x" is in fact easier to
read. Then again, this is the opinion of people who coded back in the
1980s, and my general feeling is that bit operations are generally less
well understood by people born after 2000...

Glad to spark a bikeshedding flamewar, anyway. /s

Petr T