Re: [PATCH 2/7] xen/bitops: Implement ffs() in common logic

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Bertrand Marquis" <bertrand.marquis@arm.com>,
	"Michal Orzel" <michal.orzel@amd.com>,
	"Oleksii Kurochko" <oleksii.kurochko@gmail.com>,
	"Shawn Anastasio" <sanastasio@raptorengineering.com>,
	"consulting @ bugseng . com" <consulting@bugseng.com>,
	"Simone Ballarin" <simone.ballarin@bugseng.com>,
	"Federico Serafini" <federico.serafini@bugseng.com>,
	"Nicola Vetrini" <nicola.vetrini@bugseng.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 2/7] xen/bitops: Implement ffs() in common logic
Date: Thu, 14 Mar 2024 17:35:02 +0100	[thread overview]
Message-ID: <06cedbcc-fee9-4a58-a0bb-42f6b594ccf8@suse.com> (raw)
In-Reply-To: <6a3c09a1-f35f-4d1e-bbba-342388da71ef@citrix.com>

On 14.03.2024 17:23, Andrew Cooper wrote:
> On 14/03/2024 2:16 pm, Jan Beulich wrote:
>> On 13.03.2024 18:27, Andrew Cooper wrote:
>>> --- a/xen/arch/arm/include/asm/bitops.h
>>> +++ b/xen/arch/arm/include/asm/bitops.h
>>> @@ -157,7 +157,7 @@ static inline int fls(unsigned int x)
>>>  }
>>>  
>>>  
>>> -#define ffs(x) ({ unsigned int __t = (x); fls(ISOLATE_LSB(__t)); })
>>> +#define arch_ffs(x) ({ unsigned int __t = (x); fls(ISOLATE_LSB(__t)); })
>> The way the macro is invoked, I don't think the helper local variable
>> is then needed anymore?
> 
> I strongly suspect It is still needed.  ISOLATE_LSB() double-expands its
> parameter.

Even that double evaluation doesn't matter when the invoking entity is an
inline function, and it doesn't use any non-trivial expression as argument.

>>> --- a/xen/include/xen/bitops.h
>>> +++ b/xen/include/xen/bitops.h
>>> @@ -110,6 +110,21 @@ static inline int generic_flsl(unsigned long x)
>>>  
>>>  #include <asm/bitops.h>
>>>  
>>> +/*
>>> + * Find First Set bit.  Bits are labelled from 1.
>>> + */
>>> +static always_inline __pure unsigned int ffs(unsigned int x)
>> Why always_inline?
> 
> For all the normal reasons to counter Clang and GCC doing stupid things
> with inlines that contain assembly.

Hmm, there are issues when the asm() would look "complex" to the compiler,
but that's not the case here. I was asking because, as you imply by how
you responded, we may need to gain many more always_inline when at some
time even you were arguing against overriding compiler decisions like this
(unless I'm mis-remembering).

Jan