Re: [PATCH v2][4.17?] core-parking: fix build with gcc12 and NR_CPUS=1

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 24/10/2022 08:26, Jan Beulich wrote:
> On 22.10.2022 17:30, Julien Grall wrote:
> > Is this intended for 4.17?
> Well, yes, it was meant to be - it has been ...
>
> > On 09/09/2022 15:30, Jan Beulich wrote:
> ... well over a month since it was sent.
>
> > > --- a/xen/arch/x86/sysctl.c
> > > +++ b/xen/arch/x86/sysctl.c
> > > @@ -157,7 +157,7 @@ long arch_do_sysctl(
> > >            long (*fn)(void *);
> > >            void *hcpu;
> > >    
> > > -        switch ( op )
> > > +        switch ( op | -(CONFIG_NR_CPUS == 1) )
> > This code is quite confusing to read and potentially risky as you are
> > are relying the top bit of 'op' to never be 1. While I am expecting this
> > will ever be the case, this will be a "fun" issue to debug if this ever
> > happen. So I would suggest to check CONFIG_NR_CPUS == 1 separately.
> You're aware that we use this pattern in a few other places already (I
> guess in my local tree I have one or two which aren't upstream yet)? Just
> grep for "switch[^_].*[|]" to see them.
I could only spot two upstream in arch/x86/hvm/svm/svm.c and 
arch/x86/hvm/vmx/vmx.c.
But I am not convinced this is a good enough reason to continue to use 
this approach. There are a few bad code examples in Xen and we have been 
pushing against continue to spread certain construct.
> Also note that it's not just the
> top bit of "op" - we merely assume "op" will never be ~0.
Yes, I misread the code.

> Personally I
> prefer this way of coding the logic, but if at least one of the other x86
> maintainers agreed with you, I'd be okay to switch to using a separate
> if().
I am curious why, is it just a matter of taste?

If you really want to go down this route, then I think you should add 
"ASSERT(op != ~0U);" to catch someone introducing a value match that one 
we exclude.
Cheers,

--
Julien Grall