Re: [PATCH V1 2/3] mshv: Redesign hypervisor memory deposit logic

[Mukesh R <mrathor@linux.microsoft.com>]

On 9/9/26 10:32, Michael Kelley wrote:
> From: Mukesh R <mrathor@linux.microsoft.com> Sent: Tuesday, September 8, 2026 3:33 PM
>>
>> On 9/8/26 11:02, Michael Kelley wrote:
>>> From: Mukesh R <mrathor@linux.microsoft.com> Sent: Monday, September 7, 2026 7:14 PM
>>>>
>>>> The current memory deposit implementation has a few issues and bugs:
>>>>    o It is very slow
>>>>    o Contiguous range requirement is broken, and is critical bug
>>>>    o An incorrect assumption is made that contiguous memory size would
>>>>      always be power of 2.
>>>
>>> Is this HV_MAX_CONTIGUOUS_ALLOCATION_PAGES? That value is
>>> defined as a constant in hvhdk_mini.h. So is there a possibility that
>>> the constant will change in the future, or in some new variation of
>>> the overall environment?
>>
>> Yes, I was told it could change in future. It is also not guaranteed to
>> be power of 2 (even tho it is currently).
>>
>>>>    o Two pages are allocated, only one is really needed. This adds to
>>>>      overhead.
>>>>    o For a 512 page deposit, the allocation is split into two: one for 511
>>>>      and second for 1. Thus, an order 9 allocation never happens. A
>>>>      contiguous 2M range, if possible, significantly improves performance
>>>>      in the hypervisor.
>>>>    o Since a page is already allocated to collect the frames, there is
>>>>      not really a need to use per cpu input page, and hence avoid local
>>>>      irq disable.
>>>>    o In hv_call_deposit_pages(), in case of error, under err_free_allocations
>>>>      label, all pages are freed without checking status to see if some pages
>>>>      were deposited. This is a critical bug as it would free pages that hyp
>>>>      may be using.
>>>>
>>>> All of above is addressed by:
>>>>    o Allocate 2M by default, this is the recommendation from the hypervisor
>>>>      team, and greatly improves performance.
>>>
>>> Can you be more specific about "improves performance"? Is the
>>> improvement on the guest side, or on the hypervisor side? And what's
>>> the key leverage point in improving performance, regardless of which
>>> side? It might be helpful to record this for future reference to prevent
>>> a change from being made that unknowingly hurts the key leverage
>>> point.
>>
>> Well, overall improvement I'd say. Lesser HV_STATUS_INSUFFICIENT_MEMORY
>> means lesser interruptions means faster non-deposit hypercalls. With
>> larger deposits, it will not come back as often with insufficient memory.
>> If it only uses 1M say out of the 2, next time it will reuse the remaining
>> meg before coming back and asking for more. IOW, incomplete hypercalls
>> repeatedly coming back asking for more ram add lot of overhead.
>> Eg. the guest setup part reduces from 8 sec to 1 sec with this, I
>> don't have further breakdown of which ioctls.
> 
> OK -- this part wasn't clear to me from reading the commit message.
> The primary goal is to reduce hypercall failures due to insufficient
> memory, followed by the cycle of adding memory and trying again. By
> adding memory in larger chunks in the first place, you reduce those
> cycles. Improving the performance of the code for doing the deposit
> helps, but that's probably not where you get the most noticeable
> benefit. Could you clarify this in the commit message?
> 
>>
>>>>    o Always start with a full 2M range allocation, thus getting contiguous if
>>>>      available. In cases where possible, the deposits are much faster.
>>>
>>> Is this faster because the hypervisor can consume the pages faster
>>> if they are contiguous?
>>
>> I didn't ask but my guess is that it can map 2M page in its page
>> tables, so all the benefits of large pages: faster L2 mapping, less
>> tlb overhead, etc..
> 
> Makes sense. This seems like it is probably a 2nd order benefit
> after reducing hypercall failures due to insufficient memory.
> 
>>
>>>>    o Allocate only one page in the deposit function and collect 511 pfns
>>>>      there. Just use a local variable for last pfn.
>>>>    o Use the page as input to hypercall. Since this page is locally allocated,
>>>>      irq disable can be avoided helping speed up the deposit.
> 
> Again, seems like a 2nd order perf improvement in the code
> for doing the memory deposit.
> 
>>>>    o Fix the physical contiguous memory requirement.
>>>>    o Lastly, remove pre-deposits hv_call_create_vp() and
>>>>      hv_call_initialize_partition() as they were removed internally while
>>>>      ago, most likely because they didn't help much.
>>>>
>>>> Signed-off-by: Mukesh R <mrathor@linux.microsoft.com>
>>>> ---
>>>>    drivers/hv/hv_proc.c           | 198 +++++++++++++++++++++++++++++----
>>>>    drivers/hv/mshv_root_hv_call.c |  10 +-
>>>>    include/asm-generic/mshyperv.h |   5 -
>>>>    3 files changed, 179 insertions(+), 34 deletions(-)
>>>>
> 
> [snip]
> 
>>>> +/*
>>>> + * Deposit memory in the hypervisor. Even if @contiguous is false, a contiguous
>>>> + * 2M worth of pfns is utmost desired for performance reasons. But short of
> 
> Again, this comment isn't clear on *why* the 2M helps performance.

It can be mapped as large page in hyp page tables.

>>>> + * that, we deposit whatever contiguous chunks we can get. If @contiguous is
>>>> + * true, then the entire range has to be physically contiguous. Note, in that
>>>> + * case, upon withdrawl, hypervisor could return any page in between the range,
>>>> + * so we must split that also. Lastly, HV_MAX_CONTIGUOUS_ALLOCATION_PAGES is
>>>> + * not guaranteed to always be power of 2.
>>>> + */
>>>> +static int hv_call_deposit_pages(int node, u64 partition_id, bool contiguous)
>>>> +{
>>>> +	struct hv_deposit_memory *hc_input;
>>>> +	int i, rc, num_pages;
>>>> +	u64 status, *pfna, lastpfn = 0;
>>>> +	bool trunc_extra = false;
>>>> +
>>>> +	BUILD_BUG_ON(HV_MAX_CONTIGUOUS_ALLOCATION_PAGES > HV_DEPOSIT_MAX);
>>>> +
>>>> +	if (contiguous) {
>>>> +		num_pages = roundup_pow_of_two(
>>>> +				 HV_MAX_CONTIGUOUS_ALLOCATION_PAGES);
>>>> +		trunc_extra = num_pages != HV_MAX_CONTIGUOUS_ALLOCATION_PAGES;
>>>> +	} else {
>>>> +		num_pages = HV_DEPOSIT_MAX;
>>>> +	}
>>>> +
>>>> +	hc_input = (struct hv_deposit_memory *)get_zeroed_page(GFP_KERNEL);
>>>
>>> Mike Rapoport has a kernel-wide effort underway to replace
>>> __get_free_page() with kmalloc() and get_zeroed_page() with kzalloc().
>>> See [1] for one example of the many patches he has submitted. The
>>> commit message has a short explanation. Going with kzalloc() here
>>> would probably avoid a future change.
>>
>> There might be places where a get_page may not be strictly needed, but
>> in this case we do for the hypercall (we use it as input page), and
>> this hypercall can go upto exactly one page. I don't think an operating
>> system can ever get away without having a get_page() api :). kmalloc()
>> could add an aligntment parameter, but it would just call get_page
>> anyways for page aligned requests.
>>
> 
> What Mike wants to do is:
> 
> 	hc_input = kzalloc(PAGE_SIZE, GFP_KERNEL);
> 
> This is guaranteed to provide page-aligned memory, just like
> get_zeroed_page(). See 2nd paragraph under "Description" at [1].

Confusing... the first line after Description also says:

"kmalloc is the normal method of allocating memory for objects smaller
  than page size in the kernel."

I think it is clearer to keep get_page, it also helps keep common code
with earlier versions

Thanks,
-Mukesh



> Michael
> 
> [1] https://www.kernel.org/doc/html/latest/core-api/mm-api.html#c.kmalloc