[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single
From: |
David Gibson |
Subject: |
Re: [PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case |
Date: |
Mon, 13 Jan 2020 17:16:48 +1000 |
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é <address@hidden> wrote:
>
> > On 1/9/20 6:43 PM, Greg Kurz wrote:
> > > On Thu, 9 Jan 2020 16:21:22 +0100
> > > Philippe Mathieu-Daudé <address@hidden> 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.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
- Re: [PATCH 02/15] hw/ppc/spapr_rtas: Use local MachineState variable, (continued)
[PATCH 05/15] device-hotplug: Replace current_machine by qdev_get_machine(), Philippe Mathieu-Daudé, 2020/01/09
[PATCH 06/15] migration/savevm: Replace current_machine by qdev_get_machine(), Philippe Mathieu-Daudé, 2020/01/09
[PATCH 07/15] hw/core/machine-qmp-cmds: Replace current_machine by qdev_get_machine(), Philippe Mathieu-Daudé, 2020/01/09
[PATCH 08/15] target/arm/monitor: Replace current_machine by qdev_get_machine(), Philippe Mathieu-Daudé, 2020/01/09