Re: [RFC PATCH v2 1/3] mm/huge_memory: make persistent huge zero folio read-only

[Mike Rapoport <rppt@kernel.org>]

Hi,

On Wed, Jun 10, 2026 at 11:20:22AM +0800, Lance Yang wrote:
> Hi Dave,
> 
> Thanks for taking the time to review.
> 
> On Tue, Jun 09, 2026 at 12:33:36PM -0700, Dave Hansen wrote:
> >On 6/9/26 07:37, Xueyuan Chen wrote:
> >> +bool __weak arch_make_pages_readonly(struct page *page, int nr_pages)
> >> +{
> >> +	return false;
> >> +}
> >
> >This is a rather wonky function. It's going to cause all kinds of fun if
> >it is used like this:
> >
> >	arch_make_pages_readonly(syscall_table, 1);
> 
> Ouch, yeah, it is ...

We already have set_direct_map* APIs, why don't you add a new one there?
set_direct_map_ro() for example.
 
> >It's also kinda weird to have it return a bool, and not check that bool
> >at the single call site. Some things come to mind:
> >
> >1. This function needs commenting. It needs to say what it does, when
> >   architectures should override it and what their implementations
> >   should look like. It needs to be clear that this can't be used for
> >   anything really important. What should architectures do with alias
> >   mappings? Are they allowed to touch non-direct map aliases? Are they
> >   required to?
> 
> Agreed. Needs a real comment ...
> 
> Just meant as a best-effort direct/linear-map permission chang, nothing
> stronger than that. I should spell out what happens, or does not happen,
> to non-direct-map aliases, if anything, and make clear callers cannot
> treat this as a hard guarantee :D

It's not only about highmem, anything that changes the direct map might
fail to allocate memory when splitting larger mappings.
 
> >2. The return type needs to be reconsidered. Is 'bool' even acceptable?
> >   Should it just be 'void' if callers can't do anything when it fails?
> 
> Maybe ignoring it is OK now, but someone may need the return value later?
> 
> >3. What should the naming be? "readonly" vs "ro". Should it have a
> >   "maybe" since it's kinda optional?
> 
> Fair point. "make" may be overstating it a bit ...
> 
> With a return value, arch_try_make_pages_readonly() sounds about right
> to me. If we end up with void and pure best-effort semantics, maybe
> arch_maybe_make_pages_readonly() fits better :)

Realistically, I wouldn't expect 32-bit configs to enable
PERSISTENT_HUGE_ZERO_FOLIO or even THP, so naming this function to reflect
32-bit behaviour seems odd going forward.
 
> >4. Should this new API be folio or page-based in the first place?
> 
> For page vs folio, I was mostly following David's RFC v1 suggestion.
> 
> Current caller is a folio, sure, but the page-range helper leaves room
> for non-folio users later. Happy to add a simple folio wrapper if that
> reads better ;)
> 
> >5. Is mm/huge_memory.c the right place to define a generic mm function,
> >   even a stub?
> 
> Ah, you're right! My bad, wrong place for a generic stub. Will move it
> out for RFC v3.

We have include/linux/set_memory.h for such function declarations and their
stubs.
 
> Thanks, Lance
> 

-- 
Sincerely yours,
Mike.