Re: [PATCH 2/2] mm: convert do_set_pmd() to take a folio

[Matthew Wilcox <willy@infradead.org>]

On Wed, May 07, 2025 at 08:36:54PM +0800, Baolin Wang wrote:
> On 2025/5/7 20:10, Matthew Wilcox wrote:
> > Because I see nowhere in this patch that you initialise 'page'.
> 
> Please look at the following code in do_set_pmd(), and the 'page' will be
> initialized before using.
> 
>         if (thp_disabled_by_hw() || vma_thp_disabled(vma, vma->vm_flags))
>                 return ret;
> 
>         if (!thp_vma_suitable_order(vma, haddr, PMD_ORDER))
>                 return ret;
> 
>         if (folio_order(folio) != HPAGE_PMD_ORDER)
>                 return ret;
>         page = &folio->page;

Ah, fair, I missed that.

> > And that's really the important part.  You seem to be assuming that a
> > folio will never be larger than PMD size, and I'm not comfortable with
> 
> No, I have no this assumption. But do_set_pmd() is used to establish PMD
> mappings for the PMD-sized folios, and we already have PMD-sized checks to
> validate the folio size:
> 
>         if (!thp_vma_suitable_order(vma, haddr, PMD_ORDER))
>                 return ret;
> 
>         if (folio_order(folio) != HPAGE_PMD_ORDER)
>                 return ret;
> 
> > that assumption.  It's a limitation I put in place a few years ago so we
> > didn't have to find and fix all those assumptions immediately, but I
> > imagine that some day we'll want to have larger folios.
> > 
> > So unless you can derive _which_ page in the folio we want to map from
> 
> IMO, for PMD mapping of a PMD-sized folio, we do not need to know _which_
> page in the folio we want to map, because we'll always map the entire
> PMD-sized folio.

There's a difference between "Assert that the folio is PMD sized" inside
the function because we know there are still problems, and "Change the
interface so we can't specify which page inside the folio is the one
we're actually interested in".

I reiterate the NACK to this patch.