Re: [PATCH v3 4/9] mm: introduce skip_none_ptes()

[David Hildenbrand <david@redhat.com>]

On 18.11.24 04:35, Qi Zheng wrote:
> 
> 
> On 2024/11/15 22:59, David Hildenbrand wrote:
>> On 15.11.24 15:41, Qi Zheng wrote:
>>>
>>>
>>> On 2024/11/15 18:22, David Hildenbrand wrote:
>>>>>>> *nr_skip = nr;
>>>>>>>
>>>>>>> and then:
>>>>>>>
>>>>>>> zap_pte_range
>>>>>>> --> nr = do_zap_pte_range(tlb, vma, pte, addr, end, details,
>>>>>>> &skip_nr,
>>>>>>>                             rss, &force_flush, &force_break);
>>>>>>>          if (can_reclaim_pt) {
>>>>>>>              none_nr += count_pte_none(pte, nr);
>>>>>>>              none_nr += nr_skip;
>>>>>>>          }
>>>>>>>
>>>>>>> Right?
>>>>>>
>>>>>> Yes. I did not look closely at the patch that adds the counting of
>>>>>
>>>>> Got it.
>>>>>
>>>>>> pte_none though (to digest why it is required :) ).
>>>>>
>>>>> Because 'none_nr == PTRS_PER_PTE' is used in patch #7 to detect
>>>>> empty PTE page.
>>>>
>>>> Okay, so the problem is that "nr" would be "all processed entries" but
>>>> there are cases where we "process an entry but not zap it".
>>>>
>>>> What you really only want to know is "was any entry not zapped", which
>>>> could be a simple input boolean variable passed into do_zap_pte_range?
>>>>
>>>> Because as soon as any entry was processed but  no zapped, you can
>>>> immediately give up on reclaiming that table.
>>>
>>> Yes, we can set can_reclaim_pt to false when a !pte_none() entry is
>>> found in count_pte_none().
>>
>> I'm not sure if well need cont_pte_none(), but I'll have to take a look
>> at your new patch to see how this fits together with doing the pte_none
>> detection+skipping in do_zap_pte_range().
>>
>> I was wondering if you cannot simply avoid the additional scanning and
>> simply set "can_reclaim_pt" if you skip a zap.
> 
> Maybe we can return the information whether the zap was skipped from
> zap_present_ptes() and zap_nonpresent_ptes() through parameters like I
> did in [PATCH v1 3/7] and [PATCH v1 4/7].
> 
> In theory, we can detect empty PTE pages in the following two ways:
> 
> 1) If no zap is skipped, it means that all pte entries have been
>      zap, and the PTE page must be empty.
> 2) If all pte entries are detected to be none, then the PTE page is
>      empty.
> 
> In the error case, 1) may cause non-empty PTE pages to be reclaimed
> (which is unacceptable), while the 2) will at most cause empty PTE pages
> to not be reclaimed.
> 
> So the most reliable and efficient method may be:
> 
> a. If there is a zap that is skipped, stop scanning and do not reclaim
>      the PTE page;
> b. Otherwise, as now, detect the empty PTE page through count_pte_none()

Is there a need for count_pte_none() that I am missing?

Assume we have

nr = do_zap_pte_range(&any_skipped)


If "nr" is the number of processed entries (including pte_none()), and
"any_skipped" is set whenever we skipped to zap a !pte_none entry, we 
can detect what we need, no?

If any_skipped == false after the call, we now have "nr" pte_none() 
entries. -> We can continue trying to reclaim

If any_skipped == true, we have at least one !pte_none() entry among the 
"nr" entries. -> We cannot and must not reclaim.

-- 
Cheers,

David / dhildenb