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

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

[Hillf Danton]

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>
Signed-off-by: Hillf Danton <hdanton@sina.com>
---
This is inspired by one of the works from Sebastian.

--- 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;
 
-	raw_write_seqcount_latch(&seqcount);
+	smp_mb();
+	seq = lru_drain_seq;
 
 	cpumask_clear(&has_work);
 
@@ -759,8 +758,11 @@ void lru_add_drain_all(void)
 	for_each_cpu(cpu, &has_work)
 		flush_work(&per_cpu(lru_add_drain_work, cpu));
 
-done:
 	mutex_unlock(&lock);
+
+	smp_mb();
+	if (seq != lru_drain_seq)
+		goto more_work;
 }
 #else
 void lru_add_drain_all(void)
--

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

[Konstantin Khlebnikov]

On 01/06/2020 17.37, 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.

That patch tells nothing about such change in behaviour.

Callers like invalidate_bdev() really need synchronous drain to be sure
that pages have no extra reference from per-cpu vectors.

> 
> 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>
> Signed-off-by: Hillf Danton <hdanton@sina.com>
> ---
> This is inspired by one of the works from Sebastian.
> 
> --- 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;
>   
> -	raw_write_seqcount_latch(&seqcount);
> +	smp_mb();
> +	seq = lru_drain_seq;
>   
>   	cpumask_clear(&has_work);
>   
> @@ -759,8 +758,11 @@ void lru_add_drain_all(void)
>   	for_each_cpu(cpu, &has_work)
>   		flush_work(&per_cpu(lru_add_drain_work, cpu));
>   
> -done:
>   	mutex_unlock(&lock);
> +
> +	smp_mb();
> +	if (seq != lru_drain_seq)
> +		goto more_work;
>   }
>   #else
>   void lru_add_drain_all(void)
> --
> 

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

[Sebastian Andrzej Siewior]

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>
> Signed-off-by: Hillf Danton <hdanton@sina.com>
> ---
> This is inspired by one of the works from Sebastian.

Not me, it was Ahmed.

> --- 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;
>  
> -	raw_write_seqcount_latch(&seqcount);
> +	smp_mb();
> +	seq = lru_drain_seq;
>  
>  	cpumask_clear(&has_work);
>  
> @@ -759,8 +758,11 @@ void lru_add_drain_all(void)
>  	for_each_cpu(cpu, &has_work)
>  		flush_work(&per_cpu(lru_add_drain_work, cpu));
>  
> -done:
>  	mutex_unlock(&lock);
> +
> +	smp_mb();
> +	if (seq != lru_drain_seq)
> +		goto more_work;
>  }
>  #else
>  void lru_add_drain_all(void)
> --

Sebastian

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

[Ahmed S. Darwish]

Hi Hillf,

For some reason, **all of your posts** from <hdanton@sina.com> do not
appear on lore.kernel.org.

Check, for example, https://lore.kernel.org/lkml/?q=hdanton%40sina.com,
where thread replies are there but not the actual posts.

Just wanted to let you know... Please continue below.

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>
> > Signed-off-by: Hillf Danton <hdanton@sina.com>
> > ---
> > This is inspired by one of the works from Sebastian.
>
> Not me, it was Ahmed.
>
> > --- 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

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, 2) whether it's
been verified that call-sites won't be affected.

Thanks,

--
Ahmed S. Darwish
Linutronix GmbH

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

[Hillf Danton]

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