[04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case
diff mbox series

Message ID 20200109152133.23649-5-philmd@redhat.com
State New
Headers show
Series
  • Replace current_machine by qdev_get_machine()
Related show

Commit Message

Philippe Mathieu-Daudé Jan. 9, 2020, 3:21 p.m. UTC
We only access these variables in RTAS_SYSPARM_SPLPAR_CHARACTERISTICS
case, restrict their scope to avoid unnecessary initialization.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/ppc/spapr_rtas.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Greg Kurz Jan. 9, 2020, 5:43 p.m. UTC | #1
On Thu,  9 Jan 2020 16:21:22 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> We only access these variables in RTAS_SYSPARM_SPLPAR_CHARACTERISTICS
> case, restrict their scope to avoid unnecessary initialization.
> 

I guess a decent compiler can be smart enough detect that the initialization
isn't needed outside of the RTAS_SYSPARM_SPLPAR_CHARACTERISTICS branch...
Anyway, reducing scope isn't bad. The only hitch I could see is that some
people do prefer to have all variables declared upfront, but there's a nested
param_val variable already so I guess it's okay.

> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/ppc/spapr_rtas.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 6f06e9d7fe..7237e5ebf2 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -267,8 +267,6 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>                                            uint32_t nret, target_ulong rets)
>  {
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> -    MachineState *ms = MACHINE(spapr);
> -    unsigned int max_cpus = ms->smp.max_cpus;
>      target_ulong parameter = rtas_ld(args, 0);
>      target_ulong buffer = rtas_ld(args, 1);
>      target_ulong length = rtas_ld(args, 2);
> @@ -276,6 +274,8 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>  
>      switch (parameter) {
>      case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: {
> +        MachineState *ms = MACHINE(spapr);
> +        unsigned int max_cpus = ms->smp.max_cpus;

The max_cpus variable used to be a global. Now that it got moved
below ms->smp, I'm not sure it's worth keeping it IMHO. What about
dropping it completely and do:

        char *param_val = g_strdup_printf("MaxEntCap=%d,"
                                          "DesMem=%" PRIu64 ","
                                          "DesProcs=%d,"
                                          "MaxPlatProcs=%d",
                                          ms->smp.max_cpus,
                                          current_machine->ram_size / MiB,
                                          ms->smp.cpus,
                                          ms->smp.max_cpus);

And maybe insert an empty line between the declaration of param_val
and the code for a better readability ?

>          char *param_val = g_strdup_printf("MaxEntCap=%d,"
>                                            "DesMem=%" PRIu64 ","
>                                            "DesProcs=%d,"
Philippe Mathieu-Daudé Jan. 10, 2020, 9:34 a.m. UTC | #2
On 1/9/20 6:43 PM, Greg Kurz wrote:
> On Thu,  9 Jan 2020 16:21:22 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> We only access these variables in RTAS_SYSPARM_SPLPAR_CHARACTERISTICS
>> case, restrict their scope to avoid unnecessary initialization.
>>
> 
> I guess a decent compiler can be smart enough detect that the initialization
> isn't needed outside of the RTAS_SYSPARM_SPLPAR_CHARACTERISTICS branch...
> Anyway, reducing scope isn't bad. The only hitch I could see is that some
> people do prefer to have all variables declared upfront, but there's a nested
> param_val variable already so I guess it's okay.

I don't want to outsmart compilers :)

The MACHINE() macro is not a simple cast, it does object introspection 
with OBJECT_CHECK(), thus is not free. Since 
object_dynamic_cast_assert() argument is not const, I'm not sure the 
compiler can remove the call.

Richard, Eric, do you know?

>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   hw/ppc/spapr_rtas.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>> index 6f06e9d7fe..7237e5ebf2 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -267,8 +267,6 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>>                                             uint32_t nret, target_ulong rets)
>>   {
>>       PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>> -    MachineState *ms = MACHINE(spapr);
>> -    unsigned int max_cpus = ms->smp.max_cpus;
>>       target_ulong parameter = rtas_ld(args, 0);
>>       target_ulong buffer = rtas_ld(args, 1);
>>       target_ulong length = rtas_ld(args, 2);
>> @@ -276,6 +274,8 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>>   
>>       switch (parameter) {
>>       case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: {
>> +        MachineState *ms = MACHINE(spapr);
>> +        unsigned int max_cpus = ms->smp.max_cpus;
> 
> The max_cpus variable used to be a global. Now that it got moved
> below ms->smp, I'm not sure it's worth keeping it IMHO. What about
> dropping it completely and do:
> 
>          char *param_val = g_strdup_printf("MaxEntCap=%d,"
>                                            "DesMem=%" PRIu64 ","
>                                            "DesProcs=%d,"
>                                            "MaxPlatProcs=%d",
>                                            ms->smp.max_cpus,
>                                            current_machine->ram_size / MiB,
>                                            ms->smp.cpus,
>                                            ms->smp.max_cpus);

OK, good idea.

> And maybe insert an empty line between the declaration of param_val
> and the code for a better readability ?
> 
>>           char *param_val = g_strdup_printf("MaxEntCap=%d,"
>>                                             "DesMem=%" PRIu64 ","
>>                                             "DesProcs=%d,"
>
Greg Kurz Jan. 10, 2020, 9:50 a.m. UTC | #3
On Fri, 10 Jan 2020 10:34:07 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 1/9/20 6:43 PM, Greg Kurz wrote:
> > On Thu,  9 Jan 2020 16:21:22 +0100
> > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > 
> >> We only access these variables in RTAS_SYSPARM_SPLPAR_CHARACTERISTICS
> >> case, restrict their scope to avoid unnecessary initialization.
> >>
> > 
> > I guess a decent compiler can be smart enough detect that the initialization
> > isn't needed outside of the RTAS_SYSPARM_SPLPAR_CHARACTERISTICS branch...
> > Anyway, reducing scope isn't bad. The only hitch I could see is that some
> > people do prefer to have all variables declared upfront, but there's a nested
> > param_val variable already so I guess it's okay.
> 
> I don't want to outsmart compilers :)
> 
> The MACHINE() macro is not a simple cast, it does object introspection 
> with OBJECT_CHECK(), thus is not free. Since 

Sure, I understand the motivation in avoiding an unneeded call
to calling object_dynamic_cast_assert().

> object_dynamic_cast_assert() argument is not const, I'm not sure the 
> compiler can remove the call.
> 

Not remove the call, but delay it to the branch that uses it,
ie. parameter == RTAS_SYSPARM_SPLPAR_CHARACTERISTICS.

> Richard, Eric, do you know?
> 
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>   hw/ppc/spapr_rtas.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> >> index 6f06e9d7fe..7237e5ebf2 100644
> >> --- a/hw/ppc/spapr_rtas.c
> >> +++ b/hw/ppc/spapr_rtas.c
> >> @@ -267,8 +267,6 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
> >>                                             uint32_t nret, target_ulong rets)
> >>   {
> >>       PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> >> -    MachineState *ms = MACHINE(spapr);
> >> -    unsigned int max_cpus = ms->smp.max_cpus;
> >>       target_ulong parameter = rtas_ld(args, 0);
> >>       target_ulong buffer = rtas_ld(args, 1);
> >>       target_ulong length = rtas_ld(args, 2);
> >> @@ -276,6 +274,8 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
> >>   
> >>       switch (parameter) {
> >>       case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: {
> >> +        MachineState *ms = MACHINE(spapr);
> >> +        unsigned int max_cpus = ms->smp.max_cpus;
> > 
> > The max_cpus variable used to be a global. Now that it got moved
> > below ms->smp, I'm not sure it's worth keeping it IMHO. What about
> > dropping it completely and do:
> > 
> >          char *param_val = g_strdup_printf("MaxEntCap=%d,"
> >                                            "DesMem=%" PRIu64 ","
> >                                            "DesProcs=%d,"
> >                                            "MaxPlatProcs=%d",
> >                                            ms->smp.max_cpus,
> >                                            current_machine->ram_size / MiB,
> >                                            ms->smp.cpus,
> >                                            ms->smp.max_cpus);
> 
> OK, good idea.
> 
> > And maybe insert an empty line between the declaration of param_val
> > and the code for a better readability ?
> > 
> >>           char *param_val = g_strdup_printf("MaxEntCap=%d,"
> >>                                             "DesMem=%" PRIu64 ","
> >>                                             "DesProcs=%d,"
> > 
>
Eric Blake Jan. 10, 2020, 7:18 p.m. UTC | #4
On 1/10/20 3:50 AM, Greg Kurz wrote:

>>> I guess a decent compiler can be smart enough detect that the initialization
>>> isn't needed outside of the RTAS_SYSPARM_SPLPAR_CHARACTERISTICS branch...
>>> Anyway, reducing scope isn't bad. The only hitch I could see is that some
>>> people do prefer to have all variables declared upfront, but there's a nested
>>> param_val variable already so I guess it's okay.
>>
>> I don't want to outsmart compilers :)

Or conversely play the game of which compilers will warn about an 
atypical construct.

>>
>> The MACHINE() macro is not a simple cast, it does object introspection
>> with OBJECT_CHECK(), thus is not free. Since
> 
> Sure, I understand the motivation in avoiding an unneeded call
> to calling object_dynamic_cast_assert().
> 
>> object_dynamic_cast_assert() argument is not const, I'm not sure the
>> compiler can remove the call.
>>
> 
> Not remove the call, but delay it to the branch that uses it,
> ie. parameter == RTAS_SYSPARM_SPLPAR_CHARACTERISTICS.
> 
>> Richard, Eric, do you know?
>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>>    hw/ppc/spapr_rtas.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>>> index 6f06e9d7fe..7237e5ebf2 100644
>>>> --- a/hw/ppc/spapr_rtas.c
>>>> +++ b/hw/ppc/spapr_rtas.c
>>>> @@ -267,8 +267,6 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>>>>                                              uint32_t nret, target_ulong rets)
>>>>    {
>>>>        PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>>>> -    MachineState *ms = MACHINE(spapr);
>>>> -    unsigned int max_cpus = ms->smp.max_cpus;
>>>>        target_ulong parameter = rtas_ld(args, 0);
>>>>        target_ulong buffer = rtas_ld(args, 1);
>>>>        target_ulong length = rtas_ld(args, 2);
>>>> @@ -276,6 +274,8 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>>>>    
>>>>        switch (parameter) {
>>>>        case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: {
>>>> +        MachineState *ms = MACHINE(spapr);
>>>> +        unsigned int max_cpus = ms->smp.max_cpus;

Declaring an initializer inside a switch statement can trigger warnings 
under some compilation scenarios (particularly if the variable is 
referenced outside of the scope where it was introduced).  But here, you 
are using 'case label: { ...' to create a scope, which presumably ends 
before the next case label, and is thus not going to trigger compiler 
warnings.

An alternative is indeed leaving the declaration up front but deferring 
the (possibly expensive) initializer to the case label that needs it:

MachineState *ms;
switch (parameter) {
case ...:
   ms = MACHINE(spapr);

and done that way, you might not even need the extra {} behind the case 
label (I didn't read the file to see if there is already some other 
reason for having introduced that sub-scope).

As for whether compilers are smart enough to defer non-trivial 
initialization to the one case label that uses the variable, I wouldn't 
count on it.  If the non-trivial initialization includes a function call 
(which the MACHINE() macro does), it's much harder to prove whether that 
function call has side effects that may be needed prior to the switch 
statement.  So limiting the scope of the initialization (whether by 
dropping the declaration, or just deferring the call) DOES make sense.
David Gibson Jan. 13, 2020, 7:16 a.m. UTC | #5
On Fri, Jan 10, 2020 at 10:50:55AM +0100, Greg Kurz wrote:
> On Fri, 10 Jan 2020 10:34:07 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
> > On 1/9/20 6:43 PM, Greg Kurz wrote:
> > > On Thu,  9 Jan 2020 16:21:22 +0100
> > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > > 
> > >> We only access these variables in RTAS_SYSPARM_SPLPAR_CHARACTERISTICS
> > >> case, restrict their scope to avoid unnecessary initialization.
> > >>
> > > 
> > > I guess a decent compiler can be smart enough detect that the initialization
> > > isn't needed outside of the RTAS_SYSPARM_SPLPAR_CHARACTERISTICS branch...
> > > Anyway, reducing scope isn't bad. The only hitch I could see is that some
> > > people do prefer to have all variables declared upfront, but there's a nested
> > > param_val variable already so I guess it's okay.
> > 
> > I don't want to outsmart compilers :)
> > 
> > The MACHINE() macro is not a simple cast, it does object introspection 
> > with OBJECT_CHECK(), thus is not free. Since 
> 
> Sure, I understand the motivation in avoiding an unneeded call
> to calling object_dynamic_cast_assert().
> 
> > object_dynamic_cast_assert() argument is not const, I'm not sure the 
> > compiler can remove the call.
> > 
> 
> Not remove the call, but delay it to the branch that uses it,
> ie. parameter == RTAS_SYSPARM_SPLPAR_CHARACTERISTICS.

I think any performance consideration here is a red herring.  This
particular RTAS call is a handful-of-times-per-boot thing, and only
AFAIK used by AIX guests.

I'm in favour of the change on the grounds of code locality and
readability.

Patch
diff mbox series

diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 6f06e9d7fe..7237e5ebf2 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -267,8 +267,6 @@  static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
                                           uint32_t nret, target_ulong rets)
 {
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
-    MachineState *ms = MACHINE(spapr);
-    unsigned int max_cpus = ms->smp.max_cpus;
     target_ulong parameter = rtas_ld(args, 0);
     target_ulong buffer = rtas_ld(args, 1);
     target_ulong length = rtas_ld(args, 2);
@@ -276,6 +274,8 @@  static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
 
     switch (parameter) {
     case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: {
+        MachineState *ms = MACHINE(spapr);
+        unsigned int max_cpus = ms->smp.max_cpus;
         char *param_val = g_strdup_printf("MaxEntCap=%d,"
                                           "DesMem=%" PRIu64 ","
                                           "DesProcs=%d,"