Re: [XEN PATCH][for-4.19 v4 1/8] xen/include: add macro ISOLATE_LOW_BIT

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 16 Nov 2023, Jan Beulich wrote:
> On 16.11.2023 11:02, Nicola Vetrini wrote:
> > On 2023-11-16 09:26, Jan Beulich wrote:
> >> On 31.10.2023 11:20, Jan Beulich wrote:
> >>> On 31.10.2023 11:03, Nicola Vetrini wrote:
> >>>> On 2023-10-31 09:28, Nicola Vetrini wrote:
> >>>>> On 2023-10-31 08:43, Jan Beulich wrote:
> >>>>>> On 30.10.2023 23:44, Stefano Stabellini wrote:
> >>>>>>> On Mon, 30 Oct 2023, Jan Beulich wrote:
> >>>>>>>> On 27.10.2023 15:34, Nicola Vetrini wrote:
> >>>>>>>>> --- a/xen/include/xen/macros.h
> >>>>>>>>> +++ b/xen/include/xen/macros.h
> >>>>>>>>> @@ -8,8 +8,14 @@
> >>>>>>>>>  #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
> >>>>>>>>>  #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
> >>>>>>>>>
> >>>>>>>>> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
> >>>>>>>>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
> >>>>>>>>> +/*
> >>>>>>>>> + * Given an unsigned integer argument, expands to a mask where
> >>>>>>>>> just the least
> >>>>>>>>> + * significant nonzero bit of the argument is set, or 0 if no 
> >>>>>>>>> bits
> >>>>>>>>> are set.
> >>>>>>>>> + */
> >>>>>>>>> +#define ISOLATE_LOW_BIT(x) ((x) & -(x))
> >>>>>>>>
> >>>>>>>> Not even considering future Misra changes (which aiui may require
> >>>>>>>> that
> >>>>>>>> anyway), this generalization of the macro imo demands that its
> >>>>>>>> argument
> >>>>>>>> now be evaluated only once.
> >>>>>>>
> >>>>>>> Fur sure that would be an improvement, but I don't see a trivial 
> >>>>>>> way
> >>>>>>> to
> >>>>>>> do it and this issue is also present today before the patch.
> >>>>>>
> >>>>>> This was an issue here for MASK_EXTR() and MASK_INSR(), yes, but 
> >>>>>> the
> >>>>>> new
> >>>>>> macro has wider use, and there was no issue elsewhere so far.
> >>>>>>
> >>>>>>> I think it
> >>>>>>> would be better to avoid scope-creeping this patch as we are 
> >>>>>>> already
> >>>>>>> at
> >>>>>>> v4 for something that was expected to be a trivial mechanical 
> >>>>>>> change.
> >>>>>>> I
> >>>>>>> would rather review the fix as a separate patch, maybe sent by you 
> >>>>>>> as
> >>>>>>> you probably have a specific implementation in mind?
> >>>>>>
> >>>>>> #define ISOLATE_LOW_BIT(x) ({ \
> >>>>>>     typeof(x) x_ = (x); \
> >>>>>>     x_ & -x_; \
> >>>>>> })
> >>>>>>
> >>>>>> Hard to see the scope creep here. What I would consider scope creep 
> >>>>>> I
> >>>>>> specifically didn't even ask for: I'd like this macro to be
> >>>>>> overridable
> >>>>>> by an arch. Specifically (see my earlier naming hint) I'd like to 
> >>>>>> use
> >>>>>> x86's BMI insn BLSI in the context of "x86: allow Kconfig control 
> >>>>>> over
> >>>>>> psABI level", when ABI v2 or higher is in use.
> >>>>>
> >>>>> I appreciate you suggesting an implementation; I'll send a v5
> >>>>> incorporating it.
> >>>>
> >>>> There's an issue with this approach, though: since the macro is used
> >>>> indirectly
> >>>> in expressions that are e.g. case labels or array sizes, the build 
> >>>> fails
> >>>> (see [1] for instance).
> >>>> Perhaps it's best to leave it as is?
> >>>
> >>> Hmm. I'm afraid it's not an option to "leave as is", not the least 
> >>> because
> >>> - as said - I'm under the impression that another Misra rule requires
> >>> macro arguments to be evaluated exactly once. Best I can think of 
> >>> right
> >>> away is to have a macro for limited use (to address such build issues)
> >>> plus an inline function (for general use). But yes, maybe that then 
> >>> indeed
> >>> needs to be a 2nd step.
> >>
> >> While I've committed this patch (hoping that I got the necessary 
> >> context
> >> adjustment right for the 
> >> automation/eclair_analysis/ECLAIR/deviations.ecl
> >> change), I'd like to come back to this before going further with users 
> >> of
> >> the new macro: I still think we ought to try to get to the single
> >> evaluation wherever possible. The macro would then be used only in 
> >> cases
> >> where the alternative construct (perhaps an isolate_lsb() macro, living
> >> perhaps in xen/bitops.h) cannot be used. ISOLATE_LSB() would then want 
> >> to
> >> gain a comment directing people to the "better" sibling. Thoughts?
> > 
> > Having the users in place would help me estimate the remaining work that 
> > needs to be done on this rule and see if my local counts match up with 
> > the counts in staging.
> 
> By "having the users in place", you mean you want other patches in this
> and the dependent series to be committed as-is (except for the name
> change)? That's what I'd like to avoid, as it would mean touching all
> those use sites again where the proposed isolate_lsb() could be used
> instead. I'd rather see all use sites be put into their final shape
> right away.

This request is coming a bit late and also after all the patches have
been reviewed already. I for one am not looking forward to review them
again.

That said, if you could be more specified maybe it could become
actionable:

- do you have a pseudo code implementation of the "better" macro you
  would like to propose?
- do you have an list of call sites you would like to be changed to use
  the "better" macro?


Also, you might remember past discussions about time spent making
changes yourself vs. others doing the same. This is one of those cases
that it would be faster for you to make the change and send a patch than
explaining someone else how to do it, then review the result (and
review it again as it probably won't be exactly as you asked the first
time.)

If you don't want the call sites to be changes twice, may I suggest you
provide a patch on top of Nicola's series, I review and ack your patch,
and Nicola or I rebase & resend the series so that the call sites are
only changes once as you would like? I think that's going to be way
faster.