mm: drop hotplug lock from lru_add_drain_all

PATCH In review Michal Hocko 2 participants 7 messages Nov 14, 2017

Subsystems (3)

Memory management MM swap Memory hot(un)plug

lore

Share & tools

lore

Michal Hocko

author

2017-11-14 13:54 UTC #0 lore raw

PATCH 1/1 mm: drop hotplug lock from lru_add_drain_all

From: Michal Hocko <[email protected]>

Pulling cpu hotplug locks inside the mm core function like lru_add_drain_all just asks for problems and the recent lockdep splat
[1] just proves this. While the usage in that particular case might be wrong we should prevent from locking as lru_add_drain_all is used at many places. It seems that this is not all that hard to achieve actually.

We have done the same thing for drain_all_pages which is analogous by a459eeb7b852 ("mm, page_alloc: do not depend on cpu hotplug locks inside the allocator"). All we have to care about is to handle
- the work item might be executed on a different cpu in worker from unbound pool so it doesn't run on pinned on the cpu

- we have to make sure that we do not race with page_alloc_cpu_dead calling lru_add_drain_cpu

the first part is already handled because the worker calls lru_add_drain which disables preemption when calling lru_add_drain_cpu on the local cpu it is draining. The later is true because page_alloc_cpu_dead is called on the controlling CPU after the hotplugged CPU vanished completely.

[1] http://lkml.kernel.org/r/[email protected]

Signed-off-by Michal Hocko <[email protected]>

---
Hi,
this has been posted as 2 patch series [1] previously. It turned out that the first patch was simply broken and the second one could be simplified because the irq disabling is just pointless. There were no other objections so I am resending this patch which should remove quite a large space of potential lockups as lru_add_drain_all is used at many places so removing the hoptlug locking is a good thing in general.

Can we have this merged or there are still some objections?

[1] http://lkml.kernel.org/r/[email protected]

 include/linux/swap.h | 1 -
 mm/memory_hotplug.c  | 2 +-
 mm/swap.c            | 9 +--------
 3 files changed, 2 insertions(+), 10 deletions(-)

include/linux/swap.h +0 -1

index 84255b3da7c1..cfc200673e13 100644
@@ -331,7 +331,6 @@ extern void mark_page_accessed(struct page *);
 extern void lru_add_drain(void);
 extern void lru_add_drain_cpu(int cpu);
 extern void lru_add_drain_all(void);
-extern void lru_add_drain_all_cpuslocked(void);
 extern void rotate_reclaimable_page(struct page *page);
 extern void deactivate_file_page(struct page *page);
 extern void mark_page_lazyfree(struct page *page);

mm/memory_hotplug.c +1 -1

index 832a042134f8..c9f6b418be79 100644
@@ -1641,7 +1641,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
 		goto failed_removal;
 
 	cond_resched();
-	lru_add_drain_all_cpuslocked();
+	lru_add_drain_all();
 	drain_all_pages(zone);
 
 	pfn = scan_movable_pages(start_pfn, end_pfn);

mm/swap.c +1 -8

index 381e0fe9efbf..8bfdcab9f83e 100644
@@ -688,7 +688,7 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
 
 static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
 
-void lru_add_drain_all_cpuslocked(void)
+void lru_add_drain_all(void)
 {
 	static DEFINE_MUTEX(lock);
 	static struct cpumask has_work;

@@ -724,13 +724,6 @@ void lru_add_drain_all_cpuslocked(void)
 	mutex_unlock(&lock);
 }
 
-void lru_add_drain_all(void)
-{
-	get_online_cpus();
-	lru_add_drain_all_cpuslocked();
-	put_online_cpus();
-}
-
 /**
  * release_pages - batched put_page()
  * @pages: array of pages to release

Signature · 7 lines

2.15.0

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

Thomas Gleixner

2017-11-14 14:13 UTC #1 lore raw

Quoted 37 lines · Michal Hocko

From: Michal Hocko <[email protected]>
Pulling cpu hotplug locks inside the mm core function like lru_add_drain_all just asks for problems and the recent lockdep splat
[1] just proves this. While the usage in that particular case might be wrong we should prevent from locking as lru_add_drain_all is used at many places. It seems that this is not all that hard to achieve actually.
We have done the same thing for drain_all_pages which is analogous by a459eeb7b852 ("mm, page_alloc: do not depend on cpu hotplug locks inside the allocator"). All we have to care about is to handle
- the work item might be executed on a different cpu in worker from unbound pool so it doesn't run on pinned on the cpu
- we have to make sure that we do not race with page_alloc_cpu_dead calling lru_add_drain_cpu
the first part is already handled because the worker calls lru_add_drain which disables preemption when calling lru_add_drain_cpu on the local cpu it is draining. The later is true because page_alloc_cpu_dead is called on the controlling CPU after the hotplugged CPU vanished completely.
[1] http://lkml.kernel.org/r/[email protected]
Signed-off-by Michal Hocko <[email protected]>
---
Hi,
this has been posted as 2 patch series [1] previously. It turned out that the first patch was simply broken and the second one could be simplified because the irq disabling is just pointless. There were no other objections so I am resending this patch which should remove quite a large space of potential lockups as lru_add_drain_all is used at many places so removing the hoptlug locking is a good thing in general.

Quoted 1 line

Can we have this merged or there are still some objections?

No objections. The explanation makes sense, but it might be worth to have a comment at lru_add_drain_all() which explains the protection rules.

Thanks,

tglx

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

Michal Hocko

author

2017-11-14 14:23 UTC #2 lore raw

Quoted 38 lines · Thomas Gleixner

Quoted 37 lines · Michal Hocko
From: Michal Hocko <[email protected]>
Pulling cpu hotplug locks inside the mm core function like lru_add_drain_all just asks for problems and the recent lockdep splat
[1] just proves this. While the usage in that particular case might be wrong we should prevent from locking as lru_add_drain_all is used at many places. It seems that this is not all that hard to achieve actually.
We have done the same thing for drain_all_pages which is analogous by a459eeb7b852 ("mm, page_alloc: do not depend on cpu hotplug locks inside the allocator"). All we have to care about is to handle
- the work item might be executed on a different cpu in worker from unbound pool so it doesn't run on pinned on the cpu
- we have to make sure that we do not race with page_alloc_cpu_dead calling lru_add_drain_cpu
the first part is already handled because the worker calls lru_add_drain which disables preemption when calling lru_add_drain_cpu on the local cpu it is draining. The later is true because page_alloc_cpu_dead is called on the controlling CPU after the hotplugged CPU vanished completely.
[1] http://lkml.kernel.org/r/[email protected]
Signed-off-by Michal Hocko <[email protected]>
---
Hi,
this has been posted as 2 patch series [1] previously. It turned out that the first patch was simply broken and the second one could be simplified because the irq disabling is just pointless. There were no other objections so I am resending this patch which should remove quite a large space of potential lockups as lru_add_drain_all is used at many places so removing the hoptlug locking is a good thing in general.

Quoted 4 lines

Quoted 1 line
Can we have this merged or there are still some objections?
No objections. The explanation makes sense, but it might be worth to have a comment at lru_add_drain_all() which explains the protection rules.

Do you mean wrt. cpu hotplug? Something like

mm/swap.c +5 -0

index 8bfdcab9f83e..fe6d645e8536 100644
@@ -688,6 +688,11 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
 
 static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
 
+/*
+ * Doesn't need any cpu hotplug locking because we do rely on per-cpu
+ * kworkers being shut down before our page_alloc_cpu_dead callback is
+ * executed on the offlined cpu
+ */
 void lru_add_drain_all(void)
 {
 	static DEFINE_MUTEX(lock);

Signature · 8 lines

Michal Hocko
SUSE Labs

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

Michal Hocko

author

2017-11-14 14:32 UTC #3 lore raw

On Tue 14-11-17 15:23:47, Michal Hocko wrote:
[...]

Review

Quoted 8 lines

(quoted diff) +5 -0

+/*
+ * Doesn't need any cpu hotplug locking because we do rely on per-cpu
+ * kworkers being shut down before our page_alloc_cpu_dead callback is
+ * executed on the offlined cpu
+ */
 void lru_add_drain_all(void)
 {
 	static DEFINE_MUTEX(lock);

Ble the part of the comment didn't go through

mm/swap.c +7 -0

index 8bfdcab9f83e..1ab8122d2d0c 100644
@@ -688,6 +688,13 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
 
 static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
 
+/*
+ * Doesn't need any cpu hotplug locking because we do rely on per-cpu
+ * kworkers being shut down before our page_alloc_cpu_dead callback is
+ * executed on the offlined cpu.
+ * Calling this function with cpu hotplug locks held can actually lead
+ * to obscure indirect dependencies via WQ context.
+ */
 void lru_add_drain_all(void)
 {
 	static DEFINE_MUTEX(lock);

Signature · 8 lines

Michal Hocko
SUSE Labs

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

Thomas Gleixner

2017-11-14 15:05 UTC #4 lore raw

Quoted 11 lines · Michal Hocko

On Tue 14-11-17 15:23:47, Michal Hocko wrote:
[...]
mm/swap.c static void lru_add_drain_per_cpu(struct work_struct *dummy) · line 700
Quoted 8 lines
mm/swap.c +5 -0
+/*
+ * Doesn't need any cpu hotplug locking because we do rely on per-cpu
+ * kworkers being shut down before our page_alloc_cpu_dead callback is
+ * executed on the offlined cpu
+ */
 void lru_add_drain_all(void)
 {
 	static DEFINE_MUTEX(lock);

Quoted 1 line

Ble the part of the comment didn't go through

Looks good.

Thanks,

tglx

Quoted 22 lines

mm/swap.c +7 -0

index 8bfdcab9f83e..1ab8122d2d0c 100644
@@ -688,6 +688,13 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
 
 static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
 
+/*
+ * Doesn't need any cpu hotplug locking because we do rely on per-cpu
+ * kworkers being shut down before our page_alloc_cpu_dead callback is
+ * executed on the offlined cpu.
+ * Calling this function with cpu hotplug locks held can actually lead
+ * to obscure indirect dependencies via WQ context.
+ */
 void lru_add_drain_all(void)
 {
 	static DEFINE_MUTEX(lock);

Signature · 2 lines

Michal Hocko
SUSE Labs

Thomas Gleixner

Acked-by

2017-11-14 15:11 UTC #5 lore raw

Quoted 13 lines · Thomas Gleixner

Quoted 11 lines · Michal Hocko

On Tue 14-11-17 15:23:47, Michal Hocko wrote:
[...]

Quoted 8 lines

mm/swap.c +5 -0

+/*
+ * Doesn't need any cpu hotplug locking because we do rely on per-cpu
+ * kworkers being shut down before our page_alloc_cpu_dead callback is
+ * executed on the offlined cpu
+ */
 void lru_add_drain_all(void)
 {
 	static DEFINE_MUTEX(lock);

Quoted 3 lines

Quoted 1 line
Ble the part of the comment didn't go through
Looks good.

With that added, feel free to add:

Acked-by Thomas Gleixner <[email protected]>

Michal Hocko

author

2017-11-14 15:15 UTC #6 lore raw

In reply to Thomas Gleixner

Quoted 13 lines · Thomas Gleixner

Quoted 11 lines · Michal Hocko

On Tue 14-11-17 15:23:47, Michal Hocko wrote:
[...]

Quoted 8 lines

mm/swap.c +5 -0

+/*
+ * Doesn't need any cpu hotplug locking because we do rely on per-cpu
+ * kworkers being shut down before our page_alloc_cpu_dead callback is
+ * executed on the offlined cpu
+ */
 void lru_add_drain_all(void)
 {
 	static DEFINE_MUTEX(lock);

Quoted 3 lines

Quoted 1 line
Ble the part of the comment didn't go through
Looks good.

Thanks! I have folded that to the patch and will wait a day or two for more comments and then resubmit.

Signature · 8 lines

Michal Hocko
SUSE Labs

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