Re: [PATCH v2 7/7] x86/hap: Increase the number of initial mempool_size to 1024 pages

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.04.2024 17:40, Petr Beneš wrote:
> On Tue, Apr 30, 2024 at 4:47 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 28.04.2024 18:52, Petr Beneš wrote:
>>> From: Petr Beneš <w1benny@xxxxxxxxx>
>>>
>>> This change anticipates scenarios where `max_altp2m` is set to its maximum
>>> supported value (i.e., 512), ensuring sufficient memory is allocated upfront
>>> to accommodate all altp2m tables without initialization failure.
>>
>> And guests with fewer or even no altp2m-s still need the same bump? You
>> know the number of altp2m-s upon domain creation, so why bump by any more
>> than what's strictly needed for that?
> 
> I have to admit I've considered computing the value which goes to
> hap_set_allocation
> by simply adding 256 + max_altp2m, but that felt so arbitrary - the
> 256 value itself
> feels arbitrary, as I haven't found any reasoning for it anywhere.
> 
> I have also tried to make code changes to make the initial allocation
> size configurable
> via libxl (possibly reusing the shadow_memkb) - which seemed to me
> like the "correct"
> solution, but those changes were more complicated than I had
> anticipated and I would
> definitely not make it till the 4.19 deadline.
> 
> Question is, what to do now? Should I change it to 256 + max_altp2m?

Counter question: Is accounting for just the root page table really
enough? Meaning to say: I'm not convinced that minimum would really
be appropriate for altp2m use even before your changes. You growing
the number of root page tables _always_ required just makes things
worse even without considering how (many) altp2m-s are then going
to be used. Such an issue, if I'm right with this, would imo want
addressing up front, in a separate patch.

You may also want to take a look at the shadow side of things, even
if for altp2m shadow mode may not be relevant. Things like
sh_min_allocation() and shadow_min_acceptable_pages() may well need
proper counterparts in HAP now.

>>> --- a/tools/tests/paging-mempool/test-paging-mempool.c
>>> +++ b/tools/tests/paging-mempool/test-paging-mempool.c
>>> @@ -35,7 +35,7 @@ static struct xen_domctl_createdomain create = {
>>>
>>>  static uint64_t default_mempool_size_bytes =
>>>  #if defined(__x86_64__) || defined(__i386__)
>>> -    256 << 12; /* Only x86 HAP for now.  x86 Shadow needs more work. */
>>> +    1024 << 12; /* Only x86 HAP for now.  x86 Shadow needs more work. */
>>
>> I also can't derive from the description why we'd need to go from 256 to
>> 1024 here and ...
> 
> It's explained in the code few lines below:
> 
>     /*
>      * Check that the domain has the expected default allocation size.  This
>      * will fail if the logic in Xen is altered without an equivalent
>      * adjustment here.
>      */
> 
> I have verified that the default_mempool_size_bytes must reflect the number
> of pages in the initial hap_set_allocation() call.
> 
> Is it something I should include in the commit message, too?

Well, yes and no. My question wasn't about the "why", but ...

>>> --- a/xen/arch/x86/mm/hap/hap.c
>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>> @@ -468,7 +468,7 @@ int hap_enable(struct domain *d, u32 mode)
>>>      if ( old_pages == 0 )
>>>      {
>>>          paging_lock(d);
>>> -        rv = hap_set_allocation(d, 256, NULL);
>>> +        rv = hap_set_allocation(d, 1024, NULL);
>>
>> ... here. You talk of (up to) 512 pages there only.

... about the "by how many".

>> Also isn't there at least one more place where the tool stack (libxl I
>> think) would need changing, where Dom0 ballooning needs are calculated?
>> And/or doesn't the pool size have a default calculation in the tool
>> stack, too?
> 
> I have found places in libxl where the mempool_size is calculated, but
> that mempool
> size is then set AFTER the domain is created via xc_set_paging_mempool_size.
> 
> In my opinion it doesn't necessarily require change, since it's
> expected by the user
> to manually set it via shadow_memkb. The only current problem is (which this
> commit is trying to fix) that setting shadow_memkb doesn't help when
> max_altp2m > (256 - 1 + vcpus + MAX_NESTEDP2M), since the initial mempool
> size is hardcoded.

Wait - are you saying the guest config value isn't respected in certain
cases? That would be another thing wanting to be fixed separately, up
front.

Jan