Re: [Xen-devel] [PATCH 2/3] x86/sysctl: Clean up XEN_SYSCTL_cpu_hotplug

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 03/04/2019 09:53, Jan Beulich wrote:
>>>> On 02.04.19 at 21:57, <andrew.cooper3@xxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/sysctl.c
>> +++ b/xen/arch/x86/sysctl.c
>> @@ -137,27 +137,35 @@ long arch_do_sysctl(
>>      case XEN_SYSCTL_cpu_hotplug:
>>      {
>>          unsigned int cpu = sysctl->u.cpu_hotplug.cpu;
>> +        bool plug;
>> +        long (*fn)(void *) = NULL;
>> +        void *hcpu = NULL;
> May I ask that you consistently initialize (or not) all three new
> variables you introduce?

I noticed this while posting.  Not would allow the compiler to notice
when fn wasn't selected, but hvcpu is going to end up with an
initialiser by the next patch.

>
>>          switch ( sysctl->u.cpu_hotplug.op )
>>          {
>>          case XEN_SYSCTL_CPU_HOTPLUG_ONLINE:
>> -            ret = xsm_resource_plug_core(XSM_HOOK);
>> -            if ( ret )
>> -                break;
>> -            ret = continue_hypercall_on_cpu(
>> -                0, cpu_up_helper, (void *)(unsigned long)cpu);
>> +            plug = true;
>> +            fn = cpu_up_helper;
>> +            hcpu = (void *)(unsigned long)cpu;
> I wonder whether it wouldn't be better to have this cast just
> once, ...
>
>>              break;
>> +
>>          case XEN_SYSCTL_CPU_HOTPLUG_OFFLINE:
>> -            ret = xsm_resource_unplug_core(XSM_HOOK);
>> -            if ( ret )
>> -                break;
>> -            ret = continue_hypercall_on_cpu(
>> -                0, cpu_down_helper, (void *)(unsigned long)cpu);
>> +            plug = false;
>> +            fn = cpu_down_helper;
>> +            hcpu = (void *)(unsigned long)cpu;
>>              break;
>> +
>>          default:
>> -            ret = -EINVAL;
>> +            ret = -EOPNOTSUPP;
>>              break;
>>          }
>> +
>> +        if ( !ret )
>> +            ret = plug ? xsm_resource_plug_core(XSM_HOOK)
>> +                       : xsm_resource_unplug_core(XSM_HOOK);
>> +
>> +        if ( !ret )
>> +            ret = continue_hypercall_on_cpu(0, fn, hcpu);
> ... here. This would the even eliminate the need for "hcpu"
> as a local variable.

The SMT versions don't take a CPU parameter, so the selection of the
parameter has to be done before this point.

> Preferably with both remarks addressed
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

I'll drop the initialisers here, but the initialisation of hcpu is going
to have to stay where it is.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel