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

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

[Stefano Stabellini]

On Fri, 26 Oct 2023, Nicola Vetrini wrote:
> The purpose of this macro is to encapsulate the well-known expression
> 'x & -x' that in 2's complement architectures on unsigned integers will
> give a mask where only the least significant nonzero bit of 'x' is set,
> or 0 if none are set.
> 
> A deviation for ECLAIR is also introduced.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [XEN PATCH][for-4.19 v4 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Stefano Stabellini]

On Fri, 27 Oct 2023, Nicola Vetrini wrote:
> The definition of MC_NCLASSES contained a violation of MISRA C:2012
> Rule 10.1, therefore by moving it as an enumeration constant resolves the
> violation and makes it more resilient to possible additions to that enum.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [XEN PATCH][for-4.19 v4 4/8] x86_64/mm: express macro CNT using ISOLATE_LOW_BIT

[Jan Beulich]

On 27.10.2023 15:34, Nicola Vetrini wrote:
> The various definitions of macro CNT (and the related BUILD_BUG_ON)
> can be rewritten using ISOLATE_LOW_BIT, encapsulating a violation of
> MISRA C:2012 Rule 10.1.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Acked-by: Jan Beulich <jbeulich@suse.com>

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

[Jan Beulich]

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.

Also another thought regarding the name: Would ISOLATE_LSB() be acceptable
to everyone having voiced a view on the set of proposed names? It would be
at least a little shorter ...

Jan

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

[Stefano Stabellini]

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. 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?


> Also another thought regarding the name: Would ISOLATE_LSB() be acceptable
> to everyone having voiced a view on the set of proposed names? It would be
> at least a little shorter ...

I would be OK with that

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

[Jan Beulich]

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.

Jan

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

[Nicola Vetrini]

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.
> 
> Jan

I appreciate you suggesting an implementation; I'll send a v5 
incorporating it.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH][for-4.19 v4 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Nicola Vetrini]

On 2023-10-27 22:50, Stefano Stabellini wrote:
> On Fri, 27 Oct 2023, Nicola Vetrini wrote:
>> The definition of MC_NCLASSES contained a violation of MISRA C:2012
>> Rule 10.1, therefore by moving it as an enumeration constant resolves 
>> the
>> violation and makes it more resilient to possible additions to that 
>> enum.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

I appreciate your review here, but I forgot to put Andrew's A-by;
he already included this patch in its own for-next tree.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

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

[Nicola Vetrini]

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.
>> 
>> Jan
> 
> 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?

[1] https://gitlab.com/xen-project/people/bugseng/xen/-/jobs/5423693947

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

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

[Jan Beulich]

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.

Jan

> [1] https://gitlab.com/xen-project/people/bugseng/xen/-/jobs/5423693947
> 

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

[Nicola Vetrini]

On 2023-10-31 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.
> 
> Jan
> 
>> [1] 
>> https://gitlab.com/xen-project/people/bugseng/xen/-/jobs/5423693947
>> 

There is no such MISRA Rule afaik: R23.7 is similar, but only for C11 
generic selections.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

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

[Jan Beulich]

On 31.10.2023 08:43, Jan Beulich wrote:
> 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.

Actually I need to withdraw that. It meanwhile occurred to me that the
compiler ought to recognize this pattern. And indeed gcc doesn't even
have a builtin for it; its BMI intrinsic for BLSI (on x86 that is)
specifically expands to x & -x, which the backend then is expected to
deal with as appropriate. And indeed it can be observed to, with my
"x86: allow Kconfig control over psABI level" in place.

Just as a reminder: I'd still like to see the further renaming done
(to ISOLATE_LSB()). If I was to commit this patch, I'd be fine doing
so while committing.

Jan

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

[Stefano Stabellini]

On Thu, 9 Nov 2023, Jan Beulich wrote:
> On 31.10.2023 08:43, Jan Beulich wrote:
> > 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.
> 
> Actually I need to withdraw that. It meanwhile occurred to me that the
> compiler ought to recognize this pattern. And indeed gcc doesn't even
> have a builtin for it; its BMI intrinsic for BLSI (on x86 that is)
> specifically expands to x & -x, which the backend then is expected to
> deal with as appropriate. And indeed it can be observed to, with my
> "x86: allow Kconfig control over psABI level" in place.
> 
> Just as a reminder: I'd still like to see the further renaming done
> (to ISOLATE_LSB()). If I was to commit this patch, I'd be fine doing
> so while committing.

Please do

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

[Jan Beulich]

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?

Jan

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

[Nicola Vetrini]

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.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

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

[Jan Beulich]

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.

Jan

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

[Stefano Stabellini]

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.

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

[Jan Beulich]

On 17.11.2023 01:43, Stefano Stabellini wrote:
> 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?

May I remind you that I made this request (including a draft implementation)
before already, and Nicola then merely found that the evaluate-once form
simply cannot be used everywhere? Anybody could have thought of the option
of "splitting" the macro. After all I hope that there is no disagreement on
macro arguments better being evaluated just once, whenever possible.

> - do you have an list of call sites you would like to be changed to use
>   the "better" macro?

No, I don't have a list. But the pattern is pretty clear: The "better" form
ought to be used wherever it actually can be used.

> 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.

I'll see if I can find time to do so. I don't normally work on top of
other people's uncommitted patches, though ... So I may also choose to go
a slightly different route. (You realize though that all still pending
patches using the new macro need touching again anyway, don't you?)

Jan

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

[Nicola Vetrini]

Hi Jan,

>>>>> 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?
> 
> May I remind you that I made this request (including a draft 
> implementation)
> before already, and Nicola then merely found that the evaluate-once 
> form
> simply cannot be used everywhere? Anybody could have thought of the 
> option
> of "splitting" the macro. After all I hope that there is no 
> disagreement on
> macro arguments better being evaluated just once, whenever possible.
> 
>> - do you have an list of call sites you would like to be changed to 
>> use
>>   the "better" macro?
> 
> No, I don't have a list. But the pattern is pretty clear: The "better" 
> form
> ought to be used wherever it actually can be used.
> 
>> 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.
> 
> I'll see if I can find time to do so. I don't normally work on top of
> other people's uncommitted patches, though ... So I may also choose to 
> go
> a slightly different route. (You realize though that all still pending
> patches using the new macro need touching again anyway, don't you?)
> 
> Jan

Then perhaps it's best if I give it a try at doing the single evaluation 
macro, so that I can make a series modifying the call sites only once on 
top of that and send everything in one go. Before doing that, though, 
I'll make a thread where various aspects that are not so clear yet can 
be discussed, so that we can devise a robust solution (also to dig this 
out of this deep thread).

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

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

[Stefano Stabellini]

On Fri, 17 Nov 2023, Nicola Vetrini wrote:
> Hi Jan,
> 
> > > > > > 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?
> > 
> > May I remind you that I made this request (including a draft implementation)
> > before already, and Nicola then merely found that the evaluate-once form
> > simply cannot be used everywhere? Anybody could have thought of the option
> > of "splitting" the macro. After all I hope that there is no disagreement on
> > macro arguments better being evaluated just once, whenever possible.
> > 
> > > - do you have an list of call sites you would like to be changed to use
> > >   the "better" macro?
> > 
> > No, I don't have a list. But the pattern is pretty clear: The "better" form
> > ought to be used wherever it actually can be used.
> > 
> > > 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.
> > 
> > I'll see if I can find time to do so. I don't normally work on top of
> > other people's uncommitted patches, though ... So I may also choose to go
> > a slightly different route. (You realize though that all still pending
> > patches using the new macro need touching again anyway, don't you?)
> > 
> > Jan
> 
> Then perhaps it's best if I give it a try at doing the single evaluation
> macro, so that I can make a series modifying the call sites only once on top
> of that and send everything in one go. Before doing that, though, I'll make a
> thread where various aspects that are not so clear yet can be discussed, so
> that we can devise a robust solution (also to dig this out of this deep
> thread).

In the meantime I committed patches from #5 onward as they don't depend
on ISOLATE_LSB