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

[Igor Mammedov]

On Mon, 15 Apr 2013 11:48:45 -0300
Eduardo Habkost <address@hidden> wrote:

> 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.
> 

Rephrasing suggestion:
drop 18/19 and use apic_id = apic_id_for_cpu_index(id) in 19/19
Am I correct?