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

[David Hildenbrand <david@redhat.com>]

On 18.11.24 11:56, Qi Zheng wrote:
> 
> 
> On 2024/11/18 18:41, David Hildenbrand wrote:
>> 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?
> 
> As we discussed before, we should skip all consecutive none ptes,
 > pte and addr are already incremented before returning.

It's probably best to send the resulting patch so I can either 
understand why count_pte_none() is required or comment on how to get rid 
of it.

-- 
Cheers,

David / dhildenb