On Wed, 18 Jan 2017 13:19:45 -0200
Eduardo Habkost <ehabk...@redhat.com> wrote:
> On Wed, Jan 18, 2017 at 03:48:45PM +0100, Igor Mammedov wrote:
> > Move vcpu's assocciated numa_node field out of generic CPUState
> > into inherited classes that actually care about cpu<->numa mapping
> > and make monitor 'info numa' get vcpu's assocciated node id via
> > node-id property.
> > It allows to drop implicit node id intialization in
> > numa_post_machine_init() and would allow switching to mapping
> > defined by target's CpuInstanceProperties instead of global
> > numa_info[i].node_cpu bitmaps.
>
> I agree to allow the node-id assignment logic to be defined by
> arch-specific code, but:
>
> Considering that we still have the generic CPU<->node mapping in
> numa_info[].node_cpu, I don't see the benefit of moving the
> numa_info[].node_cpu => node-id translation from common generic
> code to duplicated arch-specific code.
it's duplicated for a time being until all targets that use
node_cpu are internally converted to socket/core/thread-id interface.
This patch is a part if re-factoring RFC which I'm about to post.
> What about adding this to cpu_generic_realize():
>
> node = (numa_get_node_for_cpu(cpu)
> if (node >= 0) {
> int cur_node = object_property_find(cpu, "node-id") ?
> object_property_get_int(cpu, "node-id") : -1;
> if (cur_node >= 0 && cur_node != node) {
> error_setg(&err, "Conflict between -numa option and CPU node-id");
> goto out;
> }
> object_property_set_int(cpu, "node-id", node, &err);
> }
What I don't like here is that putting above snippet into
cpu_generic_realize() is just doing the same implicit init
that numa_post_machine_init() has been doing before just
a bit later. It looks like a quick fix for 'info numa' issues.
However it would add stumble block on getting rid of
numa_get_node_for_cpu() { numa_info[].node_cpu } and keeping
node-mapping along with the rest of topology in Machine object
instead of globals.
I'm in process of getting rid of numa_info[].node_cpu/numa_get_node_for_cpu()
altogether. And incrementally done it for PC, so I'd be forced
to reduplicate above snippet from cpu_generic_realize() in concrete
users anyway to do conversion without breaking other users.
[...]
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1978,6 +1978,11 @@ static void pc_cpu_pre_plug(HotplugHandler
> > *hotplug_dev,
> >
> > cs = CPU(cpu);
> > cs->cpu_index = idx;
> > +
> > + idx = numa_get_node_for_cpu(cs->cpu_index);
> > + if (idx < nb_numa_nodes) {
> > + cpu->numa_nid = idx;
> > + }
this is the only place where I add above mentioned duplication,
which sort of compensated by removed numa_post_machine_init().
And it's here only to keep working+fix 'info numa'.
The other targets currently already do numa_get_node_for_cpu(),
lookup so I'm not changing there anything.
Later(not in this patch) I'm removing numa_get_node_for_cpu() from
pc_cpu_pre_plug() and from PC code altogether and replacing it
with node-id from possible_cpus.
PS:
since travis-ci seems to be broken, I'll post re-factoring
RFC without building there and it includes this patch as well.
