mm/swap.c: workaround for_each_cpu() bug on UP kernel.

Commit Message

Tetsuo Handa Feb. 7, 2019, 9:53 a.m. UTC

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/[e-mail]

Reported-by: Guenter Roeck <[e-mail]>
Signed-off-by: Tetsuo Handa <[e-mail]>
---
 mm/swap.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Guenter Roeck Feb. 7, 2019, 2:07 p.m. UTC | #1

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/[e-mail]
> 
> Reported-by: Guenter Roeck <[e-mail]>
> Signed-off-by: Tetsuo Handa <[e-mail]>

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 <[e-mail]>

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);
>   }
>

William Kucharski Feb. 7, 2019, 2:18 p.m. UTC | #2

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 <[e-mail]> 

> On Feb 7, 2019, at 2:53 AM, Tetsuo Handa <[e-mail]> 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/[e-mail]
> 
> Reported-by: Guenter Roeck <[e-mail]>
> Signed-off-by: Tetsuo Handa <[e-mail]>
> ---
> 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
>

Michal Hocko Feb. 12, 2019, 10:11 a.m. UTC | #3

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/[e-mail]
> 
> Reported-by: Guenter Roeck <[e-mail]>
> Signed-off-by: Tetsuo Handa <[e-mail]>

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()

Tetsuo Handa Feb. 12, 2019, 10:25 a.m. UTC | #4

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.

Michal Hocko Feb. 12, 2019, 11:21 a.m. UTC | #5

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 Feb. 12, 2019, 11:29 a.m. UTC | #6

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 <[e-mail]>
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/[e-mail]
Reported-by: Guenter Roeck <[e-mail]>
Debugged-by: Tetsuo Handa <[e-mail]>
Signed-off-by: Michal Hocko <[e-mail]>
---
 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()

Tetsuo Handa Feb. 12, 2019, 11:37 a.m. UTC | #7

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 <[e-mail]>
> 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/[e-mail]
> Reported-by: Guenter Roeck <[e-mail]>
> Debugged-by: Tetsuo Handa <[e-mail]>
> Signed-off-by: Michal Hocko <[e-mail]>
> ---
>  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()
>

Andrew Morton Feb. 12, 2019, 9:06 p.m. UTC | #8

On Tue, 12 Feb 2019 12:29:54 +0100 Michal Hocko <[e-mail]> 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 <[e-mail]>
> 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/[e-mail]
>
> ...
>
> --- 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.

kernel test robot Feb. 12, 2019, 11:19 p.m. UTC | #9

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

kernel test robot Feb. 12, 2019, 11:48 p.m. UTC | #10

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

Michal Hocko Feb. 13, 2019, 12:43 p.m. UTC | #11

On Tue 12-02-19 13:06:20, Andrew Morton wrote:
> On Tue, 12 Feb 2019 12:29:54 +0100 Michal Hocko <[e-mail]> 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 <[e-mail]>
> > 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/[e-mail]
> >
> > ...
> >
> > --- 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 <[e-mail]>
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/[e-mail]
Reported-by: Guenter Roeck <[e-mail]>
Debugged-by: Tetsuo Handa <[e-mail]>
Signed-off-by: Michal Hocko <[e-mail]>
---
 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()

Andrew Morton Feb. 13, 2019, 9:37 p.m. UTC | #12

On Wed, 13 Feb 2019 13:43:34 +0100 Michal Hocko <[e-mail]> 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.

Patch

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);
 }