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

[David Hildenbrand]

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