[2/3] x86/sysctl: Clean up XEN_SYSCTL_cpu_hotplug
diff mbox series

Message ID 1554235048-3373-3-git-send-email-andrew.cooper3@citrix.com
State New, archived
Headers show
Series
  • x86/smt: Runtime SMT controls
Related show

Commit Message

Andrew Cooper April 2, 2019, 7:57 p.m. UTC
A future change is going to introduce two more cases.  Instead of opcoding the
XSM checks and contine_hypercall logic, collect the data into local variables.

Switch the default return value to -EOPNOTSUPP to distinguish a bad op from a
bad cpu index.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/sysctl.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

Comments

Jan Beulich April 3, 2019, 8:53 a.m. UTC | #1
>>> On 02.04.19 at 21:57, <andrew.cooper3@citrix.com> 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?

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

Preferably with both remarks addressed
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan
Andrew Cooper April 3, 2019, 9:06 a.m. UTC | #2
On 03/04/2019 09:53, Jan Beulich wrote:
>>>> On 02.04.19 at 21:57, <andrew.cooper3@citrix.com> 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@suse.com>

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

~Andrew
Jan Beulich April 3, 2019, 9:38 a.m. UTC | #3
>>> On 03.04.19 at 11:06, <andrew.cooper3@citrix.com> wrote:
> On 03/04/2019 09:53, Jan Beulich wrote:
>>>>> On 02.04.19 at 21:57, <andrew.cooper3@citrix.com> 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.

Right, I've noticed the need for hcpu to stay as is while looking at
patch 3. Before you remove the other initializer, though - are you
sure you don't need to instead add one, for older gcc to not choke?

Jan
Andrew Cooper April 4, 2019, 1:31 p.m. UTC | #4
On 03/04/2019 10:38, Jan Beulich wrote:
>>>> On 03.04.19 at 11:06, <andrew.cooper3@citrix.com> wrote:
>> On 03/04/2019 09:53, Jan Beulich wrote:
>>>>>> On 02.04.19 at 21:57, <andrew.cooper3@citrix.com> 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.
> Right, I've noticed the need for hcpu to stay as is while looking at
> patch 3. Before you remove the other initializer, though - are you
> sure you don't need to instead add one, for older gcc to not choke?

CI is happy

https://gitlab.com/xen-project/people/andyhhp/xen/pipelines/55211926

(This is specifically why I added the CentOS 6 build)

~Andrew

Patch
diff mbox series

diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 1916a3d..b3cc4b5 100644
--- 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;
 
         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;
             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);
     }
     break;