Re: [PATCH 1/7] mshv: Convert from page pointers to PFNs

[Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>]

On Mon, Apr 20, 2026 at 05:18:10PM +0000, Michael Kelley wrote:
> From: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com> Sent: Monday, April 20, 2026 9:22 AM
> > 
> > On Mon, Apr 13, 2026 at 09:08:16PM +0000, Michael Kelley wrote:
> > > From: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com> Sent: Monday, March 30, 2026 1:04 PM
> > > >
> 
> [snip]
> 
> > > > @@ -57,60 +58,61 @@ static int mshv_chunk_stride(struct page *page,
> > > >  /**
> > > >   * mshv_region_process_chunk - Processes a contiguous chunk of memory pages
> > > >   *                             in a region.
> > > > - * @region     : Pointer to the memory region structure.
> > > > - * @flags      : Flags to pass to the handler.
> > > > - * @page_offset: Offset into the region's pages array to start processing.
> > > > - * @page_count : Number of pages to process.
> > > > - * @handler    : Callback function to handle the chunk.
> > > > + * @region    : Pointer to the memory region structure.
> > > > + * @flags     : Flags to pass to the handler.
> > > > + * @pfn_offset: Offset into the region's PFNs array to start processing.
> > > > + * @pfn_count : Number of PFNs to process.
> > > > + * @handler   : Callback function to handle the chunk.
> > > >   *
> > > > - * This function scans the region's pages starting from @page_offset,
> > > > - * checking for contiguous present pages of the same size (normal or huge).
> > > > - * It invokes @handler for the chunk of contiguous pages found. Returns the
> > > > - * number of pages handled, or a negative error code if the first page is
> > > > - * not present or the handler fails.
> > > > + * This function scans the region's PFNs starting from @pfn_offset,
> > > > + * checking for contiguous valid PFNs backed by pages of the same size
> > > > + * (normal or huge). It invokes @handler for the chunk of contiguous valid
> > > > + * PFNs found. Returns the number of PFNs handled, or a negative error code
> > > > + * if the first PFN is invalid or the handler fails.
> > > >   *
> > > > - * Note: The @handler callback must be able to handle both normal and huge
> > > > - * pages.
> > > > + * Note: The @handler callback must be able to handle valid PFNs backed by
> > > > + * both normal and huge pages.
> > > >   *
> > > >   * Return: Number of pages handled, or negative error code.
> > > >   */
> > > > -static long mshv_region_process_chunk(struct mshv_mem_region *region,
> > > > -				      u32 flags,
> > > > -				      u64 page_offset, u64 page_count,
> > > > -				      int (*handler)(struct mshv_mem_region *region,
> > > > -						     u32 flags,
> > > > -						     u64 page_offset,
> > > > -						     u64 page_count,
> > > > -						     bool huge_page))
> > > > +static long mshv_region_process_pfns(struct mshv_mem_region *region,
> > > > +				     u32 flags,
> > > > +				     u64 pfn_offset, u64 pfn_count,
> > > > +				     int (*handler)(struct mshv_mem_region *region,
> > > > +						    u32 flags,
> > > > +						    u64 pfn_offset,
> > > > +						    u64 pfn_count,
> > > > +						    bool huge_page))
> > > >  {
> > > > -	u64 gfn = region->start_gfn + page_offset;
> > > > +	u64 gfn = region->start_gfn + pfn_offset;
> > > >  	u64 count;
> > > > -	struct page *page;
> > > > +	unsigned long pfn;
> > > >  	int stride, ret;
> > > >
> > > > -	page = region->mreg_pages[page_offset];
> > > > -	if (!page)
> > > > +	pfn = region->mreg_pfns[pfn_offset];
> > > > +	if (!pfn_valid(pfn))
> > > >  		return -EINVAL;
> > > >
> > > > -	stride = mshv_chunk_stride(page, gfn, page_count);
> > > > +	stride = mshv_chunk_stride(pfn_to_page(pfn), gfn, pfn_count);
> > > >  	if (stride < 0)
> > > >  		return stride;
> > > >
> > > >  	/* Start at stride since the first stride is validated */
> > > > -	for (count = stride; count < page_count; count += stride) {
> > > > -		page = region->mreg_pages[page_offset + count];
> > > > +	for (count = stride; count < pfn_count ; count += stride) {
> > > > +		pfn = region->mreg_pfns[pfn_offset + count];
> > > >
> > > > -		/* Break if current page is not present */
> > > > -		if (!page)
> > > > +		/* Break if current pfn is invalid */
> > > > +		if (!pfn_valid(pfn))
> > >
> > > pfn_valid() is a relatively expensive test to be doing in a loop
> > > on what may be every single page. It does an RCU lock/unlock
> > > and make other checks that aren't necessary here. Since
> > > mreg_pfns[] is populated from mm calls, the only invalid PFNs
> > > would be MSHV_INVALID_PFN that code in this module has
> > > explicitly put there. Just testing against MSHV_INVALID_PFN
> > > would be a lot faster here and elsewhere in this module. It's
> > > really a "pfn set/not set" test. Defining a pfn_set() macro
> > > here in this module that tests against MSHV_INVALID_PFN
> > > would accomplish the same thing more efficiently.
> > >
> > 
> > Yes, we could do it the way you suggest. For completeness, I should add
> > that pfn_valid() is expensive only on 32-bit ARM and ARC, which we
> > don’t care about.
> > 
> 
> Could you elaborate? On x86, I'm seeing that pfn_valid() is about
> 220 bytes of code. It's the version in include/linux/mmzone.h, not
> the simple version in include/asm-generic/memory_model.h. The
> latter is used only for CONFIG_FLATMEM=y. Or is the root partition
> kernel build setting CONFIG_FLATMEM_MANUAL and hence getting
> the simple version?
> 

I was wrong: this long function is indeed compiled for x86.
Still, it's not big of a runtime impact as taking the rcu lock is cheap,
but I'll simplify as proposed.

Thanks,
Stanislav

> Michael