[PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

drain_all_pages backs off when called from a kworker context since
0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue
context") because the original IPI based pcp draining has been replaced
by a WQ based one and the check wanted to prevent from recursion and
inter workers dependencies. This has made some sense at the time
because the system WQ has been used and one worker holding the lock
could be blocked while waiting for new workers to emerge which can be a
problem under OOM conditions.

Since then ce612879ddc7 ("mm: move pcp and lru-pcp draining into single
wq") has moved draining to a dedicated (mm_percpu_wq) WQ with a rescuer
so we shouldn't depend on any other WQ activity to make a forward
progress so calling drain_all_pages from a worker context is safe as
long as this doesn't happen from mm_percpu_wq itself which is not the
case because all workers are required to _not_ depend on any MM locks.

Why is this a problem in the first place? ACPI driven memory hot-remove
(acpi_device_hotplug) is executed from the worker context. We end
up calling __offline_pages to free all the pages and that requires
both lru_add_drain_all_cpuslocked and drain_all_pages to do their job
otherwise we can have dangling pages on pcp lists and fail the offline
operation (__test_page_isolated_in_pageblock would see a page with 0
ref. count but without PageBuddy set).

Fix the issue by removing the worker check in drain_all_pages.
lru_add_drain_all_cpuslocked doesn't have this restriction so it works
as expected.

Fixes: 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue context")
Signed-off-by: Michal Hocko <mhocko@suse.com>
---

 mm/page_alloc.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index ffead2159001..a94e1847bb0d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2477,10 +2477,6 @@ void drain_all_pages(struct zone *zone)
 	if (WARN_ON_ONCE(!mm_percpu_wq))
 		return;
 
-	/* Workqueues cannot recurse */
-	if (current->flags & PF_WQ_WORKER)
-		return;
-
 	/*
 	 * Do not drain if one is already in progress unless it's specific to
 	 * a zone. Such callers are primarily CMA and memory hotplug and need
-- 
2.13.2

[PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

drain_all_pages backs off when called from a kworker context since
0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue
context") because the original IPI based pcp draining has been replaced
by a WQ based one and the check wanted to prevent from recursion and
inter workers dependencies. This has made some sense at the time
because the system WQ has been used and one worker holding the lock
could be blocked while waiting for new workers to emerge which can be a
problem under OOM conditions.

Since then ce612879ddc7 ("mm: move pcp and lru-pcp draining into single
wq") has moved draining to a dedicated (mm_percpu_wq) WQ with a rescuer
so we shouldn't depend on any other WQ activity to make a forward
progress so calling drain_all_pages from a worker context is safe as
long as this doesn't happen from mm_percpu_wq itself which is not the
case because all workers are required to _not_ depend on any MM locks.

Why is this a problem in the first place? ACPI driven memory hot-remove
(acpi_device_hotplug) is executed from the worker context. We end
up calling __offline_pages to free all the pages and that requires
both lru_add_drain_all_cpuslocked and drain_all_pages to do their job
otherwise we can have dangling pages on pcp lists and fail the offline
operation (__test_page_isolated_in_pageblock would see a page with 0
ref. count but without PageBuddy set).

Fix the issue by removing the worker check in drain_all_pages.
lru_add_drain_all_cpuslocked doesn't have this restriction so it works
as expected.

Fixes: 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue context")
Signed-off-by: Michal Hocko <mhocko@suse.com>
---

 mm/page_alloc.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index ffead2159001..a94e1847bb0d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2477,10 +2477,6 @@ void drain_all_pages(struct zone *zone)
 	if (WARN_ON_ONCE(!mm_percpu_wq))
 		return;
 
-	/* Workqueues cannot recurse */
-	if (current->flags & PF_WQ_WORKER)
-		return;
-
 	/*
 	 * Do not drain if one is already in progress unless it's specific to
 	 * a zone. Such callers are primarily CMA and memory hotplug and need
-- 
2.13.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[Andrew Morton]

On Mon, 28 Aug 2017 11:33:41 +0200 Michal Hocko <mhocko@kernel.org> wrote:

> drain_all_pages backs off when called from a kworker context since
> 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue
> context") because the original IPI based pcp draining has been replaced
> by a WQ based one and the check wanted to prevent from recursion and
> inter workers dependencies. This has made some sense at the time
> because the system WQ has been used and one worker holding the lock
> could be blocked while waiting for new workers to emerge which can be a
> problem under OOM conditions.
> 
> Since then ce612879ddc7 ("mm: move pcp and lru-pcp draining into single
> wq") has moved draining to a dedicated (mm_percpu_wq) WQ with a rescuer
> so we shouldn't depend on any other WQ activity to make a forward
> progress so calling drain_all_pages from a worker context is safe as
> long as this doesn't happen from mm_percpu_wq itself which is not the
> case because all workers are required to _not_ depend on any MM locks.
> 
> Why is this a problem in the first place? ACPI driven memory hot-remove
> (acpi_device_hotplug) is executed from the worker context. We end
> up calling __offline_pages to free all the pages and that requires
> both lru_add_drain_all_cpuslocked and drain_all_pages to do their job
> otherwise we can have dangling pages on pcp lists and fail the offline
> operation (__test_page_isolated_in_pageblock would see a page with 0
> ref. count but without PageBuddy set).
> 
> Fix the issue by removing the worker check in drain_all_pages.
> lru_add_drain_all_cpuslocked doesn't have this restriction so it works
> as expected.
> 
> Fixes: 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue context")
> Signed-off-by: Michal Hocko <mhocko@suse.com>

No cc:stable?

Re: [PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[Andrew Morton]

On Mon, 28 Aug 2017 11:33:41 +0200 Michal Hocko <mhocko@kernel.org> wrote:

> drain_all_pages backs off when called from a kworker context since
> 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue
> context") because the original IPI based pcp draining has been replaced
> by a WQ based one and the check wanted to prevent from recursion and
> inter workers dependencies. This has made some sense at the time
> because the system WQ has been used and one worker holding the lock
> could be blocked while waiting for new workers to emerge which can be a
> problem under OOM conditions.
> 
> Since then ce612879ddc7 ("mm: move pcp and lru-pcp draining into single
> wq") has moved draining to a dedicated (mm_percpu_wq) WQ with a rescuer
> so we shouldn't depend on any other WQ activity to make a forward
> progress so calling drain_all_pages from a worker context is safe as
> long as this doesn't happen from mm_percpu_wq itself which is not the
> case because all workers are required to _not_ depend on any MM locks.
> 
> Why is this a problem in the first place? ACPI driven memory hot-remove
> (acpi_device_hotplug) is executed from the worker context. We end
> up calling __offline_pages to free all the pages and that requires
> both lru_add_drain_all_cpuslocked and drain_all_pages to do their job
> otherwise we can have dangling pages on pcp lists and fail the offline
> operation (__test_page_isolated_in_pageblock would see a page with 0
> ref. count but without PageBuddy set).
> 
> Fix the issue by removing the worker check in drain_all_pages.
> lru_add_drain_all_cpuslocked doesn't have this restriction so it works
> as expected.
> 
> Fixes: 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue context")
> Signed-off-by: Michal Hocko <mhocko@suse.com>

No cc:stable?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[Tetsuo Handa]

On 2017/08/29 7:33, Andrew Morton wrote:
> On Mon, 28 Aug 2017 11:33:41 +0200 Michal Hocko <mhocko@kernel.org> wrote:
> 
>> drain_all_pages backs off when called from a kworker context since
>> 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue
>> context") because the original IPI based pcp draining has been replaced
>> by a WQ based one and the check wanted to prevent from recursion and
>> inter workers dependencies. This has made some sense at the time
>> because the system WQ has been used and one worker holding the lock
>> could be blocked while waiting for new workers to emerge which can be a
>> problem under OOM conditions.
>>
>> Since then ce612879ddc7 ("mm: move pcp and lru-pcp draining into single
>> wq") has moved draining to a dedicated (mm_percpu_wq) WQ with a rescuer
>> so we shouldn't depend on any other WQ activity to make a forward
>> progress so calling drain_all_pages from a worker context is safe as
>> long as this doesn't happen from mm_percpu_wq itself which is not the
>> case because all workers are required to _not_ depend on any MM locks.
>>
>> Why is this a problem in the first place? ACPI driven memory hot-remove
>> (acpi_device_hotplug) is executed from the worker context. We end
>> up calling __offline_pages to free all the pages and that requires
>> both lru_add_drain_all_cpuslocked and drain_all_pages to do their job
>> otherwise we can have dangling pages on pcp lists and fail the offline
>> operation (__test_page_isolated_in_pageblock would see a page with 0
>> ref. count but without PageBuddy set).
>>
>> Fix the issue by removing the worker check in drain_all_pages.
>> lru_add_drain_all_cpuslocked doesn't have this restriction so it works
>> as expected.
>>
>> Fixes: 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue context")
>> Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> No cc:stable?
> 

Michal, are you sure that this patch does not cause deadlock?

As shown in "[PATCH] mm: Use WQ_HIGHPRI for mm_percpu_wq." thread, currently work
items on mm_percpu_wq seem to be blocked by other work items not on mm_percpu_wq.

Re: [PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[Tetsuo Handa]

On 2017/08/29 7:33, Andrew Morton wrote:
> On Mon, 28 Aug 2017 11:33:41 +0200 Michal Hocko <mhocko@kernel.org> wrote:
> 
>> drain_all_pages backs off when called from a kworker context since
>> 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue
>> context") because the original IPI based pcp draining has been replaced
>> by a WQ based one and the check wanted to prevent from recursion and
>> inter workers dependencies. This has made some sense at the time
>> because the system WQ has been used and one worker holding the lock
>> could be blocked while waiting for new workers to emerge which can be a
>> problem under OOM conditions.
>>
>> Since then ce612879ddc7 ("mm: move pcp and lru-pcp draining into single
>> wq") has moved draining to a dedicated (mm_percpu_wq) WQ with a rescuer
>> so we shouldn't depend on any other WQ activity to make a forward
>> progress so calling drain_all_pages from a worker context is safe as
>> long as this doesn't happen from mm_percpu_wq itself which is not the
>> case because all workers are required to _not_ depend on any MM locks.
>>
>> Why is this a problem in the first place? ACPI driven memory hot-remove
>> (acpi_device_hotplug) is executed from the worker context. We end
>> up calling __offline_pages to free all the pages and that requires
>> both lru_add_drain_all_cpuslocked and drain_all_pages to do their job
>> otherwise we can have dangling pages on pcp lists and fail the offline
>> operation (__test_page_isolated_in_pageblock would see a page with 0
>> ref. count but without PageBuddy set).
>>
>> Fix the issue by removing the worker check in drain_all_pages.
>> lru_add_drain_all_cpuslocked doesn't have this restriction so it works
>> as expected.
>>
>> Fixes: 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue context")
>> Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> No cc:stable?
> 

Michal, are you sure that this patch does not cause deadlock?

As shown in "[PATCH] mm: Use WQ_HIGHPRI for mm_percpu_wq." thread, currently work
items on mm_percpu_wq seem to be blocked by other work items not on mm_percpu_wq.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[Michal Hocko]

On Tue 29-08-17 20:20:39, Tetsuo Handa wrote:
> On 2017/08/29 7:33, Andrew Morton wrote:
> > On Mon, 28 Aug 2017 11:33:41 +0200 Michal Hocko <mhocko@kernel.org> wrote:
> > 
> >> drain_all_pages backs off when called from a kworker context since
> >> 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue
> >> context") because the original IPI based pcp draining has been replaced
> >> by a WQ based one and the check wanted to prevent from recursion and
> >> inter workers dependencies. This has made some sense at the time
> >> because the system WQ has been used and one worker holding the lock
> >> could be blocked while waiting for new workers to emerge which can be a
> >> problem under OOM conditions.
> >>
> >> Since then ce612879ddc7 ("mm: move pcp and lru-pcp draining into single
> >> wq") has moved draining to a dedicated (mm_percpu_wq) WQ with a rescuer
> >> so we shouldn't depend on any other WQ activity to make a forward
> >> progress so calling drain_all_pages from a worker context is safe as
> >> long as this doesn't happen from mm_percpu_wq itself which is not the
> >> case because all workers are required to _not_ depend on any MM locks.
> >>
> >> Why is this a problem in the first place? ACPI driven memory hot-remove
> >> (acpi_device_hotplug) is executed from the worker context. We end
> >> up calling __offline_pages to free all the pages and that requires
> >> both lru_add_drain_all_cpuslocked and drain_all_pages to do their job
> >> otherwise we can have dangling pages on pcp lists and fail the offline
> >> operation (__test_page_isolated_in_pageblock would see a page with 0
> >> ref. count but without PageBuddy set).
> >>
> >> Fix the issue by removing the worker check in drain_all_pages.
> >> lru_add_drain_all_cpuslocked doesn't have this restriction so it works
> >> as expected.
> >>
> >> Fixes: 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue context")
> >> Signed-off-by: Michal Hocko <mhocko@suse.com>
> > 
> > No cc:stable?
> > 
> 
> Michal, are you sure that this patch does not cause deadlock?
> 
> As shown in "[PATCH] mm: Use WQ_HIGHPRI for mm_percpu_wq." thread, currently work
> items on mm_percpu_wq seem to be blocked by other work items not on mm_percpu_wq.

But we have a rescuer so we should make a forward progress eventually.
Or am I missing something. Tejun, could you have a look please?/

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[Michal Hocko]

On Tue 29-08-17 20:20:39, Tetsuo Handa wrote:
> On 2017/08/29 7:33, Andrew Morton wrote:
> > On Mon, 28 Aug 2017 11:33:41 +0200 Michal Hocko <mhocko@kernel.org> wrote:
> > 
> >> drain_all_pages backs off when called from a kworker context since
> >> 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue
> >> context") because the original IPI based pcp draining has been replaced
> >> by a WQ based one and the check wanted to prevent from recursion and
> >> inter workers dependencies. This has made some sense at the time
> >> because the system WQ has been used and one worker holding the lock
> >> could be blocked while waiting for new workers to emerge which can be a
> >> problem under OOM conditions.
> >>
> >> Since then ce612879ddc7 ("mm: move pcp and lru-pcp draining into single
> >> wq") has moved draining to a dedicated (mm_percpu_wq) WQ with a rescuer
> >> so we shouldn't depend on any other WQ activity to make a forward
> >> progress so calling drain_all_pages from a worker context is safe as
> >> long as this doesn't happen from mm_percpu_wq itself which is not the
> >> case because all workers are required to _not_ depend on any MM locks.
> >>
> >> Why is this a problem in the first place? ACPI driven memory hot-remove
> >> (acpi_device_hotplug) is executed from the worker context. We end
> >> up calling __offline_pages to free all the pages and that requires
> >> both lru_add_drain_all_cpuslocked and drain_all_pages to do their job
> >> otherwise we can have dangling pages on pcp lists and fail the offline
> >> operation (__test_page_isolated_in_pageblock would see a page with 0
> >> ref. count but without PageBuddy set).
> >>
> >> Fix the issue by removing the worker check in drain_all_pages.
> >> lru_add_drain_all_cpuslocked doesn't have this restriction so it works
> >> as expected.
> >>
> >> Fixes: 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue context")
> >> Signed-off-by: Michal Hocko <mhocko@suse.com>
> > 
> > No cc:stable?
> > 
> 
> Michal, are you sure that this patch does not cause deadlock?
> 
> As shown in "[PATCH] mm: Use WQ_HIGHPRI for mm_percpu_wq." thread, currently work
> items on mm_percpu_wq seem to be blocked by other work items not on mm_percpu_wq.

But we have a rescuer so we should make a forward progress eventually.
Or am I missing something. Tejun, could you have a look please?/

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[Michal Hocko]

On Mon 28-08-17 15:33:59, Andrew Morton wrote:
> On Mon, 28 Aug 2017 11:33:41 +0200 Michal Hocko <mhocko@kernel.org> wrote:
> 
> > drain_all_pages backs off when called from a kworker context since
> > 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue
> > context") because the original IPI based pcp draining has been replaced
> > by a WQ based one and the check wanted to prevent from recursion and
> > inter workers dependencies. This has made some sense at the time
> > because the system WQ has been used and one worker holding the lock
> > could be blocked while waiting for new workers to emerge which can be a
> > problem under OOM conditions.
> > 
> > Since then ce612879ddc7 ("mm: move pcp and lru-pcp draining into single
> > wq") has moved draining to a dedicated (mm_percpu_wq) WQ with a rescuer
> > so we shouldn't depend on any other WQ activity to make a forward
> > progress so calling drain_all_pages from a worker context is safe as
> > long as this doesn't happen from mm_percpu_wq itself which is not the
> > case because all workers are required to _not_ depend on any MM locks.
> > 
> > Why is this a problem in the first place? ACPI driven memory hot-remove
> > (acpi_device_hotplug) is executed from the worker context. We end
> > up calling __offline_pages to free all the pages and that requires
> > both lru_add_drain_all_cpuslocked and drain_all_pages to do their job
> > otherwise we can have dangling pages on pcp lists and fail the offline
> > operation (__test_page_isolated_in_pageblock would see a page with 0
> > ref. count but without PageBuddy set).
> > 
> > Fix the issue by removing the worker check in drain_all_pages.
> > lru_add_drain_all_cpuslocked doesn't have this restriction so it works
> > as expected.
> > 
> > Fixes: 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue context")
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> No cc:stable?

I wouldn't be opposed I have just seen so many things broken in this
area I didn't consider this important enough. This would be 4.11+

Thanks!
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[Michal Hocko]

On Mon 28-08-17 15:33:59, Andrew Morton wrote:
> On Mon, 28 Aug 2017 11:33:41 +0200 Michal Hocko <mhocko@kernel.org> wrote:
> 
> > drain_all_pages backs off when called from a kworker context since
> > 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue
> > context") because the original IPI based pcp draining has been replaced
> > by a WQ based one and the check wanted to prevent from recursion and
> > inter workers dependencies. This has made some sense at the time
> > because the system WQ has been used and one worker holding the lock
> > could be blocked while waiting for new workers to emerge which can be a
> > problem under OOM conditions.
> > 
> > Since then ce612879ddc7 ("mm: move pcp and lru-pcp draining into single
> > wq") has moved draining to a dedicated (mm_percpu_wq) WQ with a rescuer
> > so we shouldn't depend on any other WQ activity to make a forward
> > progress so calling drain_all_pages from a worker context is safe as
> > long as this doesn't happen from mm_percpu_wq itself which is not the
> > case because all workers are required to _not_ depend on any MM locks.
> > 
> > Why is this a problem in the first place? ACPI driven memory hot-remove
> > (acpi_device_hotplug) is executed from the worker context. We end
> > up calling __offline_pages to free all the pages and that requires
> > both lru_add_drain_all_cpuslocked and drain_all_pages to do their job
> > otherwise we can have dangling pages on pcp lists and fail the offline
> > operation (__test_page_isolated_in_pageblock would see a page with 0
> > ref. count but without PageBuddy set).
> > 
> > Fix the issue by removing the worker check in drain_all_pages.
> > lru_add_drain_all_cpuslocked doesn't have this restriction so it works
> > as expected.
> > 
> > Fixes: 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue context")
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> No cc:stable?

I wouldn't be opposed I have just seen so many things broken in this
area I didn't consider this important enough. This would be 4.11+

Thanks!
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[Michal Hocko]

On Tue 29-08-17 13:28:23, Michal Hocko wrote:
> On Tue 29-08-17 20:20:39, Tetsuo Handa wrote:
> > On 2017/08/29 7:33, Andrew Morton wrote:
> > > On Mon, 28 Aug 2017 11:33:41 +0200 Michal Hocko <mhocko@kernel.org> wrote:
> > > 
> > >> drain_all_pages backs off when called from a kworker context since
> > >> 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue
> > >> context") because the original IPI based pcp draining has been replaced
> > >> by a WQ based one and the check wanted to prevent from recursion and
> > >> inter workers dependencies. This has made some sense at the time
> > >> because the system WQ has been used and one worker holding the lock
> > >> could be blocked while waiting for new workers to emerge which can be a
> > >> problem under OOM conditions.
> > >>
> > >> Since then ce612879ddc7 ("mm: move pcp and lru-pcp draining into single
> > >> wq") has moved draining to a dedicated (mm_percpu_wq) WQ with a rescuer
> > >> so we shouldn't depend on any other WQ activity to make a forward
> > >> progress so calling drain_all_pages from a worker context is safe as
> > >> long as this doesn't happen from mm_percpu_wq itself which is not the
> > >> case because all workers are required to _not_ depend on any MM locks.
> > >>
> > >> Why is this a problem in the first place? ACPI driven memory hot-remove
> > >> (acpi_device_hotplug) is executed from the worker context. We end
> > >> up calling __offline_pages to free all the pages and that requires
> > >> both lru_add_drain_all_cpuslocked and drain_all_pages to do their job
> > >> otherwise we can have dangling pages on pcp lists and fail the offline
> > >> operation (__test_page_isolated_in_pageblock would see a page with 0
> > >> ref. count but without PageBuddy set).
> > >>
> > >> Fix the issue by removing the worker check in drain_all_pages.
> > >> lru_add_drain_all_cpuslocked doesn't have this restriction so it works
> > >> as expected.
> > >>
> > >> Fixes: 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue context")
> > >> Signed-off-by: Michal Hocko <mhocko@suse.com>
> > > 
> > > No cc:stable?
> > > 
> > 
> > Michal, are you sure that this patch does not cause deadlock?
> > 
> > As shown in "[PATCH] mm: Use WQ_HIGHPRI for mm_percpu_wq." thread, currently work
> > items on mm_percpu_wq seem to be blocked by other work items not on mm_percpu_wq.
> 
> But we have a rescuer so we should make a forward progress eventually.
> Or am I missing something. Tejun, could you have a look please?

ping... I would really appreaciate if you could double check my thinking
Tejun. This is a tricky area and I would like to prevent further subtle
issues here.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[Michal Hocko]

On Tue 29-08-17 13:28:23, Michal Hocko wrote:
> On Tue 29-08-17 20:20:39, Tetsuo Handa wrote:
> > On 2017/08/29 7:33, Andrew Morton wrote:
> > > On Mon, 28 Aug 2017 11:33:41 +0200 Michal Hocko <mhocko@kernel.org> wrote:
> > > 
> > >> drain_all_pages backs off when called from a kworker context since
> > >> 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue
> > >> context") because the original IPI based pcp draining has been replaced
> > >> by a WQ based one and the check wanted to prevent from recursion and
> > >> inter workers dependencies. This has made some sense at the time
> > >> because the system WQ has been used and one worker holding the lock
> > >> could be blocked while waiting for new workers to emerge which can be a
> > >> problem under OOM conditions.
> > >>
> > >> Since then ce612879ddc7 ("mm: move pcp and lru-pcp draining into single
> > >> wq") has moved draining to a dedicated (mm_percpu_wq) WQ with a rescuer
> > >> so we shouldn't depend on any other WQ activity to make a forward
> > >> progress so calling drain_all_pages from a worker context is safe as
> > >> long as this doesn't happen from mm_percpu_wq itself which is not the
> > >> case because all workers are required to _not_ depend on any MM locks.
> > >>
> > >> Why is this a problem in the first place? ACPI driven memory hot-remove
> > >> (acpi_device_hotplug) is executed from the worker context. We end
> > >> up calling __offline_pages to free all the pages and that requires
> > >> both lru_add_drain_all_cpuslocked and drain_all_pages to do their job
> > >> otherwise we can have dangling pages on pcp lists and fail the offline
> > >> operation (__test_page_isolated_in_pageblock would see a page with 0
> > >> ref. count but without PageBuddy set).
> > >>
> > >> Fix the issue by removing the worker check in drain_all_pages.
> > >> lru_add_drain_all_cpuslocked doesn't have this restriction so it works
> > >> as expected.
> > >>
> > >> Fixes: 0ccce3b924212 ("mm, page_alloc: drain per-cpu pages from workqueue context")
> > >> Signed-off-by: Michal Hocko <mhocko@suse.com>
> > > 
> > > No cc:stable?
> > > 
> > 
> > Michal, are you sure that this patch does not cause deadlock?
> > 
> > As shown in "[PATCH] mm: Use WQ_HIGHPRI for mm_percpu_wq." thread, currently work
> > items on mm_percpu_wq seem to be blocked by other work items not on mm_percpu_wq.
> 
> But we have a rescuer so we should make a forward progress eventually.
> Or am I missing something. Tejun, could you have a look please?

ping... I would really appreaciate if you could double check my thinking
Tejun. This is a tricky area and I would like to prevent further subtle
issues here.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[Tejun Heo]

Hello, Sorry about the delay.

On Thu, Aug 31, 2017 at 07:33:42AM +0200, Michal Hocko wrote:
> > > Michal, are you sure that this patch does not cause deadlock?
> > > 
> > > As shown in "[PATCH] mm: Use WQ_HIGHPRI for mm_percpu_wq." thread, currently work
> > > items on mm_percpu_wq seem to be blocked by other work items not on mm_percpu_wq.

IIUC that wasn't a deadlock but more a legitimate starvation from too
many tasks trying to reclaim directly.

> > But we have a rescuer so we should make a forward progress eventually.
> > Or am I missing something. Tejun, could you have a look please?
> 
> ping... I would really appreaciate if you could double check my thinking
> Tejun. This is a tricky area and I would like to prevent further subtle
> issues here.

So, this shouldn't be an issue.  This may get affected by direct
reclaim frenzy but it's only a small piece of the whole symptom and we
gotta fix that at the source.

Thanks.

-- 
tejun

Re: [PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[Tejun Heo]

Hello, Sorry about the delay.

On Thu, Aug 31, 2017 at 07:33:42AM +0200, Michal Hocko wrote:
> > > Michal, are you sure that this patch does not cause deadlock?
> > > 
> > > As shown in "[PATCH] mm: Use WQ_HIGHPRI for mm_percpu_wq." thread, currently work
> > > items on mm_percpu_wq seem to be blocked by other work items not on mm_percpu_wq.

IIUC that wasn't a deadlock but more a legitimate starvation from too
many tasks trying to reclaim directly.

> > But we have a rescuer so we should make a forward progress eventually.
> > Or am I missing something. Tejun, could you have a look please?
> 
> ping... I would really appreaciate if you could double check my thinking
> Tejun. This is a tricky area and I would like to prevent further subtle
> issues here.

So, this shouldn't be an issue.  This may get affected by direct
reclaim frenzy but it's only a small piece of the whole symptom and we
gotta fix that at the source.

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[Michal Hocko]

On Mon 18-09-17 20:38:22, Tejun Heo wrote:
> Hello, Sorry about the delay.
> 
> On Thu, Aug 31, 2017 at 07:33:42AM +0200, Michal Hocko wrote:
> > > > Michal, are you sure that this patch does not cause deadlock?
> > > > 
> > > > As shown in "[PATCH] mm: Use WQ_HIGHPRI for mm_percpu_wq." thread, currently work
> > > > items on mm_percpu_wq seem to be blocked by other work items not on mm_percpu_wq.
> 
> IIUC that wasn't a deadlock but more a legitimate starvation from too
> many tasks trying to reclaim directly.
> 
> > > But we have a rescuer so we should make a forward progress eventually.
> > > Or am I missing something. Tejun, could you have a look please?
> > 
> > ping... I would really appreaciate if you could double check my thinking
> > Tejun. This is a tricky area and I would like to prevent further subtle
> > issues here.
> 
> So, this shouldn't be an issue.  This may get affected by direct
> reclaim frenzy but it's only a small piece of the whole symptom and we
> gotta fix that at the source.

OK, so there shouldn't be any issue with the patch, right?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[Michal Hocko]

On Mon 18-09-17 20:38:22, Tejun Heo wrote:
> Hello, Sorry about the delay.
> 
> On Thu, Aug 31, 2017 at 07:33:42AM +0200, Michal Hocko wrote:
> > > > Michal, are you sure that this patch does not cause deadlock?
> > > > 
> > > > As shown in "[PATCH] mm: Use WQ_HIGHPRI for mm_percpu_wq." thread, currently work
> > > > items on mm_percpu_wq seem to be blocked by other work items not on mm_percpu_wq.
> 
> IIUC that wasn't a deadlock but more a legitimate starvation from too
> many tasks trying to reclaim directly.
> 
> > > But we have a rescuer so we should make a forward progress eventually.
> > > Or am I missing something. Tejun, could you have a look please?
> > 
> > ping... I would really appreaciate if you could double check my thinking
> > Tejun. This is a tricky area and I would like to prevent further subtle
> > issues here.
> 
> So, this shouldn't be an issue.  This may get affected by direct
> reclaim frenzy but it's only a small piece of the whole symptom and we
> gotta fix that at the source.

OK, so there shouldn't be any issue with the patch, right?
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[Tejun Heo]

Hello,

On Tue, Sep 19, 2017 at 11:45:21AM +0200, Michal Hocko wrote:
> > So, this shouldn't be an issue.  This may get affected by direct
> > reclaim frenzy but it's only a small piece of the whole symptom and we
> > gotta fix that at the source.
> 
> OK, so there shouldn't be any issue with the patch, right?

idk, it'll make the code path more susceptible to direct reclaim
starvations, so it's difficult to claim that there won't be *any*
problems; however, given the extent of the starvation problem, this
likely won't add any noticeable issues.

Thanks.

-- 
tejun

Re: [PATCH] mm, memory_hotplug: do not back off draining pcp free pages from kworker context

[Tejun Heo]

Hello,

On Tue, Sep 19, 2017 at 11:45:21AM +0200, Michal Hocko wrote:
> > So, this shouldn't be an issue.  This may get affected by direct
> > reclaim frenzy but it's only a small piece of the whole symptom and we
> > gotta fix that at the source.
> 
> OK, so there shouldn't be any issue with the patch, right?

idk, it'll make the code path more susceptible to direct reclaim
starvations, so it's difficult to claim that there won't be *any*
problems; however, given the extent of the starvation problem, this
likely won't add any noticeable issues.

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>