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

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 <[email protected]>

Quoted 24 lines · On Mon, 28 Aug 2017 11:33:41 +0200 Michal Hocko <[email protected]> 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).

Quoted 6 lines

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 <[email protected]>

No cc:stable?

Quoted 33 lines · Andrew Morton

Quoted 30 lines · On Mon, 28 Aug 2017 11:33:41 +0200 Michal Hocko <[email protected]> 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 <[email protected]>

Quoted 2 lines

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.

Quoted 37 lines · Tetsuo Handa

Quoted 35 lines · Andrew Morton

Quoted 24 lines · On Mon, 28 Aug 2017 11:33:41 +0200 Michal Hocko <[email protected]> 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).

Quoted 6 lines

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 <[email protected]>

No cc:stable?

Quoted 4 lines

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?

Quoted 4 lines

Quoted 4 lines

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.

Quoted 2 lines

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.

Hello, Sorry about the delay.

Quoted 4 lines · Michal Hocko

Quoted 4 lines

Quoted 4 lines

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.

Quoted 3 lines

Quoted 2 lines

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?

Quoted 3 lines

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.

Quoted 3 lines

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?

Quoted 24 lines · On Mon, 28 Aug 2017 11:33:41 +0200 Michal Hocko <[email protected]> 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).

Quoted 6 lines

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 <[email protected]>

No cc:stable?