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

[Lance Yang <lance.yang@linux.dev>]

On Thu, Jun 11, 2026 at 09:49:22AM +0300, Mike Rapoport wrote:
>Hi,

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.

Good point. I was trying to make this generic, but that's a bit too cute.
This is really a direct-map thing, so adding a set_direct_map_ro() helper
there makes more sense.

> 
>> >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.

Yes, exactly.

> 
>> >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.

Cool with that, if no one objects. No need to bake that into the name I
guess. int return plus a comment should be enough, just like
set_direct_map_*() family already does :)

>> >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.

Yep, will move it there :)

Thanks, Lance