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

[Qi Zheng <zhengqi.arch@bytedance.com>]

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().

Something like this:

nr = do_zap_pte_range(&any_skipped);
if (can_reclaim_pt) {
	VM_BUG_ON(!any_skipped && count_pte_none(nr) == nr);
	if (any_skipped)
		can_reclaim_pt = false;
}

nr: the number of processed entries (excluding pte_none())
any_skipped: set whenever we skipped to zap a !pte_none entry


```
do_zap_pte_range
-->     pte_t ptent = ptep_get(pte);
	int max_nr = (end - addr) / PAGE_SIZE;

	/* Skip all consecutive pte_none(). */
	if (pte_none(ptent)) {
		int nr;

		for (nr = 1; nr < max_nr; nr++) {
			ptent = ptep_get(pte + nr);
			if (!pte_none(ptent))
				break;
		}
		max_nr -= nr;
		if (!max_nr)
			return 0;
		pte += nr;
		addr += nr * PAGE_SIZE;
	}

	if (pte_present(ptent))
		return zap_present_ptes(tlb, vma, pte, ptent, max_nr,
					addr, details, rss, force_flush,
					force_break, any_skipped);

	return zap_nonpresent_ptes(tlb, vma, pte, ptent, max_nr, addr,
					 details, rss, any_skipped);
```

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

>