Re: [PATCH] mm: Drop unused set_pte_safe()

[Ryan Roberts]

On 10/09/2024 18:20, Matthew Wilcox wrote:
> On Tue, Sep 10, 2024 at 10:08:10AM +0100, Ryan Roberts wrote:
>> On 10/09/2024 10:04, Anshuman Khandual wrote:
>>> All set_pte_safe() usage have been dropped after the commit eccd906484d1
>>> ("x86/mm: Do not use set_{pud, pmd}_safe() when splitting a large page")
>>> This just drops now unused helper set_pte_safe().
>>
>> It would be good to add some comment here to mention that the macro was buggy
>> due to doing direct dereferencing of the pte, and that if it were to be kept, it
>> should have been updated to use a single call to ptep_get().
> 
> I'm not sure that the _macro_ would have been buggy in such a scenario.
> If I understand correctly, the _caller_ would have been buggy:
> 
> /*
>  * Use set_p*_safe(), and elide TLB flushing, when confident that *no*
>  * TLB flush will be required as a result of the "set". For example, use
>  * in scenarios where it is known ahead of time that the routine is
>  * setting non-present entries, or re-setting an existing entry to the
>  * same value. Otherwise, use the typical "set" helpers and flush the
>  * TLB.
>  */
> 
> so if *ptep changes between the two calls, that's the caller's bug,
> right?

I was referring to the fact that the ptep pointer is being directly dereferenced
by the macro. It should really be using ptep_get(). See commit c33c794828f21
("mm: ptep_get() conversion") for the explanation.

So it really depends on your definition of a bug; arm64 doesn't use this helper
so its not affected by the magic that arm64 does in its ptep_get(). So you'll
never see incorrect behavior as a result (and I suspect in this specific case,
even if arm64 did use it, you probably wouldn't notice a problem in practice).
Perhaps it would have been better described as a code smell.

And yes, it's my bug/code smell; I didn't catch this when doing the conversion;
I guess Coccinelle has no type information so didn't highlight it, and I only
used the compiler trick for arm64, which doesn't use this macro.

> 
> Otherwise, set_pmd_safe() would be buggy ...

arm64 doesn't do any magic in pmdp_get() like it does for ptep_get() (yet ;-) ).
So direct dereference of a pmdp pointer is a less hard requirement.

But there is still the issue of READ_ONCE() vs *pmdp. As I understand it,
technically these should all be READ_ONCE() to ensure single-copy atomicity, as
explained in commit 20a004e7b017c ("arm64: mm: Use READ_ONCE/WRITE_ONCE when
accessing page tables"). Although, probably in practice it doesn't really break
anything here since the PTL is held so the only racing writer is the page table
walker writing access and dirty bits, which shouldn't get in the way of the
tests done here.

Thanks,
Ryan

> 
>> With that:
>>
>> Reviewed-by: Ryan Roberts <ryan.roberts@xxxxxxx>
>>
>> Thanks,
>> Ryan
>>
>>>
>>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>>> Cc: David Hildenbrand <david@xxxxxxxxxx>
>>> Cc: Ryan Roberts <ryan.roberts@xxxxxxx>
>>> Cc: "Mike Rapoport (IBM)" <rppt@xxxxxxxxxx>
>>> Cc: linux-mm@xxxxxxxxx
>>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>>> ---
>>>  include/linux/pgtable.h | 6 ------
>>>  1 file changed, 6 deletions(-)
>>>
>>> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
>>> index 2a6a3cccfc36..aeabbf0db7c8 100644
>>> --- a/include/linux/pgtable.h
>>> +++ b/include/linux/pgtable.h
>>> @@ -1058,12 +1058,6 @@ static inline int pgd_same(pgd_t pgd_a, pgd_t pgd_b)
>>>   * same value. Otherwise, use the typical "set" helpers and flush the
>>>   * TLB.
>>>   */
>>> -#define set_pte_safe(ptep, pte) \
>>> -({ \
>>> -	WARN_ON_ONCE(pte_present(*ptep) && !pte_same(*ptep, pte)); \
>>> -	set_pte(ptep, pte); \
>>> -})
>>> -
>>>  #define set_pmd_safe(pmdp, pmd) \
>>>  ({ \
>>>  	WARN_ON_ONCE(pmd_present(*pmdp) && !pmd_same(*pmdp, pmd)); \
>>
>>