On Wed, Aug 14, 2024 at 11:16 PM Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, 14 Aug 2024 16:52:56 +0200 Mateusz Guzik <mjguzik@xxxxxxxxx> wrote:
>
> > Commit 362a61ad6119 ("fix SMP data race in pagetable setup vs walking")
> > added the following:
> >
> > + smp_wmb(); /* Could be smp_wmb__xxx(before|after)_spin_lock */
> > +
> > spin_lock(&mm->page_table_lock);
> >
> > However, over the years the fence along with the comment got moved
> > around the file, eventually landing in a spot where it is *NOT* followed
> > by a lock acquire (or any other operation which might happen to provide
> > any fence on a given arch), rendering the comment stale.
> >
> > ...
> >
> > I fully concede I could not be arsed to check if the fence is still
> > needed to begin with, I ran into this while looking at something else.
> > The comment puzzled me for a minute suggesting pmd_populate has an
> > immediate lock acquire inside.
> >
> > ...
> >
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -436,7 +436,7 @@ void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t *pte)
> > * seen in-order. See the alpha page table accessors for the
> > * smp_rmb() barriers in page table walking code.
> > */
> > - smp_wmb(); /* Could be smp_wmb__xxx(before|after)_spin_lock */
> > + smp_wmb();
> > pmd_populate(mm, pmd, *pte);
> > *pte = NULL;
> > }
>
> It's best to document all such barriers, so the preferred patch would
> be to fix the comment rather than removing it.
>
> And if the barrier now does nothing then of course removing the thing
> would be best.
>
> So I'd suggest that the wrong comment be left there, if only to tell
> developers why the barrier used to be there!
The comment above it (only partially seen in the context) documents
what the purpose is.
The comment I'm removing merely mentions a no longer applicable
optimization opportunity: it used to be immediately followed by
spin_lock. If the architecture at hand provides a full fence when
acquiring a lock *and* has a costly smp_wmb, then a hypothetical
smp_wmb__before_spin_lock could be used to elide it.
--
Mateusz Guzik <mjguzik gmail.com>
