Re: [PATCH 2/2] mm: swap: use smp_mb__after_atomic() to order LRU bit set

[Yang Shi]

On 3/16/20 10:49 AM, Yang Shi wrote:
> On 3/16/20 10:40 AM, Vlastimil Babka wrote:
> > On 3/13/20 7:34 PM, Yang Shi wrote:
> > > Memory barrier is needed after setting LRU bit, but smp_mb() is too
> > > strong. Some architectures, i.e. x86, imply memory barrier with atomic
> > > operations, so replacing it with smp_mb__after_atomic() sounds better,
> > > which is nop on strong ordered machines, and full memory barriers on
> > > others. With this change the vm-calability cases would perform better
> > > on x86, I saw total 6% improvement with this patch and previous inline
> > > fix.
> > >
> > > The test data (lru-file-readtwice throughput) against v5.6-rc4:
> > > ÂÂÂÂmainlineÂÂÂ w/ inline fixÂÂÂ w/ both (adding this)
> > > ÂÂÂÂ150MBÂÂÂÂÂÂÂ 154MBÂÂÂÂÂÂÂ 159MB
> > >
> > > Fixes: 9c4e6b1a7027 ("mm, mlock, vmscan: no more skipping pagevecs")
> > > Cc: Shakeel Butt <shakeelb@xxxxxxxxxx>
> > > Cc: Vlastimil Babka <vbabka@xxxxxxx>
> > > Signed-off-by: Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx>
> > According to my understanding of Documentation/memory_barriers.txt 
> > this would be
> > correct (but it might not say much :)
>
> This is my understanding too.
>
> > Acked-by: Vlastimil Babka <vbabka@xxxxxxx>
> >
> > But i have some suggestions...
> >
> > > ---
> > > Â mm/swap.c | 6 +++---
> > > Â 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/mm/swap.c b/mm/swap.c
> > > index cf39d24..118bac4 100644
> > > --- a/mm/swap.c
> > > +++ b/mm/swap.c
> > > @@ -945,20 +945,20 @@ static void __pagevec_lru_add_fn(struct page 
> > > *page, struct lruvec *lruvec,
> > > ÂÂÂÂÂÂ * #0: __pagevec_lru_add_fnÂÂÂÂÂÂÂ #1: clear_page_mlock
> > > ÂÂÂÂÂÂ *
> > > ÂÂÂÂÂÂ * SetPageLRU()ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ TestClearPageMlocked()
> > > -ÂÂÂÂ * smp_mb() // explicit orderingÂÂÂ // above provides strict
> > > +ÂÂÂÂ * MB()ÂÂÂÂ // explicit orderingÂÂÂ // above provides strict
> > Why MB()? That would be the first appareance of 'MB()' in the whole 
> > tree. I
> > think it's fine keeping smp_mb()...
>
> I would like to use a more general name, maybe just use "memory barrier"?

Keeping smp_mb() should be just fine...

> > > ÂÂÂÂÂÂ *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ // ordering
> > > ÂÂÂÂÂÂ * PageMlocked()ÂÂÂÂÂÂÂÂÂÂÂ PageLRU()
> > > ÂÂÂÂÂÂ *
> > > ÂÂÂÂÂÂ *
> > > ÂÂÂÂÂÂ * if '#1' does not observe setting of PG_lru by '#0' and fails
> > > ÂÂÂÂÂÂ * isolation, the explicit barrier will make sure that 
> > > page_evictable
> > > -ÂÂÂÂ * check will put the page in correct LRU. Without smp_mb(), 
> > > SetPageLRU
> > > +ÂÂÂÂ * check will put the page in correct LRU. Without MB(), 
> > > SetPageLRU
> > ... same here ...
> >
> > > ÂÂÂÂÂÂ * can be reordered after PageMlocked check and can make '#1' 
> > > to fail
> > > ÂÂÂÂÂÂ * the isolation of the page whose Mlocked bit is cleared (#0 
> > > is also
> > > ÂÂÂÂÂÂ * looking at the same page) and the evictable page will be 
> > > stranded
> > > ÂÂÂÂÂÂ * in an unevictable LRU.
> > Only here I would note that SetPageLRU() is an atomic bitop so we can 
> > use the
> > __after_atomic() variant. And I would move the actual SetPageLRU() 
> > call from
> > above the comment here right before the barrier.
>
> Sure. Thanks.
>
> > > ÂÂÂÂÂÂ */
> > > -ÂÂÂ smp_mb();
> > > +ÂÂÂ smp_mb__after_atomic();
> > Thanks.
> >
> > > Â ÂÂÂÂÂ if (page_evictable(page)) {
> > > ÂÂÂÂÂÂÂÂÂ lru = page_lru(page);