Re: [Libguestfs] [p2v PATCH 2/6] restrict vCPU topology to (a) fully populated physical, or (b) 1 * N * 1

This concern arises because in the child process, only the one thread returning from fork() exists, thus if any locks are held by other threads in the parent process, those will never be released in the child process. But such a fork() should be safe: existent code already starts e.g. nbdkit on the same call path, namely start_conversion_thread() -> start_conversion() -> start_nbd_server() -> start_nbdkit() -> fork().)

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1590721 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- generate-p2v-config.pl | 45 ++++++++-------- p2v.h | 6 +++ cpuid.c | 39 ++++++++------ gui.c | 4 +- main.c | 8 +-- physical-xml.c | 54 ++++++++++---------- test-virt-p2v-cmdline.sh | 5 +- 7 files changed, 88 insertions(+), 73 deletions(-) diff --git a/generate-p2v-config.pl b/generate-p2v-config.pl index 06a2e2c8b0e4..5c33f814e3e7 100755 --- a/generate-p2v-config.pl +++ b/generate-p2v-config.pl @@ -110,16 +110,19 @@ my @fields = [ ], ), ConfigString->new(name => 'guestname'), - ConfigInt->new(name => 'vcpus', value => 0), + ConfigSection->new( + name => 'vcpu', + elements => [ + ConfigBool->new(name => 'dense_topo'), + ConfigInt->new(name => 'cores', value => 0), + ], + ), ConfigUInt64->new(name => 'memory'), ConfigSection->new( name => 'cpu', elements => [ ConfigString->new(name => 'vendor'), ConfigString->new(name => 'model'), - ConfigUnsigned->new(name => 'sockets'), - ConfigUnsigned->new(name => 'cores'), - ConfigUnsigned->new(name => 'threads'), ConfigBool->new(name => 'acpi'), ConfigBool->new(name => 'apic'), ConfigBool->new(name => 'pae'), @@ -231,11 +234,21 @@ The name of the guest that is created. The default is to try to derive a name from the physical machine’s hostname (if possible) else use a randomly generated name.", ), - "p2v.vcpus" => manual_entry->new( + "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.", + ), + "p2v.vcpu.cores" => manual_entry->new( shortopt => "N", description => " -The number of virtual CPUs to give to the guest. The default is to -use the same as the number of physical CPUs.", +This setting is ignored if C<p2v.vcpu.dense_topo> is enabled. +Otherwise, it specifies the flat number of vCPU cores to give to the +guest (placing all of those cores into a single socket, and exposing one +thread per core). The default value is the number of online logical +processors on the physical machine.", ), "p2v.memory" => manual_entry->new( shortopt => "n(M|G)", @@ -258,24 +271,6 @@ the same CPU vendor as the physical machine.", description => " The vCPU model, eg. \"IvyBridge\". The default is to use the same CPU model as the physical machine.", - ), - "p2v.cpu.sockets" => manual_entry->new( - shortopt => "N", - description => " -Number of vCPU sockets to use. The default is to use the same as the -physical machine.", - ), - "p2v.cpu.cores" => manual_entry->new( - shortopt => "N", - description => " -Number of vCPU cores to use. The default is to use the same as the -physical machine.", - ), - "p2v.cpu.threads" => manual_entry->new( - shortopt => "N", - description => " -Number of vCPU hyperthreads to use. The default is to use the same -as the physical machine.", ), "p2v.cpu.acpi" => manual_entry->new( shortopt => "", # ignored for booleans diff --git a/p2v.h b/p2v.h index 20c4ac7e506a..32dc55d94de5 100644 --- a/p2v.h +++ b/p2v.h @@ -60,6 +60,12 @@ extern int feature_colours_option; extern int force_colour; /* cpuid.c */ +struct cpu_topo { + unsigned sockets; + unsigned cores; + unsigned threads; +}; +extern void get_cpu_topology (struct cpu_topo *topo); extern void get_cpu_config (struct cpu_config *); /* rtc.c */ diff --git a/cpuid.c b/cpuid.c index 84603bbb3374..31a1720dc23c 100644 --- a/cpuid.c +++ b/cpuid.c @@ -154,22 +154,32 @@ get_vendor (char **lscpu, struct cpu_config *cpu) } /** - * Read the CPU topology from lscpu output. + * Read the CPU topology from a separate lscpu invocation. */ -static void -get_topology (char **lscpu, struct cpu_config *cpu) +void +get_cpu_topology (struct cpu_topo *topo) { - const char *v; - - v = get_field (lscpu, "Socket(s)"); - if (v) - ignore_value (sscanf (v, "%u", &cpu->sockets)); - v = get_field (lscpu, "Core(s) per socket"); - if (v) - ignore_value (sscanf (v, "%u", &cpu->cores)); - v = get_field (lscpu, "Thread(s) per core"); - if (v) - ignore_value (sscanf (v, "%u", &cpu->threads)); + + CLEANUP_FREE_STRING_LIST char **lscpu = NULL; + + lscpu = get_lscpu (); + + if (lscpu != NULL) { + const char *sockets, *cores, *threads; + + sockets = get_field (lscpu, "Socket(s)"); + cores = get_field (lscpu, "Core(s) per socket"); + threads = get_field (lscpu, "Thread(s) per core"); + if (sockets && cores && threads) { + ignore_value (sscanf (sockets, "%u", &topo->sockets)); + ignore_value (sscanf (cores, "%u", &topo->cores)); + ignore_value (sscanf (threads, "%u", &topo->threads)); + return; + } + } + topo->sockets = 1; + topo->cores = 1; + topo->threads = 1; } /** @@ -211,7 +221,6 @@ get_cpu_config (struct cpu_config *cpu) lscpu = get_lscpu (); if (lscpu != NULL) { get_vendor (lscpu, cpu); - get_topology (lscpu, cpu); get_flags (lscpu, cpu); } diff --git a/gui.c b/gui.c index 5c4f1343095a..b4a9fed18410 100644 --- a/gui.c +++ b/gui.c @@ -767,7 +767,7 @@ create_conversion_dialog (struct config *config) set_alignment (vcpus_label, 1., 0.5); vcpus_entry = gtk_entry_new (); gtk_label_set_mnemonic_widget (GTK_LABEL (vcpus_label), vcpus_entry); - snprintf (vcpus_str, sizeof vcpus_str, "%d", config->vcpus); + snprintf (vcpus_str, sizeof vcpus_str, "%d", config->vcpu.cores); gtk_entry_set_text (GTK_ENTRY (vcpus_entry), vcpus_str); table_attach (target_tbl, vcpus_entry, 1, 2, 1, 2, GTK_FILL, GTK_FILL, 1, 1); @@ -2010,7 +2010,7 @@ start_conversion_clicked (GtkWidget *w, gpointer data) return; } - config->vcpus = get_vcpus_from_conv_dlg (); + config->vcpu.cores = get_vcpus_from_conv_dlg (); config->memory = get_memory_from_conv_dlg (); /* Get the list of disks to be converted. */ diff --git a/main.c b/main.c index 0ebb7291c7ce..6ac94fcb5f8e 100644 --- a/main.c +++ b/main.c @@ -291,16 +291,18 @@ set_config_defaults (struct config *config) } config->guestname = strdup (hostname); + config->vcpu.dense_topo = false; + /* Defaults for #vcpus and memory are taken from the physical machine. */ i = sysconf (_SC_NPROCESSORS_ONLN); if (i == -1) { perror ("sysconf: _SC_NPROCESSORS_ONLN"); - config->vcpus = 1; + config->vcpu.cores = 1; } else if (i == 0) - config->vcpus = 1; + config->vcpu.cores = 1; else - config->vcpus = i; + config->vcpu.cores = i; i = sysconf (_SC_PHYS_PAGES); if (i == -1) { diff --git a/physical-xml.c b/physical-xml.c index 4e830ea7ee0c..c27869de1c35 100644 --- a/physical-xml.c +++ b/physical-xml.c @@ -62,6 +62,7 @@ generate_physical_xml (struct config *config, struct data_conn *data_conns, uint64_t memkb; CLEANUP_XMLFREETEXTWRITER xmlTextWriterPtr xo = NULL; size_t i; + struct cpu_topo topo; xo = xmlNewTextWriterFilename (filename, 0); if (xo == NULL) @@ -104,34 +105,35 @@ generate_physical_xml (struct config *config, struct data_conn *data_conns, string_format ("%" PRIu64, memkb); } end_element (); - single_element_format ("vcpu", "%d", config->vcpus); - - if (config->cpu.vendor || config->cpu.model || - config->cpu.sockets || config->cpu.cores || config->cpu.threads) { - /* https://libvirt.org/formatdomain.html#elementsCPU */ - start_element ("cpu") { - attribute ("match", "minimum"); - if (config->cpu.vendor) - single_element ("vendor", config->cpu.vendor); - if (config->cpu.model) { - start_element ("model") { - attribute ("fallback", "allow"); - string (config->cpu.model); - } end_element (); - } - if (config->cpu.sockets || config->cpu.cores || config->cpu.threads) { - start_element ("topology") { - if (config->cpu.sockets) - attribute_format ("sockets", "%u", config->cpu.sockets); - if (config->cpu.cores) - attribute_format ("cores", "%u", config->cpu.cores); - if (config->cpu.threads) - attribute_format ("threads", "%u", config->cpu.threads); - } end_element (); - } - } end_element (); + if (config->vcpu.dense_topo) + get_cpu_topology (&topo); + else { + topo.sockets = 1; + topo.cores = config->vcpu.cores; + topo.threads = 1; } + single_element_format ("vcpu", "%u", + topo.sockets * topo.cores * topo.threads); + + /* https://libvirt.org/formatdomain.html#elementsCPU */ + start_element ("cpu") { + attribute ("match", "minimum"); + if (config->cpu.vendor) + single_element ("vendor", config->cpu.vendor); + if (config->cpu.model) { + start_element ("model") { + attribute ("fallback", "allow"); + string (config->cpu.model); + } end_element (); + } + start_element ("topology") { + attribute_format ("sockets", "%u", topo.sockets); + attribute_format ("cores", "%u", topo.cores); + attribute_format ("threads", "%u", topo.threads); + } end_element (); + } end_element (); + switch (config->rtc.basis) { case BASIS_UNKNOWN: /* Don't emit any <clock> element. */ diff --git a/test-virt-p2v-cmdline.sh b/test-virt-p2v-cmdline.sh index cd76044195a3..37dc1662a593 100755 --- a/test-virt-p2v-cmdline.sh +++ b/test-virt-p2v-cmdline.sh @@ -26,7 +26,7 @@ out=test-virt-p2v-cmdline.out rm -f $out # The Linux kernel command line. -$VG virt-p2v --cmdline='p2v.server=localhost p2v.port=123 p2v.username=user p2v.password=secret p2v.skip_test_connection p2v.name=test p2v.vcpus=4 p2v.memory=1G p2v.disks=sda,sdb,sdc p2v.removable=sdd p2v.interfaces=eth0,eth1 p2v.o=local p2v.oa=sparse p2v.oc=qemu:///session p2v.of=raw p2v.os=/var/tmp p2v.network=em1:wired,other p2v.dump_config_and_exit' > $out +$VG virt-p2v --cmdline='p2v.server=localhost p2v.port=123 p2v.username=user p2v.password=secret p2v.skip_test_connection p2v.name=test p2v.vcpu.cores=4 p2v.memory=1G p2v.disks=sda,sdb,sdc p2v.removable=sdd p2v.interfaces=eth0,eth1 p2v.o=local p2v.oa=sparse p2v.oc=qemu:///session p2v.of=raw p2v.os=/var/tmp p2v.network=em1:wired,other p2v.dump_config_and_exit' > $out # For debugging purposes. cat $out @@ -37,7 +37,8 @@ grep "^remote\.port.*123" $out grep "^auth\.username.*user" $out grep "^auth\.sudo.*false" $out grep "^guestname.*test" $out -grep "^vcpus.*4" $out +grep "^vcpu.dense_topo.*false" $out +grep "^vcpu.cores.*4" $out grep "^memory.*"$((1024*1024*1024)) $out grep "^disks.*sda sdb sdc" $out grep "^removable.*sdd" $out