[PATCH] mm: whack now bogus comment in pmd_install() concerning a fence

[PATCH] mm: whack now bogus comment in pmd_install() concerning a fence

[Mateusz Guzik]

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.

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---

./scripts/get_maintainer.pl only showed akpm@ and the lists, adding the
--git switch showed more names, but I only picked some of them. I don't
know who makes the most sense to add here.

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.

 mm/memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memory.c b/mm/memory.c
index 34f8402d2046..0a6893833fac 100644
--- 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;
 	}
-- 
2.43.0

Re: [PATCH] mm: whack now bogus comment in pmd_install() concerning a fence

[Andrew Morton]

On Wed, 14 Aug 2024 16:52:56 +0200 Mateusz Guzik <mjguzik@gmail.com> 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!

Re: [PATCH] mm: whack now bogus comment in pmd_install() concerning a fence

[Mateusz Guzik]

On Wed, Aug 14, 2024 at 11:16 PM Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> On Wed, 14 Aug 2024 16:52:56 +0200 Mateusz Guzik <mjguzik@gmail.com> 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>