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

[Mukesh R]

On 9/9/26 10:32, Michael Kelley wrote:
> From: Mukesh R <mrathor@xxxxxxxxxxxxxxxxxxx> Sent: Tuesday, September 8, 2026 3:33 PM
> > On 9/8/26 11:02, Michael Kelley wrote:
> > > From: Mukesh R <mrathor@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > >    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