| Message ID | 20200109152133.23649-5-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Replace current_machine by qdev_get_machine() | expand |
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,"
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," >
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," > > >
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.
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.
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,"
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(-)