| Message ID | 1554235048-3373-3-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/smt: Runtime SMT controls | expand |
>>> 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
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
>>> 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
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
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;
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(-)