>>Looks like we still need to do some more mapping though, as the foreach >>function still uses '$i % $sockets' for the numa node index.
Oh, I didn't review the hotplug memory part yet (I have tested succefully my patch without memory hotplug for now) >>If the above makes sense to you I can prepare a patch for you to review? I think it's ok, I'll check that next week. ----- Mail original ----- De: "Wolfgang Bumiller" <[email protected]> À: "aderumier" <[email protected]> Cc: "pve-devel" <[email protected]> Envoyé: Vendredi 29 Juillet 2016 11:14:22 Objet: Re: [pve-devel] [PATCH 2/2] hugepages: use hostnodes value as numanode for topology On Fri, Jul 29, 2016 at 10:40:37AM +0200, Alexandre DERUMIER wrote: > >>I don't really see the relation there, why would this restriction apply > >>specifically with hugepages enabled? Or is it just a performance > >>concern? > > yes,it's only when hugepages is enabled. > > As we reserved hugepages, I really don't known how qemu will try to split > memory with > > hostnodes=0;1,memory=4096 > > or > > hostnodes=0;2-3,memory=4096 > > > I think it's random, so in this case we should reserved pages on all nodes to > be sure. > Seem to be a little be complex to manage. Makes sense. Looks like we still need to do some more mapping though, as the foreach function still uses '$i % $sockets' for the numa node index. With custom numa entries this matches neither those nor the host nodes, especially with hostnodes defined. I think we can do it this way: * in foreach_dimm: Count existing $conf->{numaX} and if any are defined use that count instead of $sockets. * go through foreach* uses and make sure numa nodes are mapped accordingly: - qemu_memory_hotplug(): foreach_dimm's passed $numanode parameter is used in two ways: | my $hugepages_topology->{$hugepages_size}->{$numanode} = hugepages_nr($dimm_size, $hugepages_size); This needs to be mapped to $conf->{numa$numanode}'s hostnode. | eval { PVE::QemuServer::vm_mon_cmd($vmid, "device_add", driver => "pc-dimm", id => "$name", memdev => "mem-$name", node => $numanode) }; This should be fine once foreach_dimm() takes numaX entries into account as it refers to the guest's node index AFAIK. - config(): Should be fine, refers to guest node index. - hugepages_topology(): Needs to be mapped to hostnode. If the above makes sense to you I can prepare a patch for you to review? > > > ----- Mail original ----- > De: "Wolfgang Bumiller" <[email protected]> > À: "aderumier" <[email protected]> > Cc: "pve-devel" <[email protected]> > Envoyé: Vendredi 29 Juillet 2016 09:48:04 > Objet: Re: [pve-devel] [PATCH 2/2] hugepages: use hostnodes value as numanode > for topology > > On Fri, Jul 29, 2016 at 05:17:34AM +0200, Alexandre Derumier wrote: > > also disallow more than 1 hostnode value > > I don't really see the relation there, why would this restriction apply > specifically with hugepages enabled? Or is it just a performance > concern? > > Patch seems fine otherwise. > > > > > Signed-off-by: Alexandre Derumier <[email protected]> > > --- > > PVE/QemuServer/Memory.pm | 35 +++++++++++++++++++++++------------ > > 1 file changed, 23 insertions(+), 12 deletions(-) > > > > diff --git a/PVE/QemuServer/Memory.pm b/PVE/QemuServer/Memory.pm > > index 58c7217..37f285a 100644 > > --- a/PVE/QemuServer/Memory.pm > > +++ b/PVE/QemuServer/Memory.pm > > @@ -230,17 +230,8 @@ sub config { > > # hostnodes > > my $hostnodelists = $numa->{hostnodes}; > > if (defined($hostnodelists)) { > > - my $hostnodes; > > - foreach my $hostnoderange (@$hostnodelists) { > > - my ($start, $end) = @$hostnoderange; > > - $hostnodes .= ',' if $hostnodes; > > - $hostnodes .= $start; > > - $hostnodes .= "-$end" if defined($end); > > - $end //= $start; > > - for (my $i = $start; $i <= $end; ++$i ) { > > - die "host NUMA node$i doesn't exist\n" if ! -d > > "/sys/devices/system/node/node$i/"; > > - } > > - } > > + > > + my $hostnodes = print_numa_hostnodes($hostnodelists); > > > > # policy > > my $policy = $numa->{policy}; > > @@ -311,6 +302,23 @@ sub print_mem_object { > > > > } > > > > +sub print_numa_hostnodes { > > + my ($hostnodelists) = @_; > > + > > + my $hostnodes; > > + foreach my $hostnoderange (@$hostnodelists) { > > + my ($start, $end) = @$hostnoderange; > > + $hostnodes .= ',' if $hostnodes; > > + $hostnodes .= $start; > > + $hostnodes .= "-$end" if defined($end); > > + $end //= $start; > > + for (my $i = $start; $i <= $end; ++$i ) { > > + die "host NUMA node$i doesn't exist\n" if ! -d > > "/sys/devices/system/node/node$i/"; > > + } > > + } > > + return $hostnodes; > > +} > > + > > sub hugepages_mount { > > > > my $mountdata = PVE::ProcFSTools::parse_proc_mounts(); > > @@ -406,9 +414,12 @@ sub hugepages_topology { > > > > $numa_custom_topology = 1; > > my $numa_memory = $numa->{memory}; > > + my $hostnodelists = $numa->{hostnodes}; > > + my $hostnodes = print_numa_hostnodes($hostnodelists); > > > > + die "more than 1 hostnode value in numa node is not supported when > > hugepages are enabled" if $hostnodes !~ m/^(\d)$/; > > my $hugepages_size = hugepages_size($conf, $numa_memory); > > - $hugepages_topology->{$hugepages_size}->{$i} += > > hugepages_nr($numa_memory, $hugepages_size); > > + $hugepages_topology->{$hugepages_size}->{$hostnodes} += > > hugepages_nr($numa_memory, $hugepages_size); > > > > } > > > > -- > > 2.1.4 > > > > _______________________________________________ > > pve-devel mailing list > > [email protected] > > http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > > > _______________________________________________ pve-devel mailing list [email protected] http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
