Re: [PATCH v6 3/3] x86/mm: add set_direct_map_ro_noflush()

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

On 8/25/26 18:57, Dave Hansen wrote:
> On 8/25/26 09:43, David Hildenbrand (Arm) wrote:
>>> int set_direct_map_invalid_noflush(struct page *page)
>>> int set_direct_map_default_noflush(struct page *page)
>>> int set_direct_map_valid_noflush(struct page *page, unsigned nr, ...
>>> int set_direct_map_ro_noflush(const void *addr, unsigned long nr_pages)
>>>
>>> Which one of these things is not like the other, despite being named
>>> just like them?
>>>
>> That's called out in the cover letter:
>>
>> "
>> This series adds set_direct_map_ro_noflush() so mm code can make a
>> direct-map range read-only, then uses it for the persistent huge zero
>> folio. The helper is direct-map specific, takes an address-based range as
>> discussed for set_direct_map* helpers[2], and leaves TLB invalidation to
>> the caller.
>> "
> 
> My concern is not so much what the function is doing or whether or how
> the specific function is documented. It's more about whether the new
> function is consistent across all functions with a similar purpose and
> name. Also, if it is _not_ consistent there needs to be reasoning behind
> the inconsistency. I think that is missing here.

Yes, the intend from the submitter was to prepare for the interface change to
keep them consistent.

I agree that for now it should just consume pages instead of an address and keep
the interface consistent in this series.

> 
> In this case, look at the call site:
> 
> 	addr = (unsigned long)folio_address(huge_zero_folio);
> 	set_direct_map_ro_noflush((void *)addr, HPAGE_PMD_NR);
> 
> It *has* a folio. But it does a folio_address() and two casts to massage
> it into the type for set_direct_map_ro_noflush().
> 
> If set_direct_map_ro_noflush() just took a 'struct page *', there would
> be one folio=>page conversion, no casting, and complete consistency with
> the other set_direct_map*() functions.

Again, I agree with that, and the idea was to keep the interface consistent as
it gets converted; it's just that the series that does the conversion stalled.

> 
> I'd also be OK with set_direct_map_ro_noflush() taking a folio, with the
> implication being that it might eventually make sense to convert the
> other set_direct_map*() functions to folios. But page vs. folio
> confusion is much less likely to cause bugs than a void* versus another
> pointer.
> 
> IOW, what I think I want is:
> 
> int set_direct_map_ro_noflush(struct page *page, unsigned long nr_pages)

Yes, or as will said

	int set_direct_map_ro(struct page *page, unsigned long nr_pages)

performing the flush internally.

> 
> Or _maybe_:
> 
> int set_direct_map_ro_noflush(struct folio *folio)

I think I raised it before, but using folios on this interface is not a good
idea. Primarily because

a) Once we decouple struct folio from struct page this interface would only be
available for folios and ...

b) ... the huge zero page (currently huge zero folio) is on of the examples that
*currently* is a folio but very likely won't be a folio in the future. Only
anon+pagecache that really need mapcounts and all that will be folios.

c) I don't expect any further real folio users (IOW, anonymous folios or
pagecache folios), but could imagine non-folio users.

-- 
Cheers,

David