[pve-devel] [PATCH 2/2] hugepages: use hostnodes value as numanode for topology

[Alexandre DERUMIER]

>>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" <w.bumiller at proxmox.com>
À: "aderumier" <aderumier at odiso.com>
Cc: "pve-devel" <pve-devel at pve.proxmox.com>
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" <w.bumiller at proxmox.com> 
> À: "aderumier" <aderumier at odiso.com> 
> Cc: "pve-devel" <pve-devel at pve.proxmox.com> 
> 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 <aderumier at odiso.com> 
> > --- 
> > 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 
> > pve-devel at pve.proxmox.com 
> > http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel 
> > 
> 
>