Re: [PATCH 2/12] mm: rmap use pte lock not mmap_sem to set PageMlocked

Hugh Dickins
2015-10-19T19:53:27Z

On Mon, 19 Oct 2015, Kirill A. Shutemov wrote:
> On Mon, Oct 19, 2015 at 04:20:05AM -0700, Hugh Dickins wrote:
> > > Note how munlock_vma_pages_range() via __munlock_pagevec() does
> > > TestClearPageMlocked() without (or "between") pte or page lock. But the pte
> > > lock is being taken after clearing VM_LOCKED, so perhaps it's safe against
> > > try_to_unmap_one...
> >
> > A mind-trick I found helpful for understanding the barriers here, is
> > to imagine that the munlocker repeats its "vma->vm_flags &= ~VM_LOCKED"
> > every time it takes the pte lock: it does not actually do that, it
> > doesn't need to of course; but that does help show that ~VM_LOCKED
> > must be visible to anyone getting that pte lock afterwards.
>
> How can you make sure that any other codepath that changes vm_flags would
> not make (vm_flags & VM_LOCKED) temporary true while dealing with other
> flags?
>
> Compiler can convert things like "vma->vm_flags &= ~VM_FOO;" to whatever
> it wants as long as end result is the same. It's very unlikely that it
> will generate code to set all bits to one and then clear all which should
> be cleared, but it's theoretically possible.

I think that's in the realm of the fanciful. But yes, it quite often
turns out that what I think is fanciful, is something that Paul has
heard compiler writers say they want to do, even if he has managed
to discourage them from doing it so far.

But more to the point, when you write of "end result", the compiler
would have no idea that releasing mmap_sem is the point at which
end result must be established: wouldn't it have to establish end
result before the next unlock operation, and before the end of the
compilation unit? pte unlock being the relevant unlock operation
in this case, at least with my patch if not without.

>
> I think we need to have at lease WRITE_ONCE() everywhere we update
> vm_flags and READ_ONCE() where we read it without mmap_sem taken.

Not a series I'll embark upon myself,
and the patch at hand doesn't make things worse.

Hugh