Richard W.M. Jones
2022-Sep-08 13:36 UTC
[Libguestfs] [p2v PATCH 2/6] restrict vCPU topology to (a) fully populated physical, or (b) 1 * N * 1
(Adding Dan for input) On Thu, Sep 08, 2022 at 03:23:41PM +0200, Laszlo Ersek wrote:> On 09/08/22 10:03, Richard W.M. Jones wrote: > > On Mon, Sep 05, 2022 at 01:25:27PM +0200, Laszlo Ersek wrote: > >> + "p2v.vcpu.dense_topo" => manual_entry->new( > >> + shortopt => "", # ignored for booleans > >> + description => " > >> +Copy the physical machine's CPU topology, densely populated, to the > >> +guest. Disabled by default. If disabled, the C<p2v.vcpu.cores> setting > >> +takes effect.", > > > > > > I just realised I'm not completely sure what "densely populated" > > means here. I think we should have a bit more explanation. > > > > How about something like: > > > > "p2v.vcpu.dense_topo" => manual_entry->new( > > shortopt => "", # ignored for booleans > > description => " > > Copy the physical machine's complete CPU topology (sockets, cores and > > threads) to the guest. Disabled by default. If disabled, the > > C<p2v.vcpu.cores> setting takes effect.", > > > > (Which might also imply that we rename this something like > > "complete_topo" or "full_topo" but I'll leave that to you.) > > By "dense", I meant to express that there are no gaps in the onlining of > the CPU topology. > > Assume we have 2 sockets, 2 cores/socket, 2 theads/core. Assume CPU#1 > (in socket#1) is hot-pluggable, but isn't currently plugged, only CPU#0 > (in socket#0) is present -- making for 1*2*2 = 4 logical processors in > total. A physical machine may well boot like this. Then our topology is > 2*2*2, but we only have 4 logical processors, so the topology is not > densely populated. The language is supposed to express that in any such > case, we'll ignore the online / plugged / etc count, and we'll just grab > the static topology, and fully / densely populate it with logical > processors. > > "Complete topology" does not express this. Sticking with the above > example, the topology is already complete on the physical machine (we > have full information about the levels of the hierarchy), but it's not > densely populated. > > Another example would be 1 * 4 * 2 physical (a normal low-end machine by > today's standards), where the sysadmin disables (say) cores #1 and #2 > using /sys/devices/system/cpu/cpu{1,2}/online. (I think this may even be > possible on the kernel command line, for whatever reason necessary.) In > this case, during conversion, if "dense_topo" is set, we carry over not > just the topology (= the 1 * 4 * 2 hierarchy), but we also densely > populate it (producing 8 logical processors in the conversion output, > disregarding the "gaps" on the source; i.e. that only 4 logical > processors were available on the physical machine originally.) > > I considered "complete", and thought it didn't express my intent. "Full" > is so-so -- my problem is it seems to have two meanings; one is in fact > what I'm trying to say with "dense", but the other meaning is just > "complete", which I don't find good. > > The choices p2v should offer are: > > - Just carry over a flat VCPU count N --> this maps to a 1 socket * N > cores/socket * 1 thread / core topology, fully populated. > > - Otherwise (i.e., when the dense_topo knob is enabled), convert the > original topology (S sockets * C/S cores/socket * T threads/core), *AND* > fully populate that topology (disregarding the original "online count" > on the physical machine, which may easily be less than the (S * C * T) > product.)I think the "mot juste" has to express that we're trying to model as closely as possible the real physical topology. (The denseness doesn't seem to be so important - are there many machines where CPUs are not online? Can that even happen when virt-p2v is running?) How about: authentic_topo physical_topo accurate_topo ...? The patch is totally fine, we're just quibbling about the word "dense" :-) Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
Laszlo Ersek
2022-Sep-08 14:22 UTC
[Libguestfs] [p2v PATCH 2/6] restrict vCPU topology to (a) fully populated physical, or (b) 1 * N * 1
On 09/08/22 15:36, Richard W.M. Jones wrote:> (Adding Dan for input) > > On Thu, Sep 08, 2022 at 03:23:41PM +0200, Laszlo Ersek wrote: >> On 09/08/22 10:03, Richard W.M. Jones wrote: >>> On Mon, Sep 05, 2022 at 01:25:27PM +0200, Laszlo Ersek wrote: >>>> + "p2v.vcpu.dense_topo" => manual_entry->new( >>>> + shortopt => "", # ignored for booleans >>>> + description => " >>>> +Copy the physical machine's CPU topology, densely populated, to the >>>> +guest. Disabled by default. If disabled, the C<p2v.vcpu.cores> setting >>>> +takes effect.", >>> >>> >>> I just realised I'm not completely sure what "densely populated" >>> means here. I think we should have a bit more explanation. >>> >>> How about something like: >>> >>> "p2v.vcpu.dense_topo" => manual_entry->new( >>> shortopt => "", # ignored for booleans >>> description => " >>> Copy the physical machine's complete CPU topology (sockets, cores and >>> threads) to the guest. Disabled by default. If disabled, the >>> C<p2v.vcpu.cores> setting takes effect.", >>> >>> (Which might also imply that we rename this something like >>> "complete_topo" or "full_topo" but I'll leave that to you.) >> >> By "dense", I meant to express that there are no gaps in the onlining of >> the CPU topology. >> >> Assume we have 2 sockets, 2 cores/socket, 2 theads/core. Assume CPU#1 >> (in socket#1) is hot-pluggable, but isn't currently plugged, only CPU#0 >> (in socket#0) is present -- making for 1*2*2 = 4 logical processors in >> total. A physical machine may well boot like this. Then our topology is >> 2*2*2, but we only have 4 logical processors, so the topology is not >> densely populated. The language is supposed to express that in any such >> case, we'll ignore the online / plugged / etc count, and we'll just grab >> the static topology, and fully / densely populate it with logical >> processors. >> >> "Complete topology" does not express this. Sticking with the above >> example, the topology is already complete on the physical machine (we >> have full information about the levels of the hierarchy), but it's not >> densely populated. >> >> Another example would be 1 * 4 * 2 physical (a normal low-end machine by >> today's standards), where the sysadmin disables (say) cores #1 and #2 >> using /sys/devices/system/cpu/cpu{1,2}/online. (I think this may even be >> possible on the kernel command line, for whatever reason necessary.) In >> this case, during conversion, if "dense_topo" is set, we carry over not >> just the topology (= the 1 * 4 * 2 hierarchy), but we also densely >> populate it (producing 8 logical processors in the conversion output, >> disregarding the "gaps" on the source; i.e. that only 4 logical >> processors were available on the physical machine originally.) >> >> I considered "complete", and thought it didn't express my intent. "Full" >> is so-so -- my problem is it seems to have two meanings; one is in fact >> what I'm trying to say with "dense", but the other meaning is just >> "complete", which I don't find good. >> >> The choices p2v should offer are: >> >> - Just carry over a flat VCPU count N --> this maps to a 1 socket * N >> cores/socket * 1 thread / core topology, fully populated. >> >> - Otherwise (i.e., when the dense_topo knob is enabled), convert the >> original topology (S sockets * C/S cores/socket * T threads/core), *AND* >> fully populate that topology (disregarding the original "online count" >> on the physical machine, which may easily be less than the (S * C * T) >> product.) > > I think the "mot juste" has to express that we're trying to model as > closely as possible the real physical topology. (The denseness > doesn't seem to be so important - are there many machines where CPUs > are not online?Well it's a possibility.> Can that even happen when virt-p2v is running?)I think so, yes; although it should be really rare (physically removed CPU from multi-socket systems, or some cores offlined on the kernel cmdline perhaps -- I think there could be reasons for that).> > How about: > > authentic_topo > physical_topo > accurate_topo > > ...? > > The patch is totally fine, we're just quibbling about the > word "dense" :-)I'll rename "dense_topo" to "phys_topo", and also adopt your suggestion for the manual. Thanks! Laszlo
Daniel P. Berrangé
2022-Sep-08 14:39 UTC
[Libguestfs] [p2v PATCH 2/6] restrict vCPU topology to (a) fully populated physical, or (b) 1 * N * 1
On Thu, Sep 08, 2022 at 02:36:15PM +0100, Richard W.M. Jones wrote:> (Adding Dan for input) > > On Thu, Sep 08, 2022 at 03:23:41PM +0200, Laszlo Ersek wrote: > > On 09/08/22 10:03, Richard W.M. Jones wrote: > > > On Mon, Sep 05, 2022 at 01:25:27PM +0200, Laszlo Ersek wrote: > > >> + "p2v.vcpu.dense_topo" => manual_entry->new( > > >> + shortopt => "", # ignored for booleans > > >> + description => " > > >> +Copy the physical machine's CPU topology, densely populated, to the > > >> +guest. Disabled by default. If disabled, the C<p2v.vcpu.cores> setting > > >> +takes effect.", > > > > > > > > > I just realised I'm not completely sure what "densely populated" > > > means here. I think we should have a bit more explanation. > > > > > > How about something like: > > > > > > "p2v.vcpu.dense_topo" => manual_entry->new( > > > shortopt => "", # ignored for booleans > > > description => " > > > Copy the physical machine's complete CPU topology (sockets, cores and > > > threads) to the guest. Disabled by default. If disabled, the > > > C<p2v.vcpu.cores> setting takes effect.", > > > > > > (Which might also imply that we rename this something like > > > "complete_topo" or "full_topo" but I'll leave that to you.) > > > > By "dense", I meant to express that there are no gaps in the onlining of > > the CPU topology. > > > > Assume we have 2 sockets, 2 cores/socket, 2 theads/core. Assume CPU#1 > > (in socket#1) is hot-pluggable, but isn't currently plugged, only CPU#0 > > (in socket#0) is present -- making for 1*2*2 = 4 logical processors in > > total. A physical machine may well boot like this. Then our topology is > > 2*2*2, but we only have 4 logical processors, so the topology is not > > densely populated. The language is supposed to express that in any such > > case, we'll ignore the online / plugged / etc count, and we'll just grab > > the static topology, and fully / densely populate it with logical > > processors. > > > > "Complete topology" does not express this. Sticking with the above > > example, the topology is already complete on the physical machine (we > > have full information about the levels of the hierarchy), but it's not > > densely populated. > > > > Another example would be 1 * 4 * 2 physical (a normal low-end machine by > > today's standards), where the sysadmin disables (say) cores #1 and #2 > > using /sys/devices/system/cpu/cpu{1,2}/online. (I think this may even be > > possible on the kernel command line, for whatever reason necessary.) In > > this case, during conversion, if "dense_topo" is set, we carry over not > > just the topology (= the 1 * 4 * 2 hierarchy), but we also densely > > populate it (producing 8 logical processors in the conversion output, > > disregarding the "gaps" on the source; i.e. that only 4 logical > > processors were available on the physical machine originally.) > > > > I considered "complete", and thought it didn't express my intent. "Full" > > is so-so -- my problem is it seems to have two meanings; one is in fact > > what I'm trying to say with "dense", but the other meaning is just > > "complete", which I don't find good. > > > > The choices p2v should offer are: > > > > - Just carry over a flat VCPU count N --> this maps to a 1 socket * N > > cores/socket * 1 thread / core topology, fully populated. > > > > - Otherwise (i.e., when the dense_topo knob is enabled), convert the > > original topology (S sockets * C/S cores/socket * T threads/core), *AND* > > fully populate that topology (disregarding the original "online count" > > on the physical machine, which may easily be less than the (S * C * T) > > product.) > > I think the "mot juste" has to express that we're trying to model as > closely as possible the real physical topology. (The denseness > doesn't seem to be so important - are there many machines where CPUs > are not online? Can that even happen when virt-p2v is running?) > > How about: > > authentic_topo > physical_topo > accurate_topo > > ...? > > The patch is totally fine, we're just quibbling about the > word "dense" :-)Why not 'host_topo' since it is mirroring the host ? With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|