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

[David Hildenbrand <david@redhat.com>]

On 18.11.24 11:34, Qi Zheng wrote:
> 
> 
> On 2024/11/18 17:29, David Hildenbrand wrote:
>> 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?
> 
> When any_skipped == false, at least add VM_BUG_ON() to recheck none ptes.
> 
>>
>> 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
> 
> I prefer that "nr" should not include pte_none().
> 

Why? do_zap_pte_range() should tell you how far to advance, nothing 
less, nothing more.

Let's just keep it simple and avoid count_pte_none().

I'm probably missing something important?

-- 
Cheers,

David / dhildenb