Re: [PATCH 4/7] mm: Use pmdp_get() for accessing PMD entries

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 9/13/24 16:08, Ryan Roberts wrote:
> On 13/09/2024 09:44, Anshuman Khandual wrote:
>> Convert PMD accesses via pmdp_get() helper that defaults as READ_ONCE() but
>> also provides the platform an opportunity to override when required.
>>
>> Cc: Dimitri Sivanich <dimitri.sivanich@hpe.com>
>> Cc: Muchun Song <muchun.song@linux.dev>
>> Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
>> Cc: Miaohe Lin <linmiaohe@huawei.com>
>> Cc: Naoya Horiguchi <nao.horiguchi@gmail.com>
>> Cc: Pasha Tatashin <pasha.tatashin@soleen.com>
>> Cc: Dennis Zhou <dennis@kernel.org>
>> Cc: Tejun Heo <tj@kernel.org>
>> Cc: Christoph Lameter <cl@linux.com>
>> Cc: Uladzislau Rezki <urezki@gmail.com>
>> Cc: Christoph Hellwig <hch@infradead.org>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Ryan Roberts <ryan.roberts@arm.com>
>> Cc: "Mike Rapoport (IBM)" <rppt@kernel.org>
>> Cc: linux-kernel@vger.kernel.org
>> Cc: linux-fsdevel@vger.kernel.org
>> Cc: linux-mm@kvack.org
>> Cc: kasan-dev@googlegroups.com
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  drivers/misc/sgi-gru/grufault.c |  4 +--
>>  fs/proc/task_mmu.c              | 26 +++++++-------
>>  include/linux/huge_mm.h         |  3 +-
>>  include/linux/mm.h              |  2 +-
>>  include/linux/pgtable.h         | 14 ++++----
>>  mm/gup.c                        | 14 ++++----
>>  mm/huge_memory.c                | 60 ++++++++++++++++-----------------
>>  mm/hugetlb_vmemmap.c            |  4 +--
>>  mm/kasan/init.c                 | 10 +++---
>>  mm/kasan/shadow.c               |  4 +--
>>  mm/khugepaged.c                 |  4 +--
>>  mm/madvise.c                    |  6 ++--
>>  mm/memory-failure.c             |  6 ++--
>>  mm/memory.c                     | 25 +++++++-------
>>  mm/mempolicy.c                  |  4 +--
>>  mm/migrate.c                    |  4 +--
>>  mm/migrate_device.c             | 10 +++---
>>  mm/mlock.c                      |  6 ++--
>>  mm/mprotect.c                   |  2 +-
>>  mm/mremap.c                     |  4 +--
>>  mm/page_table_check.c           |  2 +-
>>  mm/pagewalk.c                   |  4 +--
>>  mm/percpu.c                     |  2 +-
>>  mm/pgtable-generic.c            | 16 ++++-----
>>  mm/ptdump.c                     |  2 +-
>>  mm/rmap.c                       |  2 +-
>>  mm/sparse-vmemmap.c             |  4 +--
>>  mm/vmalloc.c                    | 12 +++----
>>  28 files changed, 129 insertions(+), 127 deletions(-)
>>
>> diff --git a/drivers/misc/sgi-gru/grufault.c b/drivers/misc/sgi-gru/grufault.c
>> index 3557d78ee47a..f3d6249b7dfb 100644
>> --- a/drivers/misc/sgi-gru/grufault.c
>> +++ b/drivers/misc/sgi-gru/grufault.c
>> @@ -224,10 +224,10 @@ static int atomic_pte_lookup(struct vm_area_struct *vma, unsigned long vaddr,
>>  		goto err;
>>  
>>  	pmdp = pmd_offset(pudp, vaddr);
>> -	if (unlikely(pmd_none(*pmdp)))
>> +	if (unlikely(pmd_none(pmdp_get(pmdp))))
>>  		goto err;
>>  #ifdef CONFIG_X86_64
>> -	if (unlikely(pmd_leaf(*pmdp)))
>> +	if (unlikely(pmd_leaf(pmdp_get(pmdp))))
> Just a general comment about multiple gets; before, the compiler most likely
> turned multiple '*pmdp' dereferences into a single actual load. But READ_ONCE()
> inside pmdp_get() ensures you get a load for every call to pmdp_get(). This has
> 2 potential problems:
> 
>  - More loads could potentially regress speed in some hot paths
> 
>  - In paths that don't hold an appropriate PTL the multiple loads could race
> with a writer, meaning each load gets a different value. The intent of the code
> is usually that each check is operating on the same value.

Makes sense, above two concerns are potential problems I guess.

> 
> For the ptep_get() conversion, I solved this by reading into a temporary once
> then using the temporary for the comparisons.

Alright.

> 
> I'm not sure if these are real problems in practice, but seems safest to
> continue to follow this established pattern?

Yes, will make the necessary changes across the series which might create some
amount of code churn but seems like it would be worth. Planning to add old_pxd
local variables when required and load them from the address, as soon as 'pxd'
pointer becomes valid.