Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1576027 > unrolled thread

[PATCH] mm-page_alloc-use-static-global-work_struct-for-draining-per-cpu-pages-fix

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH]  mm-page_alloc-use-static-global-work_struct-for-draining-per-cpu-pages-fix Michal Hocko <mhocko@kernel.org> - 2017-02-07 21:30 +0100
    Re: [PATCH]  mm-page_alloc-use-static-global-work_struct-for-draining-per-cpu-pages-fix Andrew Morton <akpm@linux-foundation.org> - 2017-02-07 23:20 +0100
      Re: [PATCH]  mm-page_alloc-use-static-global-work_struct-for-draining-per-cpu-pages-fix Michal Hocko <mhocko@kernel.org> - 2017-02-08 09:20 +0100
      Re: [PATCH]  mm-page_alloc-use-static-global-work_struct-for-draining-per-cpu-pages-fix Tejun Heo <htejun@gmail.com> - 2017-02-08 19:20 +0100

#1576027 — [PATCH] mm-page_alloc-use-static-global-work_struct-for-draining-per-cpu-pages-fix

From: Michal Hocko <mhocko@suse.com>

there is no need to have both pcpu_drain and pcpu_drain_mutex visible
outside of drain_all_pages. This might just attract abuse.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
Hi Andrew, Mel,
I think this would be a good cleanup to be folded into
mm-page_alloc-use-static-global-work_struct-for-draining-per-cpu-pages.patch.

 mm/page_alloc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index b6411816787a..6c48053bcd81 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -92,10 +92,6 @@ EXPORT_PER_CPU_SYMBOL(_numa_mem_);
 int _node_numa_mem_[MAX_NUMNODES];
 #endif
 
-/* work_structs for global per-cpu drains */
-DEFINE_MUTEX(pcpu_drain_mutex);
-DEFINE_PER_CPU(struct work_struct, pcpu_drain);
-
 #ifdef CONFIG_GCC_PLUGIN_LATENT_ENTROPY
 volatile unsigned long latent_entropy __latent_entropy;
 EXPORT_SYMBOL(latent_entropy);
@@ -2364,6 +2360,8 @@ static void drain_local_pages_wq(struct work_struct *work)
  */
 void drain_all_pages(struct zone *zone)
 {
+	static DEFINE_PER_CPU(struct work_struct, pcpu_drain);
+	static DEFINE_MUTEX(pcpu_drain_mutex);
 	int cpu;
 
 	/*
-- 
2.11.0

[toc] | [next] | [standalone]

#1576112

On Wed, 8 Feb 2017 05:54:56 +0800 kbuild test robot <lkp@intel.com> wrote:

> Hi Michal,
> 
> [auto build test ERROR on mmotm/master]
> [also build test ERROR on v4.10-rc7 next-20170207]
> [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-page_alloc-use-static-global-work_struct-for-draining-per-cpu-pages-fix/20170208-050036
> base:   git://git.cmpxchg.org/linux-mmotm.git master
> config: i386-randconfig-x001-201706 (attached as .config)
> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386 
> 
> All error/warnings (new ones prefixed by >>):
> 
>    In file included from include/asm-generic/percpu.h:6:0,
>                     from arch/x86/include/asm/percpu.h:542,
>                     from arch/x86/include/asm/preempt.h:5,
>                     from include/linux/preempt.h:59,
>                     from include/linux/spinlock.h:50,
>                     from include/linux/mmzone.h:7,
>                     from include/linux/gfp.h:5,
>                     from include/linux/mm.h:9,
>                     from mm/page_alloc.c:18:
>    mm/page_alloc.c: In function 'drain_all_pages':
> >> include/linux/percpu-defs.h:91:33: error: section attribute cannot be specified for local variables
>      extern __PCPU_DUMMY_ATTRS char __pcpu_unique_##name;  \
>                                     ^

huh, yes.  The DEFINE_PER_CPU() macro is broken.

If you do

foo()
{
	static DEFINE_PER_CPU(int, bar);
}

then it won't compile, as described here.  It should.

And if you do

static DEFINE_PER_CPU(int, bar);

then you still get global symbols (__pcpu_unique_bar).

The kernel does the above thing in, umm, 466 places and afaict they're
all broken.  If two code sites ever use the same identifier, they'll
get linkage errors.

huh.  Seems hard to fix.

[toc] | [prev] | [next] | [standalone]

#1576323

On Tue 07-02-17 14:14:20, Andrew Morton wrote:
> On Wed, 8 Feb 2017 05:54:56 +0800 kbuild test robot <lkp@intel.com> wrote:
> 
> > Hi Michal,
> > 
> > [auto build test ERROR on mmotm/master]
> > [also build test ERROR on v4.10-rc7 next-20170207]
> > [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-page_alloc-use-static-global-work_struct-for-draining-per-cpu-pages-fix/20170208-050036
> > base:   git://git.cmpxchg.org/linux-mmotm.git master
> > config: i386-randconfig-x001-201706 (attached as .config)
> > compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> > reproduce:
> >         # save the attached .config to linux build tree
> >         make ARCH=i386 
> > 
> > All error/warnings (new ones prefixed by >>):
> > 
> >    In file included from include/asm-generic/percpu.h:6:0,
> >                     from arch/x86/include/asm/percpu.h:542,
> >                     from arch/x86/include/asm/preempt.h:5,
> >                     from include/linux/preempt.h:59,
> >                     from include/linux/spinlock.h:50,
> >                     from include/linux/mmzone.h:7,
> >                     from include/linux/gfp.h:5,
> >                     from include/linux/mm.h:9,
> >                     from mm/page_alloc.c:18:
> >    mm/page_alloc.c: In function 'drain_all_pages':
> > >> include/linux/percpu-defs.h:91:33: error: section attribute cannot be specified for local variables
> >      extern __PCPU_DUMMY_ATTRS char __pcpu_unique_##name;  \
> >                                     ^
> 
> huh, yes.  The DEFINE_PER_CPU() macro is broken.
> 
> If you do
> 
> foo()
> {
> 	static DEFINE_PER_CPU(int, bar);
> }
> 
> then it won't compile, as described here.  It should.
> 
> And if you do
> 
> static DEFINE_PER_CPU(int, bar);
> 
> then you still get global symbols (__pcpu_unique_bar).
> 
> The kernel does the above thing in, umm, 466 places and afaict they're
> all broken.  If two code sites ever use the same identifier, they'll
> get linkage errors.
> 
> huh.  Seems hard to fix.

Nasty! Unfortunately, I am not familiar with the static pcp code magic
to come up with a fix.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1576801

Hello, Andrew.

On Tue, Feb 07, 2017 at 02:14:20PM -0800, Andrew Morton wrote:
> >      extern __PCPU_DUMMY_ATTRS char __pcpu_unique_##name;  \
> >                                     ^
> 
> huh, yes.  The DEFINE_PER_CPU() macro is broken.

Yeah, that was the trade off I had to take with percpu vars to force
s390 and alpha to generate long references (GOT based addressing) for
percpu variables; otherwise, they generate memory deref which is too
limited to access the special percpu addresses.  It's explained in
include/linux/percpu-defs.h.

> If you do
> 
> foo()
> {
> 	static DEFINE_PER_CPU(int, bar);
> }
> 
> then it won't compile, as described here.  It should.
> 
> And if you do
> 
> static DEFINE_PER_CPU(int, bar);
> 
> then you still get global symbols (__pcpu_unique_bar).
> 
> The kernel does the above thing in, umm, 466 places and afaict they're
> all broken.  If two code sites ever use the same identifier, they'll
> get linkage errors.

So, we have CONFIG_DEBUG_FORCE_WEAK_PER_CPU to catch those cases on
archs other than s390 or alpha.

> huh.  Seems hard to fix.

This was the only way I could come up with to support alpha and s390.
All the restrictions are there to ensure that.  If we can do s390 and
alpha w/o the global weak reference, neither restriction is necessary.

Thanks.

-- 
tejun

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web