Re: [PATCH v3] mm/memory: use pmdp_get_lockless() for the pmd read in do_fault_around()

["David Hildenbrand (Arm)" <david@kernel.org>]

>> +++ b/mm/memory.c
>> @@ -6017,7 +6017,8 @@ static vm_fault_t do_fault_around(struct vm_fault *vmf)
>>  	to_pte = min3(from_pte + nr_pages, (pgoff_t)PTRS_PER_PTE,
>>  		      pte_off + vma_pages(vmf->vma) - vma_off) - 1;
>>
>> -	if (pmd_none(*vmf->pmd)) {
> 
> This pattern is definitely bad I think, we have it in many other places so
> should probably convert those to pmdp_get() at least?

Usama is working on this as part of separating HW and SW PMDs (he only sent out
the PTE part so far).

> 
>> +	/* pmd_install() rechecks pmd_none() under the pmd lock. */
>> +	if (pmd_none(pmdp_get_lockless(vmf->pmd))) {
> 
> It definitely seems right to use at least pmdp_get() here, but I wonder
> about the need for pmdp_get_lockless() here.

Whenever we're not holding the page table lock we should be using
pmdp_get_lockless().

Even if they are semantically equivalent for most cases, it serves as a good
reminder that there are no guarantees.

And that you have to look again under PT lock.

> 
> pmd_none() only checks the lower word, so a torn 64-bit word on 32-bit
> (i.e. the PAE style cases we are worried about) makes no difference:
> 
> From x86 pmd_none():
> 
> 	/* Only check low word on 32-bit platforms, since it might be
> 	   out of sync with upper half. */
> 
> And anyway I guess there is no valid PMD in such systems that would have
> the lower word cleared and upper word not cleared.
> 

I don't understand the discussion really.

No PT lock -> use the lockless variant.

Until we can remove the lockless variant.

Are we consistent with that? Likely not.

But that's all independent of this patch here.

-- 
Cheers,

David