* [PATCH] mm: drop hotplug lock from lru_add_drain_all
@ 2017-11-14 13:53 ` Michal Hocko
0 siblings, 0 replies; 16+ messages in thread
From: Michal Hocko @ 2017-11-14 13:53 UTC (permalink / raw)
To: Andrew Morton
Cc: Tejun Heo, Peter Zijlstra, Thomas Gleixner, Johannes Weiner,
Mel Gorman, linux-mm, LKML, Michal Hocko
From: Michal Hocko <mhocko@suse.com>
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/089e0825eec8955c1f055c83d476@google.com
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
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/20171102093613.3616-1-mhocko@kernel.org
include/linux/swap.h | 1 -
mm/memory_hotplug.c | 2 +-
mm/swap.c | 9 +--------
3 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/include/linux/swap.h b/include/linux/swap.h
index 84255b3da7c1..cfc200673e13 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -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);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 832a042134f8..c9f6b418be79 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -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);
diff --git a/mm/swap.c b/mm/swap.c
index 381e0fe9efbf..8bfdcab9f83e 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -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
--
2.15.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH] mm: drop hotplug lock from lru_add_drain_all
@ 2017-11-14 13:53 ` Michal Hocko
0 siblings, 0 replies; 16+ messages in thread
From: Michal Hocko @ 2017-11-14 13:53 UTC (permalink / raw)
To: Andrew Morton
Cc: Tejun Heo, Peter Zijlstra, Thomas Gleixner, Johannes Weiner,
Mel Gorman, linux-mm, LKML, Michal Hocko
From: Michal Hocko <mhocko@suse.com>
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/089e0825eec8955c1f055c83d476@google.com
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
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/20171102093613.3616-1-mhocko@kernel.org
include/linux/swap.h | 1 -
mm/memory_hotplug.c | 2 +-
mm/swap.c | 9 +--------
3 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/include/linux/swap.h b/include/linux/swap.h
index 84255b3da7c1..cfc200673e13 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -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);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 832a042134f8..c9f6b418be79 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -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);
diff --git a/mm/swap.c b/mm/swap.c
index 381e0fe9efbf..8bfdcab9f83e 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -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
--
2.15.0
--
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>
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: drop hotplug lock from lru_add_drain_all
2017-11-14 13:53 ` Michal Hocko
@ 2017-11-14 14:13 ` Thomas Gleixner
-1 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2017-11-14 14:13 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Tejun Heo, Peter Zijlstra, Johannes Weiner,
Mel Gorman, linux-mm, LKML, Michal Hocko
On Tue, 14 Nov 2017, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> 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/089e0825eec8955c1f055c83d476@google.com
>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> 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?
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: drop hotplug lock from lru_add_drain_all
@ 2017-11-14 14:13 ` Thomas Gleixner
0 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2017-11-14 14:13 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Tejun Heo, Peter Zijlstra, Johannes Weiner,
Mel Gorman, linux-mm, LKML, Michal Hocko
On Tue, 14 Nov 2017, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> 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/089e0825eec8955c1f055c83d476@google.com
>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> 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?
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 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>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: drop hotplug lock from lru_add_drain_all
2017-11-14 14:13 ` Thomas Gleixner
@ 2017-11-14 14:23 ` Michal Hocko
-1 siblings, 0 replies; 16+ messages in thread
From: Michal Hocko @ 2017-11-14 14:23 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Andrew Morton, Tejun Heo, Peter Zijlstra, Johannes Weiner,
Mel Gorman, linux-mm, LKML
On Tue 14-11-17 15:13:27, Thomas Gleixner wrote:
> On Tue, 14 Nov 2017, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> >
> > 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/089e0825eec8955c1f055c83d476@google.com
> >
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > ---
> > 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?
>
> 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
diff --git a/mm/swap.c b/mm/swap.c
index 8bfdcab9f83e..fe6d645e8536 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -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);
--
Michal Hocko
SUSE Labs
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: drop hotplug lock from lru_add_drain_all
@ 2017-11-14 14:23 ` Michal Hocko
0 siblings, 0 replies; 16+ messages in thread
From: Michal Hocko @ 2017-11-14 14:23 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Andrew Morton, Tejun Heo, Peter Zijlstra, Johannes Weiner,
Mel Gorman, linux-mm, LKML
On Tue 14-11-17 15:13:27, Thomas Gleixner wrote:
> On Tue, 14 Nov 2017, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> >
> > 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/089e0825eec8955c1f055c83d476@google.com
> >
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > ---
> > 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?
>
> 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
diff --git a/mm/swap.c b/mm/swap.c
index 8bfdcab9f83e..fe6d645e8536 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -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);
--
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>
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: drop hotplug lock from lru_add_drain_all
2017-11-14 14:23 ` Michal Hocko
@ 2017-11-14 14:32 ` Michal Hocko
-1 siblings, 0 replies; 16+ messages in thread
From: Michal Hocko @ 2017-11-14 14:32 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Andrew Morton, Tejun Heo, Peter Zijlstra, Johannes Weiner,
Mel Gorman, linux-mm, LKML
On Tue 14-11-17 15:23:47, Michal Hocko wrote:
[...]
> +/*
> + * 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
diff --git a/mm/swap.c b/mm/swap.c
index 8bfdcab9f83e..1ab8122d2d0c 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -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);
--
Michal Hocko
SUSE Labs
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: drop hotplug lock from lru_add_drain_all
@ 2017-11-14 14:32 ` Michal Hocko
0 siblings, 0 replies; 16+ messages in thread
From: Michal Hocko @ 2017-11-14 14:32 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Andrew Morton, Tejun Heo, Peter Zijlstra, Johannes Weiner,
Mel Gorman, linux-mm, LKML
On Tue 14-11-17 15:23:47, Michal Hocko wrote:
[...]
> +/*
> + * 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
diff --git a/mm/swap.c b/mm/swap.c
index 8bfdcab9f83e..1ab8122d2d0c 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -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);
--
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>
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: drop hotplug lock from lru_add_drain_all
2017-11-14 14:32 ` Michal Hocko
@ 2017-11-14 15:05 ` Thomas Gleixner
-1 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2017-11-14 15:05 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Tejun Heo, Peter Zijlstra, Johannes Weiner,
Mel Gorman, linux-mm, LKML
On Tue, 14 Nov 2017, Michal Hocko wrote:
> On Tue 14-11-17 15:23:47, Michal Hocko wrote:
> [...]
> > +/*
> > + * 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
Looks good.
Thanks,
tglx
> diff --git a/mm/swap.c b/mm/swap.c
> index 8bfdcab9f83e..1ab8122d2d0c 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -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);
> --
> Michal Hocko
> SUSE Labs
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: drop hotplug lock from lru_add_drain_all
@ 2017-11-14 15:05 ` Thomas Gleixner
0 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2017-11-14 15:05 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Tejun Heo, Peter Zijlstra, Johannes Weiner,
Mel Gorman, linux-mm, LKML
On Tue, 14 Nov 2017, Michal Hocko wrote:
> On Tue 14-11-17 15:23:47, Michal Hocko wrote:
> [...]
> > +/*
> > + * 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
Looks good.
Thanks,
tglx
> diff --git a/mm/swap.c b/mm/swap.c
> index 8bfdcab9f83e..1ab8122d2d0c 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -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);
> --
> 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>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: drop hotplug lock from lru_add_drain_all
2017-11-14 15:05 ` Thomas Gleixner
@ 2017-11-14 15:11 ` Thomas Gleixner
-1 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2017-11-14 15:11 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Tejun Heo, Peter Zijlstra, Johannes Weiner,
Mel Gorman, linux-mm, LKML
On Tue, 14 Nov 2017, Thomas Gleixner wrote:
> On Tue, 14 Nov 2017, Michal Hocko wrote:
>
> > On Tue 14-11-17 15:23:47, Michal Hocko wrote:
> > [...]
> > > +/*
> > > + * 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
>
> Looks good.
With that added, feel free to add:
Acked-by: Thomas Gleixner <tglx@linutronix.de>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: drop hotplug lock from lru_add_drain_all
@ 2017-11-14 15:11 ` Thomas Gleixner
0 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2017-11-14 15:11 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Tejun Heo, Peter Zijlstra, Johannes Weiner,
Mel Gorman, linux-mm, LKML
On Tue, 14 Nov 2017, Thomas Gleixner wrote:
> On Tue, 14 Nov 2017, Michal Hocko wrote:
>
> > On Tue 14-11-17 15:23:47, Michal Hocko wrote:
> > [...]
> > > +/*
> > > + * 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
>
> Looks good.
With that added, feel free to add:
Acked-by: Thomas Gleixner <tglx@linutronix.de>
--
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>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: drop hotplug lock from lru_add_drain_all
2017-11-14 15:05 ` Thomas Gleixner
@ 2017-11-14 15:14 ` Michal Hocko
-1 siblings, 0 replies; 16+ messages in thread
From: Michal Hocko @ 2017-11-14 15:14 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Andrew Morton, Tejun Heo, Peter Zijlstra, Johannes Weiner,
Mel Gorman, linux-mm, LKML
On Tue 14-11-17 16:05:21, Thomas Gleixner wrote:
> On Tue, 14 Nov 2017, Michal Hocko wrote:
>
> > On Tue 14-11-17 15:23:47, Michal Hocko wrote:
> > [...]
> > > +/*
> > > + * 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
>
> Looks good.
Thanks! I have folded that to the patch and will wait a day or two for
more comments and then resubmit.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: drop hotplug lock from lru_add_drain_all
@ 2017-11-14 15:14 ` Michal Hocko
0 siblings, 0 replies; 16+ messages in thread
From: Michal Hocko @ 2017-11-14 15:14 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Andrew Morton, Tejun Heo, Peter Zijlstra, Johannes Weiner,
Mel Gorman, linux-mm, LKML
On Tue 14-11-17 16:05:21, Thomas Gleixner wrote:
> On Tue, 14 Nov 2017, Michal Hocko wrote:
>
> > On Tue 14-11-17 15:23:47, Michal Hocko wrote:
> > [...]
> > > +/*
> > > + * 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
>
> Looks good.
Thanks! I have folded that to the patch and will wait a day or two for
more comments and then resubmit.
--
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>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] mm: drop hotplug lock from lru_add_drain_all
@ 2017-11-16 12:05 ` Michal Hocko
0 siblings, 0 replies; 16+ messages in thread
From: Michal Hocko @ 2017-11-16 12:05 UTC (permalink / raw)
To: Andrew Morton
Cc: Tejun Heo, Peter Zijlstra, Thomas Gleixner, Johannes Weiner,
Mel Gorman, linux-mm, LKML, Michal Hocko
From: Michal Hocko <mhocko@suse.com>
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/089e0825eec8955c1f055c83d476@google.com
[add a cpu hotplug locking interaction as per tglx]
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
include/linux/swap.h | 1 -
mm/memory_hotplug.c | 2 +-
mm/swap.c | 16 ++++++++--------
3 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/include/linux/swap.h b/include/linux/swap.h
index 84255b3da7c1..cfc200673e13 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -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);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 832a042134f8..c9f6b418be79 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -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);
diff --git a/mm/swap.c b/mm/swap.c
index 381e0fe9efbf..1ab8122d2d0c 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -688,7 +688,14 @@ 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)
+/*
+ * 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);
static struct cpumask has_work;
@@ -724,13 +731,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
--
2.15.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH] mm: drop hotplug lock from lru_add_drain_all
@ 2017-11-16 12:05 ` Michal Hocko
0 siblings, 0 replies; 16+ messages in thread
From: Michal Hocko @ 2017-11-16 12:05 UTC (permalink / raw)
To: Andrew Morton
Cc: Tejun Heo, Peter Zijlstra, Thomas Gleixner, Johannes Weiner,
Mel Gorman, linux-mm, LKML, Michal Hocko
From: Michal Hocko <mhocko@suse.com>
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/089e0825eec8955c1f055c83d476@google.com
[add a cpu hotplug locking interaction as per tglx]
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
include/linux/swap.h | 1 -
mm/memory_hotplug.c | 2 +-
mm/swap.c | 16 ++++++++--------
3 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/include/linux/swap.h b/include/linux/swap.h
index 84255b3da7c1..cfc200673e13 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -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);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 832a042134f8..c9f6b418be79 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -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);
diff --git a/mm/swap.c b/mm/swap.c
index 381e0fe9efbf..1ab8122d2d0c 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -688,7 +688,14 @@ 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)
+/*
+ * 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);
static struct cpumask has_work;
@@ -724,13 +731,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
--
2.15.0
--
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>
^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-11-16 12:05 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-14 13:53 [PATCH] mm: drop hotplug lock from lru_add_drain_all Michal Hocko
2017-11-14 13:53 ` Michal Hocko
2017-11-14 14:13 ` Thomas Gleixner
2017-11-14 14:13 ` Thomas Gleixner
2017-11-14 14:23 ` Michal Hocko
2017-11-14 14:23 ` Michal Hocko
2017-11-14 14:32 ` Michal Hocko
2017-11-14 14:32 ` Michal Hocko
2017-11-14 15:05 ` Thomas Gleixner
2017-11-14 15:05 ` Thomas Gleixner
2017-11-14 15:11 ` Thomas Gleixner
2017-11-14 15:11 ` Thomas Gleixner
2017-11-14 15:14 ` Michal Hocko
2017-11-14 15:14 ` Michal Hocko
2017-11-16 12:05 Michal Hocko
2017-11-16 12:05 ` Michal Hocko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.