Re: [Qemu-devel] [PATCH 18/19] target-i386: expose all possible CPUs as /machine/icc-bridge/cpu[0..N] links

[Eduardo Habkost]

On Mon, Apr 15, 2013 at 04:15:08PM +0200, Igor Mammedov wrote:
> On Fri, 12 Apr 2013 09:44:09 -0300
> Eduardo Habkost <address@hidden> wrote:
> 
> > On Fri, Apr 12, 2013 at 12:01:03PM +0200, Igor Mammedov wrote:
> > > On Thu, 11 Apr 2013 14:19:37 -0300
> > > Eduardo Habkost <address@hidden> wrote:
> > > 
> > > > On Thu, Apr 11, 2013 at 04:51:57PM +0200, Igor Mammedov wrote:
> > > > > ... and leave links for not present CPUs empty.
> > > > > 
> > > > > It will allow users to query for possible APIC IDs and use them
> > > > > with cpu-add QMP command.
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <address@hidden>
> > > > 
> > > > I don't see anything wrong with having icc-bridge links as well, but I
> > > > would really like to have a target-independent namespace with links,
> > > > that could be used to query for the available/valid CPU IDs for cpu-add
> > > > commands instead of icc-bridge. The IDs on that namespace could be
> > > > considered completely opaque.
> > > 
> > > Considering that -numa in present state is not compatible with cpu-add
> > > and that all CPU ID in this case are are sequence [0..maxcpus-1], this
> > > patch could be dropped without any harm. libvirt could just use
> > > numbers from this sequence like it's doing with current cpu_set without
> > > any ID discovery. 
> > 
> > But it's not -numa that makes APIC ID probing necessary, it's
> > non-power-of-2 core/thread counts on -smp (that make APIC IDs not match
> > CPU indexes).
> > 
> > "Don't use CPU hotplug with -numa" is easy to be understood by users and
> > by libvirt, but "don't use CPU hotplug with non-power-of-2 cores/threads
> > counts" is harder to explain.
> > 
> > 
> > > 
> > > So, I've postponed target independent until we have -numa reworked,
> > > then we could have /machine/node/socket/cpu containers with links.
> > > The problem that needs to be solved, is the links storage ownership.
> > > Who should allocate and own it? If machine was QOM object already,
> > > I'd go with machine but it's not yet.
> > 
> > If we use CPU index as argument to cpu-add, we don't need to handle all
> > those problems right now, we don't need to expose an APIC ID discovery
> > interface, we make it work even with non-power-of-2 cores/threads
> yes, you will get non-power-of-2 working without ID look-up.
> 
> > counts, and we make it work with -numa.
> But you won't get this since, only next non-plugged ID will work, due to how
> cpu_index is allocated. You can't just overwrite it with new value without
> breaking current code.

OK, I think I get it: the problem is the cpu_index field specifically,
because it is set automatically, and there may be lots of assumptions in
the code about it. Maybe I should rephrase my suggestion: let's replace
"CPU index" above with "a predictable ID allocation system where IDs
will be in the range 0..(max_cpus-1)". We don't even need to touch the
cpu_index field in the CPU objects if we think this is too risky.

I am just proposing that we use IDs so that we just need to add a
   apic_id = apic_id_for_cpu_index(id)
line at the beginning of the cpu-add implementation, and everything else
would look exactly the same.

Nothing else would change in your implementation, except that now we
won't need a ID lookup system for 1.5 because libvirt can assume that
the next available CPU ID will be smp_cpus+1.

There would be no requirement to make the index-based IDs contiguous,
just like there's no requirement to make APIC IDs contiguous. The only
difference is that CPU indexes will predictable IDs, that will be always
in the range 0..(max_cpus-1) and won't require an ID lookup mechanism.
And it will work with non-power-of-2 threads/counts, and it will work
with -numa.

> 
> > 
> > So, my big question is: why are we trying so hard to avoid using CPU
> > indexes as argument to cpu-add, if it's so much easier, and it is an
> > obvious solution that makes the interface target-independent without any
> > extra effort?
> Using cpu_index instead of APIC ID definitely is not effort free and requires
> quite a bit of rewrite how its used currently, APIC ID is much much easier
> and less risky choice in this regard.
> 
> As for target-independence, any kind of ID is target-independent if treated as
> opaque.

True, as long as we have a target-independent ID lookup system, which we
don't have today. That's the part where we would need less effort.


> Given that with unplug should come not-contiguous ID usage, the
> interface to track which CPUs are plugged would be needed anyway. So it could
> be introduced with this series and provide ID look-up meanwhile. That would
> give libvirt time actually to start using it, and just remove not-contiguous
> ID restriction when unplug is ready with all necessary infrastructure
> already around.

Let's rephrase my suggestion: I don't think we should use the internal
"cpu->cpu_index" field as ID, necessarily. I only suggest that we make
the IDs predictably in the range 0..(max_cpus-1) so we:
1) don't need an ID lookup mechanism;
2) keep it compatible with the existing -numa options.

If we really want to use APIC ID one day, we may implement an lookup
mechanism that will provide IDs to libvirt, and declare the "IDs between
0..(max_cpus-1)" assumption as deprecated, so we can start using APIC
IDs as the (opaque) CPU IDs in the future. But before we do that, we
would implement a better "-numa" interface first.

You even suggested that we did something similar, above:

> > > Considering that -numa in present state is not compatible with cpu-add
> > > and that all CPU ID in this case are are sequence [0..maxcpus-1], this
> > > patch could be dropped without any harm. libvirt could just use
> > > numbers from this sequence like it's doing with current cpu_set without
> > > any ID discovery. 

What I suggest is that we let libvirt make exactly the same assumptions
you suggest, but if we add a single line to the code:
    apic_id = apic_id_for_cpu_index(id)
we will make this intermediate solution work with NUMA _and_ work with
non-power-of-2 cores/threads counts.

The other cases (non-numa with power-of-2 cores/threads counts) would
work exactly the same, because in those cases
apic_id_for_cpu_index(id) == id.

-- 
Eduardo