Re: [RFC PATCH] mm: swap: remove lru drain waiters

[Hillf Danton <hdanton@sina.com>]

On Wed, 3 Jun 2020 12:24:12 +0200 "Ahmed S. Darwish" wrote:
> On Wed, Jun 03, 2020 at 10:21:45AM +0200, Sebastian Andrzej Siewior wrote:
> > On 2020-06-01 22:37:34 [+0800], Hillf Danton wrote:
> > >
> > > After updating the lru drain sequence, new comers avoid waiting for
> > > the current drainer, because he is flushing works on each online CPU,
> > > by trying to lock the mutex; the drainer OTOH tries to do works for
> > > those who fail to acquire the lock by checking the lru drain sequence
> > > after releasing lock.
> > >
> > > See eef1a429f234 ("mm/swap.c: piggyback lru_add_drain_all() calls")
> > > for reasons why we can skip waiting for the lock.
> > >
> > > The memory barriers around the sequence and the lock come together
> > > to remove waiters without their drain works bandoned.
> > >
> > > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > > Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>

      Cc: Ahmed S. Darwish <a.darwish@linutronix.de>

> > > Signed-off-by: Hillf Danton <hdanton@sina.com>
> > > ---
> > > This is inspired by one of the works from Sebastian.
> >
This is inspired by one of the works from Ahmed

  https://lore.kernel.org/linux-mm/20200519214547.352050-3-a.darwish@linutronix.de/

> >
> > > --- a/mm/swap.c
> > > +++ b/mm/swap.c
> > > @@ -714,10 +714,11 @@ static void lru_add_drain_per_cpu(struct
> > >   */
> > >  void lru_add_drain_all(void)
> > >  {
> > > -	static seqcount_t seqcount = SEQCNT_ZERO(seqcount);
> > > +	static unsigned int lru_drain_seq;
> > >  	static DEFINE_MUTEX(lock);
> > >  	static struct cpumask has_work;
> > > -	int cpu, seq;
> > > +	int cpu;
> > > +	unsigned int seq;
> > >
> > >  	/*
> > >  	 * Make sure nobody triggers this path before mm_percpu_wq is fully
> > > @@ -726,18 +727,16 @@ void lru_add_drain_all(void)
> > >  	if (WARN_ON(!mm_percpu_wq))
> > >  		return;
> > >
> > > -	seq = raw_read_seqcount_latch(&seqcount);
> > > +	lru_drain_seq++;
> > > +	smp_mb();
> > >
> > > -	mutex_lock(&lock);
> > > +more_work:
> > >
> > > -	/*
> > > -	 * Piggyback on drain started and finished while we waited for lock:
> > > -	 * all pages pended at the time of our enter were drained from vectors.
> > > -	 */
> > > -	if (__read_seqcount_retry(&seqcount, seq))
> > > -		goto done;
> > > +	if (!mutex_trylock(&lock))
> > > +		return;
> > >
> 
> The patch I've posted makes sure to preserve the existing draining
> logic. It only fixes an erroneous usage of seqcount_t latching, plus a
> memory barriers bugfix, found by John, and is to be included in v2:
> 
>     https://lkml.kernel.org/r/87y2pg9erj.fsf@vostro.fn.ogness.net

Thanks for your link.
> 
> On the other hand, you're making the draining operation completely
> asynchronous for a number of callers. This is such a huge change, and I
> fail to see: 1) any rationale for it in the changelog,

The changlog does not match the change. It is based on my understanding
that percpu pagevec is itself in nature async operation. The introduction
of seqcount casts a shaft of light on making lru drain async without any
individual drain work lost.

> 2) whether it's been verified that call-sites won't be affected.

No. It's 10x harder than s/lock/trylock/, you see.

Hillf