All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mm/swap.c: workaround for_each_cpu() bug on UP kernel.
@ 2019-02-07  9:53 Tetsuo Handa
  2019-02-07 14:07 ` Guenter Roeck
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Tetsuo Handa @ 2019-02-07  9:53 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Chris Metcalf, Rusty Russell, linux-mm, Tetsuo Handa, Guenter Roeck

Since for_each_cpu(cpu, mask) added by commit 2d3854a37e8b767a ("cpumask:
introduce new API, without changing anything") did not evaluate the mask
argument if NR_CPUS == 1 due to CONFIG_SMP=n, lru_add_drain_all() is
hitting WARN_ON() at __flush_work() added by commit 4d43d395fed12463
("workqueue: Try to catch flush_work() without INIT_WORK().")
by unconditionally calling flush_work() [1].

We should fix for_each_cpu() etc. but we need enough grace period for
allowing people to test and fix unexpected behaviors including build
failures. Therefore, this patch temporarily duplicates flush_work() for
NR_CPUS == 1 case. This patch will be reverted after for_each_cpu() etc.
are fixed.

[1] https://lkml.kernel.org/r/18a30387-6aa5-6123-e67c-57579ecc3f38@roeck-us.net

Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 mm/swap.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/swap.c b/mm/swap.c
index 4929bc1..e5e8e15 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -694,11 +694,16 @@ void lru_add_drain_all(void)
 			INIT_WORK(work, lru_add_drain_per_cpu);
 			queue_work_on(cpu, mm_percpu_wq, work);
 			cpumask_set_cpu(cpu, &has_work);
+#if NR_CPUS == 1
+			flush_work(work);
+#endif
 		}
 	}
 
+#if NR_CPUS != 1
 	for_each_cpu(cpu, &has_work)
 		flush_work(&per_cpu(lru_add_drain_work, cpu));
+#endif
 
 	mutex_unlock(&lock);
 }
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/swap.c: workaround for_each_cpu() bug on UP kernel.
  2019-02-07  9:53 [PATCH] mm/swap.c: workaround for_each_cpu() bug on UP kernel Tetsuo Handa
@ 2019-02-07 14:07 ` Guenter Roeck
  2019-02-07 14:18 ` William Kucharski
  2019-02-12 10:11 ` Michal Hocko
  2 siblings, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2019-02-07 14:07 UTC (permalink / raw)
  To: Tetsuo Handa, Andrew Morton; +Cc: Chris Metcalf, Rusty Russell, linux-mm

On 2/7/19 1:53 AM, Tetsuo Handa wrote:
> Since for_each_cpu(cpu, mask) added by commit 2d3854a37e8b767a ("cpumask:
> introduce new API, without changing anything") did not evaluate the mask
> argument if NR_CPUS == 1 due to CONFIG_SMP=n, lru_add_drain_all() is
> hitting WARN_ON() at __flush_work() added by commit 4d43d395fed12463
> ("workqueue: Try to catch flush_work() without INIT_WORK().")
> by unconditionally calling flush_work() [1].
> 
> We should fix for_each_cpu() etc. but we need enough grace period for
> allowing people to test and fix unexpected behaviors including build
> failures. Therefore, this patch temporarily duplicates flush_work() for
> NR_CPUS == 1 case. This patch will be reverted after for_each_cpu() etc.
> are fixed.
> 
> [1] https://lkml.kernel.org/r/18a30387-6aa5-6123-e67c-57579ecc3f38@roeck-us.net
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

I agree with the fix/workaround. I tried a complete build with fixed macros,
but that doesn't work because (at least) x86 assumes that the "mask" parameter
is _not_ evaluated for non-SMP builds - arch/x86/kernel/cpu/cacheinfo.c
passes cpu_llc_shared_mask(cpu) as parameter, and that is only defined
for SMP builds.

On the plus side, I did not find any other issues, but that doesn't mean
much since various build and boot tests in -next fail for other reasons.

Acked-by: Guenter Roeck <linux@roeck-us.net>

Guenter

> ---
>   mm/swap.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/mm/swap.c b/mm/swap.c
> index 4929bc1..e5e8e15 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -694,11 +694,16 @@ void lru_add_drain_all(void)
>   			INIT_WORK(work, lru_add_drain_per_cpu);
>   			queue_work_on(cpu, mm_percpu_wq, work);
>   			cpumask_set_cpu(cpu, &has_work);
> +#if NR_CPUS == 1
> +			flush_work(work);
> +#endif
>   		}
>   	}
>   
> +#if NR_CPUS != 1
>   	for_each_cpu(cpu, &has_work)
>   		flush_work(&per_cpu(lru_add_drain_work, cpu));
> +#endif
>   
>   	mutex_unlock(&lock);
>   }
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/swap.c: workaround for_each_cpu() bug on UP kernel.
  2019-02-07  9:53 [PATCH] mm/swap.c: workaround for_each_cpu() bug on UP kernel Tetsuo Handa
  2019-02-07 14:07 ` Guenter Roeck
@ 2019-02-07 14:18 ` William Kucharski
  2019-02-12 10:11 ` Michal Hocko
  2 siblings, 0 replies; 13+ messages in thread
From: William Kucharski @ 2019-02-07 14:18 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: Andrew Morton, Chris Metcalf, Rusty Russell, linux-mm, Guenter Roeck

Would you mind adding a comment explaining this?

That way if, for some reason, the patch isn't reverted in a timely manner, anyone
stumbling upon the code knows why it's done the way it is without having to track
down this mail thread.

Reviewed by: William Kucharski <william.kucharski@oracle.com> 

> On Feb 7, 2019, at 2:53 AM, Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> 
> Since for_each_cpu(cpu, mask) added by commit 2d3854a37e8b767a ("cpumask:
> introduce new API, without changing anything") did not evaluate the mask
> argument if NR_CPUS == 1 due to CONFIG_SMP=n, lru_add_drain_all() is
> hitting WARN_ON() at __flush_work() added by commit 4d43d395fed12463
> ("workqueue: Try to catch flush_work() without INIT_WORK().")
> by unconditionally calling flush_work() [1].
> 
> We should fix for_each_cpu() etc. but we need enough grace period for
> allowing people to test and fix unexpected behaviors including build
> failures. Therefore, this patch temporarily duplicates flush_work() for
> NR_CPUS == 1 case. This patch will be reverted after for_each_cpu() etc.
> are fixed.
> 
> [1] https://lkml.kernel.org/r/18a30387-6aa5-6123-e67c-57579ecc3f38@roeck-us.net
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> mm/swap.c | 5 +++++
> 1 file changed, 5 insertions(+)
> 
> diff --git a/mm/swap.c b/mm/swap.c
> index 4929bc1..e5e8e15 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -694,11 +694,16 @@ void lru_add_drain_all(void)
> 			INIT_WORK(work, lru_add_drain_per_cpu);
> 			queue_work_on(cpu, mm_percpu_wq, work);
> 			cpumask_set_cpu(cpu, &has_work);
> +#if NR_CPUS == 1
> +			flush_work(work);
> +#endif
> 		}
> 	}
> 
> +#if NR_CPUS != 1
> 	for_each_cpu(cpu, &has_work)
> 		flush_work(&per_cpu(lru_add_drain_work, cpu));
> +#endif
> 
> 	mutex_unlock(&lock);
> }
> -- 
> 1.8.3.1
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/swap.c: workaround for_each_cpu() bug on UP kernel.
  2019-02-07  9:53 [PATCH] mm/swap.c: workaround for_each_cpu() bug on UP kernel Tetsuo Handa
  2019-02-07 14:07 ` Guenter Roeck
  2019-02-07 14:18 ` William Kucharski
@ 2019-02-12 10:11 ` Michal Hocko
  2019-02-12 10:25   ` Tetsuo Handa
  2 siblings, 1 reply; 13+ messages in thread
From: Michal Hocko @ 2019-02-12 10:11 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: Andrew Morton, Chris Metcalf, Rusty Russell, linux-mm, Guenter Roeck

On Thu 07-02-19 18:53:09, Tetsuo Handa wrote:
> Since for_each_cpu(cpu, mask) added by commit 2d3854a37e8b767a ("cpumask:
> introduce new API, without changing anything") did not evaluate the mask
> argument if NR_CPUS == 1 due to CONFIG_SMP=n, lru_add_drain_all() is
> hitting WARN_ON() at __flush_work() added by commit 4d43d395fed12463
> ("workqueue: Try to catch flush_work() without INIT_WORK().")
> by unconditionally calling flush_work() [1].
> 
> We should fix for_each_cpu() etc. but we need enough grace period for
> allowing people to test and fix unexpected behaviors including build
> failures. Therefore, this patch temporarily duplicates flush_work() for
> NR_CPUS == 1 case. This patch will be reverted after for_each_cpu() etc.
> are fixed.
> 
> [1] https://lkml.kernel.org/r/18a30387-6aa5-6123-e67c-57579ecc3f38@roeck-us.net
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

This patch is ugly as hell! I do agree that for_each_cpu not working on
CONFIG_SMP=n sucks but why do we even care about lru_add_drain_all when
there is a single cpu? Why don't we simply do

diff --git a/mm/swap.c b/mm/swap.c
index aa483719922e..952f24b09070 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -660,6 +660,7 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
 
 static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
 
+#ifdef CONFIG_SMP
 /*
  * Doesn't need any cpu hotplug locking because we do rely on per-cpu
  * kworkers being shut down before our page_alloc_cpu_dead callback is
@@ -702,6 +703,10 @@ void lru_add_drain_all(void)
 
 	mutex_unlock(&lock);
 }
+#else
+#define lru_add_drain_all() lru_add_drain()
+
+#endif
 
 /**
  * release_pages - batched put_page()
-- 
Michal Hocko
SUSE Labs


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/swap.c: workaround for_each_cpu() bug on UP kernel.
  2019-02-12 10:11 ` Michal Hocko
@ 2019-02-12 10:25   ` Tetsuo Handa
  2019-02-12 11:21     ` Michal Hocko
  0 siblings, 1 reply; 13+ messages in thread
From: Tetsuo Handa @ 2019-02-12 10:25 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, Chris Metcalf, Rusty Russell, linux-mm, Guenter Roeck

On 2019/02/12 19:11, Michal Hocko wrote:
> This patch is ugly as hell! I do agree that for_each_cpu not working on
> CONFIG_SMP=n sucks but why do we even care about lru_add_drain_all when
> there is a single cpu? Why don't we simply do
> 
> diff --git a/mm/swap.c b/mm/swap.c
> index aa483719922e..952f24b09070 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -660,6 +660,7 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
>  
>  static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
>  
> +#ifdef CONFIG_SMP
>  /*
>   * Doesn't need any cpu hotplug locking because we do rely on per-cpu
>   * kworkers being shut down before our page_alloc_cpu_dead callback is
> @@ -702,6 +703,10 @@ void lru_add_drain_all(void)
>  
>  	mutex_unlock(&lock);
>  }
> +#else
> +#define lru_add_drain_all() lru_add_drain()
> +
> +#endif

If there is no need to evaluate the "if" conditions, I'm fine with this shortcut.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/swap.c: workaround for_each_cpu() bug on UP kernel.
  2019-02-12 10:25   ` Tetsuo Handa
@ 2019-02-12 11:21     ` Michal Hocko
  2019-02-12 11:29       ` Michal Hocko
  0 siblings, 1 reply; 13+ messages in thread
From: Michal Hocko @ 2019-02-12 11:21 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: Andrew Morton, Chris Metcalf, Rusty Russell, linux-mm, Guenter Roeck

On Tue 12-02-19 19:25:46, Tetsuo Handa wrote:
> On 2019/02/12 19:11, Michal Hocko wrote:
> > This patch is ugly as hell! I do agree that for_each_cpu not working on
> > CONFIG_SMP=n sucks but why do we even care about lru_add_drain_all when
> > there is a single cpu? Why don't we simply do
> > 
> > diff --git a/mm/swap.c b/mm/swap.c
> > index aa483719922e..952f24b09070 100644
> > --- a/mm/swap.c
> > +++ b/mm/swap.c
> > @@ -660,6 +660,7 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
> >  
> >  static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
> >  
> > +#ifdef CONFIG_SMP
> >  /*
> >   * Doesn't need any cpu hotplug locking because we do rely on per-cpu
> >   * kworkers being shut down before our page_alloc_cpu_dead callback is
> > @@ -702,6 +703,10 @@ void lru_add_drain_all(void)
> >  
> >  	mutex_unlock(&lock);
> >  }
> > +#else
> > +#define lru_add_drain_all() lru_add_drain()
> > +
> > +#endif
> 
> If there is no need to evaluate the "if" conditions, I'm fine with this shortcut.

lru_add_drain does drain only pagevecs which have pages and so we do not
really have to duplicate the check. There is also no need to defer the
execution to the workqueue for a local cpu. So we are left with only the
lock to prevent parallel execution but the preemption disabling acts the
same purpose on UP so the approach should be equivalent from the
correctness point of view.
-- 
Michal Hocko
SUSE Labs


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/swap.c: workaround for_each_cpu() bug on UP kernel.
  2019-02-12 11:21     ` Michal Hocko
@ 2019-02-12 11:29       ` Michal Hocko
  2019-02-12 11:37         ` Tetsuo Handa
                           ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Michal Hocko @ 2019-02-12 11:29 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: Andrew Morton, Chris Metcalf, Rusty Russell, linux-mm, Guenter Roeck

On Tue 12-02-19 12:21:17, Michal Hocko wrote:
> On Tue 12-02-19 19:25:46, Tetsuo Handa wrote:
> > On 2019/02/12 19:11, Michal Hocko wrote:
> > > This patch is ugly as hell! I do agree that for_each_cpu not working on
> > > CONFIG_SMP=n sucks but why do we even care about lru_add_drain_all when
> > > there is a single cpu? Why don't we simply do
> > > 
> > > diff --git a/mm/swap.c b/mm/swap.c
> > > index aa483719922e..952f24b09070 100644
> > > --- a/mm/swap.c
> > > +++ b/mm/swap.c
> > > @@ -660,6 +660,7 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
> > >  
> > >  static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
> > >  
> > > +#ifdef CONFIG_SMP
> > >  /*
> > >   * Doesn't need any cpu hotplug locking because we do rely on per-cpu
> > >   * kworkers being shut down before our page_alloc_cpu_dead callback is
> > > @@ -702,6 +703,10 @@ void lru_add_drain_all(void)
> > >  
> > >  	mutex_unlock(&lock);
> > >  }
> > > +#else
> > > +#define lru_add_drain_all() lru_add_drain()
> > > +
> > > +#endif
> > 
> > If there is no need to evaluate the "if" conditions, I'm fine with this shortcut.
> 
> lru_add_drain does drain only pagevecs which have pages and so we do not
> really have to duplicate the check. There is also no need to defer the
> execution to the workqueue for a local cpu. So we are left with only the
> lock to prevent parallel execution but the preemption disabling acts the
> same purpose on UP so the approach should be equivalent from the
> correctness point of view.

The patch with the full changelog follows:


From db104f132bd6e1c02ecbe65e62c12caa7e4e2e2a Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Tue, 12 Feb 2019 12:25:28 +0100
Subject: [PATCH] mm: handle lru_add_drain_all for UP properly

Since for_each_cpu(cpu, mask) added by commit 2d3854a37e8b767a ("cpumask:
introduce new API, without changing anything") did not evaluate the mask
argument if NR_CPUS == 1 due to CONFIG_SMP=n, lru_add_drain_all() is
hitting WARN_ON() at __flush_work() added by commit 4d43d395fed12463
("workqueue: Try to catch flush_work() without INIT_WORK().")
by unconditionally calling flush_work() [1].

Workaround this issue by using CONFIG_SMP=n specific lru_add_drain_all
implementation. There is no real need to defer the implementation to the
workqueue as the draining is going to happen on the local cpu. So alias
lru_add_drain_all to lru_add_drain which does all the necessary work.

[1] https://lkml.kernel.org/r/18a30387-6aa5-6123-e67c-57579ecc3f38@roeck-us.net
Reported-by: Guenter Roeck <linux@roeck-us.net>
Debugged-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/swap.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/swap.c b/mm/swap.c
index 4929bc1be60e..88a6021fce11 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -660,6 +660,7 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
 
 static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
 
+#ifdef CONFIG_SMP
 /*
  * Doesn't need any cpu hotplug locking because we do rely on per-cpu
  * kworkers being shut down before our page_alloc_cpu_dead callback is
@@ -702,6 +703,10 @@ void lru_add_drain_all(void)
 
 	mutex_unlock(&lock);
 }
+#else
+#define lru_add_drain_all() lru_add_drain()
+
+#endif
 
 /**
  * release_pages - batched put_page()
-- 
2.20.1
-- 
Michal Hocko
SUSE Labs


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/swap.c: workaround for_each_cpu() bug on UP kernel.
  2019-02-12 11:29       ` Michal Hocko
@ 2019-02-12 11:37         ` Tetsuo Handa
  2019-02-12 21:06         ` Andrew Morton
                           ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Tetsuo Handa @ 2019-02-12 11:37 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Michal Hocko, Chris Metcalf, Rusty Russell, linux-mm, Guenter Roeck

Andrew, please replace

  mm-swapc-workaround-for_each_cpu-bug-on-up-kernel.patch

with Michal's patch.

(I didn't debug this. Guenter already debugged this before reporting.)

On 2019/02/12 20:29, Michal Hocko wrote:
>>From db104f132bd6e1c02ecbe65e62c12caa7e4e2e2a Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Tue, 12 Feb 2019 12:25:28 +0100
> Subject: [PATCH] mm: handle lru_add_drain_all for UP properly
> 
> Since for_each_cpu(cpu, mask) added by commit 2d3854a37e8b767a ("cpumask:
> introduce new API, without changing anything") did not evaluate the mask
> argument if NR_CPUS == 1 due to CONFIG_SMP=n, lru_add_drain_all() is
> hitting WARN_ON() at __flush_work() added by commit 4d43d395fed12463
> ("workqueue: Try to catch flush_work() without INIT_WORK().")
> by unconditionally calling flush_work() [1].
> 
> Workaround this issue by using CONFIG_SMP=n specific lru_add_drain_all
> implementation. There is no real need to defer the implementation to the
> workqueue as the draining is going to happen on the local cpu. So alias
> lru_add_drain_all to lru_add_drain which does all the necessary work.
> 
> [1] https://lkml.kernel.org/r/18a30387-6aa5-6123-e67c-57579ecc3f38@roeck-us.net
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Debugged-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  mm/swap.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/mm/swap.c b/mm/swap.c
> index 4929bc1be60e..88a6021fce11 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -660,6 +660,7 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
>  
>  static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
>  
> +#ifdef CONFIG_SMP
>  /*
>   * Doesn't need any cpu hotplug locking because we do rely on per-cpu
>   * kworkers being shut down before our page_alloc_cpu_dead callback is
> @@ -702,6 +703,10 @@ void lru_add_drain_all(void)
>  
>  	mutex_unlock(&lock);
>  }
> +#else
> +#define lru_add_drain_all() lru_add_drain()
> +
> +#endif
>  
>  /**
>   * release_pages - batched put_page()
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/swap.c: workaround for_each_cpu() bug on UP kernel.
  2019-02-12 11:29       ` Michal Hocko
  2019-02-12 11:37         ` Tetsuo Handa
@ 2019-02-12 21:06         ` Andrew Morton
  2019-02-13 12:43           ` Michal Hocko
  2019-02-12 23:19         ` [PATCH] mm: handle lru_add_drain_all for UP properly kbuild test robot
  2019-02-12 23:48         ` kbuild test robot
  3 siblings, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2019-02-12 21:06 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Tetsuo Handa, Chris Metcalf, Rusty Russell, linux-mm, Guenter Roeck

On Tue, 12 Feb 2019 12:29:54 +0100 Michal Hocko <mhocko@kernel.org> wrote:

> On Tue 12-02-19 12:21:17, Michal Hocko wrote:
> > On Tue 12-02-19 19:25:46, Tetsuo Handa wrote:
> > > On 2019/02/12 19:11, Michal Hocko wrote:
> > > > This patch is ugly as hell! I do agree that for_each_cpu not working on
> > > > CONFIG_SMP=n sucks but why do we even care about lru_add_drain_all when
> > > > there is a single cpu? Why don't we simply do
> > > > 
> > > > diff --git a/mm/swap.c b/mm/swap.c
> > > > index aa483719922e..952f24b09070 100644
> > > > --- a/mm/swap.c
> > > > +++ b/mm/swap.c
> > > > @@ -660,6 +660,7 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
> > > >  
> > > >  static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
> > > >  
> > > > +#ifdef CONFIG_SMP
> > > >  /*
> > > >   * Doesn't need any cpu hotplug locking because we do rely on per-cpu
> > > >   * kworkers being shut down before our page_alloc_cpu_dead callback is
> > > > @@ -702,6 +703,10 @@ void lru_add_drain_all(void)
> > > >  
> > > >  	mutex_unlock(&lock);
> > > >  }
> > > > +#else
> > > > +#define lru_add_drain_all() lru_add_drain()
> > > > +
> > > > +#endif
> > > 
> > > If there is no need to evaluate the "if" conditions, I'm fine with this shortcut.
> > 
> > lru_add_drain does drain only pagevecs which have pages and so we do not
> > really have to duplicate the check. There is also no need to defer the
> > execution to the workqueue for a local cpu. So we are left with only the
> > lock to prevent parallel execution but the preemption disabling acts the
> > same purpose on UP so the approach should be equivalent from the
> > correctness point of view.
> 
> The patch with the full changelog follows:
> 
> 
> >From db104f132bd6e1c02ecbe65e62c12caa7e4e2e2a Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Tue, 12 Feb 2019 12:25:28 +0100
> Subject: [PATCH] mm: handle lru_add_drain_all for UP properly
> 
> Since for_each_cpu(cpu, mask) added by commit 2d3854a37e8b767a ("cpumask:
> introduce new API, without changing anything") did not evaluate the mask
> argument if NR_CPUS == 1 due to CONFIG_SMP=n, lru_add_drain_all() is
> hitting WARN_ON() at __flush_work() added by commit 4d43d395fed12463
> ("workqueue: Try to catch flush_work() without INIT_WORK().")
> by unconditionally calling flush_work() [1].
> 
> Workaround this issue by using CONFIG_SMP=n specific lru_add_drain_all
> implementation. There is no real need to defer the implementation to the
> workqueue as the draining is going to happen on the local cpu. So alias
> lru_add_drain_all to lru_add_drain which does all the necessary work.
> 
> [1] https://lkml.kernel.org/r/18a30387-6aa5-6123-e67c-57579ecc3f38@roeck-us.net
>
> ...
>
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -660,6 +660,7 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
>  
>  static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
>  
> +#ifdef CONFIG_SMP
>  /*
>   * Doesn't need any cpu hotplug locking because we do rely on per-cpu
>   * kworkers being shut down before our page_alloc_cpu_dead callback is
> @@ -702,6 +703,10 @@ void lru_add_drain_all(void)
>  
>  	mutex_unlock(&lock);
>  }
> +#else
> +#define lru_add_drain_all() lru_add_drain()
> +
> +#endif
>  
>  /**
>   * release_pages - batched put_page()

How can this even link?  Lots of compilation units call
lru_add_drain_all() but the implementation just got removed.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm: handle lru_add_drain_all for UP properly
  2019-02-12 11:29       ` Michal Hocko
  2019-02-12 11:37         ` Tetsuo Handa
  2019-02-12 21:06         ` Andrew Morton
@ 2019-02-12 23:19         ` kbuild test robot
  2019-02-12 23:48         ` kbuild test robot
  3 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2019-02-12 23:19 UTC (permalink / raw)
  To: Michal Hocko
  Cc: kbuild-all, Tetsuo Handa, Andrew Morton, Chris Metcalf,
	Rusty Russell, linux-mm, Guenter Roeck

[-- Attachment #1: Type: text/plain, Size: 1022 bytes --]

Hi Michal,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.0-rc4 next-20190212]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Michal-Hocko/mm-handle-lru_add_drain_all-for-UP-properly/20190213-063735
config: riscv-tinyconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 8.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.2.0 make.cross ARCH=riscv 

All errors (new ones prefixed by >>):

   mm/fadvise.o: In function `.L18':
>> fadvise.c:(.text+0x1e8): undefined reference to `lru_add_drain_all'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 4459 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm: handle lru_add_drain_all for UP properly
  2019-02-12 11:29       ` Michal Hocko
                           ` (2 preceding siblings ...)
  2019-02-12 23:19         ` [PATCH] mm: handle lru_add_drain_all for UP properly kbuild test robot
@ 2019-02-12 23:48         ` kbuild test robot
  3 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2019-02-12 23:48 UTC (permalink / raw)
  To: Michal Hocko
  Cc: kbuild-all, Tetsuo Handa, Andrew Morton, Chris Metcalf,
	Rusty Russell, linux-mm, Guenter Roeck

[-- Attachment #1: Type: text/plain, Size: 1616 bytes --]

Hi Michal,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.0-rc4 next-20190212]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Michal-Hocko/mm-handle-lru_add_drain_all-for-UP-properly/20190213-063735
config: sh-titan_defconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.2.0 make.cross ARCH=sh 

All errors (new ones prefixed by >>):

   sh4-linux-gnu-ld: mm/fadvise.o: in function `vfs_fadvise':
   fadvise.c:(.text+0x270): undefined reference to `lru_add_drain_all'
   sh4-linux-gnu-ld: mm/compaction.o: in function `sysctl_compaction_handler':
>> compaction.c:(.text+0x1d8c): undefined reference to `lru_add_drain_all'
   sh4-linux-gnu-ld: mm/migrate.o: in function `migrate_prep':
>> migrate.c:(.text+0xee4): undefined reference to `lru_add_drain_all'
   sh4-linux-gnu-ld: mm/memfd.o: in function `memfd_fcntl':
>> memfd.c:(.text+0x2fc): undefined reference to `lru_add_drain_all'
   sh4-linux-gnu-ld: fs/block_dev.o: in function `invalidate_bdev':
>> block_dev.c:(.text+0x274): undefined reference to `lru_add_drain_all'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 16431 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/swap.c: workaround for_each_cpu() bug on UP kernel.
  2019-02-12 21:06         ` Andrew Morton
@ 2019-02-13 12:43           ` Michal Hocko
  2019-02-13 21:37             ` Andrew Morton
  0 siblings, 1 reply; 13+ messages in thread
From: Michal Hocko @ 2019-02-13 12:43 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Tetsuo Handa, Chris Metcalf, Rusty Russell, linux-mm, Guenter Roeck

On Tue 12-02-19 13:06:20, Andrew Morton wrote:
> On Tue, 12 Feb 2019 12:29:54 +0100 Michal Hocko <mhocko@kernel.org> wrote:
> 
> > On Tue 12-02-19 12:21:17, Michal Hocko wrote:
> > > On Tue 12-02-19 19:25:46, Tetsuo Handa wrote:
> > > > On 2019/02/12 19:11, Michal Hocko wrote:
> > > > > This patch is ugly as hell! I do agree that for_each_cpu not working on
> > > > > CONFIG_SMP=n sucks but why do we even care about lru_add_drain_all when
> > > > > there is a single cpu? Why don't we simply do
> > > > > 
> > > > > diff --git a/mm/swap.c b/mm/swap.c
> > > > > index aa483719922e..952f24b09070 100644
> > > > > --- a/mm/swap.c
> > > > > +++ b/mm/swap.c
> > > > > @@ -660,6 +660,7 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
> > > > >  
> > > > >  static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
> > > > >  
> > > > > +#ifdef CONFIG_SMP
> > > > >  /*
> > > > >   * Doesn't need any cpu hotplug locking because we do rely on per-cpu
> > > > >   * kworkers being shut down before our page_alloc_cpu_dead callback is
> > > > > @@ -702,6 +703,10 @@ void lru_add_drain_all(void)
> > > > >  
> > > > >  	mutex_unlock(&lock);
> > > > >  }
> > > > > +#else
> > > > > +#define lru_add_drain_all() lru_add_drain()
> > > > > +
> > > > > +#endif
> > > > 
> > > > If there is no need to evaluate the "if" conditions, I'm fine with this shortcut.
> > > 
> > > lru_add_drain does drain only pagevecs which have pages and so we do not
> > > really have to duplicate the check. There is also no need to defer the
> > > execution to the workqueue for a local cpu. So we are left with only the
> > > lock to prevent parallel execution but the preemption disabling acts the
> > > same purpose on UP so the approach should be equivalent from the
> > > correctness point of view.
> > 
> > The patch with the full changelog follows:
> > 
> > 
> > >From db104f132bd6e1c02ecbe65e62c12caa7e4e2e2a Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.com>
> > Date: Tue, 12 Feb 2019 12:25:28 +0100
> > Subject: [PATCH] mm: handle lru_add_drain_all for UP properly
> > 
> > Since for_each_cpu(cpu, mask) added by commit 2d3854a37e8b767a ("cpumask:
> > introduce new API, without changing anything") did not evaluate the mask
> > argument if NR_CPUS == 1 due to CONFIG_SMP=n, lru_add_drain_all() is
> > hitting WARN_ON() at __flush_work() added by commit 4d43d395fed12463
> > ("workqueue: Try to catch flush_work() without INIT_WORK().")
> > by unconditionally calling flush_work() [1].
> > 
> > Workaround this issue by using CONFIG_SMP=n specific lru_add_drain_all
> > implementation. There is no real need to defer the implementation to the
> > workqueue as the draining is going to happen on the local cpu. So alias
> > lru_add_drain_all to lru_add_drain which does all the necessary work.
> > 
> > [1] https://lkml.kernel.org/r/18a30387-6aa5-6123-e67c-57579ecc3f38@roeck-us.net
> >
> > ...
> >
> > --- a/mm/swap.c
> > +++ b/mm/swap.c
> > @@ -660,6 +660,7 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
> >  
> >  static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
> >  
> > +#ifdef CONFIG_SMP
> >  /*
> >   * Doesn't need any cpu hotplug locking because we do rely on per-cpu
> >   * kworkers being shut down before our page_alloc_cpu_dead callback is
> > @@ -702,6 +703,10 @@ void lru_add_drain_all(void)
> >  
> >  	mutex_unlock(&lock);
> >  }
> > +#else
> > +#define lru_add_drain_all() lru_add_drain()
> > +
> > +#endif
> >  
> >  /**
> >   * release_pages - batched put_page()
> 
> How can this even link?  Lots of compilation units call
> lru_add_drain_all() but the implementation just got removed.

Yeah, my bad. Should have compile tested...


From a13b4420f064abc9fe86dbb33f2fe3b508c9fac7 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Tue, 12 Feb 2019 12:25:28 +0100
Subject: [PATCH] mm: handle lru_add_drain_all for UP properly

Since for_each_cpu(cpu, mask) added by commit 2d3854a37e8b767a ("cpumask:
introduce new API, without changing anything") did not evaluate the mask
argument if NR_CPUS == 1 due to CONFIG_SMP=n, lru_add_drain_all() is
hitting WARN_ON() at __flush_work() added by commit 4d43d395fed12463
("workqueue: Try to catch flush_work() without INIT_WORK().")
by unconditionally calling flush_work() [1].

Workaround this issue by using CONFIG_SMP=n specific lru_add_drain_all
implementation. There is no real need to defer the implementation to the
workqueue as the draining is going to happen on the local cpu. So alias
lru_add_drain_all to lru_add_drain which does all the necessary work.

[1] https://lkml.kernel.org/r/18a30387-6aa5-6123-e67c-57579ecc3f38@roeck-us.net
Reported-by: Guenter Roeck <linux@roeck-us.net>
Debugged-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/swap.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/mm/swap.c b/mm/swap.c
index 4929bc1be60e..12711434a1b9 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -660,6 +660,7 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
 
 static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
 
+#ifdef CONFIG_SMP
 /*
  * Doesn't need any cpu hotplug locking because we do rely on per-cpu
  * kworkers being shut down before our page_alloc_cpu_dead callback is
@@ -702,6 +703,12 @@ void lru_add_drain_all(void)
 
 	mutex_unlock(&lock);
 }
+#else
+void lru_add_drain_all(void)
+{
+	lru_add_drain();
+}
+#endif
 
 /**
  * release_pages - batched put_page()
-- 
2.20.1

-- 
Michal Hocko
SUSE Labs


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/swap.c: workaround for_each_cpu() bug on UP kernel.
  2019-02-13 12:43           ` Michal Hocko
@ 2019-02-13 21:37             ` Andrew Morton
  0 siblings, 0 replies; 13+ messages in thread
From: Andrew Morton @ 2019-02-13 21:37 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Tetsuo Handa, Chris Metcalf, Rusty Russell, linux-mm,
	Guenter Roeck, Tejun Heo

On Wed, 13 Feb 2019 13:43:34 +0100 Michal Hocko <mhocko@kernel.org> wrote:

> Since for_each_cpu(cpu, mask) added by commit 2d3854a37e8b767a ("cpumask:
> introduce new API, without changing anything") did not evaluate the mask
> argument if NR_CPUS == 1 due to CONFIG_SMP=n, lru_add_drain_all() is
> hitting WARN_ON() at __flush_work() added by commit 4d43d395fed12463
> ("workqueue: Try to catch flush_work() without INIT_WORK().")
> by unconditionally calling flush_work() [1].
> 
> Workaround this issue by using CONFIG_SMP=n specific lru_add_drain_all
> implementation. There is no real need to defer the implementation to the
> workqueue as the draining is going to happen on the local cpu. So alias
> lru_add_drain_all to lru_add_drain which does all the necessary work.
> 

I assume that warning comes out a LOT of times under the correct
circumstances.

Tejun, I think a WARN_ON_ONCE() would be better.


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-02-13 21:37 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-07  9:53 [PATCH] mm/swap.c: workaround for_each_cpu() bug on UP kernel Tetsuo Handa
2019-02-07 14:07 ` Guenter Roeck
2019-02-07 14:18 ` William Kucharski
2019-02-12 10:11 ` Michal Hocko
2019-02-12 10:25   ` Tetsuo Handa
2019-02-12 11:21     ` Michal Hocko
2019-02-12 11:29       ` Michal Hocko
2019-02-12 11:37         ` Tetsuo Handa
2019-02-12 21:06         ` Andrew Morton
2019-02-13 12:43           ` Michal Hocko
2019-02-13 21:37             ` Andrew Morton
2019-02-12 23:19         ` [PATCH] mm: handle lru_add_drain_all for UP properly kbuild test robot
2019-02-12 23:48         ` kbuild test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.