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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 22/05/2024 09:15, Jan Beulich wrote:
> 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.
I agree with what Jan wrote. I don't have any strong opinion on which 
order they should be merged. But, if your series is intended to be 
merged before Andrew's one then please rebase to vanilla staging.
I looked at the rest of the patch and it LGTM.

Cheers,

--
Julien Grall