Re: [pve-devel] [PATCH v2 qemu-server 6/9] memory: use 64 slots && static dimm size when max is defined

["DERUMIER, Alexandre" <Alexandre.DERUMIER@groupe-cyllene.com>]

> 
> Question about the existing code: The loops below can count up to
> $dimm_id 255, but in the commit message you say that there are at
> most
> 255 slots (so the highest ID is 254?). But yeah, it only becomes
> relevant when going all the way to approximately 4 TiB.
yes, the max slot is 255 (id0->id254).
If I remember (2015 ^_^), the last iteration (dimm id 255) of the 8x32
loop, was over the value supported by qemu or the conf (because of the
static memory), so it always return .

> 
> > @@ -209,7 +216,7 @@ sub foreach_dimm{
> >             &$func($conf, $vmid, $name, $dimm_size, $numanode,
> > $current_size, $memory);
> >             return  $current_size if $current_size >= $memory;
> >         }
> > -       $dimm_size *= 2;
> > +       $dimm_size *= 2 if !$confmem->{max};
> >      }
> >  }
> >  
> > @@ -220,7 +227,12 @@ sub foreach_reverse_dimm {
> 
> Question about the existing code: There is
> my $dimm_id = 253;
> Shouldn't that start at 254 (highest valid ID we can count up to?).
> Again only becomes relevant with a lot of memory.
> 
mmm, I really didn't remember. I need to double check, but I think it
should 254 indeed.

> >      my $current_size = 0;
> >      my $dimm_size = 0;
> >  
> > -    if($conf->{hugepages} && $conf->{hugepages} == 1024) {
> > +    my $confmem = parse_memory($conf->{memory});
> > +    if ($confmem->{max}) {
> > +       $dimm_id = $MAX_SLOTS - 1;
> > +       $current_size = $confmem->{max};
> 
> Does this need to be $confmem->{max} + $static_size? See below for a
> description of the issue. Didn't think about it in detail, so please
> double check ;)

mmm, I wonder if I don't lower the number of slots, as "max" option
from config, is "static + x dimm slots", but in this case it should
depend of the number of sockets. 

> > +       $dimm_size = $confmem->{max} / $MAX_SLOTS;
> > +    } elsif ($conf->{hugepages} && $conf->{hugepages} == 1024) {
> >         $current_size = 8355840;
> >         $dimm_size = 131072;
> >      } else {
> 
> Nit: the loops below here are
>     for (my $j = 0; $j < 8; $j++) {
>         for (my $i = 0; $i < 32; $i++) {
> so it looks like potentially iterating more often than $MAX_SLOTS and
> reaching negative $dimm_ids. I know that we should always return from
> the loop earlier than that, but maybe it can be improved by
> extracting
> the inner part in a sub/closure and using different loops depending
> on
> how many slots there are? Same applies to foreach_dimm().
> 
> 
Yes, I think it should be better.

> Real issue: something is wrong with the calculation for unplugging in
> combination with 'max' (it uses the wrong dimm IDs):
> 
> 
I have verify myself, indeed , it's really buggy.

I'll work on it this weekend, thanks for the review !