Re: [PATCH v10 03/14] xen/bitops: implement fls{l}() in common logic

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.05.2024 09:37, Oleksii K. wrote:
> On Tue, 2024-05-21 at 13:18 +0200, Jan Beulich wrote:
>> On 17.05.2024 15:54, Oleksii Kurochko wrote:
>>> To avoid the compilation error below, it is needed to update to
>>> places
>>> in common/page_alloc.c where flsl() is used as now flsl() returns
>>> unsigned int:
>>>
>>> ./include/xen/kernel.h:18:21: error: comparison of distinct pointer
>>> types lacks a cast [-Werror]
>>>        18 |         (void) (&_x == &_y);            \
>>>           |                     ^~
>>>     common/page_alloc.c:1843:34: note: in expansion of macro 'min'
>>>      1843 |         unsigned int inc_order = min(MAX_ORDER, flsl(e
>>> - s) - 1);
>>>
>>> generic_fls{l} was used instead of __builtin_clz{l}(x) as if x is
>>> 0,
>>> the result in undefined.
>>>
>>> The prototype of the per-architecture fls{l}() functions was
>>> changed to
>>> return 'unsigned int' to align with the generic implementation of
>>> these
>>> functions and avoid introducing signed/unsigned mismatches.
>>>
>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>>> ---
>>>  The patch is almost independent from Andrew's patch series
>>>  (
>>> https://lore.kernel.org/xen-devel/20240313172716.2325427-1-andrew.cooper3@xxxxxxxxxx/T/#t
>>> )
>>>  except test_fls() function which IMO can be merged as a separate
>>> patch after Andrew's patch
>>>  will be fully ready.
>>
>> If there wasn't this dependency (I don't think it's "almost
>> independent"),
>> I'd be offering R-b with again one nit below.
> 
> Aren't all changes, except those in xen/common/bitops.c, independent? I
> could move these changes in xen/common/bitops.c to a separate commit. I
> think it is safe to commit them ( an introduction of common logic for
> fls{l}() and tests ) separately since the CI tests have passed.

Technically they might be, but contextually there are further conflicts.
Just try "patch --dry-run" on top of a plain staging tree. You really
need to settle, perhaps consulting Andrew, whether you want to go on top
of his change, or ahead of it. I'm not willing to approve a patch that's
presented one way but then is (kind of) expected to go in the other way.

Jan