[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 03/24] hw/arm/virt: use machine->p
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 03/24] hw/arm/virt: use machine->possible_cpus for storing possible topology info |
Date: |
Thu, 4 May 2017 16:33:30 +0200 |
On Thu, 4 May 2017 15:16:02 +0200
Andrew Jones <address@hidden> wrote:
> On Thu, May 04, 2017 at 02:55:09PM +0200, Igor Mammedov wrote:
> > On Thu, 4 May 2017 11:38:22 +0200
> > Andrew Jones <address@hidden> wrote:
> >
> > > On Wed, May 03, 2017 at 02:56:57PM +0200, Igor Mammedov wrote:
> > > > for now precalculate and store mp_afinity in possible_cpus
> > > > as ARM cpus don't have socket/core/thread-id properties yet.
> > > > In follow patches possible_cpus will be used for storing
> > > > and setting NUMA node mapping and replace legacy bitmap
> > > > based numa_info[node_id].node_cpu/numa_get_node_for_cpu()
> > > >
> > > > For the lack of better idea, this patch cannibalizes
> > > > possible_cpus.cpus[x].props.thread_id so that
> > > > *_cpu_index_to_props() callback could return addressable
> > > > by props CPU which will be used by machine_set_cpu_numa_node()
> > > > in follow up patches to assign a CPU to node. But
> > > > cannibalizing is fine for now as that thread_id isn't exposed
> > > > to users (no hotpluggable_cpus callback support for ARM yet)
> > > > and it will be used only internally until 'device_add cpu'
> > > > is supported where we can decide on which properties to use.
> > > >
> > > > Signed-off-by: Igor Mammedov <address@hidden>
> > > > ---
> > > > v2:
> > > > (Drew)
> > > > - discarding result of possible_cpu_arch_ids() makes
> > > > call not obvious and is confusing. Instead assign
> > > > possible_cpu_arch_ids() result to local var and use
> > > > it instead of direct access to machine->possible_cpus
> > > > field, as it's done in pc.c
> > > > ---
> > > > hw/arm/virt.c | 40 +++++++++++++++++++++++++++++++++++++---
> > > > 1 file changed, 37 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > > index 61ae437..e2c5626 100644
> > > > --- a/hw/arm/virt.c
> > > > +++ b/hw/arm/virt.c
> > > > @@ -1221,6 +1221,8 @@ static void machvirt_init(MachineState *machine)
> > > > {
> > > > VirtMachineState *vms = VIRT_MACHINE(machine);
> > > > VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(machine);
> > > > + MachineClass *mc = MACHINE_GET_CLASS(machine);
> > > > + const CPUArchIdList *possible_cpus;
> > > > qemu_irq pic[NUM_IRQS];
> > > > MemoryRegion *sysmem = get_system_memory();
> > > > MemoryRegion *secure_sysmem = NULL;
> > > > @@ -1344,10 +1346,16 @@ static void machvirt_init(MachineState *machine)
> > > > exit(1);
> > > > }
> > > >
> > > > - for (n = 0; n < smp_cpus; n++) {
> > > > - Object *cpuobj = object_new(typename);
> > > > + possible_cpus = mc->possible_cpu_arch_ids(machine);
> > > > + for (n = 0; n < possible_cpus->len; n++) {
> > > > + Object *cpuobj;
> > > >
> > > > - object_property_set_int(cpuobj, virt_cpu_mp_affinity(vms, n),
> > > > + if (n >= smp_cpus) {
> > > > + break;
> > > > + }
> > >
> > > Why the break instead of just looping 'n < smp_cpus' like x86 does? Is
> > > there some future work where looping up to possible_cpus->len (aka
> > > max_cpus) is what we'll eventually want? If so, then we need a TODO
> > > comment here. If not, then we should clean this up by removing the break.
> > >
> > There is no plans to loop here upto possible_cpus->len.
> >
> > It seemed to me more consistent/safer to use index limited
> > by possible_cpus->len to index possible_cpus->cpus[n] array
> > than index limited by smp_cpus though the former currently is
> > always less than smp_cpus.
> ^ greater than or equal to
> >
> > If you prefer 'n < smp_cpus' loop, then I can switch to it.
>
> I just don't like the 'if (n >= smp_cpus) { break; }' - the whole thing
> would look much nicer without it. And, if there's a valid concern that
> possible_cpus->len can be < smp_cpus, then we should check it in x86
> too. Anyway we can check both conditions in the 'for', which would
> look a bit more pleasing to me...
>
> for (n = 0; n < possible_cpus->len && n < smp_cpus; n++) {
nice, I'll do it this way on respin.
> Object *cpuobj = object_new(typename);
> object_property_set_int(cpuobj, possible_cpus->cpus[n].arch_id,
> "mp-affinity", NULL);
> ...
>
> All that said, it's just a nit in the end, so
>
> Reviewed-by: Andrew Jones <address@hidden>
- [Qemu-arm] [PATCH v2 01/24] tests: add CPUs to numa node mapping test, (continued)
[Qemu-arm] [PATCH v2 04/24] hw/arm/virt: explicitly allocate cpu_index for cpus, Igor Mammedov, 2017/05/03
[Qemu-arm] [PATCH v2 05/24] numa: move source of default CPUs to NUMA node mapping into boards, Igor Mammedov, 2017/05/03
Re: [Qemu-arm] [PATCH v2 05/24] numa: move source of default CPUs to NUMA node mapping into boards, Eduardo Habkost, 2017/05/03
Re: [Qemu-arm] [PATCH v2 05/24] numa: move source of default CPUs to NUMA node mapping into boards, Eduardo Habkost, 2017/05/03