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

[Xueyuan Chen]

Hi David, Dave,

Thanks for the review and the discussion.

I'll switch the helper to a page-based signature and do the flush
internally:

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

I originally chose the address-based signature to prepare for the
interface conversion, but since that work stalled, I'll keep the
helper consistent with the other set_direct_map*() helpers.

The remaining comments will be addressed in v7, which
will be based on the latest mm-unstable.

Thanks,
Xueyuan

On Wed, Aug 26, 2026 at 1:31 AM David Hildenbrand (Arm)
<david@xxxxxxxxxx> wrote:
>
> 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