On Mon, Apr 15, 2013 at 05:16:26PM +0200, Igor Mammedov wrote: > On Mon, 15 Apr 2013 11:48:45 -0300 > Eduardo Habkost <ehabk...@redhat.com> 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 <ehabk...@redhat.com> 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 <ehabk...@redhat.com> 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 <imamm...@redhat.com> > > > > > > > > > > > > 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
