Re: [PATCH 03/11] mm: page_vma_mapped_walk(): use pmd_read_atomic()

[Hugh Dickins]

On Thu, 10 Jun 2021, Jason Gunthorpe wrote:
> On Thu, Jun 10, 2021 at 12:06:17PM +0300, Kirill A. Shutemov wrote:
> > On Wed, Jun 09, 2021 at 11:38:11PM -0700, Hugh Dickins wrote:
> > > page_vma_mapped_walk() cleanup: use pmd_read_atomic() with barrier()
> > > instead of READ_ONCE() for pmde: some architectures (e.g. i386 with PAE)
> > > have a multi-word pmd entry, for which READ_ONCE() is not good enough.
> > > 
> > > Signed-off-by: Hugh Dickins <hughd@xxxxxxxxxx>
> > > Cc: <stable@xxxxxxxxxxxxxxx>
> > >  mm/page_vma_mapped.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
> > > index 7c0504641fb8..973c3c4e72cc 100644
> > > +++ b/mm/page_vma_mapped.c
> > > @@ -182,13 +182,16 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
> > >  	pud = pud_offset(p4d, pvmw->address);
> > >  	if (!pud_present(*pud))
> > >  		return false;
> > > +
> > >  	pvmw->pmd = pmd_offset(pud, pvmw->address);
> > >  	/*
> > >  	 * Make sure the pmd value isn't cached in a register by the
> > >  	 * compiler and used as a stale value after we've observed a
> > >  	 * subsequent update.
> > >  	 */
> > > -	pmde = READ_ONCE(*pvmw->pmd);
> > > +	pmde = pmd_read_atomic(pvmw->pmd);
> > > +	barrier();
> > > +
> > 
> > Hm. It makes me wounder if barrier() has to be part of pmd_read_atomic().
> > mm/hmm.c uses the same pattern as you are and I tend to think that the
> > rest of pmd_read_atomic() users may be broken.
> > 
> > Am I wrong?
> 
> I agree with you, something called _atomic should not require the
> caller to provide barriers.
> 
> I think the issue is simply that the two implementations of
> pmd_read_atomic() should use READ_ONCE() internally, no?

I've had great difficulty coming up with answers for you.

This patch was based on two notions I've had lodged in my mind
for several years:

1) that pmd_read_atomic() is the creme-de-la-creme for reading a pmd_t
atomically (even if the pmd_t spans multiple words); but reading again
after all this time the comment above it, it seems to be more specialized
than I'd thought (biggest selling point being for when you want to check
pmd_none(), which we don't).  And has no READ_ONCE() or barrier() inside,
so really needs that barrier() to be as safe as the previous READ_ONCE().

2) the barrier() in mm_find_pmd(), that replaced an earlier READ_ONCE(),
because READ_ONCE() did not work (did not give the necessary guarantee? or
did not build?) on architectures with multiple word pmd_ts e.g. i386 PAE.

But I've now come across some changes that Will Deacon made last year:
the include/asm-generic/rwonce.h READ_ONCE() now appears to allow for
native word type *or* type sizeof(long long) (e.g. i386 PAE) - given
"a strong prevailing wind" anyway :)  And 8e958839e4b9 ("sparc32: mm:
Restructure sparc32 MMU page-table layout") put an end to sparc32's
typedef struct { unsigned long pmdv[16]; } pmd_t.

It looks like my justification for this 03/11 patch is out-of-date,
and the patch should be dropped from the series.

As to your questions about pmd_read_atomic() usage elsewhere:
please don't force me to think so hard!  (And you've set me half-
wondering, whether there are sneaky THP transitions, perhaps of the
"unstable" kind, that page_vma_mapped_walk() should be paying more
attention to: but for sanity's sake I won't go there, not now.)

Hugh