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 05:16:26PM +0200, Igor Mammedov wrote:
> 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?

Exactly. I mean: I expect that to be enough and not require other
changes, but I didn't look at all patches line-by-line yet.

-- 
Eduardo