| Message ID | 1468833560-17397-2-git-send-email-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote: > The spapr implementation of query-hotpluggable-cpus builds the list of > hotpluggable cores from the end (most removed from the list head) > because that's the easiest way with a singly linked list. Because it > also traverses the possible CPU cores starting from low indexes the > resulting list has the cpu cores listed in reverse order by core-id. > > That's not generally harmful, but it means the output from "info > hotpluggable-cpus" is a little harder to read than it could be. > > Therefore, traverse the cpus in reverse order so that the final list > ends up in increasing-core-id order. To make this interface usable with in-order hotplug the ordering of the entries should be codified in the schema documentation. (see my response on the cover letter for justification).
On Tue, Jul 19, 2016 at 01:52:40PM +0200, Peter Krempa wrote: > On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote: > > The spapr implementation of query-hotpluggable-cpus builds the list of > > hotpluggable cores from the end (most removed from the list head) > > because that's the easiest way with a singly linked list. Because it > > also traverses the possible CPU cores starting from low indexes the > > resulting list has the cpu cores listed in reverse order by core-id. > > > > That's not generally harmful, but it means the output from "info > > hotpluggable-cpus" is a little harder to read than it could be. > > > > Therefore, traverse the cpus in reverse order so that the final list > > ends up in increasing-core-id order. > > To make this interface usable with in-order hotplug the ordering of the > entries should be codified in the schema documentation. (see my response > on the cover letter for justification). I'm not really sure what you mean by this.
On Wed, Jul 20, 2016 at 11:00:05 +1000, David Gibson wrote: > On Tue, Jul 19, 2016 at 01:52:40PM +0200, Peter Krempa wrote: > > On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote: > > > The spapr implementation of query-hotpluggable-cpus builds the list of > > > hotpluggable cores from the end (most removed from the list head) > > > because that's the easiest way with a singly linked list. Because it > > > also traverses the possible CPU cores starting from low indexes the > > > resulting list has the cpu cores listed in reverse order by core-id. > > > > > > That's not generally harmful, but it means the output from "info > > > hotpluggable-cpus" is a little harder to read than it could be. > > > > > > Therefore, traverse the cpus in reverse order so that the final list > > > ends up in increasing-core-id order. > > > > To make this interface usable with in-order hotplug the ordering of the > > entries should be codified in the schema documentation. (see my response > > on the cover letter for justification). > > I'm not really sure what you mean by this. Currently the order of entries in reply of query-hotpluggable-cpus is arbitrary as this patch hints. If qemu won't support arbitrary order hotplug but libvirt should be able to use the new interface, then the order of replies of query-hotpluggable-cpus need to corelate (in a documented fashion) with the order we need to plug the cpus in. By not documenting any order libvirt can just guess it (by reimplementing the algorithm in qemu). I've pointed this out in a sub-thread of the cover-letter. Peter
On Wed, Jul 20, 2016 at 09:01:11AM +0200, Peter Krempa wrote: > On Wed, Jul 20, 2016 at 11:00:05 +1000, David Gibson wrote: > > On Tue, Jul 19, 2016 at 01:52:40PM +0200, Peter Krempa wrote: > > > On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote: > > > > The spapr implementation of query-hotpluggable-cpus builds the list of > > > > hotpluggable cores from the end (most removed from the list head) > > > > because that's the easiest way with a singly linked list. Because it > > > > also traverses the possible CPU cores starting from low indexes the > > > > resulting list has the cpu cores listed in reverse order by core-id. > > > > > > > > That's not generally harmful, but it means the output from "info > > > > hotpluggable-cpus" is a little harder to read than it could be. > > > > > > > > Therefore, traverse the cpus in reverse order so that the final list > > > > ends up in increasing-core-id order. > > > > > > To make this interface usable with in-order hotplug the ordering of the > > > entries should be codified in the schema documentation. (see my response > > > on the cover letter for justification). > > > > I'm not really sure what you mean by this. > > Currently the order of entries in reply of query-hotpluggable-cpus is > arbitrary as this patch hints. > > If qemu won't support arbitrary order hotplug but libvirt should be able > to use the new interface, then the order of replies of > query-hotpluggable-cpus need to corelate (in a documented fashion) with > the order we need to plug the cpus in. By not documenting any order > libvirt can just guess it (by reimplementing the algorithm in qemu). > I've pointed this out in a sub-thread of the cover-letter. Right, that's kind of the point of this patch - it makes the order returned by query-hotpluggable-cpus match the required order for "in order" hotplug. That may not be necessary so - it looks like between Igor and myself we might have an eleventh hour way of enabling any-order hotplug. That said, I'm still inclined to push this patch at some point for the sake of easier to read debugging output.
On Thu, 21 Jul 2016 23:07:06 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Wed, Jul 20, 2016 at 09:01:11AM +0200, Peter Krempa wrote: > > On Wed, Jul 20, 2016 at 11:00:05 +1000, David Gibson wrote: > > > On Tue, Jul 19, 2016 at 01:52:40PM +0200, Peter Krempa wrote: > > > > On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote: > > > > > The spapr implementation of query-hotpluggable-cpus builds the list of > > > > > hotpluggable cores from the end (most removed from the list head) > > > > > because that's the easiest way with a singly linked list. Because it > > > > > also traverses the possible CPU cores starting from low indexes the > > > > > resulting list has the cpu cores listed in reverse order by core-id. > > > > > > > > > > That's not generally harmful, but it means the output from "info > > > > > hotpluggable-cpus" is a little harder to read than it could be. > > > > > > > > > > Therefore, traverse the cpus in reverse order so that the final list > > > > > ends up in increasing-core-id order. > > > > > > > > To make this interface usable with in-order hotplug the ordering of the > > > > entries should be codified in the schema documentation. (see my response > > > > on the cover letter for justification). > > > > > > I'm not really sure what you mean by this. > > > > Currently the order of entries in reply of query-hotpluggable-cpus is > > arbitrary as this patch hints. > > > > If qemu won't support arbitrary order hotplug but libvirt should be able > > to use the new interface, then the order of replies of > > query-hotpluggable-cpus need to corelate (in a documented fashion) with > > the order we need to plug the cpus in. By not documenting any order > > libvirt can just guess it (by reimplementing the algorithm in qemu). > > > I've pointed this out in a sub-thread of the cover-letter. > > Right, that's kind of the point of this patch - it makes the order > returned by query-hotpluggable-cpus match the required order for "in > order" hotplug. That may not be necessary so - it looks like between > Igor and myself we might have an eleventh hour way of enabling > any-order hotplug. > > That said, I'm still inclined to push this patch at some point for the > sake of easier to read debugging output. it's qmp command output intended for machine parsing, so don't really need it
On Thu, 21 Jul 2016 16:30:06 +0200 Igor Mammedov <imammedo@redhat.com> wrote: > On Thu, 21 Jul 2016 23:07:06 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Wed, Jul 20, 2016 at 09:01:11AM +0200, Peter Krempa wrote: > > > On Wed, Jul 20, 2016 at 11:00:05 +1000, David Gibson wrote: > > > > On Tue, Jul 19, 2016 at 01:52:40PM +0200, Peter Krempa wrote: > > > > > On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote: > > > > > > The spapr implementation of query-hotpluggable-cpus builds the list of > > > > > > hotpluggable cores from the end (most removed from the list head) > > > > > > because that's the easiest way with a singly linked list. Because it > > > > > > also traverses the possible CPU cores starting from low indexes the > > > > > > resulting list has the cpu cores listed in reverse order by core-id. > > > > > > > > > > > > That's not generally harmful, but it means the output from "info > > > > > > hotpluggable-cpus" is a little harder to read than it could be. > > > > > > > > > > > > Therefore, traverse the cpus in reverse order so that the final list > > > > > > ends up in increasing-core-id order. > > > > > > > > > > To make this interface usable with in-order hotplug the ordering of the > > > > > entries should be codified in the schema documentation. (see my response > > > > > on the cover letter for justification). > > > > > > > > I'm not really sure what you mean by this. > > > > > > Currently the order of entries in reply of query-hotpluggable-cpus is > > > arbitrary as this patch hints. > > > > > > If qemu won't support arbitrary order hotplug but libvirt should be able > > > to use the new interface, then the order of replies of > > > query-hotpluggable-cpus need to corelate (in a documented fashion) with > > > the order we need to plug the cpus in. By not documenting any order > > > libvirt can just guess it (by reimplementing the algorithm in qemu). > > > > > I've pointed this out in a sub-thread of the cover-letter. > > > > Right, that's kind of the point of this patch - it makes the order > > returned by query-hotpluggable-cpus match the required order for "in > > order" hotplug. That may not be necessary so - it looks like between > > Igor and myself we might have an eleventh hour way of enabling > > any-order hotplug. > > > > That said, I'm still inclined to push this patch at some point for the > > sake of easier to read debugging output. > it's qmp command output intended for machine parsing, so don't really need it > It is also hmp output as indicated in the changelog with "info hotpluggable-cpus" Cheers. -- Greg
On Wed, Jul 20, 2016 at 09:01:11AM +0200, Peter Krempa wrote: > On Wed, Jul 20, 2016 at 11:00:05 +1000, David Gibson wrote: > > On Tue, Jul 19, 2016 at 01:52:40PM +0200, Peter Krempa wrote: > > > On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote: > > > > The spapr implementation of query-hotpluggable-cpus builds the list of > > > > hotpluggable cores from the end (most removed from the list head) > > > > because that's the easiest way with a singly linked list. Because it > > > > also traverses the possible CPU cores starting from low indexes the > > > > resulting list has the cpu cores listed in reverse order by core-id. > > > > > > > > That's not generally harmful, but it means the output from "info > > > > hotpluggable-cpus" is a little harder to read than it could be. > > > > > > > > Therefore, traverse the cpus in reverse order so that the final list > > > > ends up in increasing-core-id order. > > > > > > To make this interface usable with in-order hotplug the ordering of the > > > entries should be codified in the schema documentation. (see my response > > > on the cover letter for justification). > > > > I'm not really sure what you mean by this. > > Currently the order of entries in reply of query-hotpluggable-cpus is > arbitrary as this patch hints. > > If qemu won't support arbitrary order hotplug but libvirt should be able > to use the new interface, then the order of replies of > query-hotpluggable-cpus need to corelate (in a documented fashion) with > the order we need to plug the cpus in. By not documenting any order > libvirt can just guess it (by reimplementing the algorithm in qemu). > > I've pointed this out in a sub-thread of the cover-letter. So, based on Peter's comments (and others) I've concluded that the cpu-add implementation in terms of the new interface isn't really useful. However, I think this re-ordering is still a good idea, because: * It makes info hotpluggable-cpus easier to read in the HMP * Although we certainly need a better approach to handling hotplug + NUMA, correcting the order should allow the existing NUMA interface to work with only as much guesswork in libvirt as we already have. * We haven't released a qemu with query-hotpluggable-cpus yet, so we shouldn't need version conditions in order to use the order. For that reason I've tentatively merged this patch into ppc-for-2.7. It would be good to get an R-b or A-b from someone before I send a pull request (which I'm hoping to do tomorrow).
On Mon, 25 Jul 2016 16:09:04 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Wed, Jul 20, 2016 at 09:01:11AM +0200, Peter Krempa wrote: > > On Wed, Jul 20, 2016 at 11:00:05 +1000, David Gibson wrote: > > > On Tue, Jul 19, 2016 at 01:52:40PM +0200, Peter Krempa wrote: > > > > On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote: > > > > > The spapr implementation of query-hotpluggable-cpus builds the list of > > > > > hotpluggable cores from the end (most removed from the list head) > > > > > because that's the easiest way with a singly linked list. Because it > > > > > also traverses the possible CPU cores starting from low indexes the > > > > > resulting list has the cpu cores listed in reverse order by core-id. > > > > > > > > > > That's not generally harmful, but it means the output from "info > > > > > hotpluggable-cpus" is a little harder to read than it could be. > > > > > > > > > > Therefore, traverse the cpus in reverse order so that the final list > > > > > ends up in increasing-core-id order. > > > > > > > > To make this interface usable with in-order hotplug the ordering of the > > > > entries should be codified in the schema documentation. (see my response > > > > on the cover letter for justification). > > > > > > I'm not really sure what you mean by this. > > > > Currently the order of entries in reply of query-hotpluggable-cpus is > > arbitrary as this patch hints. > > > > If qemu won't support arbitrary order hotplug but libvirt should be able > > to use the new interface, then the order of replies of > > query-hotpluggable-cpus need to corelate (in a documented fashion) with > > the order we need to plug the cpus in. By not documenting any order > > libvirt can just guess it (by reimplementing the algorithm in qemu). > > > > I've pointed this out in a sub-thread of the cover-letter. > > So, based on Peter's comments (and others) I've concluded that the > cpu-add implementation in terms of the new interface isn't really > useful. > > However, I think this re-ordering is still a good idea, because: > * It makes info hotpluggable-cpus easier to read in the HMP > > * Although we certainly need a better approach to handling hotplug + > NUMA, correcting the order should allow the existing NUMA > interface to work with only as much guesswork in libvirt as we > already have. > > * We haven't released a qemu with query-hotpluggable-cpus yet, so we > shouldn't need version conditions in order to use the order. I've talked with Peter and meanwhile (till we have sane NUMA interface) libvirt will be guessing ids based on query-hotpluggable-cpus output sorted by socket/core/thread-id, so forward/reverse order won't really matter to it. Forward sorting is fine wrt 'info hotpluggable-cpus', however it might be better to do sorting in HMP callback so that each target won't have to do it nor regress command output in future or introduce another ordering. Considering that ordering affects only HMP won't require any compat glue even if it's not fixed in 2.7 release. > > For that reason I've tentatively merged this patch into ppc-for-2.7. > It would be good to get an R-b or A-b from someone before I send a > pull request (which I'm hoping to do tomorrow). >
On Mon, Jul 25, 2016 at 10:14:18AM +0200, Igor Mammedov wrote: > On Mon, 25 Jul 2016 16:09:04 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Wed, Jul 20, 2016 at 09:01:11AM +0200, Peter Krempa wrote: > > > On Wed, Jul 20, 2016 at 11:00:05 +1000, David Gibson wrote: > > > > On Tue, Jul 19, 2016 at 01:52:40PM +0200, Peter Krempa wrote: > > > > > On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote: > > > > > > The spapr implementation of query-hotpluggable-cpus builds the list of > > > > > > hotpluggable cores from the end (most removed from the list head) > > > > > > because that's the easiest way with a singly linked list. Because it > > > > > > also traverses the possible CPU cores starting from low indexes the > > > > > > resulting list has the cpu cores listed in reverse order by core-id. > > > > > > > > > > > > That's not generally harmful, but it means the output from "info > > > > > > hotpluggable-cpus" is a little harder to read than it could be. > > > > > > > > > > > > Therefore, traverse the cpus in reverse order so that the final list > > > > > > ends up in increasing-core-id order. > > > > > > > > > > To make this interface usable with in-order hotplug the ordering of the > > > > > entries should be codified in the schema documentation. (see my response > > > > > on the cover letter for justification). > > > > > > > > I'm not really sure what you mean by this. > > > > > > Currently the order of entries in reply of query-hotpluggable-cpus is > > > arbitrary as this patch hints. > > > > > > If qemu won't support arbitrary order hotplug but libvirt should be able > > > to use the new interface, then the order of replies of > > > query-hotpluggable-cpus need to corelate (in a documented fashion) with > > > the order we need to plug the cpus in. By not documenting any order > > > libvirt can just guess it (by reimplementing the algorithm in qemu). > > > > > > I've pointed this out in a sub-thread of the cover-letter. > > > > So, based on Peter's comments (and others) I've concluded that the > > cpu-add implementation in terms of the new interface isn't really > > useful. > > > > However, I think this re-ordering is still a good idea, because: > > * It makes info hotpluggable-cpus easier to read in the HMP > > > > * Although we certainly need a better approach to handling hotplug + > > NUMA, correcting the order should allow the existing NUMA > > interface to work with only as much guesswork in libvirt as we > > already have. > > > > * We haven't released a qemu with query-hotpluggable-cpus yet, so we > > shouldn't need version conditions in order to use the order. > I've talked with Peter and meanwhile (till we have sane NUMA interface) > libvirt will be guessing ids based on query-hotpluggable-cpus output > sorted by socket/core/thread-id, so forward/reverse order won't really > matter to it. > > Forward sorting is fine wrt 'info hotpluggable-cpus', however it might > be better to do sorting in HMP callback so that each target won't have > to do it nor regress command output in future or introduce another > ordering. > > Considering that ordering affects only HMP won't require any compat > glue even if it's not fixed in 2.7 release. Hm, ok, I'll drop this patch. > > > > > For that reason I've tentatively merged this patch into ppc-for-2.7. > > It would be good to get an R-b or A-b from someone before I send a > > pull request (which I'm hoping to do tomorrow). > > >
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 7f33a1b..6426b45 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2371,7 +2371,7 @@ static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine) int spapr_max_cores = max_cpus / smp_threads; int smt = kvmppc_smt_threads(); - for (i = 0; i < spapr_max_cores; i++) { + for (i = spapr_max_cores - 1; i >= 0; i--) { HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1); HotpluggableCPU *cpu_item = g_new0(typeof(*cpu_item), 1); CpuInstanceProperties *cpu_props = g_new0(typeof(*cpu_props), 1);
The spapr implementation of query-hotpluggable-cpus builds the list of hotpluggable cores from the end (most removed from the list head) because that's the easiest way with a singly linked list. Because it also traverses the possible CPU cores starting from low indexes the resulting list has the cpu cores listed in reverse order by core-id. That's not generally harmful, but it means the output from "info hotpluggable-cpus" is a little harder to read than it could be. Therefore, traverse the cpus in reverse order so that the final list ends up in increasing-core-id order. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)