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

["David Hildenbrand (Arm)" <david@kernel.org>]

On 6/9/26 21:33, 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);
> 
> 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?

Yes, kerneldoc please.

> 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?
> 3. What should the naming be? "readonly" vs "ro". Should it have a
>    "maybe" since it's kinda optional?

We're adjusting the directmap, remapping a r/w page to be r/o. I think we should
be very clear about which transition we expect+support.

Also, I rather hate the "set_memory" naming scheme ... "set_direct_map" is
clearer. Anyhow ...

Now we are throwing a "arch_make_pages_*" into the mix.

Should it really contain the "arch"?
Should it really contain the "make" ?

Why can't we just reuse set_memory_ro and pass address+nr_pages? (highmem check?
Could that be moved in there?)

Or do we want a "change_direct_map_ro()" / "remap_direct_map_ro" interface?


> 4. Should this new API be folio or page-based in the first place?
> 5. Is mm/huge_memory.c the right place to define a generic mm function,
>    even a stub?

+1


-- 
Cheers,

David