Re: [PATCH v5 12/14] mm: add config option for clearing page-extents

[David Hildenbrand <david@redhat.com>]

On 14.07.25 22:35, Ankur Arora wrote:
> 
> [ Added Zi Yan. ]
> 
> David Hildenbrand <david@redhat.com> writes:
> 
>> On 11.07.25 19:32, Ankur Arora wrote:
>>> David Hildenbrand <david@redhat.com> writes:
>>>
>>>> On 10.07.25 02:59, Ankur Arora wrote:
>>>>> Add CONFIG_CLEAR_PAGE_EXTENT to allow clearing of page-extents
>>>>> where architecturally supported.
>>>>> This is only available with !CONFIG_HIGHMEM because the intent is to
>>>>> use architecture support to clear contiguous extents in a single
>>>>> operation (ex. via FEAT_MOPS on arm64, string instructions on x86)
>>>>> which excludes any possibility of interspersing kmap()/kunmap().
>>>>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>>>>> ---
>>>>
>>>> Staring at the next patch, I think this can easily be squashed into the next
>>>> patch where you add actual MM core support.
>>> I wanted to do this in a separate patch to explicitly document what the
>>> responsibility of the interface provided by the architecture is.
>>> That said, the commit message didn't actually do a good job of doing
>>> that :D.
>>> Copying the more detailed commit message from my reply to Andrew,
>>> one important part of the clear_pages() is that it be interruptible
>>> because clear_pages_resched() implicitly depends on it.
>>>
>>>> This is only enabled with !CONFIG_HIGHMEM because the intent is
>>>> to use architecture support to clear contiguous extents in a
>>>> single interruptible operation (ex. via FEAT_MOPS on arm64,
>>>> string instructions on x86).
>>>
>>>> Given that we might be zeroing the whole extent with a single
>>>> instruction, this excludes any possibility of constructing
>>>> intermediate HIGHMEM maps.
>>> Do you think it is best documented in the next patch in a comment
>>> instead?
>>
>> I would just add + document it as part of the next patch.
>>
>> Looking at the bigger picture now, you introduce
>>
>> 	ARCH_HAS_CLEAR_PAGES
>>
>> To say whether an architecture provides clear_pages().
>>
>> Now we want to conditionally use that to optimize folio_zero_user().
>>
>> Remind me, why do we want to glue this to THP / HUGETLBFS only? I would assume
>> that the code footprint is rather small, and the systems out there that are
>> compiled with ARCH_HAS_CLEAR_PAGES but without THP / HUGETLBFS are rather ...
>> rare (mostly 32BIT x86 only).

Agreed.

> 
> I thought about this some more and there are a few other interfaces that
> end up clearing pages:
> 
>> clear_highpage()
>> clear_highpage_kasan_tagged()
>> tag_clear_highpage()
> 
> In this set, there are many loops of the form:
> 
>     for (i = 0; i < n; i++)
>         clear_highpage();
> 
> At least some of these (including kernel_init_pages()) could be migrated
> to variations on a clear_highpages() which could be:
> 
>      static inline void clear_highpages(struct page *page, u32 num_pages)
>      {
>              if (!IS_ENABLED(CONFIG_HIGHMEM))
>                      clear_pages_resched(page, num_pages);
>              else
>                      for (i = 0; i < num_pages; ++i)
>                              clear_highpage(page + i);
>      }
> 
> (clear_pages_resched() should be safe to be used from here because
> everybody using this should be in a schedulable context.)
> 
> (The kernel_init_pages() was also suggested by Zi Yan in a review of v3 [1].)
> 
>> clear_user_highpage()
> 
> Only users folio_zero_user(), __collapse_huge_page_copy() and
> userfaultd.
> 
>> clear_user_page()
> Not many users apart from the highmem interface.
> 
>> clear_page()
> 
> Not many users apart from the highmem interface.
> 
> I'm happy to do this work, just not sure how to stage it. In particular I
> would like to avoid a series which tries to address all of the cases.
> 
> Maybe it makes sense to handle just add the clear_highpages() variants,
> folio_zero_user() handling and some of the obvious users of
> clear_highpages() for v6?

Yes, no need for excessive handling.

What I was getting at was: could we get rid of the kconfig option and 
simply glue it to the availability of clear_pages() in a reasonable way.

-- 
Cheers,

David / dhildenb