[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aN__QWQZkXN8k1-V@skinsburskii.localdomain>
Date: Fri, 3 Oct 2025 09:52:17 -0700
From: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>
To: Michael Kelley <mhklinux@...look.com>
Cc: "kys@...rosoft.com" <kys@...rosoft.com>,
"haiyangz@...rosoft.com" <haiyangz@...rosoft.com>,
"wei.liu@...nel.org" <wei.liu@...nel.org>,
"decui@...rosoft.com" <decui@...rosoft.com>,
"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 3/5] Drivers: hv: Batch GPA unmap operations to
improve large region performance
On Fri, Oct 03, 2025 at 12:27:13AM +0000, Michael Kelley wrote:
> From: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com> Sent: Thursday, October 2, 2025 9:36 AM
> >
> > Reduce overhead when unmapping large memory regions by batching GPA unmap
> > operations in 2MB-aligned chunks.
> >
> > Use a dedicated constant for batch size to improve code clarity and
> > maintainability.
> >
> > Signed-off-by: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>
> > ---
> > drivers/hv/mshv_root.h | 2 ++
> > drivers/hv/mshv_root_hv_call.c | 2 +-
> > drivers/hv/mshv_root_main.c | 15 ++++++++++++---
> > 3 files changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hv/mshv_root.h b/drivers/hv/mshv_root.h
> > index e3931b0f12693..97e64d5341b6e 100644
> > --- a/drivers/hv/mshv_root.h
> > +++ b/drivers/hv/mshv_root.h
> > @@ -32,6 +32,8 @@ static_assert(HV_HYP_PAGE_SIZE == MSHV_HV_PAGE_SIZE);
> >
> > #define MSHV_PIN_PAGES_BATCH_SIZE (0x10000000ULL / HV_HYP_PAGE_SIZE)
> >
> > +#define MSHV_MAX_UNMAP_GPA_PAGES 512
> > +
> > struct mshv_vp {
> > u32 vp_index;
> > struct mshv_partition *vp_partition;
> > diff --git a/drivers/hv/mshv_root_hv_call.c b/drivers/hv/mshv_root_hv_call.c
> > index c9c274f29c3c6..0696024ccfe31 100644
> > --- a/drivers/hv/mshv_root_hv_call.c
> > +++ b/drivers/hv/mshv_root_hv_call.c
> > @@ -17,7 +17,7 @@
> > /* Determined empirically */
> > #define HV_INIT_PARTITION_DEPOSIT_PAGES 208
> > #define HV_MAP_GPA_DEPOSIT_PAGES 256
> > -#define HV_UMAP_GPA_PAGES 512
> > +#define HV_UMAP_GPA_PAGES MSHV_MAX_UNMAP_GPA_PAGES
> >
> > #define HV_PAGE_COUNT_2M_ALIGNED(pg_count) (!((pg_count) & (0x200 - 1)))
> >
> > diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> > index 7ef66c43e1515..8bf0b5af25802 100644
> > --- a/drivers/hv/mshv_root_main.c
> > +++ b/drivers/hv/mshv_root_main.c
> > @@ -1378,6 +1378,7 @@ mshv_map_user_memory(struct mshv_partition *partition,
> > static void mshv_partition_destroy_region(struct mshv_mem_region *region)
> > {
> > struct mshv_partition *partition = region->partition;
> > + u64 page_offset;
> > u32 unmap_flags = 0;
> > int ret;
> >
> > @@ -1396,9 +1397,17 @@ static void mshv_partition_destroy_region(struct mshv_mem_region *region)
> > if (region->flags.large_pages)
> > unmap_flags |= HV_UNMAP_GPA_LARGE_PAGE;
> >
> > - /* ignore unmap failures and continue as process may be exiting */
> > - hv_call_unmap_gpa_pages(partition->pt_id, region->start_gfn,
> > - region->nr_pages, unmap_flags);
> > + for (page_offset = 0; page_offset < region->nr_pages; page_offset++) {
> > + if (!region->pages[page_offset])
> > + continue;
> > +
> > + hv_call_unmap_gpa_pages(partition->pt_id,
> > + ALIGN(region->start_gfn + page_offset,
> > + MSHV_MAX_UNMAP_GPA_PAGES),
>
> Seems like this should be ALIGN_DOWN() instead of ALIGN(). ALIGN() rounds
> up to get the requested alignment, which could skip over some non-zero
> entries in region->pages[].
>
Indeed, the goal is to unmap in 2MB chunks as it's the max payload that
can be passed to hypervisor.
I'll replace it with ALIGN_DOWN in the next revision.
> And I'm assuming the unmap hypercall is idempotent in that if the specified
> partition PFN range includes some pages that aren't mapped, the hypercall
> just skips them and keeps going without returning an error.
>
It might be the case, but it's not reliable.
If the region size isn’t aligned to MSHV_MAX_UNMAP_GPA_PAGES (i.e., not
aligned to 2M), the hypervisor can return an error for the trailing
invalid (non-zero) PFNs.
I’ll fix this in the next revision.
> > + MSHV_MAX_UNMAP_GPA_PAGES, unmap_flags);
> > +
> > + page_offset += MSHV_MAX_UNMAP_GPA_PAGES - 1;
>
> This update to the page_offset doesn't take into account the effect of the
> ALIGN (or ALIGN_DOWN) call. With a change to ALIGN_DOWN(), it may
> increment too far and perhaps cause the "for" loop to be exited prematurely,
> which would fail to unmap some of the pages.
>
I’m not sure I see the problem here. If we align the offset by
MSHV_MAX_UNMAP_GPA_PAGES and unmap the same number of pages, then we
should increment the offset by that very same number, shouldn’t we?
Thanks,
Stanislav
> > + }
> >
> > mshv_region_invalidate(region);
> >
> >
> >
>
Powered by blists - more mailing lists