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

[Dave Hansen <dave.hansen@intel.com>]

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.

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.

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)

Or _maybe_:

int set_direct_map_ro_noflush(struct folio *folio)