Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Yang Li]

On Wed, Mar 15, 2017 at 2:39 AM, Michal Hocko  wrote:
> On Tue 14-03-17 18:07:38, Yang Li wrote:
>> On Mon, Mar 13, 2017 at 4:58 AM, Michal Hocko  wrote:
>> > On Fri 10-03-17 17:31:56, Yang Li wrote:
>> >> On Tue, Mar 7, 2017 at 7:17 AM, Michal Hocko  wrote:
>> >> > From: Michal Hocko 
>> >> >
>> >> > We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
>> >> > vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
>> >> > per cpu lru caches. This seems more than necessary because both can run
>> >> > on a single WQ. Both do not block on locks requiring a memory allocation
>> >> > nor perform any allocations themselves. We will save one rescuer thread
>> >> > this way.
>> >> >
>> >> > On the other hand drain_all_pages() queues work on the system wq which
>> >> > doesn't have rescuer and so this depend on memory allocation (when all
>> >> > workers are stuck allocating and new ones cannot be created). This is
>> >> > not critical as there should be somebody invoking the OOM killer (e.g.
>> >> > the forking worker) and get the situation unstuck and eventually
>> >> > performs the draining. Quite annoying though. This worker should be
>> >> > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
>> >> > and vmstat.
>> >> >
>> >> > Changes since v1
>> >> > - rename vmstat_wq to mm_percpu_wq - per Mel
>> >> > - make sure we are not trying to enqueue anything while the WQ hasn't
>> >> >   been intialized yet. This shouldn't happen because the initialization
>> >> >   is done from an init code but some init section might be triggering
>> >> >   those paths indirectly so just warn and skip the draining in that case
>> >> >   per Vlastimil
>> >>
>> >> So what's the plan if this really happens?  Shall we put the
>> >> initialization of the mm_percpu_wq earlier?
>> >
>> > yes
>> >
>> >> Or if it is really harmless we can probably remove the warnings.
>> >
>> > Yeah, it is harmless but if we can move it earlier then it would be
>> > prefferable to fix this.
>> >
>> >>
>> >> I'm seeing this on arm64 with a linux-next tree:
>> > [...]
>> >> [0.279000] [] drain_all_pages+0x244/0x25c
>> >> [0.279065] [] start_isolate_page_range+0x14c/0x1f0
>> >> [0.279137] [] alloc_contig_range+0xec/0x354
>> >> [0.279203] [] cma_alloc+0x100/0x1fc
>> >> [0.279263] [] dma_alloc_from_contiguous+0x3c/0x44
>> >> [0.279336] [] atomic_pool_init+0x7c/0x208
>> >> [0.279399] [] arm64_dma_init+0x44/0x4c
>> >> [0.279461] [] do_one_initcall+0x38/0x128
>> >> [0.279525] [] kernel_init_freeable+0x1a0/0x240
>> >> [0.279596] [] kernel_init+0x10/0xfc
>> >> [0.279654] [] ret_from_fork+0x10/0x20
>> >
>> > The following should address this. I didn't get to test it yet though.
>> > ---
>> > diff --git a/include/linux/mm.h b/include/linux/mm.h
>> > index 21ee5503c702..8362dca071cb 100644
>> > --- a/include/linux/mm.h
>> > +++ b/include/linux/mm.h
>> > @@ -32,6 +32,8 @@ struct user_struct;
>> >  struct writeback_control;
>> >  struct bdi_writeback;
>> >
>> > +void init_mm_internals(void);
>> > +
>> >  #ifndef CONFIG_NEED_MULTIPLE_NODES /* Don't use mapnrs, do it 
>> > properly */
>> >  extern unsigned long max_mapnr;
>> >
>> > diff --git a/init/main.c b/init/main.c
>> > index 51aa8f336819..c72d35250e84 100644
>> > --- a/init/main.c
>> > +++ b/init/main.c
>> > @@ -1023,6 +1023,8 @@ static noinline void __init 
>> > kernel_init_freeable(void)
>> >
>> > workqueue_init();
>> >
>> > +   init_mm_internals();
>> > +
>> > do_pre_smp_initcalls();
>> > lockup_detector_init();
>> >
>> > diff --git a/mm/vmstat.c b/mm/vmstat.c
>> > index 4bbc775f9d08..d0871fc1aeca 100644
>> > --- a/mm/vmstat.c
>> > +++ b/mm/vmstat.c
>> > @@ -1762,7 +1762,7 @@ static int vmstat_cpu_dead(unsigned int cpu)
>> >
>> >  struct workqueue_struct *mm_percpu_wq;
>> >
>> > -static int __init setup_vmstat(void)
>> > +void __init init_mm_internals(void)
>> >  {
>> > int ret __maybe_unused;
>> >
>> > @@ -1792,9 +1792,7 @@ static int __init setup_vmstat(void)
>> > proc_create("vmstat", S_IRUGO, NULL, _vmstat_file_operations);
>> > proc_create("zoneinfo", S_IRUGO, NULL, 
>> > _zoneinfo_file_operations);
>> >  #endif
>> > -   return 0;
>> >  }
>> > -module_init(setup_vmstat)
>> >
>> >  #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_COMPACTION)
>>
>> I did a test on arm64.  This do fix the warnings.
>
> Thanks! Can I assume your
> Tested-by: Yang Li 

Sure.

Tested-by: Li Yang 

Regards,
Leo

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Yang Li]

On Wed, Mar 15, 2017 at 2:39 AM, Michal Hocko  wrote:
> On Tue 14-03-17 18:07:38, Yang Li wrote:
>> On Mon, Mar 13, 2017 at 4:58 AM, Michal Hocko  wrote:
>> > On Fri 10-03-17 17:31:56, Yang Li wrote:
>> >> On Tue, Mar 7, 2017 at 7:17 AM, Michal Hocko  wrote:
>> >> > From: Michal Hocko 
>> >> >
>> >> > We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
>> >> > vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
>> >> > per cpu lru caches. This seems more than necessary because both can run
>> >> > on a single WQ. Both do not block on locks requiring a memory allocation
>> >> > nor perform any allocations themselves. We will save one rescuer thread
>> >> > this way.
>> >> >
>> >> > On the other hand drain_all_pages() queues work on the system wq which
>> >> > doesn't have rescuer and so this depend on memory allocation (when all
>> >> > workers are stuck allocating and new ones cannot be created). This is
>> >> > not critical as there should be somebody invoking the OOM killer (e.g.
>> >> > the forking worker) and get the situation unstuck and eventually
>> >> > performs the draining. Quite annoying though. This worker should be
>> >> > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
>> >> > and vmstat.
>> >> >
>> >> > Changes since v1
>> >> > - rename vmstat_wq to mm_percpu_wq - per Mel
>> >> > - make sure we are not trying to enqueue anything while the WQ hasn't
>> >> >   been intialized yet. This shouldn't happen because the initialization
>> >> >   is done from an init code but some init section might be triggering
>> >> >   those paths indirectly so just warn and skip the draining in that case
>> >> >   per Vlastimil
>> >>
>> >> So what's the plan if this really happens?  Shall we put the
>> >> initialization of the mm_percpu_wq earlier?
>> >
>> > yes
>> >
>> >> Or if it is really harmless we can probably remove the warnings.
>> >
>> > Yeah, it is harmless but if we can move it earlier then it would be
>> > prefferable to fix this.
>> >
>> >>
>> >> I'm seeing this on arm64 with a linux-next tree:
>> > [...]
>> >> [0.279000] [] drain_all_pages+0x244/0x25c
>> >> [0.279065] [] start_isolate_page_range+0x14c/0x1f0
>> >> [0.279137] [] alloc_contig_range+0xec/0x354
>> >> [0.279203] [] cma_alloc+0x100/0x1fc
>> >> [0.279263] [] dma_alloc_from_contiguous+0x3c/0x44
>> >> [0.279336] [] atomic_pool_init+0x7c/0x208
>> >> [0.279399] [] arm64_dma_init+0x44/0x4c
>> >> [0.279461] [] do_one_initcall+0x38/0x128
>> >> [0.279525] [] kernel_init_freeable+0x1a0/0x240
>> >> [0.279596] [] kernel_init+0x10/0xfc
>> >> [0.279654] [] ret_from_fork+0x10/0x20
>> >
>> > The following should address this. I didn't get to test it yet though.
>> > ---
>> > diff --git a/include/linux/mm.h b/include/linux/mm.h
>> > index 21ee5503c702..8362dca071cb 100644
>> > --- a/include/linux/mm.h
>> > +++ b/include/linux/mm.h
>> > @@ -32,6 +32,8 @@ struct user_struct;
>> >  struct writeback_control;
>> >  struct bdi_writeback;
>> >
>> > +void init_mm_internals(void);
>> > +
>> >  #ifndef CONFIG_NEED_MULTIPLE_NODES /* Don't use mapnrs, do it 
>> > properly */
>> >  extern unsigned long max_mapnr;
>> >
>> > diff --git a/init/main.c b/init/main.c
>> > index 51aa8f336819..c72d35250e84 100644
>> > --- a/init/main.c
>> > +++ b/init/main.c
>> > @@ -1023,6 +1023,8 @@ static noinline void __init 
>> > kernel_init_freeable(void)
>> >
>> > workqueue_init();
>> >
>> > +   init_mm_internals();
>> > +
>> > do_pre_smp_initcalls();
>> > lockup_detector_init();
>> >
>> > diff --git a/mm/vmstat.c b/mm/vmstat.c
>> > index 4bbc775f9d08..d0871fc1aeca 100644
>> > --- a/mm/vmstat.c
>> > +++ b/mm/vmstat.c
>> > @@ -1762,7 +1762,7 @@ static int vmstat_cpu_dead(unsigned int cpu)
>> >
>> >  struct workqueue_struct *mm_percpu_wq;
>> >
>> > -static int __init setup_vmstat(void)
>> > +void __init init_mm_internals(void)
>> >  {
>> > int ret __maybe_unused;
>> >
>> > @@ -1792,9 +1792,7 @@ static int __init setup_vmstat(void)
>> > proc_create("vmstat", S_IRUGO, NULL, _vmstat_file_operations);
>> > proc_create("zoneinfo", S_IRUGO, NULL, 
>> > _zoneinfo_file_operations);
>> >  #endif
>> > -   return 0;
>> >  }
>> > -module_init(setup_vmstat)
>> >
>> >  #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_COMPACTION)
>>
>> I did a test on arm64.  This do fix the warnings.
>
> Thanks! Can I assume your
> Tested-by: Yang Li 

Sure.

Tested-by: Li Yang 

Regards,
Leo

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Michal Hocko]

On Tue 14-03-17 18:07:38, Yang Li wrote:
> On Mon, Mar 13, 2017 at 4:58 AM, Michal Hocko  wrote:
> > On Fri 10-03-17 17:31:56, Yang Li wrote:
> >> On Tue, Mar 7, 2017 at 7:17 AM, Michal Hocko  wrote:
> >> > From: Michal Hocko 
> >> >
> >> > We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> >> > vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> >> > per cpu lru caches. This seems more than necessary because both can run
> >> > on a single WQ. Both do not block on locks requiring a memory allocation
> >> > nor perform any allocations themselves. We will save one rescuer thread
> >> > this way.
> >> >
> >> > On the other hand drain_all_pages() queues work on the system wq which
> >> > doesn't have rescuer and so this depend on memory allocation (when all
> >> > workers are stuck allocating and new ones cannot be created). This is
> >> > not critical as there should be somebody invoking the OOM killer (e.g.
> >> > the forking worker) and get the situation unstuck and eventually
> >> > performs the draining. Quite annoying though. This worker should be
> >> > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> >> > and vmstat.
> >> >
> >> > Changes since v1
> >> > - rename vmstat_wq to mm_percpu_wq - per Mel
> >> > - make sure we are not trying to enqueue anything while the WQ hasn't
> >> >   been intialized yet. This shouldn't happen because the initialization
> >> >   is done from an init code but some init section might be triggering
> >> >   those paths indirectly so just warn and skip the draining in that case
> >> >   per Vlastimil
> >>
> >> So what's the plan if this really happens?  Shall we put the
> >> initialization of the mm_percpu_wq earlier?
> >
> > yes
> >
> >> Or if it is really harmless we can probably remove the warnings.
> >
> > Yeah, it is harmless but if we can move it earlier then it would be
> > prefferable to fix this.
> >
> >>
> >> I'm seeing this on arm64 with a linux-next tree:
> > [...]
> >> [0.279000] [] drain_all_pages+0x244/0x25c
> >> [0.279065] [] start_isolate_page_range+0x14c/0x1f0
> >> [0.279137] [] alloc_contig_range+0xec/0x354
> >> [0.279203] [] cma_alloc+0x100/0x1fc
> >> [0.279263] [] dma_alloc_from_contiguous+0x3c/0x44
> >> [0.279336] [] atomic_pool_init+0x7c/0x208
> >> [0.279399] [] arm64_dma_init+0x44/0x4c
> >> [0.279461] [] do_one_initcall+0x38/0x128
> >> [0.279525] [] kernel_init_freeable+0x1a0/0x240
> >> [0.279596] [] kernel_init+0x10/0xfc
> >> [0.279654] [] ret_from_fork+0x10/0x20
> >
> > The following should address this. I didn't get to test it yet though.
> > ---
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 21ee5503c702..8362dca071cb 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -32,6 +32,8 @@ struct user_struct;
> >  struct writeback_control;
> >  struct bdi_writeback;
> >
> > +void init_mm_internals(void);
> > +
> >  #ifndef CONFIG_NEED_MULTIPLE_NODES /* Don't use mapnrs, do it properly 
> > */
> >  extern unsigned long max_mapnr;
> >
> > diff --git a/init/main.c b/init/main.c
> > index 51aa8f336819..c72d35250e84 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -1023,6 +1023,8 @@ static noinline void __init kernel_init_freeable(void)
> >
> > workqueue_init();
> >
> > +   init_mm_internals();
> > +
> > do_pre_smp_initcalls();
> > lockup_detector_init();
> >
> > diff --git a/mm/vmstat.c b/mm/vmstat.c
> > index 4bbc775f9d08..d0871fc1aeca 100644
> > --- a/mm/vmstat.c
> > +++ b/mm/vmstat.c
> > @@ -1762,7 +1762,7 @@ static int vmstat_cpu_dead(unsigned int cpu)
> >
> >  struct workqueue_struct *mm_percpu_wq;
> >
> > -static int __init setup_vmstat(void)
> > +void __init init_mm_internals(void)
> >  {
> > int ret __maybe_unused;
> >
> > @@ -1792,9 +1792,7 @@ static int __init setup_vmstat(void)
> > proc_create("vmstat", S_IRUGO, NULL, _vmstat_file_operations);
> > proc_create("zoneinfo", S_IRUGO, NULL, 
> > _zoneinfo_file_operations);
> >  #endif
> > -   return 0;
> >  }
> > -module_init(setup_vmstat)
> >
> >  #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_COMPACTION)
> 
> I did a test on arm64.  This do fix the warnings.

Thanks! Can I assume your
Tested-by: Yang Li 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Michal Hocko]

On Tue 14-03-17 18:07:38, Yang Li wrote:
> On Mon, Mar 13, 2017 at 4:58 AM, Michal Hocko  wrote:
> > On Fri 10-03-17 17:31:56, Yang Li wrote:
> >> On Tue, Mar 7, 2017 at 7:17 AM, Michal Hocko  wrote:
> >> > From: Michal Hocko 
> >> >
> >> > We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> >> > vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> >> > per cpu lru caches. This seems more than necessary because both can run
> >> > on a single WQ. Both do not block on locks requiring a memory allocation
> >> > nor perform any allocations themselves. We will save one rescuer thread
> >> > this way.
> >> >
> >> > On the other hand drain_all_pages() queues work on the system wq which
> >> > doesn't have rescuer and so this depend on memory allocation (when all
> >> > workers are stuck allocating and new ones cannot be created). This is
> >> > not critical as there should be somebody invoking the OOM killer (e.g.
> >> > the forking worker) and get the situation unstuck and eventually
> >> > performs the draining. Quite annoying though. This worker should be
> >> > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> >> > and vmstat.
> >> >
> >> > Changes since v1
> >> > - rename vmstat_wq to mm_percpu_wq - per Mel
> >> > - make sure we are not trying to enqueue anything while the WQ hasn't
> >> >   been intialized yet. This shouldn't happen because the initialization
> >> >   is done from an init code but some init section might be triggering
> >> >   those paths indirectly so just warn and skip the draining in that case
> >> >   per Vlastimil
> >>
> >> So what's the plan if this really happens?  Shall we put the
> >> initialization of the mm_percpu_wq earlier?
> >
> > yes
> >
> >> Or if it is really harmless we can probably remove the warnings.
> >
> > Yeah, it is harmless but if we can move it earlier then it would be
> > prefferable to fix this.
> >
> >>
> >> I'm seeing this on arm64 with a linux-next tree:
> > [...]
> >> [0.279000] [] drain_all_pages+0x244/0x25c
> >> [0.279065] [] start_isolate_page_range+0x14c/0x1f0
> >> [0.279137] [] alloc_contig_range+0xec/0x354
> >> [0.279203] [] cma_alloc+0x100/0x1fc
> >> [0.279263] [] dma_alloc_from_contiguous+0x3c/0x44
> >> [0.279336] [] atomic_pool_init+0x7c/0x208
> >> [0.279399] [] arm64_dma_init+0x44/0x4c
> >> [0.279461] [] do_one_initcall+0x38/0x128
> >> [0.279525] [] kernel_init_freeable+0x1a0/0x240
> >> [0.279596] [] kernel_init+0x10/0xfc
> >> [0.279654] [] ret_from_fork+0x10/0x20
> >
> > The following should address this. I didn't get to test it yet though.
> > ---
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 21ee5503c702..8362dca071cb 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -32,6 +32,8 @@ struct user_struct;
> >  struct writeback_control;
> >  struct bdi_writeback;
> >
> > +void init_mm_internals(void);
> > +
> >  #ifndef CONFIG_NEED_MULTIPLE_NODES /* Don't use mapnrs, do it properly 
> > */
> >  extern unsigned long max_mapnr;
> >
> > diff --git a/init/main.c b/init/main.c
> > index 51aa8f336819..c72d35250e84 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -1023,6 +1023,8 @@ static noinline void __init kernel_init_freeable(void)
> >
> > workqueue_init();
> >
> > +   init_mm_internals();
> > +
> > do_pre_smp_initcalls();
> > lockup_detector_init();
> >
> > diff --git a/mm/vmstat.c b/mm/vmstat.c
> > index 4bbc775f9d08..d0871fc1aeca 100644
> > --- a/mm/vmstat.c
> > +++ b/mm/vmstat.c
> > @@ -1762,7 +1762,7 @@ static int vmstat_cpu_dead(unsigned int cpu)
> >
> >  struct workqueue_struct *mm_percpu_wq;
> >
> > -static int __init setup_vmstat(void)
> > +void __init init_mm_internals(void)
> >  {
> > int ret __maybe_unused;
> >
> > @@ -1792,9 +1792,7 @@ static int __init setup_vmstat(void)
> > proc_create("vmstat", S_IRUGO, NULL, _vmstat_file_operations);
> > proc_create("zoneinfo", S_IRUGO, NULL, 
> > _zoneinfo_file_operations);
> >  #endif
> > -   return 0;
> >  }
> > -module_init(setup_vmstat)
> >
> >  #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_COMPACTION)
> 
> I did a test on arm64.  This do fix the warnings.

Thanks! Can I assume your
Tested-by: Yang Li 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Yang Li]

On Mon, Mar 13, 2017 at 4:58 AM, Michal Hocko  wrote:
> On Fri 10-03-17 17:31:56, Yang Li wrote:
>> On Tue, Mar 7, 2017 at 7:17 AM, Michal Hocko  wrote:
>> > From: Michal Hocko 
>> >
>> > We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
>> > vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
>> > per cpu lru caches. This seems more than necessary because both can run
>> > on a single WQ. Both do not block on locks requiring a memory allocation
>> > nor perform any allocations themselves. We will save one rescuer thread
>> > this way.
>> >
>> > On the other hand drain_all_pages() queues work on the system wq which
>> > doesn't have rescuer and so this depend on memory allocation (when all
>> > workers are stuck allocating and new ones cannot be created). This is
>> > not critical as there should be somebody invoking the OOM killer (e.g.
>> > the forking worker) and get the situation unstuck and eventually
>> > performs the draining. Quite annoying though. This worker should be
>> > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
>> > and vmstat.
>> >
>> > Changes since v1
>> > - rename vmstat_wq to mm_percpu_wq - per Mel
>> > - make sure we are not trying to enqueue anything while the WQ hasn't
>> >   been intialized yet. This shouldn't happen because the initialization
>> >   is done from an init code but some init section might be triggering
>> >   those paths indirectly so just warn and skip the draining in that case
>> >   per Vlastimil
>>
>> So what's the plan if this really happens?  Shall we put the
>> initialization of the mm_percpu_wq earlier?
>
> yes
>
>> Or if it is really harmless we can probably remove the warnings.
>
> Yeah, it is harmless but if we can move it earlier then it would be
> prefferable to fix this.
>
>>
>> I'm seeing this on arm64 with a linux-next tree:
> [...]
>> [0.279000] [] drain_all_pages+0x244/0x25c
>> [0.279065] [] start_isolate_page_range+0x14c/0x1f0
>> [0.279137] [] alloc_contig_range+0xec/0x354
>> [0.279203] [] cma_alloc+0x100/0x1fc
>> [0.279263] [] dma_alloc_from_contiguous+0x3c/0x44
>> [0.279336] [] atomic_pool_init+0x7c/0x208
>> [0.279399] [] arm64_dma_init+0x44/0x4c
>> [0.279461] [] do_one_initcall+0x38/0x128
>> [0.279525] [] kernel_init_freeable+0x1a0/0x240
>> [0.279596] [] kernel_init+0x10/0xfc
>> [0.279654] [] ret_from_fork+0x10/0x20
>
> The following should address this. I didn't get to test it yet though.
> ---
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 21ee5503c702..8362dca071cb 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -32,6 +32,8 @@ struct user_struct;
>  struct writeback_control;
>  struct bdi_writeback;
>
> +void init_mm_internals(void);
> +
>  #ifndef CONFIG_NEED_MULTIPLE_NODES /* Don't use mapnrs, do it properly */
>  extern unsigned long max_mapnr;
>
> diff --git a/init/main.c b/init/main.c
> index 51aa8f336819..c72d35250e84 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1023,6 +1023,8 @@ static noinline void __init kernel_init_freeable(void)
>
> workqueue_init();
>
> +   init_mm_internals();
> +
> do_pre_smp_initcalls();
> lockup_detector_init();
>
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 4bbc775f9d08..d0871fc1aeca 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1762,7 +1762,7 @@ static int vmstat_cpu_dead(unsigned int cpu)
>
>  struct workqueue_struct *mm_percpu_wq;
>
> -static int __init setup_vmstat(void)
> +void __init init_mm_internals(void)
>  {
> int ret __maybe_unused;
>
> @@ -1792,9 +1792,7 @@ static int __init setup_vmstat(void)
> proc_create("vmstat", S_IRUGO, NULL, _vmstat_file_operations);
> proc_create("zoneinfo", S_IRUGO, NULL, 
> _zoneinfo_file_operations);
>  #endif
> -   return 0;
>  }
> -module_init(setup_vmstat)
>
>  #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_COMPACTION)

I did a test on arm64.  This do fix the warnings.

Regards,
Leo

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Yang Li]

On Mon, Mar 13, 2017 at 4:58 AM, Michal Hocko  wrote:
> On Fri 10-03-17 17:31:56, Yang Li wrote:
>> On Tue, Mar 7, 2017 at 7:17 AM, Michal Hocko  wrote:
>> > From: Michal Hocko 
>> >
>> > We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
>> > vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
>> > per cpu lru caches. This seems more than necessary because both can run
>> > on a single WQ. Both do not block on locks requiring a memory allocation
>> > nor perform any allocations themselves. We will save one rescuer thread
>> > this way.
>> >
>> > On the other hand drain_all_pages() queues work on the system wq which
>> > doesn't have rescuer and so this depend on memory allocation (when all
>> > workers are stuck allocating and new ones cannot be created). This is
>> > not critical as there should be somebody invoking the OOM killer (e.g.
>> > the forking worker) and get the situation unstuck and eventually
>> > performs the draining. Quite annoying though. This worker should be
>> > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
>> > and vmstat.
>> >
>> > Changes since v1
>> > - rename vmstat_wq to mm_percpu_wq - per Mel
>> > - make sure we are not trying to enqueue anything while the WQ hasn't
>> >   been intialized yet. This shouldn't happen because the initialization
>> >   is done from an init code but some init section might be triggering
>> >   those paths indirectly so just warn and skip the draining in that case
>> >   per Vlastimil
>>
>> So what's the plan if this really happens?  Shall we put the
>> initialization of the mm_percpu_wq earlier?
>
> yes
>
>> Or if it is really harmless we can probably remove the warnings.
>
> Yeah, it is harmless but if we can move it earlier then it would be
> prefferable to fix this.
>
>>
>> I'm seeing this on arm64 with a linux-next tree:
> [...]
>> [0.279000] [] drain_all_pages+0x244/0x25c
>> [0.279065] [] start_isolate_page_range+0x14c/0x1f0
>> [0.279137] [] alloc_contig_range+0xec/0x354
>> [0.279203] [] cma_alloc+0x100/0x1fc
>> [0.279263] [] dma_alloc_from_contiguous+0x3c/0x44
>> [0.279336] [] atomic_pool_init+0x7c/0x208
>> [0.279399] [] arm64_dma_init+0x44/0x4c
>> [0.279461] [] do_one_initcall+0x38/0x128
>> [0.279525] [] kernel_init_freeable+0x1a0/0x240
>> [0.279596] [] kernel_init+0x10/0xfc
>> [0.279654] [] ret_from_fork+0x10/0x20
>
> The following should address this. I didn't get to test it yet though.
> ---
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 21ee5503c702..8362dca071cb 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -32,6 +32,8 @@ struct user_struct;
>  struct writeback_control;
>  struct bdi_writeback;
>
> +void init_mm_internals(void);
> +
>  #ifndef CONFIG_NEED_MULTIPLE_NODES /* Don't use mapnrs, do it properly */
>  extern unsigned long max_mapnr;
>
> diff --git a/init/main.c b/init/main.c
> index 51aa8f336819..c72d35250e84 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1023,6 +1023,8 @@ static noinline void __init kernel_init_freeable(void)
>
> workqueue_init();
>
> +   init_mm_internals();
> +
> do_pre_smp_initcalls();
> lockup_detector_init();
>
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 4bbc775f9d08..d0871fc1aeca 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1762,7 +1762,7 @@ static int vmstat_cpu_dead(unsigned int cpu)
>
>  struct workqueue_struct *mm_percpu_wq;
>
> -static int __init setup_vmstat(void)
> +void __init init_mm_internals(void)
>  {
> int ret __maybe_unused;
>
> @@ -1792,9 +1792,7 @@ static int __init setup_vmstat(void)
> proc_create("vmstat", S_IRUGO, NULL, _vmstat_file_operations);
> proc_create("zoneinfo", S_IRUGO, NULL, 
> _zoneinfo_file_operations);
>  #endif
> -   return 0;
>  }
> -module_init(setup_vmstat)
>
>  #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_COMPACTION)

I did a test on arm64.  This do fix the warnings.

Regards,
Leo

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Michal Hocko]

On Fri 10-03-17 17:31:56, Yang Li wrote:
> On Tue, Mar 7, 2017 at 7:17 AM, Michal Hocko  wrote:
> > From: Michal Hocko 
> >
> > We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> > vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> > per cpu lru caches. This seems more than necessary because both can run
> > on a single WQ. Both do not block on locks requiring a memory allocation
> > nor perform any allocations themselves. We will save one rescuer thread
> > this way.
> >
> > On the other hand drain_all_pages() queues work on the system wq which
> > doesn't have rescuer and so this depend on memory allocation (when all
> > workers are stuck allocating and new ones cannot be created). This is
> > not critical as there should be somebody invoking the OOM killer (e.g.
> > the forking worker) and get the situation unstuck and eventually
> > performs the draining. Quite annoying though. This worker should be
> > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> > and vmstat.
> >
> > Changes since v1
> > - rename vmstat_wq to mm_percpu_wq - per Mel
> > - make sure we are not trying to enqueue anything while the WQ hasn't
> >   been intialized yet. This shouldn't happen because the initialization
> >   is done from an init code but some init section might be triggering
> >   those paths indirectly so just warn and skip the draining in that case
> >   per Vlastimil
> 
> So what's the plan if this really happens?  Shall we put the
> initialization of the mm_percpu_wq earlier?

yes

> Or if it is really harmless we can probably remove the warnings.

Yeah, it is harmless but if we can move it earlier then it would be
prefferable to fix this.

> 
> I'm seeing this on arm64 with a linux-next tree:
[...]
> [0.279000] [] drain_all_pages+0x244/0x25c
> [0.279065] [] start_isolate_page_range+0x14c/0x1f0
> [0.279137] [] alloc_contig_range+0xec/0x354
> [0.279203] [] cma_alloc+0x100/0x1fc
> [0.279263] [] dma_alloc_from_contiguous+0x3c/0x44
> [0.279336] [] atomic_pool_init+0x7c/0x208
> [0.279399] [] arm64_dma_init+0x44/0x4c
> [0.279461] [] do_one_initcall+0x38/0x128
> [0.279525] [] kernel_init_freeable+0x1a0/0x240
> [0.279596] [] kernel_init+0x10/0xfc
> [0.279654] [] ret_from_fork+0x10/0x20

The following should address this. I didn't get to test it yet though.
---
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 21ee5503c702..8362dca071cb 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -32,6 +32,8 @@ struct user_struct;
 struct writeback_control;
 struct bdi_writeback;
 
+void init_mm_internals(void);
+
 #ifndef CONFIG_NEED_MULTIPLE_NODES /* Don't use mapnrs, do it properly */
 extern unsigned long max_mapnr;
 
diff --git a/init/main.c b/init/main.c
index 51aa8f336819..c72d35250e84 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1023,6 +1023,8 @@ static noinline void __init kernel_init_freeable(void)
 
workqueue_init();
 
+   init_mm_internals();
+
do_pre_smp_initcalls();
lockup_detector_init();
 
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 4bbc775f9d08..d0871fc1aeca 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1762,7 +1762,7 @@ static int vmstat_cpu_dead(unsigned int cpu)
 
 struct workqueue_struct *mm_percpu_wq;
 
-static int __init setup_vmstat(void)
+void __init init_mm_internals(void)
 {
int ret __maybe_unused;
 
@@ -1792,9 +1792,7 @@ static int __init setup_vmstat(void)
proc_create("vmstat", S_IRUGO, NULL, _vmstat_file_operations);
proc_create("zoneinfo", S_IRUGO, NULL, _zoneinfo_file_operations);
 #endif
-   return 0;
 }
-module_init(setup_vmstat)
 
 #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_COMPACTION)
 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Michal Hocko]

On Fri 10-03-17 17:31:56, Yang Li wrote:
> On Tue, Mar 7, 2017 at 7:17 AM, Michal Hocko  wrote:
> > From: Michal Hocko 
> >
> > We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> > vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> > per cpu lru caches. This seems more than necessary because both can run
> > on a single WQ. Both do not block on locks requiring a memory allocation
> > nor perform any allocations themselves. We will save one rescuer thread
> > this way.
> >
> > On the other hand drain_all_pages() queues work on the system wq which
> > doesn't have rescuer and so this depend on memory allocation (when all
> > workers are stuck allocating and new ones cannot be created). This is
> > not critical as there should be somebody invoking the OOM killer (e.g.
> > the forking worker) and get the situation unstuck and eventually
> > performs the draining. Quite annoying though. This worker should be
> > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> > and vmstat.
> >
> > Changes since v1
> > - rename vmstat_wq to mm_percpu_wq - per Mel
> > - make sure we are not trying to enqueue anything while the WQ hasn't
> >   been intialized yet. This shouldn't happen because the initialization
> >   is done from an init code but some init section might be triggering
> >   those paths indirectly so just warn and skip the draining in that case
> >   per Vlastimil
> 
> So what's the plan if this really happens?  Shall we put the
> initialization of the mm_percpu_wq earlier?

yes

> Or if it is really harmless we can probably remove the warnings.

Yeah, it is harmless but if we can move it earlier then it would be
prefferable to fix this.

> 
> I'm seeing this on arm64 with a linux-next tree:
[...]
> [0.279000] [] drain_all_pages+0x244/0x25c
> [0.279065] [] start_isolate_page_range+0x14c/0x1f0
> [0.279137] [] alloc_contig_range+0xec/0x354
> [0.279203] [] cma_alloc+0x100/0x1fc
> [0.279263] [] dma_alloc_from_contiguous+0x3c/0x44
> [0.279336] [] atomic_pool_init+0x7c/0x208
> [0.279399] [] arm64_dma_init+0x44/0x4c
> [0.279461] [] do_one_initcall+0x38/0x128
> [0.279525] [] kernel_init_freeable+0x1a0/0x240
> [0.279596] [] kernel_init+0x10/0xfc
> [0.279654] [] ret_from_fork+0x10/0x20

The following should address this. I didn't get to test it yet though.
---
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 21ee5503c702..8362dca071cb 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -32,6 +32,8 @@ struct user_struct;
 struct writeback_control;
 struct bdi_writeback;
 
+void init_mm_internals(void);
+
 #ifndef CONFIG_NEED_MULTIPLE_NODES /* Don't use mapnrs, do it properly */
 extern unsigned long max_mapnr;
 
diff --git a/init/main.c b/init/main.c
index 51aa8f336819..c72d35250e84 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1023,6 +1023,8 @@ static noinline void __init kernel_init_freeable(void)
 
workqueue_init();
 
+   init_mm_internals();
+
do_pre_smp_initcalls();
lockup_detector_init();
 
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 4bbc775f9d08..d0871fc1aeca 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1762,7 +1762,7 @@ static int vmstat_cpu_dead(unsigned int cpu)
 
 struct workqueue_struct *mm_percpu_wq;
 
-static int __init setup_vmstat(void)
+void __init init_mm_internals(void)
 {
int ret __maybe_unused;
 
@@ -1792,9 +1792,7 @@ static int __init setup_vmstat(void)
proc_create("vmstat", S_IRUGO, NULL, _vmstat_file_operations);
proc_create("zoneinfo", S_IRUGO, NULL, _zoneinfo_file_operations);
 #endif
-   return 0;
 }
-module_init(setup_vmstat)
 
 #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_COMPACTION)
 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Yang Li]

On Tue, Mar 7, 2017 at 7:17 AM, Michal Hocko  wrote:
> From: Michal Hocko 
>
> We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> per cpu lru caches. This seems more than necessary because both can run
> on a single WQ. Both do not block on locks requiring a memory allocation
> nor perform any allocations themselves. We will save one rescuer thread
> this way.
>
> On the other hand drain_all_pages() queues work on the system wq which
> doesn't have rescuer and so this depend on memory allocation (when all
> workers are stuck allocating and new ones cannot be created). This is
> not critical as there should be somebody invoking the OOM killer (e.g.
> the forking worker) and get the situation unstuck and eventually
> performs the draining. Quite annoying though. This worker should be
> using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> and vmstat.
>
> Changes since v1
> - rename vmstat_wq to mm_percpu_wq - per Mel
> - make sure we are not trying to enqueue anything while the WQ hasn't
>   been intialized yet. This shouldn't happen because the initialization
>   is done from an init code but some init section might be triggering
>   those paths indirectly so just warn and skip the draining in that case
>   per Vlastimil

So what's the plan if this really happens?  Shall we put the
initialization of the mm_percpu_wq earlier?  Or if it is really
harmless we can probably remove the warnings.

I'm seeing this on arm64 with a linux-next tree:

[0.276449] WARNING: CPU: 2 PID: 1 at mm/page_alloc.c:2423
drain_all_pages+0x244/0x25c
[0.276537] Modules linked in:

[0.276594] CPU: 2 PID: 1 Comm: swapper/0 Not tainted
4.11.0-rc1-next-20170310-00027-g64dfbc5 #127
[0.276693] Hardware name: Freescale Layerscape 2088A RDB Board (DT)
[0.276764] task: ffc07c4a6d00 task.stack: ffc07c4a8000
[0.276831] PC is at drain_all_pages+0x244/0x25c
[0.276886] LR is at start_isolate_page_range+0x14c/0x1f0
[0.276946] pc : [] lr : []
pstate: 8045
[0.277028] sp : ffc07c4abb10
[0.277066] x29: ffc07c4abb10 x28: 000ff000
[0.277128] x27: 0004 x26: 
[0.277190] x25: 000ff400 x24: 0040
[0.277252] x23: ff8008ab x22: ff8008c46000
[0.277313] x21: ff8008c2b700 x20: ff8008c2b200
[0.277374] x19: ff8008c2b200 x18: ff80088b5068
[0.277436] x17: 000e x16: 0007
[0.277497] x15: 0001 x14: 
[0.277559] x13: 0068 x12: 0001
[0.277620] x11: ff8008c2b6e0 x10: 0001
[0.277682] x9 : ff8008c2b6e0 x8 : 00cd
[0.277743] x7 : ffc07c4abb70 x6 : ff8008c2b868
[0.277804] x5 : ff8008b84b4e x4 : 
[0.277866] x3 : 0c00 x2 : 0311
[0.277927] x1 : 0001 x0 : ff8008bc8000

[0.278008] ---[ end trace 905d0cf24acf61bb ]---
[0.278060] Call trace:
[0.278089] Exception stack(0xffc07c4ab940 to 0xffc07c4aba70)
[0.278162] b940: ff8008c2b200 0080
ffc07c4abb10 ff80081636bc
[0.278249] b960: ff8008ba8dd0 ff8008ba8c80
ff8008c2c580 ff8008bc5f08
[0.278336] b980: ffbebffdafff ffbebffdb000
ff8008c797d8 ffbebffdafff
[0.278423] b9a0: ffbebffdb000 ffc07c667000
ffc07c4ab9c0 ff800817ff7c
[0.278510] b9c0: ffc07c4aba40 ff8008180618
03d0 0f60
[0.278597] b9e0: ff8008bc8000 0001
0311 0c00
[0.278684] ba00:  ff8008b84b4e
ff8008c2b868 ffc07c4abb70
[0.278771] ba20: 00cd ff8008c2b6e0
0001 ff8008c2b6e0
[0.278858] ba40: 0001 0068
 0001
[0.278945] ba60: 0007 000e
[0.279000] [] drain_all_pages+0x244/0x25c
[0.279065] [] start_isolate_page_range+0x14c/0x1f0
[0.279137] [] alloc_contig_range+0xec/0x354
[0.279203] [] cma_alloc+0x100/0x1fc
[0.279263] [] dma_alloc_from_contiguous+0x3c/0x44
[0.279336] [] atomic_pool_init+0x7c/0x208
[0.279399] [] arm64_dma_init+0x44/0x4c
[0.279461] [] do_one_initcall+0x38/0x128
[0.279525] [] kernel_init_freeable+0x1a0/0x240
[0.279596] [] kernel_init+0x10/0xfc
[0.279654] [] ret_from_fork+0x10/0x20


Regards,
Leo

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Yang Li]

On Tue, Mar 7, 2017 at 7:17 AM, Michal Hocko  wrote:
> From: Michal Hocko 
>
> We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> per cpu lru caches. This seems more than necessary because both can run
> on a single WQ. Both do not block on locks requiring a memory allocation
> nor perform any allocations themselves. We will save one rescuer thread
> this way.
>
> On the other hand drain_all_pages() queues work on the system wq which
> doesn't have rescuer and so this depend on memory allocation (when all
> workers are stuck allocating and new ones cannot be created). This is
> not critical as there should be somebody invoking the OOM killer (e.g.
> the forking worker) and get the situation unstuck and eventually
> performs the draining. Quite annoying though. This worker should be
> using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> and vmstat.
>
> Changes since v1
> - rename vmstat_wq to mm_percpu_wq - per Mel
> - make sure we are not trying to enqueue anything while the WQ hasn't
>   been intialized yet. This shouldn't happen because the initialization
>   is done from an init code but some init section might be triggering
>   those paths indirectly so just warn and skip the draining in that case
>   per Vlastimil

So what's the plan if this really happens?  Shall we put the
initialization of the mm_percpu_wq earlier?  Or if it is really
harmless we can probably remove the warnings.

I'm seeing this on arm64 with a linux-next tree:

[0.276449] WARNING: CPU: 2 PID: 1 at mm/page_alloc.c:2423
drain_all_pages+0x244/0x25c
[0.276537] Modules linked in:

[0.276594] CPU: 2 PID: 1 Comm: swapper/0 Not tainted
4.11.0-rc1-next-20170310-00027-g64dfbc5 #127
[0.276693] Hardware name: Freescale Layerscape 2088A RDB Board (DT)
[0.276764] task: ffc07c4a6d00 task.stack: ffc07c4a8000
[0.276831] PC is at drain_all_pages+0x244/0x25c
[0.276886] LR is at start_isolate_page_range+0x14c/0x1f0
[0.276946] pc : [] lr : []
pstate: 8045
[0.277028] sp : ffc07c4abb10
[0.277066] x29: ffc07c4abb10 x28: 000ff000
[0.277128] x27: 0004 x26: 
[0.277190] x25: 000ff400 x24: 0040
[0.277252] x23: ff8008ab x22: ff8008c46000
[0.277313] x21: ff8008c2b700 x20: ff8008c2b200
[0.277374] x19: ff8008c2b200 x18: ff80088b5068
[0.277436] x17: 000e x16: 0007
[0.277497] x15: 0001 x14: 
[0.277559] x13: 0068 x12: 0001
[0.277620] x11: ff8008c2b6e0 x10: 0001
[0.277682] x9 : ff8008c2b6e0 x8 : 00cd
[0.277743] x7 : ffc07c4abb70 x6 : ff8008c2b868
[0.277804] x5 : ff8008b84b4e x4 : 
[0.277866] x3 : 0c00 x2 : 0311
[0.277927] x1 : 0001 x0 : ff8008bc8000

[0.278008] ---[ end trace 905d0cf24acf61bb ]---
[0.278060] Call trace:
[0.278089] Exception stack(0xffc07c4ab940 to 0xffc07c4aba70)
[0.278162] b940: ff8008c2b200 0080
ffc07c4abb10 ff80081636bc
[0.278249] b960: ff8008ba8dd0 ff8008ba8c80
ff8008c2c580 ff8008bc5f08
[0.278336] b980: ffbebffdafff ffbebffdb000
ff8008c797d8 ffbebffdafff
[0.278423] b9a0: ffbebffdb000 ffc07c667000
ffc07c4ab9c0 ff800817ff7c
[0.278510] b9c0: ffc07c4aba40 ff8008180618
03d0 0f60
[0.278597] b9e0: ff8008bc8000 0001
0311 0c00
[0.278684] ba00:  ff8008b84b4e
ff8008c2b868 ffc07c4abb70
[0.278771] ba20: 00cd ff8008c2b6e0
0001 ff8008c2b6e0
[0.278858] ba40: 0001 0068
 0001
[0.278945] ba60: 0007 000e
[0.279000] [] drain_all_pages+0x244/0x25c
[0.279065] [] start_isolate_page_range+0x14c/0x1f0
[0.279137] [] alloc_contig_range+0xec/0x354
[0.279203] [] cma_alloc+0x100/0x1fc
[0.279263] [] dma_alloc_from_contiguous+0x3c/0x44
[0.279336] [] atomic_pool_init+0x7c/0x208
[0.279399] [] arm64_dma_init+0x44/0x4c
[0.279461] [] do_one_initcall+0x38/0x128
[0.279525] [] kernel_init_freeable+0x1a0/0x240
[0.279596] [] kernel_init+0x10/0xfc
[0.279654] [] ret_from_fork+0x10/0x20


Regards,
Leo

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Michal Hocko]

On Thu 09-03-17 14:26:02, Mel Gorman wrote:
> On Tue, Mar 07, 2017 at 02:17:51PM +0100, Michal Hocko wrote:
> > From: Michal Hocko 
> > 
> > We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> > vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> > per cpu lru caches. This seems more than necessary because both can run
> > on a single WQ. Both do not block on locks requiring a memory allocation
> > nor perform any allocations themselves. We will save one rescuer thread
> > this way.
> > 
> > On the other hand drain_all_pages() queues work on the system wq which
> > doesn't have rescuer and so this depend on memory allocation (when all
> > workers are stuck allocating and new ones cannot be created). This is
> > not critical as there should be somebody invoking the OOM killer (e.g.
> > the forking worker) and get the situation unstuck and eventually
> > performs the draining. Quite annoying though. This worker should be
> > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> > and vmstat.
> > 
> > Changes since v1
> > - rename vmstat_wq to mm_percpu_wq - per Mel
> > - make sure we are not trying to enqueue anything while the WQ hasn't
> >   been intialized yet. This shouldn't happen because the initialization
> >   is done from an init code but some init section might be triggering
> >   those paths indirectly so just warn and skip the draining in that case
> >   per Vlastimil
> > - do not propagate error from setup_vmstat to keep the previous behavior
> >   per Mel
> > 
> > Suggested-by: Tetsuo Handa 
> > Signed-off-by: Michal Hocko 
> 
> Acked-by: Mel Gorman 

Thanks!

> > +struct workqueue_struct *mm_percpu_wq;
> > +
> >  static int __init setup_vmstat(void)
> >  {
> > -#ifdef CONFIG_SMP
> > -   int ret;
> > +   int ret __maybe_unused;
> >  
> > +   mm_percpu_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 
> > 0);
> > +
> > +#ifdef CONFIG_SMP
> > ret = cpuhp_setup_state_nocalls(CPUHP_MM_VMSTAT_DEAD, "mm/vmstat:dead",
> > NULL, vmstat_cpu_dead);
> > if (ret < 0)
> 
> Should the workqueue also have been renamed to mm_percpu_wq?

Of course. Andrew, could you fold the following in
---
diff --git a/mm/vmstat.c b/mm/vmstat.c
index ff9c49c47f32..4bbc775f9d08 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1766,7 +1766,7 @@ static int __init setup_vmstat(void)
 {
int ret __maybe_unused;
 
-   mm_percpu_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 
0);
+   mm_percpu_wq = alloc_workqueue("mm_percpu_wq", 
WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
 
 #ifdef CONFIG_SMP
ret = cpuhp_setup_state_nocalls(CPUHP_MM_VMSTAT_DEAD, "mm/vmstat:dead",
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Michal Hocko]

On Thu 09-03-17 14:26:02, Mel Gorman wrote:
> On Tue, Mar 07, 2017 at 02:17:51PM +0100, Michal Hocko wrote:
> > From: Michal Hocko 
> > 
> > We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> > vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> > per cpu lru caches. This seems more than necessary because both can run
> > on a single WQ. Both do not block on locks requiring a memory allocation
> > nor perform any allocations themselves. We will save one rescuer thread
> > this way.
> > 
> > On the other hand drain_all_pages() queues work on the system wq which
> > doesn't have rescuer and so this depend on memory allocation (when all
> > workers are stuck allocating and new ones cannot be created). This is
> > not critical as there should be somebody invoking the OOM killer (e.g.
> > the forking worker) and get the situation unstuck and eventually
> > performs the draining. Quite annoying though. This worker should be
> > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> > and vmstat.
> > 
> > Changes since v1
> > - rename vmstat_wq to mm_percpu_wq - per Mel
> > - make sure we are not trying to enqueue anything while the WQ hasn't
> >   been intialized yet. This shouldn't happen because the initialization
> >   is done from an init code but some init section might be triggering
> >   those paths indirectly so just warn and skip the draining in that case
> >   per Vlastimil
> > - do not propagate error from setup_vmstat to keep the previous behavior
> >   per Mel
> > 
> > Suggested-by: Tetsuo Handa 
> > Signed-off-by: Michal Hocko 
> 
> Acked-by: Mel Gorman 

Thanks!

> > +struct workqueue_struct *mm_percpu_wq;
> > +
> >  static int __init setup_vmstat(void)
> >  {
> > -#ifdef CONFIG_SMP
> > -   int ret;
> > +   int ret __maybe_unused;
> >  
> > +   mm_percpu_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 
> > 0);
> > +
> > +#ifdef CONFIG_SMP
> > ret = cpuhp_setup_state_nocalls(CPUHP_MM_VMSTAT_DEAD, "mm/vmstat:dead",
> > NULL, vmstat_cpu_dead);
> > if (ret < 0)
> 
> Should the workqueue also have been renamed to mm_percpu_wq?

Of course. Andrew, could you fold the following in
---
diff --git a/mm/vmstat.c b/mm/vmstat.c
index ff9c49c47f32..4bbc775f9d08 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1766,7 +1766,7 @@ static int __init setup_vmstat(void)
 {
int ret __maybe_unused;
 
-   mm_percpu_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 
0);
+   mm_percpu_wq = alloc_workqueue("mm_percpu_wq", 
WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
 
 #ifdef CONFIG_SMP
ret = cpuhp_setup_state_nocalls(CPUHP_MM_VMSTAT_DEAD, "mm/vmstat:dead",
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Mel Gorman]

On Tue, Mar 07, 2017 at 02:17:51PM +0100, Michal Hocko wrote:
> From: Michal Hocko 
> 
> We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> per cpu lru caches. This seems more than necessary because both can run
> on a single WQ. Both do not block on locks requiring a memory allocation
> nor perform any allocations themselves. We will save one rescuer thread
> this way.
> 
> On the other hand drain_all_pages() queues work on the system wq which
> doesn't have rescuer and so this depend on memory allocation (when all
> workers are stuck allocating and new ones cannot be created). This is
> not critical as there should be somebody invoking the OOM killer (e.g.
> the forking worker) and get the situation unstuck and eventually
> performs the draining. Quite annoying though. This worker should be
> using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> and vmstat.
> 
> Changes since v1
> - rename vmstat_wq to mm_percpu_wq - per Mel
> - make sure we are not trying to enqueue anything while the WQ hasn't
>   been intialized yet. This shouldn't happen because the initialization
>   is done from an init code but some init section might be triggering
>   those paths indirectly so just warn and skip the draining in that case
>   per Vlastimil
> - do not propagate error from setup_vmstat to keep the previous behavior
>   per Mel
> 
> Suggested-by: Tetsuo Handa 
> Signed-off-by: Michal Hocko 

Acked-by: Mel Gorman 
> +struct workqueue_struct *mm_percpu_wq;
> +
>  static int __init setup_vmstat(void)
>  {
> -#ifdef CONFIG_SMP
> - int ret;
> + int ret __maybe_unused;
>  
> + mm_percpu_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 
> 0);
> +
> +#ifdef CONFIG_SMP
>   ret = cpuhp_setup_state_nocalls(CPUHP_MM_VMSTAT_DEAD, "mm/vmstat:dead",
>   NULL, vmstat_cpu_dead);
>   if (ret < 0)

Should the workqueue also have been renamed to mm_percpu_wq?

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Mel Gorman]

On Tue, Mar 07, 2017 at 02:17:51PM +0100, Michal Hocko wrote:
> From: Michal Hocko 
> 
> We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> per cpu lru caches. This seems more than necessary because both can run
> on a single WQ. Both do not block on locks requiring a memory allocation
> nor perform any allocations themselves. We will save one rescuer thread
> this way.
> 
> On the other hand drain_all_pages() queues work on the system wq which
> doesn't have rescuer and so this depend on memory allocation (when all
> workers are stuck allocating and new ones cannot be created). This is
> not critical as there should be somebody invoking the OOM killer (e.g.
> the forking worker) and get the situation unstuck and eventually
> performs the draining. Quite annoying though. This worker should be
> using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> and vmstat.
> 
> Changes since v1
> - rename vmstat_wq to mm_percpu_wq - per Mel
> - make sure we are not trying to enqueue anything while the WQ hasn't
>   been intialized yet. This shouldn't happen because the initialization
>   is done from an init code but some init section might be triggering
>   those paths indirectly so just warn and skip the draining in that case
>   per Vlastimil
> - do not propagate error from setup_vmstat to keep the previous behavior
>   per Mel
> 
> Suggested-by: Tetsuo Handa 
> Signed-off-by: Michal Hocko 

Acked-by: Mel Gorman 
> +struct workqueue_struct *mm_percpu_wq;
> +
>  static int __init setup_vmstat(void)
>  {
> -#ifdef CONFIG_SMP
> - int ret;
> + int ret __maybe_unused;
>  
> + mm_percpu_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 
> 0);
> +
> +#ifdef CONFIG_SMP
>   ret = cpuhp_setup_state_nocalls(CPUHP_MM_VMSTAT_DEAD, "mm/vmstat:dead",
>   NULL, vmstat_cpu_dead);
>   if (ret < 0)

Should the workqueue also have been renamed to mm_percpu_wq?

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Michal Hocko]

On Wed 08-03-17 20:50:45, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Tue 07-03-17 22:50:48, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > From: Michal Hocko 
> > > > 
> > > > We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> > > > vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> > > > per cpu lru caches. This seems more than necessary because both can run
> > > > on a single WQ. Both do not block on locks requiring a memory allocation
> > > > nor perform any allocations themselves. We will save one rescuer thread
> > > > this way.
> > > > 
> > > > On the other hand drain_all_pages() queues work on the system wq which
> > > > doesn't have rescuer and so this depend on memory allocation (when all
> > > > workers are stuck allocating and new ones cannot be created). This is
> > > > not critical as there should be somebody invoking the OOM killer (e.g.
> > > > the forking worker) and get the situation unstuck and eventually
> > > > performs the draining. Quite annoying though. This worker should be
> > > > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> > > > and vmstat.
> > > 
> > > Is "there should be somebody invoking the OOM killer" really true?
> > 
> > in most cases there should be... I didn't say there will be...
> 
> It can become critical if there is nobody who can invoke the OOM killer.

I am not aware of any "real life" bug report that would make this
critical and worth backporting to stable trees and what not.

> > > According to 
> > > http://lkml.kernel.org/r/201703031948.chj81278.vohsfffoolj...@i-love.sakura.ne.jp
> > > 
> > >   kthreadd (PID = 2) is trying to allocate "struct task_struct" requested 
> > > by
> > >   workqueue managers (PID = 19, 157, 10499) but is blocked on memory 
> > > allocation.
> > > 
> > > __GFP_FS allocations could get stuck waiting for drain_all_pages() ?
> > > Also, order > 0 allocation request by the forking worker could get stuck
> > > at too_many_isolated() in mm/compaction.c ?
> > 
> > There might be some extreme cases which however do not change the
> > justification of this patch. I didn't see such cases reported anywhere
> > - other than in your stress testing where we do not really know what is
> > going on yet - and so I didn't mention them and nor I have marked the
> > patch for stable.
> 
> As shown in my stress testing, warn_alloc() is not powerful enough when
> something we did not imagine happens.

yes, your stress testing is hitting many corner cases and it is good to
deal with them long term but that doesn't mean normal workloads should
be too stressed about backporting this patch because it is not very
likely they would ever hit a problem here.
 
> > I am wondering what is the point of this feedback actually? Do you
> > see anything wrong in the patch or is this about the wording of the
> > changelog? If it is the later is your concern serious enough to warrant
> > the rewording/reposting?
> 
> I don't see anything wrong in the patch. I just thought

OK
 
>   This is not critical as there should be somebody invoking the OOM killer
>   (e.g. the forking worker) and get the situation unstuck and eventually
>   performs the draining. Quite annoying though.
> 
> part can be dropped because there is no guarantee that something we do not
> imagine won't happen.

I do not consider "something we do not imagine won't happen" as critical
enough. And the changelog tries to be explicit about the expectations
and argues why people do not have to be scared and automatically
backport it because it is urgent.

> After applying this patch, we might be able to replace
> 
> if (unlikely(!mutex_trylock(_drain_mutex))) {
> if (!zone)
> return;
> mutex_lock(_drain_mutex);
> }
> 
> with
> 
> if (mutex_lock_killable(_drain_mutex))
>   return;
> 
> because forward progress will be guaranteed by this patch and
> we can favor pcpu_drain_mutex owner to use other CPU's time for
> flushing queued works when many other allocating threads are
> almost busy looping.

But we do not really want to make this synchronous, I believe. Not with
a proper justification numbers.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Michal Hocko]

On Wed 08-03-17 20:50:45, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Tue 07-03-17 22:50:48, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > From: Michal Hocko 
> > > > 
> > > > We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> > > > vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> > > > per cpu lru caches. This seems more than necessary because both can run
> > > > on a single WQ. Both do not block on locks requiring a memory allocation
> > > > nor perform any allocations themselves. We will save one rescuer thread
> > > > this way.
> > > > 
> > > > On the other hand drain_all_pages() queues work on the system wq which
> > > > doesn't have rescuer and so this depend on memory allocation (when all
> > > > workers are stuck allocating and new ones cannot be created). This is
> > > > not critical as there should be somebody invoking the OOM killer (e.g.
> > > > the forking worker) and get the situation unstuck and eventually
> > > > performs the draining. Quite annoying though. This worker should be
> > > > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> > > > and vmstat.
> > > 
> > > Is "there should be somebody invoking the OOM killer" really true?
> > 
> > in most cases there should be... I didn't say there will be...
> 
> It can become critical if there is nobody who can invoke the OOM killer.

I am not aware of any "real life" bug report that would make this
critical and worth backporting to stable trees and what not.

> > > According to 
> > > http://lkml.kernel.org/r/201703031948.chj81278.vohsfffoolj...@i-love.sakura.ne.jp
> > > 
> > >   kthreadd (PID = 2) is trying to allocate "struct task_struct" requested 
> > > by
> > >   workqueue managers (PID = 19, 157, 10499) but is blocked on memory 
> > > allocation.
> > > 
> > > __GFP_FS allocations could get stuck waiting for drain_all_pages() ?
> > > Also, order > 0 allocation request by the forking worker could get stuck
> > > at too_many_isolated() in mm/compaction.c ?
> > 
> > There might be some extreme cases which however do not change the
> > justification of this patch. I didn't see such cases reported anywhere
> > - other than in your stress testing where we do not really know what is
> > going on yet - and so I didn't mention them and nor I have marked the
> > patch for stable.
> 
> As shown in my stress testing, warn_alloc() is not powerful enough when
> something we did not imagine happens.

yes, your stress testing is hitting many corner cases and it is good to
deal with them long term but that doesn't mean normal workloads should
be too stressed about backporting this patch because it is not very
likely they would ever hit a problem here.
 
> > I am wondering what is the point of this feedback actually? Do you
> > see anything wrong in the patch or is this about the wording of the
> > changelog? If it is the later is your concern serious enough to warrant
> > the rewording/reposting?
> 
> I don't see anything wrong in the patch. I just thought

OK
 
>   This is not critical as there should be somebody invoking the OOM killer
>   (e.g. the forking worker) and get the situation unstuck and eventually
>   performs the draining. Quite annoying though.
> 
> part can be dropped because there is no guarantee that something we do not
> imagine won't happen.

I do not consider "something we do not imagine won't happen" as critical
enough. And the changelog tries to be explicit about the expectations
and argues why people do not have to be scared and automatically
backport it because it is urgent.

> After applying this patch, we might be able to replace
> 
> if (unlikely(!mutex_trylock(_drain_mutex))) {
> if (!zone)
> return;
> mutex_lock(_drain_mutex);
> }
> 
> with
> 
> if (mutex_lock_killable(_drain_mutex))
>   return;
> 
> because forward progress will be guaranteed by this patch and
> we can favor pcpu_drain_mutex owner to use other CPU's time for
> flushing queued works when many other allocating threads are
> almost busy looping.

But we do not really want to make this synchronous, I believe. Not with
a proper justification numbers.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Tetsuo Handa]

Michal Hocko wrote:
> On Tue 07-03-17 22:50:48, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > From: Michal Hocko 
> > > 
> > > We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> > > vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> > > per cpu lru caches. This seems more than necessary because both can run
> > > on a single WQ. Both do not block on locks requiring a memory allocation
> > > nor perform any allocations themselves. We will save one rescuer thread
> > > this way.
> > > 
> > > On the other hand drain_all_pages() queues work on the system wq which
> > > doesn't have rescuer and so this depend on memory allocation (when all
> > > workers are stuck allocating and new ones cannot be created). This is
> > > not critical as there should be somebody invoking the OOM killer (e.g.
> > > the forking worker) and get the situation unstuck and eventually
> > > performs the draining. Quite annoying though. This worker should be
> > > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> > > and vmstat.
> > 
> > Is "there should be somebody invoking the OOM killer" really true?
> 
> in most cases there should be... I didn't say there will be...

It can become critical if there is nobody who can invoke the OOM killer.

> 
> > According to 
> > http://lkml.kernel.org/r/201703031948.chj81278.vohsfffoolj...@i-love.sakura.ne.jp
> > 
> >   kthreadd (PID = 2) is trying to allocate "struct task_struct" requested by
> >   workqueue managers (PID = 19, 157, 10499) but is blocked on memory 
> > allocation.
> > 
> > __GFP_FS allocations could get stuck waiting for drain_all_pages() ?
> > Also, order > 0 allocation request by the forking worker could get stuck
> > at too_many_isolated() in mm/compaction.c ?
> 
> There might be some extreme cases which however do not change the
> justification of this patch. I didn't see such cases reported anywhere
> - other than in your stress testing where we do not really know what is
> going on yet - and so I didn't mention them and nor I have marked the
> patch for stable.

As shown in my stress testing, warn_alloc() is not powerful enough when
something we did not imagine happens.

> 
> I am wondering what is the point of this feedback actually? Do you
> see anything wrong in the patch or is this about the wording of the
> changelog? If it is the later is your concern serious enough to warrant
> the rewording/reposting?

I don't see anything wrong in the patch. I just thought

  This is not critical as there should be somebody invoking the OOM killer
  (e.g. the forking worker) and get the situation unstuck and eventually
  performs the draining. Quite annoying though.

part can be dropped because there is no guarantee that something we do not
imagine won't happen.

> -- 
> Michal Hocko
> SUSE Labs
> 

After applying this patch, we might be able to replace

if (unlikely(!mutex_trylock(_drain_mutex))) {
if (!zone)
return;
mutex_lock(_drain_mutex);
}

with

if (mutex_lock_killable(_drain_mutex))
return;

because forward progress will be guaranteed by this patch and
we can favor pcpu_drain_mutex owner to use other CPU's time for
flushing queued works when many other allocating threads are
almost busy looping.

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Tetsuo Handa]

Michal Hocko wrote:
> On Tue 07-03-17 22:50:48, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > From: Michal Hocko 
> > > 
> > > We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> > > vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> > > per cpu lru caches. This seems more than necessary because both can run
> > > on a single WQ. Both do not block on locks requiring a memory allocation
> > > nor perform any allocations themselves. We will save one rescuer thread
> > > this way.
> > > 
> > > On the other hand drain_all_pages() queues work on the system wq which
> > > doesn't have rescuer and so this depend on memory allocation (when all
> > > workers are stuck allocating and new ones cannot be created). This is
> > > not critical as there should be somebody invoking the OOM killer (e.g.
> > > the forking worker) and get the situation unstuck and eventually
> > > performs the draining. Quite annoying though. This worker should be
> > > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> > > and vmstat.
> > 
> > Is "there should be somebody invoking the OOM killer" really true?
> 
> in most cases there should be... I didn't say there will be...

It can become critical if there is nobody who can invoke the OOM killer.

> 
> > According to 
> > http://lkml.kernel.org/r/201703031948.chj81278.vohsfffoolj...@i-love.sakura.ne.jp
> > 
> >   kthreadd (PID = 2) is trying to allocate "struct task_struct" requested by
> >   workqueue managers (PID = 19, 157, 10499) but is blocked on memory 
> > allocation.
> > 
> > __GFP_FS allocations could get stuck waiting for drain_all_pages() ?
> > Also, order > 0 allocation request by the forking worker could get stuck
> > at too_many_isolated() in mm/compaction.c ?
> 
> There might be some extreme cases which however do not change the
> justification of this patch. I didn't see such cases reported anywhere
> - other than in your stress testing where we do not really know what is
> going on yet - and so I didn't mention them and nor I have marked the
> patch for stable.

As shown in my stress testing, warn_alloc() is not powerful enough when
something we did not imagine happens.

> 
> I am wondering what is the point of this feedback actually? Do you
> see anything wrong in the patch or is this about the wording of the
> changelog? If it is the later is your concern serious enough to warrant
> the rewording/reposting?

I don't see anything wrong in the patch. I just thought

  This is not critical as there should be somebody invoking the OOM killer
  (e.g. the forking worker) and get the situation unstuck and eventually
  performs the draining. Quite annoying though.

part can be dropped because there is no guarantee that something we do not
imagine won't happen.

> -- 
> Michal Hocko
> SUSE Labs
> 

After applying this patch, we might be able to replace

if (unlikely(!mutex_trylock(_drain_mutex))) {
if (!zone)
return;
mutex_lock(_drain_mutex);
}

with

if (mutex_lock_killable(_drain_mutex))
return;

because forward progress will be guaranteed by this patch and
we can favor pcpu_drain_mutex owner to use other CPU's time for
flushing queued works when many other allocating threads are
almost busy looping.

[PATCH] mm: move pcp and lru-pcp drainging into single wq

[Michal Hocko]

From: Michal Hocko 

We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
per cpu lru caches. This seems more than necessary because both can run
on a single WQ. Both do not block on locks requiring a memory allocation
nor perform any allocations themselves. We will save one rescuer thread
this way.

On the other hand drain_all_pages() queues work on the system wq which
doesn't have rescuer and so this depend on memory allocation (when all
workers are stuck allocating and new ones cannot be created). This is
not critical as there should be somebody invoking the OOM killer (e.g.
the forking worker) and get the situation unstuck and eventually
performs the draining. Quite annoying though. This worker should be
using WQ_RECLAIM as well. We can reuse the same one as for lru draining
and vmstat.

Changes since v1
- rename vmstat_wq to mm_percpu_wq - per Mel
- make sure we are not trying to enqueue anything while the WQ hasn't
  been intialized yet. This shouldn't happen because the initialization
  is done from an init code but some init section might be triggering
  those paths indirectly so just warn and skip the draining in that case
  per Vlastimil
- do not propagate error from setup_vmstat to keep the previous behavior
  per Mel

Suggested-by: Tetsuo Handa 
Signed-off-by: Michal Hocko 
---

Hi,
this has been previous posted [1] as an RFC. There was no fundamental
opposition and some minor comments are addressed in this patch I
believe.

To remind the original motivation, Tetsuo has noted that drain_all_pages
doesn't use WQ_RECLAIM [1] and asked whether we can move the worker to
the vmstat_wq which is WQ_RECLAIM. I think the deadlock he has described
shouldn't happen but it would be really better to have the rescuer. I
also think that we do not really need 2 or more workqueues and also pull
lru draining in.

[1] http://lkml.kernel.org/r/20170207210908.530-1-mho...@kernel.org

 mm/internal.h   |  7 +++
 mm/page_alloc.c |  9 -
 mm/swap.c   | 27 ---
 mm/vmstat.c | 14 --
 4 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index 823a7a89099b..04d08ef91224 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -486,6 +486,13 @@ unsigned long reclaim_clean_pages_from_list(struct zone 
*zone,
 enum ttu_flags;
 struct tlbflush_unmap_batch;
 
+
+/*
+ * only for MM internal work items which do not depend on
+ * any allocations or locks which might depend on allocations
+ */
+extern struct workqueue_struct *mm_percpu_wq;
+
 #ifdef CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
 void try_to_unmap_flush(void);
 void try_to_unmap_flush_dirty(void);
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 1c72dd91c82e..1aa5729c8f98 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2362,6 +2362,13 @@ void drain_all_pages(struct zone *zone)
 */
static cpumask_t cpus_with_pcps;
 
+   /*
+* Make sure nobody triggers this path before mm_percpu_wq is fully
+* initialized.
+*/
+   if (WARN_ON_ONCE(!mm_percpu_wq))
+   return;
+
/* Workqueues cannot recurse */
if (current->flags & PF_WQ_WORKER)
return;
@@ -2411,7 +2418,7 @@ void drain_all_pages(struct zone *zone)
for_each_cpu(cpu, _with_pcps) {
struct work_struct *work = per_cpu_ptr(_drain, cpu);
INIT_WORK(work, drain_local_pages_wq);
-   schedule_work_on(cpu, work);
+   queue_work_on(cpu, mm_percpu_wq, work);
}
for_each_cpu(cpu, _with_pcps)
flush_work(per_cpu_ptr(_drain, cpu));
diff --git a/mm/swap.c b/mm/swap.c
index ac98eb443a03..361bdb1575ab 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -677,30 +677,19 @@ static void lru_add_drain_per_cpu(struct work_struct 
*dummy)
 
 static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
 
-/*
- * lru_add_drain_wq is used to do lru_add_drain_all() from a WQ_MEM_RECLAIM
- * workqueue, aiding in getting memory freed.
- */
-static struct workqueue_struct *lru_add_drain_wq;
-
-static int __init lru_init(void)
-{
-   lru_add_drain_wq = alloc_workqueue("lru-add-drain", WQ_MEM_RECLAIM, 0);
-
-   if (WARN(!lru_add_drain_wq,
-   "Failed to create workqueue lru_add_drain_wq"))
-   return -ENOMEM;
-
-   return 0;
-}
-early_initcall(lru_init);
-
 void lru_add_drain_all(void)
 {
static DEFINE_MUTEX(lock);
static struct cpumask has_work;
int cpu;
 
+   /*
+* Make sure nobody triggers this path before mm_percpu_wq is fully
+* initialized.
+*/
+   if (WARN_ON(!mm_percpu_wq))
+   return;
+
mutex_lock();
get_online_cpus();
cpumask_clear(_work);
@@ -714,7 +703,7 @@ void lru_add_drain_all(void)
   

[PATCH] mm: move pcp and lru-pcp drainging into single wq

[Michal Hocko]

From: Michal Hocko 

We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
per cpu lru caches. This seems more than necessary because both can run
on a single WQ. Both do not block on locks requiring a memory allocation
nor perform any allocations themselves. We will save one rescuer thread
this way.

On the other hand drain_all_pages() queues work on the system wq which
doesn't have rescuer and so this depend on memory allocation (when all
workers are stuck allocating and new ones cannot be created). This is
not critical as there should be somebody invoking the OOM killer (e.g.
the forking worker) and get the situation unstuck and eventually
performs the draining. Quite annoying though. This worker should be
using WQ_RECLAIM as well. We can reuse the same one as for lru draining
and vmstat.

Changes since v1
- rename vmstat_wq to mm_percpu_wq - per Mel
- make sure we are not trying to enqueue anything while the WQ hasn't
  been intialized yet. This shouldn't happen because the initialization
  is done from an init code but some init section might be triggering
  those paths indirectly so just warn and skip the draining in that case
  per Vlastimil
- do not propagate error from setup_vmstat to keep the previous behavior
  per Mel

Suggested-by: Tetsuo Handa 
Signed-off-by: Michal Hocko 
---

Hi,
this has been previous posted [1] as an RFC. There was no fundamental
opposition and some minor comments are addressed in this patch I
believe.

To remind the original motivation, Tetsuo has noted that drain_all_pages
doesn't use WQ_RECLAIM [1] and asked whether we can move the worker to
the vmstat_wq which is WQ_RECLAIM. I think the deadlock he has described
shouldn't happen but it would be really better to have the rescuer. I
also think that we do not really need 2 or more workqueues and also pull
lru draining in.

[1] http://lkml.kernel.org/r/20170207210908.530-1-mho...@kernel.org

 mm/internal.h   |  7 +++
 mm/page_alloc.c |  9 -
 mm/swap.c   | 27 ---
 mm/vmstat.c | 14 --
 4 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index 823a7a89099b..04d08ef91224 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -486,6 +486,13 @@ unsigned long reclaim_clean_pages_from_list(struct zone 
*zone,
 enum ttu_flags;
 struct tlbflush_unmap_batch;
 
+
+/*
+ * only for MM internal work items which do not depend on
+ * any allocations or locks which might depend on allocations
+ */
+extern struct workqueue_struct *mm_percpu_wq;
+
 #ifdef CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
 void try_to_unmap_flush(void);
 void try_to_unmap_flush_dirty(void);
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 1c72dd91c82e..1aa5729c8f98 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2362,6 +2362,13 @@ void drain_all_pages(struct zone *zone)
 */
static cpumask_t cpus_with_pcps;
 
+   /*
+* Make sure nobody triggers this path before mm_percpu_wq is fully
+* initialized.
+*/
+   if (WARN_ON_ONCE(!mm_percpu_wq))
+   return;
+
/* Workqueues cannot recurse */
if (current->flags & PF_WQ_WORKER)
return;
@@ -2411,7 +2418,7 @@ void drain_all_pages(struct zone *zone)
for_each_cpu(cpu, _with_pcps) {
struct work_struct *work = per_cpu_ptr(_drain, cpu);
INIT_WORK(work, drain_local_pages_wq);
-   schedule_work_on(cpu, work);
+   queue_work_on(cpu, mm_percpu_wq, work);
}
for_each_cpu(cpu, _with_pcps)
flush_work(per_cpu_ptr(_drain, cpu));
diff --git a/mm/swap.c b/mm/swap.c
index ac98eb443a03..361bdb1575ab 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -677,30 +677,19 @@ static void lru_add_drain_per_cpu(struct work_struct 
*dummy)
 
 static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
 
-/*
- * lru_add_drain_wq is used to do lru_add_drain_all() from a WQ_MEM_RECLAIM
- * workqueue, aiding in getting memory freed.
- */
-static struct workqueue_struct *lru_add_drain_wq;
-
-static int __init lru_init(void)
-{
-   lru_add_drain_wq = alloc_workqueue("lru-add-drain", WQ_MEM_RECLAIM, 0);
-
-   if (WARN(!lru_add_drain_wq,
-   "Failed to create workqueue lru_add_drain_wq"))
-   return -ENOMEM;
-
-   return 0;
-}
-early_initcall(lru_init);
-
 void lru_add_drain_all(void)
 {
static DEFINE_MUTEX(lock);
static struct cpumask has_work;
int cpu;
 
+   /*
+* Make sure nobody triggers this path before mm_percpu_wq is fully
+* initialized.
+*/
+   if (WARN_ON(!mm_percpu_wq))
+   return;
+
mutex_lock();
get_online_cpus();
cpumask_clear(_work);
@@ -714,7 +703,7 @@ void lru_add_drain_all(void)
pagevec_count(_cpu(lru_lazyfree_pvecs, cpu)) ||
 

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Tetsuo Handa]

Michal Hocko wrote:
> From: Michal Hocko 
> 
> We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> per cpu lru caches. This seems more than necessary because both can run
> on a single WQ. Both do not block on locks requiring a memory allocation
> nor perform any allocations themselves. We will save one rescuer thread
> this way.
> 
> On the other hand drain_all_pages() queues work on the system wq which
> doesn't have rescuer and so this depend on memory allocation (when all
> workers are stuck allocating and new ones cannot be created). This is
> not critical as there should be somebody invoking the OOM killer (e.g.
> the forking worker) and get the situation unstuck and eventually
> performs the draining. Quite annoying though. This worker should be
> using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> and vmstat.

Is "there should be somebody invoking the OOM killer" really true?
According to 
http://lkml.kernel.org/r/201703031948.chj81278.vohsfffoolj...@i-love.sakura.ne.jp

  kthreadd (PID = 2) is trying to allocate "struct task_struct" requested by
  workqueue managers (PID = 19, 157, 10499) but is blocked on memory allocation.

__GFP_FS allocations could get stuck waiting for drain_all_pages() ?
Also, order > 0 allocation request by the forking worker could get stuck
at too_many_isolated() in mm/compaction.c ?

> 
> Changes since v1
> - rename vmstat_wq to mm_percpu_wq - per Mel
> - make sure we are not trying to enqueue anything while the WQ hasn't
>   been intialized yet. This shouldn't happen because the initialization
>   is done from an init code but some init section might be triggering
>   those paths indirectly so just warn and skip the draining in that case
>   per Vlastimil
> - do not propagate error from setup_vmstat to keep the previous behavior
>   per Mel
> 
> Suggested-by: Tetsuo Handa 
> Signed-off-by: Michal Hocko 
> ---
> 
> Hi,
> this has been previous posted [1] as an RFC. There was no fundamental
> opposition and some minor comments are addressed in this patch I
> believe.
> 
> To remind the original motivation, Tetsuo has noted that drain_all_pages
> doesn't use WQ_RECLAIM [1] and asked whether we can move the worker to
> the vmstat_wq which is WQ_RECLAIM. I think the deadlock he has described
> shouldn't happen but it would be really better to have the rescuer. I
> also think that we do not really need 2 or more workqueues and also pull
> lru draining in.
> 
> [1] http://lkml.kernel.org/r/20170207210908.530-1-mho...@kernel.org
> 
>  mm/internal.h   |  7 +++
>  mm/page_alloc.c |  9 -
>  mm/swap.c   | 27 ---
>  mm/vmstat.c | 14 --
>  4 files changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index 823a7a89099b..04d08ef91224 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -486,6 +486,13 @@ unsigned long reclaim_clean_pages_from_list(struct zone 
> *zone,
>  enum ttu_flags;
>  struct tlbflush_unmap_batch;
>  
> +
> +/*
> + * only for MM internal work items which do not depend on
> + * any allocations or locks which might depend on allocations
> + */
> +extern struct workqueue_struct *mm_percpu_wq;
> +
>  #ifdef CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
>  void try_to_unmap_flush(void);
>  void try_to_unmap_flush_dirty(void);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 1c72dd91c82e..1aa5729c8f98 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2362,6 +2362,13 @@ void drain_all_pages(struct zone *zone)
>*/
>   static cpumask_t cpus_with_pcps;
>  
> + /*
> +  * Make sure nobody triggers this path before mm_percpu_wq is fully
> +  * initialized.
> +  */
> + if (WARN_ON_ONCE(!mm_percpu_wq))
> + return;
> +
>   /* Workqueues cannot recurse */
>   if (current->flags & PF_WQ_WORKER)
>   return;
> @@ -2411,7 +2418,7 @@ void drain_all_pages(struct zone *zone)
>   for_each_cpu(cpu, _with_pcps) {
>   struct work_struct *work = per_cpu_ptr(_drain, cpu);
>   INIT_WORK(work, drain_local_pages_wq);
> - schedule_work_on(cpu, work);
> + queue_work_on(cpu, mm_percpu_wq, work);
>   }
>   for_each_cpu(cpu, _with_pcps)
>   flush_work(per_cpu_ptr(_drain, cpu));
> diff --git a/mm/swap.c b/mm/swap.c
> index ac98eb443a03..361bdb1575ab 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -677,30 +677,19 @@ static void lru_add_drain_per_cpu(struct work_struct 
> *dummy)
>  
>  static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
>  
> -/*
> - * lru_add_drain_wq is used to do lru_add_drain_all() from a WQ_MEM_RECLAIM
> - * workqueue, aiding in getting memory freed.
> - */
> -static struct workqueue_struct *lru_add_drain_wq;
> -
> -static int __init 

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Tetsuo Handa]

Michal Hocko wrote:
> From: Michal Hocko 
> 
> We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> per cpu lru caches. This seems more than necessary because both can run
> on a single WQ. Both do not block on locks requiring a memory allocation
> nor perform any allocations themselves. We will save one rescuer thread
> this way.
> 
> On the other hand drain_all_pages() queues work on the system wq which
> doesn't have rescuer and so this depend on memory allocation (when all
> workers are stuck allocating and new ones cannot be created). This is
> not critical as there should be somebody invoking the OOM killer (e.g.
> the forking worker) and get the situation unstuck and eventually
> performs the draining. Quite annoying though. This worker should be
> using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> and vmstat.

Is "there should be somebody invoking the OOM killer" really true?
According to 
http://lkml.kernel.org/r/201703031948.chj81278.vohsfffoolj...@i-love.sakura.ne.jp

  kthreadd (PID = 2) is trying to allocate "struct task_struct" requested by
  workqueue managers (PID = 19, 157, 10499) but is blocked on memory allocation.

__GFP_FS allocations could get stuck waiting for drain_all_pages() ?
Also, order > 0 allocation request by the forking worker could get stuck
at too_many_isolated() in mm/compaction.c ?

> 
> Changes since v1
> - rename vmstat_wq to mm_percpu_wq - per Mel
> - make sure we are not trying to enqueue anything while the WQ hasn't
>   been intialized yet. This shouldn't happen because the initialization
>   is done from an init code but some init section might be triggering
>   those paths indirectly so just warn and skip the draining in that case
>   per Vlastimil
> - do not propagate error from setup_vmstat to keep the previous behavior
>   per Mel
> 
> Suggested-by: Tetsuo Handa 
> Signed-off-by: Michal Hocko 
> ---
> 
> Hi,
> this has been previous posted [1] as an RFC. There was no fundamental
> opposition and some minor comments are addressed in this patch I
> believe.
> 
> To remind the original motivation, Tetsuo has noted that drain_all_pages
> doesn't use WQ_RECLAIM [1] and asked whether we can move the worker to
> the vmstat_wq which is WQ_RECLAIM. I think the deadlock he has described
> shouldn't happen but it would be really better to have the rescuer. I
> also think that we do not really need 2 or more workqueues and also pull
> lru draining in.
> 
> [1] http://lkml.kernel.org/r/20170207210908.530-1-mho...@kernel.org
> 
>  mm/internal.h   |  7 +++
>  mm/page_alloc.c |  9 -
>  mm/swap.c   | 27 ---
>  mm/vmstat.c | 14 --
>  4 files changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index 823a7a89099b..04d08ef91224 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -486,6 +486,13 @@ unsigned long reclaim_clean_pages_from_list(struct zone 
> *zone,
>  enum ttu_flags;
>  struct tlbflush_unmap_batch;
>  
> +
> +/*
> + * only for MM internal work items which do not depend on
> + * any allocations or locks which might depend on allocations
> + */
> +extern struct workqueue_struct *mm_percpu_wq;
> +
>  #ifdef CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
>  void try_to_unmap_flush(void);
>  void try_to_unmap_flush_dirty(void);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 1c72dd91c82e..1aa5729c8f98 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2362,6 +2362,13 @@ void drain_all_pages(struct zone *zone)
>*/
>   static cpumask_t cpus_with_pcps;
>  
> + /*
> +  * Make sure nobody triggers this path before mm_percpu_wq is fully
> +  * initialized.
> +  */
> + if (WARN_ON_ONCE(!mm_percpu_wq))
> + return;
> +
>   /* Workqueues cannot recurse */
>   if (current->flags & PF_WQ_WORKER)
>   return;
> @@ -2411,7 +2418,7 @@ void drain_all_pages(struct zone *zone)
>   for_each_cpu(cpu, _with_pcps) {
>   struct work_struct *work = per_cpu_ptr(_drain, cpu);
>   INIT_WORK(work, drain_local_pages_wq);
> - schedule_work_on(cpu, work);
> + queue_work_on(cpu, mm_percpu_wq, work);
>   }
>   for_each_cpu(cpu, _with_pcps)
>   flush_work(per_cpu_ptr(_drain, cpu));
> diff --git a/mm/swap.c b/mm/swap.c
> index ac98eb443a03..361bdb1575ab 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -677,30 +677,19 @@ static void lru_add_drain_per_cpu(struct work_struct 
> *dummy)
>  
>  static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
>  
> -/*
> - * lru_add_drain_wq is used to do lru_add_drain_all() from a WQ_MEM_RECLAIM
> - * workqueue, aiding in getting memory freed.
> - */
> -static struct workqueue_struct *lru_add_drain_wq;
> -
> -static int __init lru_init(void)
> -{
> - lru_add_drain_wq = alloc_workqueue("lru-add-drain", 

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Michal Hocko]

On Tue 07-03-17 22:50:48, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > From: Michal Hocko 
> > 
> > We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> > vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> > per cpu lru caches. This seems more than necessary because both can run
> > on a single WQ. Both do not block on locks requiring a memory allocation
> > nor perform any allocations themselves. We will save one rescuer thread
> > this way.
> > 
> > On the other hand drain_all_pages() queues work on the system wq which
> > doesn't have rescuer and so this depend on memory allocation (when all
> > workers are stuck allocating and new ones cannot be created). This is
> > not critical as there should be somebody invoking the OOM killer (e.g.
> > the forking worker) and get the situation unstuck and eventually
> > performs the draining. Quite annoying though. This worker should be
> > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> > and vmstat.
> 
> Is "there should be somebody invoking the OOM killer" really true?

in most cases there should be... I didn't say there will be...

> According to 
> http://lkml.kernel.org/r/201703031948.chj81278.vohsfffoolj...@i-love.sakura.ne.jp
> 
>   kthreadd (PID = 2) is trying to allocate "struct task_struct" requested by
>   workqueue managers (PID = 19, 157, 10499) but is blocked on memory 
> allocation.
> 
> __GFP_FS allocations could get stuck waiting for drain_all_pages() ?
> Also, order > 0 allocation request by the forking worker could get stuck
> at too_many_isolated() in mm/compaction.c ?

There might be some extreme cases which however do not change the
justification of this patch. I didn't see such cases reported anywhere
- other than in your stress testing where we do not really know what is
going on yet - and so I didn't mention them and nor I have marked the
patch for stable.

I am wondering what is the point of this feedback actually? Do you
see anything wrong in the patch or is this about the wording of the
changelog? If it is the later is your concern serious enough to warrant
the rewording/reposting?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Michal Hocko]

On Tue 07-03-17 22:50:48, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > From: Michal Hocko 
> > 
> > We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> > vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> > per cpu lru caches. This seems more than necessary because both can run
> > on a single WQ. Both do not block on locks requiring a memory allocation
> > nor perform any allocations themselves. We will save one rescuer thread
> > this way.
> > 
> > On the other hand drain_all_pages() queues work on the system wq which
> > doesn't have rescuer and so this depend on memory allocation (when all
> > workers are stuck allocating and new ones cannot be created). This is
> > not critical as there should be somebody invoking the OOM killer (e.g.
> > the forking worker) and get the situation unstuck and eventually
> > performs the draining. Quite annoying though. This worker should be
> > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> > and vmstat.
> 
> Is "there should be somebody invoking the OOM killer" really true?

in most cases there should be... I didn't say there will be...

> According to 
> http://lkml.kernel.org/r/201703031948.chj81278.vohsfffoolj...@i-love.sakura.ne.jp
> 
>   kthreadd (PID = 2) is trying to allocate "struct task_struct" requested by
>   workqueue managers (PID = 19, 157, 10499) but is blocked on memory 
> allocation.
> 
> __GFP_FS allocations could get stuck waiting for drain_all_pages() ?
> Also, order > 0 allocation request by the forking worker could get stuck
> at too_many_isolated() in mm/compaction.c ?

There might be some extreme cases which however do not change the
justification of this patch. I didn't see such cases reported anywhere
- other than in your stress testing where we do not really know what is
going on yet - and so I didn't mention them and nor I have marked the
patch for stable.

I am wondering what is the point of this feedback actually? Do you
see anything wrong in the patch or is this about the wording of the
changelog? If it is the later is your concern serious enough to warrant
the rewording/reposting?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Vlastimil Babka]

On 03/07/2017 02:17 PM, Michal Hocko wrote:
> From: Michal Hocko 
> 
> We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> per cpu lru caches. This seems more than necessary because both can run
> on a single WQ. Both do not block on locks requiring a memory allocation
> nor perform any allocations themselves. We will save one rescuer thread
> this way.
> 
> On the other hand drain_all_pages() queues work on the system wq which
> doesn't have rescuer and so this depend on memory allocation (when all
> workers are stuck allocating and new ones cannot be created). This is
> not critical as there should be somebody invoking the OOM killer (e.g.
> the forking worker) and get the situation unstuck and eventually
> performs the draining. Quite annoying though. This worker should be
> using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> and vmstat.
> 
> Changes since v1
> - rename vmstat_wq to mm_percpu_wq - per Mel
> - make sure we are not trying to enqueue anything while the WQ hasn't
>   been intialized yet. This shouldn't happen because the initialization
>   is done from an init code but some init section might be triggering
>   those paths indirectly so just warn and skip the draining in that case
>   per Vlastimil
> - do not propagate error from setup_vmstat to keep the previous behavior
>   per Mel
> 
> Suggested-by: Tetsuo Handa 
> Signed-off-by: Michal Hocko 

Acked-by: Vlastimil Babka 

> ---
> 
> Hi,
> this has been previous posted [1] as an RFC. There was no fundamental
> opposition and some minor comments are addressed in this patch I
> believe.
> 
> To remind the original motivation, Tetsuo has noted that drain_all_pages
> doesn't use WQ_RECLAIM [1] and asked whether we can move the worker to
> the vmstat_wq which is WQ_RECLAIM. I think the deadlock he has described
> shouldn't happen but it would be really better to have the rescuer. I
> also think that we do not really need 2 or more workqueues and also pull
> lru draining in.
> 
> [1] http://lkml.kernel.org/r/20170207210908.530-1-mho...@kernel.org
> 
>  mm/internal.h   |  7 +++
>  mm/page_alloc.c |  9 -
>  mm/swap.c   | 27 ---
>  mm/vmstat.c | 14 --
>  4 files changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index 823a7a89099b..04d08ef91224 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -486,6 +486,13 @@ unsigned long reclaim_clean_pages_from_list(struct zone 
> *zone,
>  enum ttu_flags;
>  struct tlbflush_unmap_batch;
>  
> +
> +/*
> + * only for MM internal work items which do not depend on
> + * any allocations or locks which might depend on allocations
> + */
> +extern struct workqueue_struct *mm_percpu_wq;
> +
>  #ifdef CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
>  void try_to_unmap_flush(void);
>  void try_to_unmap_flush_dirty(void);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 1c72dd91c82e..1aa5729c8f98 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2362,6 +2362,13 @@ void drain_all_pages(struct zone *zone)
>*/
>   static cpumask_t cpus_with_pcps;
>  
> + /*
> +  * Make sure nobody triggers this path before mm_percpu_wq is fully
> +  * initialized.
> +  */
> + if (WARN_ON_ONCE(!mm_percpu_wq))
> + return;
> +
>   /* Workqueues cannot recurse */
>   if (current->flags & PF_WQ_WORKER)
>   return;
> @@ -2411,7 +2418,7 @@ void drain_all_pages(struct zone *zone)
>   for_each_cpu(cpu, _with_pcps) {
>   struct work_struct *work = per_cpu_ptr(_drain, cpu);
>   INIT_WORK(work, drain_local_pages_wq);
> - schedule_work_on(cpu, work);
> + queue_work_on(cpu, mm_percpu_wq, work);
>   }
>   for_each_cpu(cpu, _with_pcps)
>   flush_work(per_cpu_ptr(_drain, cpu));
> diff --git a/mm/swap.c b/mm/swap.c
> index ac98eb443a03..361bdb1575ab 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -677,30 +677,19 @@ static void lru_add_drain_per_cpu(struct work_struct 
> *dummy)
>  
>  static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
>  
> -/*
> - * lru_add_drain_wq is used to do lru_add_drain_all() from a WQ_MEM_RECLAIM
> - * workqueue, aiding in getting memory freed.
> - */
> -static struct workqueue_struct *lru_add_drain_wq;
> -
> -static int __init lru_init(void)
> -{
> - lru_add_drain_wq = alloc_workqueue("lru-add-drain", WQ_MEM_RECLAIM, 0);
> -
> - if (WARN(!lru_add_drain_wq,
> - "Failed to create workqueue lru_add_drain_wq"))
> - return -ENOMEM;
> -
> - return 0;
> -}
> -early_initcall(lru_init);
> -
>  void lru_add_drain_all(void)
>  {
>   static DEFINE_MUTEX(lock);
>   static struct cpumask has_work;
>   int cpu;
>  
> + /*
> +  * Make 

Re: [PATCH] mm: move pcp and lru-pcp drainging into single wq

[Vlastimil Babka]

On 03/07/2017 02:17 PM, Michal Hocko wrote:
> From: Michal Hocko 
> 
> We currently have 2 specific WQ_RECLAIM workqueues in the mm code.
> vmstat_wq for updating pcp stats and lru_add_drain_wq dedicated to drain
> per cpu lru caches. This seems more than necessary because both can run
> on a single WQ. Both do not block on locks requiring a memory allocation
> nor perform any allocations themselves. We will save one rescuer thread
> this way.
> 
> On the other hand drain_all_pages() queues work on the system wq which
> doesn't have rescuer and so this depend on memory allocation (when all
> workers are stuck allocating and new ones cannot be created). This is
> not critical as there should be somebody invoking the OOM killer (e.g.
> the forking worker) and get the situation unstuck and eventually
> performs the draining. Quite annoying though. This worker should be
> using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> and vmstat.
> 
> Changes since v1
> - rename vmstat_wq to mm_percpu_wq - per Mel
> - make sure we are not trying to enqueue anything while the WQ hasn't
>   been intialized yet. This shouldn't happen because the initialization
>   is done from an init code but some init section might be triggering
>   those paths indirectly so just warn and skip the draining in that case
>   per Vlastimil
> - do not propagate error from setup_vmstat to keep the previous behavior
>   per Mel
> 
> Suggested-by: Tetsuo Handa 
> Signed-off-by: Michal Hocko 

Acked-by: Vlastimil Babka 

> ---
> 
> Hi,
> this has been previous posted [1] as an RFC. There was no fundamental
> opposition and some minor comments are addressed in this patch I
> believe.
> 
> To remind the original motivation, Tetsuo has noted that drain_all_pages
> doesn't use WQ_RECLAIM [1] and asked whether we can move the worker to
> the vmstat_wq which is WQ_RECLAIM. I think the deadlock he has described
> shouldn't happen but it would be really better to have the rescuer. I
> also think that we do not really need 2 or more workqueues and also pull
> lru draining in.
> 
> [1] http://lkml.kernel.org/r/20170207210908.530-1-mho...@kernel.org
> 
>  mm/internal.h   |  7 +++
>  mm/page_alloc.c |  9 -
>  mm/swap.c   | 27 ---
>  mm/vmstat.c | 14 --
>  4 files changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index 823a7a89099b..04d08ef91224 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -486,6 +486,13 @@ unsigned long reclaim_clean_pages_from_list(struct zone 
> *zone,
>  enum ttu_flags;
>  struct tlbflush_unmap_batch;
>  
> +
> +/*
> + * only for MM internal work items which do not depend on
> + * any allocations or locks which might depend on allocations
> + */
> +extern struct workqueue_struct *mm_percpu_wq;
> +
>  #ifdef CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
>  void try_to_unmap_flush(void);
>  void try_to_unmap_flush_dirty(void);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 1c72dd91c82e..1aa5729c8f98 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2362,6 +2362,13 @@ void drain_all_pages(struct zone *zone)
>*/
>   static cpumask_t cpus_with_pcps;
>  
> + /*
> +  * Make sure nobody triggers this path before mm_percpu_wq is fully
> +  * initialized.
> +  */
> + if (WARN_ON_ONCE(!mm_percpu_wq))
> + return;
> +
>   /* Workqueues cannot recurse */
>   if (current->flags & PF_WQ_WORKER)
>   return;
> @@ -2411,7 +2418,7 @@ void drain_all_pages(struct zone *zone)
>   for_each_cpu(cpu, _with_pcps) {
>   struct work_struct *work = per_cpu_ptr(_drain, cpu);
>   INIT_WORK(work, drain_local_pages_wq);
> - schedule_work_on(cpu, work);
> + queue_work_on(cpu, mm_percpu_wq, work);
>   }
>   for_each_cpu(cpu, _with_pcps)
>   flush_work(per_cpu_ptr(_drain, cpu));
> diff --git a/mm/swap.c b/mm/swap.c
> index ac98eb443a03..361bdb1575ab 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -677,30 +677,19 @@ static void lru_add_drain_per_cpu(struct work_struct 
> *dummy)
>  
>  static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
>  
> -/*
> - * lru_add_drain_wq is used to do lru_add_drain_all() from a WQ_MEM_RECLAIM
> - * workqueue, aiding in getting memory freed.
> - */
> -static struct workqueue_struct *lru_add_drain_wq;
> -
> -static int __init lru_init(void)
> -{
> - lru_add_drain_wq = alloc_workqueue("lru-add-drain", WQ_MEM_RECLAIM, 0);
> -
> - if (WARN(!lru_add_drain_wq,
> - "Failed to create workqueue lru_add_drain_wq"))
> - return -ENOMEM;
> -
> - return 0;
> -}
> -early_initcall(lru_init);
> -
>  void lru_add_drain_all(void)
>  {
>   static DEFINE_MUTEX(lock);
>   static struct cpumask has_work;
>   int cpu;
>  
> + /*
> +  * Make sure nobody triggers this path before mm_percpu_wq is fully
> +  * initialized.
>