Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[David Hildenbrand <david@redhat.com>]

On 30.07.24 20:44, Peter Xu wrote:
> On Mon, Jul 29, 2024 at 07:46:26PM +0200, David Hildenbrand wrote:
>> I see what you mean but this is a very similar pattern as used in
>> collapse_pte_mapped_thp(), no? There we have
>>
>> start_pte = pte_offset_map_nolock(mm, pmd, haddr, &ptl);
>> ...
>> if (!pml)
>> 	spin_lock(ptl);
>> ...
>> pte_unmap(start_pte);
>> if (!pml)
>> 	spin_unlock(ptl);
>>
>>
>> Again, I don't have a strong opinion on this, but doing it more similar to
>> collapse_pte_mapped_thp() to obtain locks makes it clearer to me. But if I
>> am missing something obvious please shout and I'll change it.
> 
> Right.. I don't think that path can change the pte pgtable either, and
> there is even the line Hugh left showing it's impossible:
> 
> 	if (!start_pte)		/* mmap_lock + page lock should prevent this */
> 		goto abort;
> 
> I was thinking maybe the page lock is the critical one, irrelevant of mmap
> lock.
> 
> No strong opinion either.  Not sure whether Hugh has some thoughts.  But
> maybe if we stick with pte_offset_map_nolock() and if there'll be a repost
> anyway, we could add a similar comment like this one showing that the pte
> pgtable should be actually as stable as the ptlock.

I think this all deserves some future cleanups :)

... for the time being I'll have to drop this patch completely. I should 
have known that virt_to_page() does not work on kmap'ed pages, but part 
of me didn't want to believe it.

CONFIG_HIGHPTE wants to make my life challenging :)

-- 
Cheers,

David / dhildenb