* [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
@ 2011-02-25 20:04 ` Mel Gorman
0 siblings, 0 replies; 46+ messages in thread
From: Mel Gorman @ 2011-02-25 20:04 UTC (permalink / raw)
To: Andrew Morton
Cc: Arthur Marsh, Clemens Ladisch, Andrea Arcangeli, Mel Gorman,
Linux-MM, Linux Kernel Mailing List
From: Andrea Arcangeli <aarcange@redhat.com>
compaction_alloc() isolates pages for migration in isolate_migratepages. While
it's scanning, IRQs are disabled on the mistaken assumption the scanning
should be short. Tests show this to be true for the most part but
contention times on the LRU lock can be increased. Before this patch,
the IRQ disabled times for a simple test looked like
Total sampled time IRQs off (not real total time): 5493
Event shrink_inactive_list..shrink_zone 1596 us count 1
Event shrink_inactive_list..shrink_zone 1530 us count 1
Event shrink_inactive_list..shrink_zone 956 us count 1
Event shrink_inactive_list..shrink_zone 541 us count 1
Event shrink_inactive_list..shrink_zone 531 us count 1
Event split_huge_page..add_to_swap 232 us count 1
Event save_args..call_softirq 36 us count 1
Event save_args..call_softirq 35 us count 2
Event __wake_up..__wake_up 1 us count 1
This patch reduces the worst-case IRQs-disabled latencies by releasing the
lock every SWAP_CLUSTER_MAX pages that are scanned and releasing the CPU if
necessary. The cost of this is that the processing performing compaction will
be slower but IRQs being disabled for too long a time has worse consequences
as the following report shows;
Total sampled time IRQs off (not real total time): 4367
Event shrink_inactive_list..shrink_zone 881 us count 1
Event shrink_inactive_list..shrink_zone 875 us count 1
Event shrink_inactive_list..shrink_zone 868 us count 1
Event shrink_inactive_list..shrink_zone 555 us count 1
Event split_huge_page..add_to_swap 495 us count 1
Event compact_zone..compact_zone_order 269 us count 1
Event split_huge_page..add_to_swap 266 us count 1
Event shrink_inactive_list..shrink_zone 85 us count 1
Event save_args..call_softirq 36 us count 2
Event __wake_up..__wake_up 1 us count 1
Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
Signed-off-by: Mel Gorman <mel@csn.ul.ie>
---
mm/compaction.c | 18 ++++++++++++++++++
1 files changed, 18 insertions(+), 0 deletions(-)
diff --git a/mm/compaction.c b/mm/compaction.c
index 11d88a2..ec9eb0f 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -279,9 +279,27 @@ static unsigned long isolate_migratepages(struct zone *zone,
}
/* Time to isolate some pages for migration */
+ cond_resched();
spin_lock_irq(&zone->lru_lock);
for (; low_pfn < end_pfn; low_pfn++) {
struct page *page;
+ bool unlocked = false;
+
+ /* give a chance to irqs before checking need_resched() */
+ if (!((low_pfn+1) % SWAP_CLUSTER_MAX)) {
+ spin_unlock_irq(&zone->lru_lock);
+ unlocked = true;
+ }
+ if (need_resched() || spin_is_contended(&zone->lru_lock)) {
+ if (!unlocked)
+ spin_unlock_irq(&zone->lru_lock);
+ cond_resched();
+ spin_lock_irq(&zone->lru_lock);
+ if (fatal_signal_pending(current))
+ break;
+ } else if (unlocked)
+ spin_lock_irq(&zone->lru_lock);
+
if (!pfn_valid_within(low_pfn))
continue;
nr_scanned++;
--
1.7.2.3
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
2011-02-25 20:04 ` Mel Gorman
@ 2011-02-25 22:32 ` Johannes Weiner
-1 siblings, 0 replies; 46+ messages in thread
From: Johannes Weiner @ 2011-02-25 22:32 UTC (permalink / raw)
To: Mel Gorman
Cc: Andrew Morton, Arthur Marsh, Clemens Ladisch, Andrea Arcangeli,
Linux-MM, Linux Kernel Mailing List
On Fri, Feb 25, 2011 at 08:04:59PM +0000, Mel Gorman wrote:
> From: Andrea Arcangeli <aarcange@redhat.com>
>
> compaction_alloc() isolates pages for migration in isolate_migratepages. While
> it's scanning, IRQs are disabled on the mistaken assumption the scanning
> should be short. Tests show this to be true for the most part but
> contention times on the LRU lock can be increased. Before this patch,
> the IRQ disabled times for a simple test looked like
>
> Total sampled time IRQs off (not real total time): 5493
> Event shrink_inactive_list..shrink_zone 1596 us count 1
> Event shrink_inactive_list..shrink_zone 1530 us count 1
> Event shrink_inactive_list..shrink_zone 956 us count 1
> Event shrink_inactive_list..shrink_zone 541 us count 1
> Event shrink_inactive_list..shrink_zone 531 us count 1
> Event split_huge_page..add_to_swap 232 us count 1
> Event save_args..call_softirq 36 us count 1
> Event save_args..call_softirq 35 us count 2
> Event __wake_up..__wake_up 1 us count 1
>
> This patch reduces the worst-case IRQs-disabled latencies by releasing the
> lock every SWAP_CLUSTER_MAX pages that are scanned and releasing the CPU if
> necessary. The cost of this is that the processing performing compaction will
> be slower but IRQs being disabled for too long a time has worse consequences
> as the following report shows;
>
> Total sampled time IRQs off (not real total time): 4367
> Event shrink_inactive_list..shrink_zone 881 us count 1
> Event shrink_inactive_list..shrink_zone 875 us count 1
> Event shrink_inactive_list..shrink_zone 868 us count 1
> Event shrink_inactive_list..shrink_zone 555 us count 1
> Event split_huge_page..add_to_swap 495 us count 1
> Event compact_zone..compact_zone_order 269 us count 1
> Event split_huge_page..add_to_swap 266 us count 1
> Event shrink_inactive_list..shrink_zone 85 us count 1
> Event save_args..call_softirq 36 us count 2
> Event __wake_up..__wake_up 1 us count 1
>
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> ---
> mm/compaction.c | 18 ++++++++++++++++++
> 1 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 11d88a2..ec9eb0f 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -279,9 +279,27 @@ static unsigned long isolate_migratepages(struct zone *zone,
> }
>
> /* Time to isolate some pages for migration */
> + cond_resched();
> spin_lock_irq(&zone->lru_lock);
> for (; low_pfn < end_pfn; low_pfn++) {
> struct page *page;
> + bool unlocked = false;
> +
> + /* give a chance to irqs before checking need_resched() */
> + if (!((low_pfn+1) % SWAP_CLUSTER_MAX)) {
> + spin_unlock_irq(&zone->lru_lock);
> + unlocked = true;
> + }
> + if (need_resched() || spin_is_contended(&zone->lru_lock)) {
> + if (!unlocked)
> + spin_unlock_irq(&zone->lru_lock);
> + cond_resched();
> + spin_lock_irq(&zone->lru_lock);
> + if (fatal_signal_pending(current))
> + break;
> + } else if (unlocked)
> + spin_lock_irq(&zone->lru_lock);
> +
I don't understand why this conditional is broken up like this.
cond_resched() will have the right checks anyway. Okay, you would
save fatal_signal_pending() in the 'did one cluster' case. Is it that
expensive? Couldn't this be simpler like
did_cluster = ((low_pfn + 1) % SWAP_CLUSTER_MAX) == 0
lock_contended = spin_is_contended(&zone->lru_lock);
if (did_cluster || lock_contended || need_resched()) {
spin_unlock_irq(&zone->lru_lock);
cond_resched();
spin_lock_irq(&zone->lru_lock);
if (fatal_signal_pending(current))
break;
}
instead?
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
@ 2011-02-25 22:32 ` Johannes Weiner
0 siblings, 0 replies; 46+ messages in thread
From: Johannes Weiner @ 2011-02-25 22:32 UTC (permalink / raw)
To: Mel Gorman
Cc: Andrew Morton, Arthur Marsh, Clemens Ladisch, Andrea Arcangeli,
Linux-MM, Linux Kernel Mailing List
On Fri, Feb 25, 2011 at 08:04:59PM +0000, Mel Gorman wrote:
> From: Andrea Arcangeli <aarcange@redhat.com>
>
> compaction_alloc() isolates pages for migration in isolate_migratepages. While
> it's scanning, IRQs are disabled on the mistaken assumption the scanning
> should be short. Tests show this to be true for the most part but
> contention times on the LRU lock can be increased. Before this patch,
> the IRQ disabled times for a simple test looked like
>
> Total sampled time IRQs off (not real total time): 5493
> Event shrink_inactive_list..shrink_zone 1596 us count 1
> Event shrink_inactive_list..shrink_zone 1530 us count 1
> Event shrink_inactive_list..shrink_zone 956 us count 1
> Event shrink_inactive_list..shrink_zone 541 us count 1
> Event shrink_inactive_list..shrink_zone 531 us count 1
> Event split_huge_page..add_to_swap 232 us count 1
> Event save_args..call_softirq 36 us count 1
> Event save_args..call_softirq 35 us count 2
> Event __wake_up..__wake_up 1 us count 1
>
> This patch reduces the worst-case IRQs-disabled latencies by releasing the
> lock every SWAP_CLUSTER_MAX pages that are scanned and releasing the CPU if
> necessary. The cost of this is that the processing performing compaction will
> be slower but IRQs being disabled for too long a time has worse consequences
> as the following report shows;
>
> Total sampled time IRQs off (not real total time): 4367
> Event shrink_inactive_list..shrink_zone 881 us count 1
> Event shrink_inactive_list..shrink_zone 875 us count 1
> Event shrink_inactive_list..shrink_zone 868 us count 1
> Event shrink_inactive_list..shrink_zone 555 us count 1
> Event split_huge_page..add_to_swap 495 us count 1
> Event compact_zone..compact_zone_order 269 us count 1
> Event split_huge_page..add_to_swap 266 us count 1
> Event shrink_inactive_list..shrink_zone 85 us count 1
> Event save_args..call_softirq 36 us count 2
> Event __wake_up..__wake_up 1 us count 1
>
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> ---
> mm/compaction.c | 18 ++++++++++++++++++
> 1 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 11d88a2..ec9eb0f 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -279,9 +279,27 @@ static unsigned long isolate_migratepages(struct zone *zone,
> }
>
> /* Time to isolate some pages for migration */
> + cond_resched();
> spin_lock_irq(&zone->lru_lock);
> for (; low_pfn < end_pfn; low_pfn++) {
> struct page *page;
> + bool unlocked = false;
> +
> + /* give a chance to irqs before checking need_resched() */
> + if (!((low_pfn+1) % SWAP_CLUSTER_MAX)) {
> + spin_unlock_irq(&zone->lru_lock);
> + unlocked = true;
> + }
> + if (need_resched() || spin_is_contended(&zone->lru_lock)) {
> + if (!unlocked)
> + spin_unlock_irq(&zone->lru_lock);
> + cond_resched();
> + spin_lock_irq(&zone->lru_lock);
> + if (fatal_signal_pending(current))
> + break;
> + } else if (unlocked)
> + spin_lock_irq(&zone->lru_lock);
> +
I don't understand why this conditional is broken up like this.
cond_resched() will have the right checks anyway. Okay, you would
save fatal_signal_pending() in the 'did one cluster' case. Is it that
expensive? Couldn't this be simpler like
did_cluster = ((low_pfn + 1) % SWAP_CLUSTER_MAX) == 0
lock_contended = spin_is_contended(&zone->lru_lock);
if (did_cluster || lock_contended || need_resched()) {
spin_unlock_irq(&zone->lru_lock);
cond_resched();
spin_lock_irq(&zone->lru_lock);
if (fatal_signal_pending(current))
break;
}
instead?
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
2011-02-25 22:32 ` Johannes Weiner
@ 2011-02-26 0:16 ` Andrea Arcangeli
-1 siblings, 0 replies; 46+ messages in thread
From: Andrea Arcangeli @ 2011-02-26 0:16 UTC (permalink / raw)
To: Johannes Weiner
Cc: Mel Gorman, Andrew Morton, Arthur Marsh, Clemens Ladisch,
Linux-MM, Linux Kernel Mailing List
On Fri, Feb 25, 2011 at 11:32:04PM +0100, Johannes Weiner wrote:
> I don't understand why this conditional is broken up like this.
> cond_resched() will have the right checks anyway. Okay, you would
> save fatal_signal_pending() in the 'did one cluster' case. Is it that
> expensive? Couldn't this be simpler like
>
> did_cluster = ((low_pfn + 1) % SWAP_CLUSTER_MAX) == 0
> lock_contended = spin_is_contended(&zone->lru_lock);
> if (did_cluster || lock_contended || need_resched()) {
> spin_unlock_irq(&zone->lru_lock);
> cond_resched();
> spin_lock_irq(&zone->lru_lock);
> if (fatal_signal_pending(current))
> break;
> }
>
> instead?
If we don't release irqs first, how can need_resched get set in the
first place if the local apic irq can't run? I guess that's why
there's no cond_resched_lock_irq. BTW, I never liked too much clearing
interrupts for locks that can't ever be taken from irq (it's a
scalability boost to reduce contention but it makes things like above
confusing and it increases irq latency a bit.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
@ 2011-02-26 0:16 ` Andrea Arcangeli
0 siblings, 0 replies; 46+ messages in thread
From: Andrea Arcangeli @ 2011-02-26 0:16 UTC (permalink / raw)
To: Johannes Weiner
Cc: Mel Gorman, Andrew Morton, Arthur Marsh, Clemens Ladisch,
Linux-MM, Linux Kernel Mailing List
On Fri, Feb 25, 2011 at 11:32:04PM +0100, Johannes Weiner wrote:
> I don't understand why this conditional is broken up like this.
> cond_resched() will have the right checks anyway. Okay, you would
> save fatal_signal_pending() in the 'did one cluster' case. Is it that
> expensive? Couldn't this be simpler like
>
> did_cluster = ((low_pfn + 1) % SWAP_CLUSTER_MAX) == 0
> lock_contended = spin_is_contended(&zone->lru_lock);
> if (did_cluster || lock_contended || need_resched()) {
> spin_unlock_irq(&zone->lru_lock);
> cond_resched();
> spin_lock_irq(&zone->lru_lock);
> if (fatal_signal_pending(current))
> break;
> }
>
> instead?
If we don't release irqs first, how can need_resched get set in the
first place if the local apic irq can't run? I guess that's why
there's no cond_resched_lock_irq. BTW, I never liked too much clearing
interrupts for locks that can't ever be taken from irq (it's a
scalability boost to reduce contention but it makes things like above
confusing and it increases irq latency a bit.
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
2011-02-25 20:04 ` Mel Gorman
@ 2011-02-28 2:17 ` KAMEZAWA Hiroyuki
-1 siblings, 0 replies; 46+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-02-28 2:17 UTC (permalink / raw)
To: Mel Gorman
Cc: Andrew Morton, Arthur Marsh, Clemens Ladisch, Andrea Arcangeli,
Linux-MM, Linux Kernel Mailing List
On Fri, 25 Feb 2011 20:04:59 +0000
Mel Gorman <mel@csn.ul.ie> wrote:
> From: Andrea Arcangeli <aarcange@redhat.com>
>
> compaction_alloc() isolates pages for migration in isolate_migratepages. While
> it's scanning, IRQs are disabled on the mistaken assumption the scanning
> should be short. Tests show this to be true for the most part but
> contention times on the LRU lock can be increased. Before this patch,
> the IRQ disabled times for a simple test looked like
>
> Total sampled time IRQs off (not real total time): 5493
> Event shrink_inactive_list..shrink_zone 1596 us count 1
> Event shrink_inactive_list..shrink_zone 1530 us count 1
> Event shrink_inactive_list..shrink_zone 956 us count 1
> Event shrink_inactive_list..shrink_zone 541 us count 1
> Event shrink_inactive_list..shrink_zone 531 us count 1
> Event split_huge_page..add_to_swap 232 us count 1
> Event save_args..call_softirq 36 us count 1
> Event save_args..call_softirq 35 us count 2
> Event __wake_up..__wake_up 1 us count 1
>
> This patch reduces the worst-case IRQs-disabled latencies by releasing the
> lock every SWAP_CLUSTER_MAX pages that are scanned and releasing the CPU if
> necessary. The cost of this is that the processing performing compaction will
> be slower but IRQs being disabled for too long a time has worse consequences
> as the following report shows;
>
> Total sampled time IRQs off (not real total time): 4367
> Event shrink_inactive_list..shrink_zone 881 us count 1
> Event shrink_inactive_list..shrink_zone 875 us count 1
> Event shrink_inactive_list..shrink_zone 868 us count 1
> Event shrink_inactive_list..shrink_zone 555 us count 1
> Event split_huge_page..add_to_swap 495 us count 1
> Event compact_zone..compact_zone_order 269 us count 1
> Event split_huge_page..add_to_swap 266 us count 1
> Event shrink_inactive_list..shrink_zone 85 us count 1
> Event save_args..call_softirq 36 us count 2
> Event __wake_up..__wake_up 1 us count 1
>
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> ---
> mm/compaction.c | 18 ++++++++++++++++++
> 1 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 11d88a2..ec9eb0f 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -279,9 +279,27 @@ static unsigned long isolate_migratepages(struct zone *zone,
> }
>
> /* Time to isolate some pages for migration */
> + cond_resched();
> spin_lock_irq(&zone->lru_lock);
> for (; low_pfn < end_pfn; low_pfn++) {
> struct page *page;
> + bool unlocked = false;
> +
> + /* give a chance to irqs before checking need_resched() */
> + if (!((low_pfn+1) % SWAP_CLUSTER_MAX)) {
> + spin_unlock_irq(&zone->lru_lock);
> + unlocked = true;
> + }
> + if (need_resched() || spin_is_contended(&zone->lru_lock)) {
> + if (!unlocked)
> + spin_unlock_irq(&zone->lru_lock);
> + cond_resched();
> + spin_lock_irq(&zone->lru_lock);
> + if (fatal_signal_pending(current))
> + break;
> + } else if (unlocked)
> + spin_lock_irq(&zone->lru_lock);
> +
> if (!pfn_valid_within(low_pfn))
> continue;
> nr_scanned++;
Hmm.... I don't like this kind of complicated locks and 'give-a-chance' logic.
BTW, I forget why we always take zone->lru_lock with IRQ disabled....
rotate_lru_page() is a bad thing ?
If so, I think it can be implemented in other way...
Thanks,
-Kame
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
@ 2011-02-28 2:17 ` KAMEZAWA Hiroyuki
0 siblings, 0 replies; 46+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-02-28 2:17 UTC (permalink / raw)
To: Mel Gorman
Cc: Andrew Morton, Arthur Marsh, Clemens Ladisch, Andrea Arcangeli,
Linux-MM, Linux Kernel Mailing List
On Fri, 25 Feb 2011 20:04:59 +0000
Mel Gorman <mel@csn.ul.ie> wrote:
> From: Andrea Arcangeli <aarcange@redhat.com>
>
> compaction_alloc() isolates pages for migration in isolate_migratepages. While
> it's scanning, IRQs are disabled on the mistaken assumption the scanning
> should be short. Tests show this to be true for the most part but
> contention times on the LRU lock can be increased. Before this patch,
> the IRQ disabled times for a simple test looked like
>
> Total sampled time IRQs off (not real total time): 5493
> Event shrink_inactive_list..shrink_zone 1596 us count 1
> Event shrink_inactive_list..shrink_zone 1530 us count 1
> Event shrink_inactive_list..shrink_zone 956 us count 1
> Event shrink_inactive_list..shrink_zone 541 us count 1
> Event shrink_inactive_list..shrink_zone 531 us count 1
> Event split_huge_page..add_to_swap 232 us count 1
> Event save_args..call_softirq 36 us count 1
> Event save_args..call_softirq 35 us count 2
> Event __wake_up..__wake_up 1 us count 1
>
> This patch reduces the worst-case IRQs-disabled latencies by releasing the
> lock every SWAP_CLUSTER_MAX pages that are scanned and releasing the CPU if
> necessary. The cost of this is that the processing performing compaction will
> be slower but IRQs being disabled for too long a time has worse consequences
> as the following report shows;
>
> Total sampled time IRQs off (not real total time): 4367
> Event shrink_inactive_list..shrink_zone 881 us count 1
> Event shrink_inactive_list..shrink_zone 875 us count 1
> Event shrink_inactive_list..shrink_zone 868 us count 1
> Event shrink_inactive_list..shrink_zone 555 us count 1
> Event split_huge_page..add_to_swap 495 us count 1
> Event compact_zone..compact_zone_order 269 us count 1
> Event split_huge_page..add_to_swap 266 us count 1
> Event shrink_inactive_list..shrink_zone 85 us count 1
> Event save_args..call_softirq 36 us count 2
> Event __wake_up..__wake_up 1 us count 1
>
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> ---
> mm/compaction.c | 18 ++++++++++++++++++
> 1 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 11d88a2..ec9eb0f 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -279,9 +279,27 @@ static unsigned long isolate_migratepages(struct zone *zone,
> }
>
> /* Time to isolate some pages for migration */
> + cond_resched();
> spin_lock_irq(&zone->lru_lock);
> for (; low_pfn < end_pfn; low_pfn++) {
> struct page *page;
> + bool unlocked = false;
> +
> + /* give a chance to irqs before checking need_resched() */
> + if (!((low_pfn+1) % SWAP_CLUSTER_MAX)) {
> + spin_unlock_irq(&zone->lru_lock);
> + unlocked = true;
> + }
> + if (need_resched() || spin_is_contended(&zone->lru_lock)) {
> + if (!unlocked)
> + spin_unlock_irq(&zone->lru_lock);
> + cond_resched();
> + spin_lock_irq(&zone->lru_lock);
> + if (fatal_signal_pending(current))
> + break;
> + } else if (unlocked)
> + spin_lock_irq(&zone->lru_lock);
> +
> if (!pfn_valid_within(low_pfn))
> continue;
> nr_scanned++;
Hmm.... I don't like this kind of complicated locks and 'give-a-chance' logic.
BTW, I forget why we always take zone->lru_lock with IRQ disabled....
rotate_lru_page() is a bad thing ?
If so, I think it can be implemented in other way...
Thanks,
-Kame
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
2011-02-28 2:17 ` KAMEZAWA Hiroyuki
@ 2011-02-28 5:48 ` Andrea Arcangeli
-1 siblings, 0 replies; 46+ messages in thread
From: Andrea Arcangeli @ 2011-02-28 5:48 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: Mel Gorman, Andrew Morton, Arthur Marsh, Clemens Ladisch,
Linux-MM, Linux Kernel Mailing List
On Mon, Feb 28, 2011 at 11:17:46AM +0900, KAMEZAWA Hiroyuki wrote:
> BTW, I forget why we always take zone->lru_lock with IRQ disabled....
To decrease lock contention in SMP to deliver overall better
performance (not sure how much it helps though). It was supposed to be
hold for a very short time (PAGEVEC_SIZE) to avoid giving irq latency
problems.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
@ 2011-02-28 5:48 ` Andrea Arcangeli
0 siblings, 0 replies; 46+ messages in thread
From: Andrea Arcangeli @ 2011-02-28 5:48 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: Mel Gorman, Andrew Morton, Arthur Marsh, Clemens Ladisch,
Linux-MM, Linux Kernel Mailing List
On Mon, Feb 28, 2011 at 11:17:46AM +0900, KAMEZAWA Hiroyuki wrote:
> BTW, I forget why we always take zone->lru_lock with IRQ disabled....
To decrease lock contention in SMP to deliver overall better
performance (not sure how much it helps though). It was supposed to be
hold for a very short time (PAGEVEC_SIZE) to avoid giving irq latency
problems.
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
2011-02-28 5:48 ` Andrea Arcangeli
@ 2011-02-28 5:54 ` KAMEZAWA Hiroyuki
-1 siblings, 0 replies; 46+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-02-28 5:54 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Mel Gorman, Andrew Morton, Arthur Marsh, Clemens Ladisch,
Linux-MM, Linux Kernel Mailing List
On Mon, 28 Feb 2011 06:48:18 +0100
Andrea Arcangeli <aarcange@redhat.com> wrote:
> On Mon, Feb 28, 2011 at 11:17:46AM +0900, KAMEZAWA Hiroyuki wrote:
> > BTW, I forget why we always take zone->lru_lock with IRQ disabled....
>
> To decrease lock contention in SMP to deliver overall better
> performance (not sure how much it helps though). It was supposed to be
> hold for a very short time (PAGEVEC_SIZE) to avoid giving irq latency
> problems.
>
memory hotplug uses MIGRATE_ISOLATED migrate types for scanning pfn range
without lru_lock. I wonder whether we can make use of it (the function
which memory hotplug may need rework for the compaction but migrate_type can
be used, I think).
Hmm.
-Kame
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
@ 2011-02-28 5:54 ` KAMEZAWA Hiroyuki
0 siblings, 0 replies; 46+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-02-28 5:54 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Mel Gorman, Andrew Morton, Arthur Marsh, Clemens Ladisch,
Linux-MM, Linux Kernel Mailing List
On Mon, 28 Feb 2011 06:48:18 +0100
Andrea Arcangeli <aarcange@redhat.com> wrote:
> On Mon, Feb 28, 2011 at 11:17:46AM +0900, KAMEZAWA Hiroyuki wrote:
> > BTW, I forget why we always take zone->lru_lock with IRQ disabled....
>
> To decrease lock contention in SMP to deliver overall better
> performance (not sure how much it helps though). It was supposed to be
> hold for a very short time (PAGEVEC_SIZE) to avoid giving irq latency
> problems.
>
memory hotplug uses MIGRATE_ISOLATED migrate types for scanning pfn range
without lru_lock. I wonder whether we can make use of it (the function
which memory hotplug may need rework for the compaction but migrate_type can
be used, I think).
Hmm.
-Kame
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
2011-02-28 5:54 ` KAMEZAWA Hiroyuki
@ 2011-02-28 9:28 ` Mel Gorman
-1 siblings, 0 replies; 46+ messages in thread
From: Mel Gorman @ 2011-02-28 9:28 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: Andrea Arcangeli, Andrew Morton, Arthur Marsh, Clemens Ladisch,
Linux-MM, Linux Kernel Mailing List
On Mon, Feb 28, 2011 at 02:54:02PM +0900, KAMEZAWA Hiroyuki wrote:
> On Mon, 28 Feb 2011 06:48:18 +0100
> Andrea Arcangeli <aarcange@redhat.com> wrote:
>
> > On Mon, Feb 28, 2011 at 11:17:46AM +0900, KAMEZAWA Hiroyuki wrote:
> > > BTW, I forget why we always take zone->lru_lock with IRQ disabled....
> >
> > To decrease lock contention in SMP to deliver overall better
> > performance (not sure how much it helps though). It was supposed to be
> > hold for a very short time (PAGEVEC_SIZE) to avoid giving irq latency
> > problems.
> >
>
> memory hotplug uses MIGRATE_ISOLATED migrate types for scanning pfn range
> without lru_lock. I wonder whether we can make use of it (the function
> which memory hotplug may need rework for the compaction but migrate_type can
> be used, I think).
>
I don't see how migrate_type would be of any benefit here particularly
as compaction does not directly affect the migratetype of a pageblock. I
have not checked closely which part of hotplug you are on about but if
you're talking about when pages actually get offlined, the zone lock is
not necessary there because the pages are not on the LRU. In compactions
case, they are. Did I misunderstand?
That said, a certain about of lockless scanning could be done here if the lock
hold times were shown to be still high. Specifically, scan until an LRU page
is found, take the lock and hold the lock for a maximum of SWAP_CLUSTER_MAX
scanned pages before releasing again. I don't think it would be a massive
improvement though.
--
Mel Gorman
SUSE Labs
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
@ 2011-02-28 9:28 ` Mel Gorman
0 siblings, 0 replies; 46+ messages in thread
From: Mel Gorman @ 2011-02-28 9:28 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: Andrea Arcangeli, Andrew Morton, Arthur Marsh, Clemens Ladisch,
Linux-MM, Linux Kernel Mailing List
On Mon, Feb 28, 2011 at 02:54:02PM +0900, KAMEZAWA Hiroyuki wrote:
> On Mon, 28 Feb 2011 06:48:18 +0100
> Andrea Arcangeli <aarcange@redhat.com> wrote:
>
> > On Mon, Feb 28, 2011 at 11:17:46AM +0900, KAMEZAWA Hiroyuki wrote:
> > > BTW, I forget why we always take zone->lru_lock with IRQ disabled....
> >
> > To decrease lock contention in SMP to deliver overall better
> > performance (not sure how much it helps though). It was supposed to be
> > hold for a very short time (PAGEVEC_SIZE) to avoid giving irq latency
> > problems.
> >
>
> memory hotplug uses MIGRATE_ISOLATED migrate types for scanning pfn range
> without lru_lock. I wonder whether we can make use of it (the function
> which memory hotplug may need rework for the compaction but migrate_type can
> be used, I think).
>
I don't see how migrate_type would be of any benefit here particularly
as compaction does not directly affect the migratetype of a pageblock. I
have not checked closely which part of hotplug you are on about but if
you're talking about when pages actually get offlined, the zone lock is
not necessary there because the pages are not on the LRU. In compactions
case, they are. Did I misunderstand?
That said, a certain about of lockless scanning could be done here if the lock
hold times were shown to be still high. Specifically, scan until an LRU page
is found, take the lock and hold the lock for a maximum of SWAP_CLUSTER_MAX
scanned pages before releasing again. I don't think it would be a massive
improvement though.
--
Mel Gorman
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
2011-02-28 9:28 ` Mel Gorman
@ 2011-02-28 9:42 ` KAMEZAWA Hiroyuki
-1 siblings, 0 replies; 46+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-02-28 9:42 UTC (permalink / raw)
To: Mel Gorman
Cc: Andrea Arcangeli, Andrew Morton, Arthur Marsh, Clemens Ladisch,
Linux-MM, Linux Kernel Mailing List
On Mon, 28 Feb 2011 09:28:14 +0000
Mel Gorman <mel@csn.ul.ie> wrote:
> On Mon, Feb 28, 2011 at 02:54:02PM +0900, KAMEZAWA Hiroyuki wrote:
> > On Mon, 28 Feb 2011 06:48:18 +0100
> > Andrea Arcangeli <aarcange@redhat.com> wrote:
> >
> > > On Mon, Feb 28, 2011 at 11:17:46AM +0900, KAMEZAWA Hiroyuki wrote:
> > > > BTW, I forget why we always take zone->lru_lock with IRQ disabled....
> > >
> > > To decrease lock contention in SMP to deliver overall better
> > > performance (not sure how much it helps though). It was supposed to be
> > > hold for a very short time (PAGEVEC_SIZE) to avoid giving irq latency
> > > problems.
> > >
> >
> > memory hotplug uses MIGRATE_ISOLATED migrate types for scanning pfn range
> > without lru_lock. I wonder whether we can make use of it (the function
> > which memory hotplug may need rework for the compaction but migrate_type can
> > be used, I think).
> >
>
> I don't see how migrate_type would be of any benefit here particularly
> as compaction does not directly affect the migratetype of a pageblock. I
> have not checked closely which part of hotplug you are on about but if
> you're talking about when pages actually get offlined, the zone lock is
> not necessary there because the pages are not on the LRU. In compactions
> case, they are. Did I misunderstand?
>
memory offline code doesn't take big lru_lock (and call isolate_lru_page())
at picking up migration target pages from LRU. While this, allocation from
the zone is allowed. memory offline is done by mem_section unit.
memory offline does.
1. making a whole section as MIGRATETYPE_ISOLATED.
2. scan pfn within section.
3. find a page on LRU
4. isolate_lru_page() -> take/release lru_lock. ----(*)
5. migrate it.
6. making all pages in the range as RESERVED.
During this, by marking the pageblock as MIGRATETYPE_ISOLATED,
- new allocation will never picks up a page in the range.
- newly freed pages in the range will never be allocated and never in pcp.
- page type of the range will never change.
then, memory offline success.
If (*) seems too heavy anyway and will be no help even if with some batching
as isolate_lru_page_pagevec() or some, okay please forget offlining.
BTW, can't we drop disable_irq() from all lru_lock related codes ?
Thanks,
-Kame
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
@ 2011-02-28 9:42 ` KAMEZAWA Hiroyuki
0 siblings, 0 replies; 46+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-02-28 9:42 UTC (permalink / raw)
To: Mel Gorman
Cc: Andrea Arcangeli, Andrew Morton, Arthur Marsh, Clemens Ladisch,
Linux-MM, Linux Kernel Mailing List
On Mon, 28 Feb 2011 09:28:14 +0000
Mel Gorman <mel@csn.ul.ie> wrote:
> On Mon, Feb 28, 2011 at 02:54:02PM +0900, KAMEZAWA Hiroyuki wrote:
> > On Mon, 28 Feb 2011 06:48:18 +0100
> > Andrea Arcangeli <aarcange@redhat.com> wrote:
> >
> > > On Mon, Feb 28, 2011 at 11:17:46AM +0900, KAMEZAWA Hiroyuki wrote:
> > > > BTW, I forget why we always take zone->lru_lock with IRQ disabled....
> > >
> > > To decrease lock contention in SMP to deliver overall better
> > > performance (not sure how much it helps though). It was supposed to be
> > > hold for a very short time (PAGEVEC_SIZE) to avoid giving irq latency
> > > problems.
> > >
> >
> > memory hotplug uses MIGRATE_ISOLATED migrate types for scanning pfn range
> > without lru_lock. I wonder whether we can make use of it (the function
> > which memory hotplug may need rework for the compaction but migrate_type can
> > be used, I think).
> >
>
> I don't see how migrate_type would be of any benefit here particularly
> as compaction does not directly affect the migratetype of a pageblock. I
> have not checked closely which part of hotplug you are on about but if
> you're talking about when pages actually get offlined, the zone lock is
> not necessary there because the pages are not on the LRU. In compactions
> case, they are. Did I misunderstand?
>
memory offline code doesn't take big lru_lock (and call isolate_lru_page())
at picking up migration target pages from LRU. While this, allocation from
the zone is allowed. memory offline is done by mem_section unit.
memory offline does.
1. making a whole section as MIGRATETYPE_ISOLATED.
2. scan pfn within section.
3. find a page on LRU
4. isolate_lru_page() -> take/release lru_lock. ----(*)
5. migrate it.
6. making all pages in the range as RESERVED.
During this, by marking the pageblock as MIGRATETYPE_ISOLATED,
- new allocation will never picks up a page in the range.
- newly freed pages in the range will never be allocated and never in pcp.
- page type of the range will never change.
then, memory offline success.
If (*) seems too heavy anyway and will be no help even if with some batching
as isolate_lru_page_pagevec() or some, okay please forget offlining.
BTW, can't we drop disable_irq() from all lru_lock related codes ?
Thanks,
-Kame
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
2011-02-28 9:42 ` KAMEZAWA Hiroyuki
@ 2011-02-28 10:18 ` Mel Gorman
-1 siblings, 0 replies; 46+ messages in thread
From: Mel Gorman @ 2011-02-28 10:18 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: Andrea Arcangeli, Andrew Morton, Arthur Marsh, Clemens Ladisch,
Linux-MM, Linux Kernel Mailing List
On Mon, Feb 28, 2011 at 06:42:30PM +0900, KAMEZAWA Hiroyuki wrote:
> On Mon, 28 Feb 2011 09:28:14 +0000
> Mel Gorman <mel@csn.ul.ie> wrote:
>
> > On Mon, Feb 28, 2011 at 02:54:02PM +0900, KAMEZAWA Hiroyuki wrote:
> > > On Mon, 28 Feb 2011 06:48:18 +0100
> > > Andrea Arcangeli <aarcange@redhat.com> wrote:
> > >
> > > > On Mon, Feb 28, 2011 at 11:17:46AM +0900, KAMEZAWA Hiroyuki wrote:
> > > > > BTW, I forget why we always take zone->lru_lock with IRQ disabled....
> > > >
> > > > To decrease lock contention in SMP to deliver overall better
> > > > performance (not sure how much it helps though). It was supposed to be
> > > > hold for a very short time (PAGEVEC_SIZE) to avoid giving irq latency
> > > > problems.
> > > >
> > >
> > > memory hotplug uses MIGRATE_ISOLATED migrate types for scanning pfn range
> > > without lru_lock. I wonder whether we can make use of it (the function
> > > which memory hotplug may need rework for the compaction but migrate_type can
> > > be used, I think).
> > >
> >
> > I don't see how migrate_type would be of any benefit here particularly
> > as compaction does not directly affect the migratetype of a pageblock. I
> > have not checked closely which part of hotplug you are on about but if
> > you're talking about when pages actually get offlined, the zone lock is
> > not necessary there because the pages are not on the LRU. In compactions
> > case, they are. Did I misunderstand?
> >
>
> memory offline code doesn't take big lru_lock (and call isolate_lru_page())
> at picking up migration target pages from LRU.
Right - the scanning doesn't hold the lock but you get and release the lock
for every LRU page encountered. This is fairly expensive but considered ok
during memory hotplug. It's less ideal in compaction which is expected
to be a more frequent operation than memory hotplug.
> While this, allocation from
> the zone is allowed. memory offline is done by mem_section unit.
>
> memory offline does.
>
> 1. making a whole section as MIGRATETYPE_ISOLATED.
> 2. scan pfn within section.
> 3. find a page on LRU
> 4. isolate_lru_page() -> take/release lru_lock. ----(*)
> 5. migrate it.
> 6. making all pages in the range as RESERVED.
>
> During this, by marking the pageblock as MIGRATETYPE_ISOLATED,
>
> - new allocation will never picks up a page in the range.
Overkill for compaction though. To use MIGRATE_ISOLATE properly, all pages
on the freelist for that block have to be moved as well. It also operates
at the granularity of a pageblock which is potentially far higher than the
allocation target. It'd might make some sense for transparent hugepage support.
> - newly freed pages in the range will never be allocated and never in pcp.
> - page type of the range will never change.
>
> then, memory offline success.
>
> If (*) seems too heavy anyway and will be no help even if with some batching
> as isolate_lru_page_pagevec() or some, okay please forget offlining.
(*) is indeed too heavy but if IRQ disabled times are found to be too high
it could be batched like I described in my previous mail. Right now, the
interrupt disabled times are not showing up as very high.
> BTW, can't we drop disable_irq() from all lru_lock related codes ?
>
I don't think so - at least not right now. Some LRU operations such as LRU
pagevec draining are run from IPI which is running from an interrupt so
minimally spin_lock_irq is necessary.
--
Mel Gorman
SUSE Labs
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
@ 2011-02-28 10:18 ` Mel Gorman
0 siblings, 0 replies; 46+ messages in thread
From: Mel Gorman @ 2011-02-28 10:18 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: Andrea Arcangeli, Andrew Morton, Arthur Marsh, Clemens Ladisch,
Linux-MM, Linux Kernel Mailing List
On Mon, Feb 28, 2011 at 06:42:30PM +0900, KAMEZAWA Hiroyuki wrote:
> On Mon, 28 Feb 2011 09:28:14 +0000
> Mel Gorman <mel@csn.ul.ie> wrote:
>
> > On Mon, Feb 28, 2011 at 02:54:02PM +0900, KAMEZAWA Hiroyuki wrote:
> > > On Mon, 28 Feb 2011 06:48:18 +0100
> > > Andrea Arcangeli <aarcange@redhat.com> wrote:
> > >
> > > > On Mon, Feb 28, 2011 at 11:17:46AM +0900, KAMEZAWA Hiroyuki wrote:
> > > > > BTW, I forget why we always take zone->lru_lock with IRQ disabled....
> > > >
> > > > To decrease lock contention in SMP to deliver overall better
> > > > performance (not sure how much it helps though). It was supposed to be
> > > > hold for a very short time (PAGEVEC_SIZE) to avoid giving irq latency
> > > > problems.
> > > >
> > >
> > > memory hotplug uses MIGRATE_ISOLATED migrate types for scanning pfn range
> > > without lru_lock. I wonder whether we can make use of it (the function
> > > which memory hotplug may need rework for the compaction but migrate_type can
> > > be used, I think).
> > >
> >
> > I don't see how migrate_type would be of any benefit here particularly
> > as compaction does not directly affect the migratetype of a pageblock. I
> > have not checked closely which part of hotplug you are on about but if
> > you're talking about when pages actually get offlined, the zone lock is
> > not necessary there because the pages are not on the LRU. In compactions
> > case, they are. Did I misunderstand?
> >
>
> memory offline code doesn't take big lru_lock (and call isolate_lru_page())
> at picking up migration target pages from LRU.
Right - the scanning doesn't hold the lock but you get and release the lock
for every LRU page encountered. This is fairly expensive but considered ok
during memory hotplug. It's less ideal in compaction which is expected
to be a more frequent operation than memory hotplug.
> While this, allocation from
> the zone is allowed. memory offline is done by mem_section unit.
>
> memory offline does.
>
> 1. making a whole section as MIGRATETYPE_ISOLATED.
> 2. scan pfn within section.
> 3. find a page on LRU
> 4. isolate_lru_page() -> take/release lru_lock. ----(*)
> 5. migrate it.
> 6. making all pages in the range as RESERVED.
>
> During this, by marking the pageblock as MIGRATETYPE_ISOLATED,
>
> - new allocation will never picks up a page in the range.
Overkill for compaction though. To use MIGRATE_ISOLATE properly, all pages
on the freelist for that block have to be moved as well. It also operates
at the granularity of a pageblock which is potentially far higher than the
allocation target. It'd might make some sense for transparent hugepage support.
> - newly freed pages in the range will never be allocated and never in pcp.
> - page type of the range will never change.
>
> then, memory offline success.
>
> If (*) seems too heavy anyway and will be no help even if with some batching
> as isolate_lru_page_pagevec() or some, okay please forget offlining.
(*) is indeed too heavy but if IRQ disabled times are found to be too high
it could be batched like I described in my previous mail. Right now, the
interrupt disabled times are not showing up as very high.
> BTW, can't we drop disable_irq() from all lru_lock related codes ?
>
I don't think so - at least not right now. Some LRU operations such as LRU
pagevec draining are run from IPI which is running from an interrupt so
minimally spin_lock_irq is necessary.
--
Mel Gorman
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
2011-02-28 10:18 ` Mel Gorman
@ 2011-02-28 23:42 ` KAMEZAWA Hiroyuki
-1 siblings, 0 replies; 46+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-02-28 23:42 UTC (permalink / raw)
To: Mel Gorman
Cc: Andrea Arcangeli, Andrew Morton, Arthur Marsh, Clemens Ladisch,
Linux-MM, Linux Kernel Mailing List
On Mon, 28 Feb 2011 10:18:27 +0000
Mel Gorman <mel@csn.ul.ie> wrote:
> > BTW, can't we drop disable_irq() from all lru_lock related codes ?
> >
>
> I don't think so - at least not right now. Some LRU operations such as LRU
> pagevec draining are run from IPI which is running from an interrupt so
> minimally spin_lock_irq is necessary.
>
pagevec draining is done by workqueue(schedule_on_each_cpu()).
I think only racy case is just lru rotation after writeback.
Thanks,
-Kame
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
@ 2011-02-28 23:42 ` KAMEZAWA Hiroyuki
0 siblings, 0 replies; 46+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-02-28 23:42 UTC (permalink / raw)
To: Mel Gorman
Cc: Andrea Arcangeli, Andrew Morton, Arthur Marsh, Clemens Ladisch,
Linux-MM, Linux Kernel Mailing List
On Mon, 28 Feb 2011 10:18:27 +0000
Mel Gorman <mel@csn.ul.ie> wrote:
> > BTW, can't we drop disable_irq() from all lru_lock related codes ?
> >
>
> I don't think so - at least not right now. Some LRU operations such as LRU
> pagevec draining are run from IPI which is running from an interrupt so
> minimally spin_lock_irq is necessary.
>
pagevec draining is done by workqueue(schedule_on_each_cpu()).
I think only racy case is just lru rotation after writeback.
Thanks,
-Kame
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
2011-02-28 23:42 ` KAMEZAWA Hiroyuki
@ 2011-03-01 4:11 ` Minchan Kim
-1 siblings, 0 replies; 46+ messages in thread
From: Minchan Kim @ 2011-03-01 4:11 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: Mel Gorman, Andrea Arcangeli, Andrew Morton, Arthur Marsh,
Clemens Ladisch, Linux-MM, Linux Kernel Mailing List
On Tue, Mar 01, 2011 at 08:42:09AM +0900, KAMEZAWA Hiroyuki wrote:
> On Mon, 28 Feb 2011 10:18:27 +0000
> Mel Gorman <mel@csn.ul.ie> wrote:
>
> > > BTW, can't we drop disable_irq() from all lru_lock related codes ?
> > >
> >
> > I don't think so - at least not right now. Some LRU operations such as LRU
> > pagevec draining are run from IPI which is running from an interrupt so
> > minimally spin_lock_irq is necessary.
> >
>
> pagevec draining is done by workqueue(schedule_on_each_cpu()).
> I think only racy case is just lru rotation after writeback.
put_page still need irq disable.
>
> Thanks,
> -Kame
>
> --
> 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/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
@ 2011-03-01 4:11 ` Minchan Kim
0 siblings, 0 replies; 46+ messages in thread
From: Minchan Kim @ 2011-03-01 4:11 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: Mel Gorman, Andrea Arcangeli, Andrew Morton, Arthur Marsh,
Clemens Ladisch, Linux-MM, Linux Kernel Mailing List
On Tue, Mar 01, 2011 at 08:42:09AM +0900, KAMEZAWA Hiroyuki wrote:
> On Mon, 28 Feb 2011 10:18:27 +0000
> Mel Gorman <mel@csn.ul.ie> wrote:
>
> > > BTW, can't we drop disable_irq() from all lru_lock related codes ?
> > >
> >
> > I don't think so - at least not right now. Some LRU operations such as LRU
> > pagevec draining are run from IPI which is running from an interrupt so
> > minimally spin_lock_irq is necessary.
> >
>
> pagevec draining is done by workqueue(schedule_on_each_cpu()).
> I think only racy case is just lru rotation after writeback.
put_page still need irq disable.
>
> Thanks,
> -Kame
>
> --
> 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/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
--
Kind regards,
Minchan Kim
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
2011-03-01 4:11 ` Minchan Kim
@ 2011-03-01 4:49 ` KAMEZAWA Hiroyuki
-1 siblings, 0 replies; 46+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-03-01 4:49 UTC (permalink / raw)
To: Minchan Kim
Cc: Mel Gorman, Andrea Arcangeli, Andrew Morton, Arthur Marsh,
Clemens Ladisch, Linux-MM, Linux Kernel Mailing List
On Tue, 1 Mar 2011 13:11:46 +0900
Minchan Kim <minchan.kim@gmail.com> wrote:
> On Tue, Mar 01, 2011 at 08:42:09AM +0900, KAMEZAWA Hiroyuki wrote:
> > On Mon, 28 Feb 2011 10:18:27 +0000
> > Mel Gorman <mel@csn.ul.ie> wrote:
> >
> > > > BTW, can't we drop disable_irq() from all lru_lock related codes ?
> > > >
> > >
> > > I don't think so - at least not right now. Some LRU operations such as LRU
> > > pagevec draining are run from IPI which is running from an interrupt so
> > > minimally spin_lock_irq is necessary.
> > >
> >
> > pagevec draining is done by workqueue(schedule_on_each_cpu()).
> > I think only racy case is just lru rotation after writeback.
>
> put_page still need irq disable.
>
Aha..ok. put_page() removes a page from LRU via __page_cache_release().
Then, we may need to remove a page from LRU under irq context.
Hmm...
Thanks,
-Kame
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
@ 2011-03-01 4:49 ` KAMEZAWA Hiroyuki
0 siblings, 0 replies; 46+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-03-01 4:49 UTC (permalink / raw)
To: Minchan Kim
Cc: Mel Gorman, Andrea Arcangeli, Andrew Morton, Arthur Marsh,
Clemens Ladisch, Linux-MM, Linux Kernel Mailing List
On Tue, 1 Mar 2011 13:11:46 +0900
Minchan Kim <minchan.kim@gmail.com> wrote:
> On Tue, Mar 01, 2011 at 08:42:09AM +0900, KAMEZAWA Hiroyuki wrote:
> > On Mon, 28 Feb 2011 10:18:27 +0000
> > Mel Gorman <mel@csn.ul.ie> wrote:
> >
> > > > BTW, can't we drop disable_irq() from all lru_lock related codes ?
> > > >
> > >
> > > I don't think so - at least not right now. Some LRU operations such as LRU
> > > pagevec draining are run from IPI which is running from an interrupt so
> > > minimally spin_lock_irq is necessary.
> > >
> >
> > pagevec draining is done by workqueue(schedule_on_each_cpu()).
> > I think only racy case is just lru rotation after writeback.
>
> put_page still need irq disable.
>
Aha..ok. put_page() removes a page from LRU via __page_cache_release().
Then, we may need to remove a page from LRU under irq context.
Hmm...
Thanks,
-Kame
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
2011-02-25 20:04 ` Mel Gorman
@ 2011-02-28 23:01 ` Minchan Kim
-1 siblings, 0 replies; 46+ messages in thread
From: Minchan Kim @ 2011-02-28 23:01 UTC (permalink / raw)
To: Mel Gorman
Cc: Andrew Morton, Arthur Marsh, Clemens Ladisch, Andrea Arcangeli,
Linux-MM, Linux Kernel Mailing List
On Fri, Feb 25, 2011 at 08:04:59PM +0000, Mel Gorman wrote:
> From: Andrea Arcangeli <aarcange@redhat.com>
>
> compaction_alloc() isolates pages for migration in isolate_migratepages. While
> it's scanning, IRQs are disabled on the mistaken assumption the scanning
> should be short. Tests show this to be true for the most part but
> contention times on the LRU lock can be increased. Before this patch,
> the IRQ disabled times for a simple test looked like
>
> Total sampled time IRQs off (not real total time): 5493
> Event shrink_inactive_list..shrink_zone 1596 us count 1
> Event shrink_inactive_list..shrink_zone 1530 us count 1
> Event shrink_inactive_list..shrink_zone 956 us count 1
> Event shrink_inactive_list..shrink_zone 541 us count 1
> Event shrink_inactive_list..shrink_zone 531 us count 1
> Event split_huge_page..add_to_swap 232 us count 1
> Event save_args..call_softirq 36 us count 1
> Event save_args..call_softirq 35 us count 2
> Event __wake_up..__wake_up 1 us count 1
>
> This patch reduces the worst-case IRQs-disabled latencies by releasing the
> lock every SWAP_CLUSTER_MAX pages that are scanned and releasing the CPU if
> necessary. The cost of this is that the processing performing compaction will
> be slower but IRQs being disabled for too long a time has worse consequences
> as the following report shows;
>
> Total sampled time IRQs off (not real total time): 4367
> Event shrink_inactive_list..shrink_zone 881 us count 1
> Event shrink_inactive_list..shrink_zone 875 us count 1
> Event shrink_inactive_list..shrink_zone 868 us count 1
> Event shrink_inactive_list..shrink_zone 555 us count 1
> Event split_huge_page..add_to_swap 495 us count 1
> Event compact_zone..compact_zone_order 269 us count 1
> Event split_huge_page..add_to_swap 266 us count 1
> Event shrink_inactive_list..shrink_zone 85 us count 1
> Event save_args..call_softirq 36 us count 2
> Event __wake_up..__wake_up 1 us count 1
>
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> ---
> mm/compaction.c | 18 ++++++++++++++++++
> 1 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 11d88a2..ec9eb0f 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -279,9 +279,27 @@ static unsigned long isolate_migratepages(struct zone *zone,
> }
>
> /* Time to isolate some pages for migration */
> + cond_resched();
> spin_lock_irq(&zone->lru_lock);
> for (; low_pfn < end_pfn; low_pfn++) {
> struct page *page;
> + bool unlocked = false;
> +
> + /* give a chance to irqs before checking need_resched() */
> + if (!((low_pfn+1) % SWAP_CLUSTER_MAX)) {
> + spin_unlock_irq(&zone->lru_lock);
> + unlocked = true;
> + }
> + if (need_resched() || spin_is_contended(&zone->lru_lock)) {
I am not sure it's good if we release the lock whenever lru->lock was contended
unconditionally? There are many kinds of lru_lock operations(add to lru,
del from lru, isolation, reclaim, activation, deactivation and so on).
Do we really need to release the lock whenever all such operations were contened?
I think what we need is just spin_is_contended_irqcontext.
Otherwise, please write down the comment for justifying for it.
> + if (!unlocked)
> + spin_unlock_irq(&zone->lru_lock);
> + cond_resched();
> + spin_lock_irq(&zone->lru_lock);
> + if (fatal_signal_pending(current))
> + break;
This patch is for reducing for irq latency but do we have to check signal
in irq hold time?
> + } else if (unlocked)
> + spin_lock_irq(&zone->lru_lock);
> +
> if (!pfn_valid_within(low_pfn))
> continue;
> nr_scanned++;
> --
> 1.7.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
@ 2011-02-28 23:01 ` Minchan Kim
0 siblings, 0 replies; 46+ messages in thread
From: Minchan Kim @ 2011-02-28 23:01 UTC (permalink / raw)
To: Mel Gorman
Cc: Andrew Morton, Arthur Marsh, Clemens Ladisch, Andrea Arcangeli,
Linux-MM, Linux Kernel Mailing List
On Fri, Feb 25, 2011 at 08:04:59PM +0000, Mel Gorman wrote:
> From: Andrea Arcangeli <aarcange@redhat.com>
>
> compaction_alloc() isolates pages for migration in isolate_migratepages. While
> it's scanning, IRQs are disabled on the mistaken assumption the scanning
> should be short. Tests show this to be true for the most part but
> contention times on the LRU lock can be increased. Before this patch,
> the IRQ disabled times for a simple test looked like
>
> Total sampled time IRQs off (not real total time): 5493
> Event shrink_inactive_list..shrink_zone 1596 us count 1
> Event shrink_inactive_list..shrink_zone 1530 us count 1
> Event shrink_inactive_list..shrink_zone 956 us count 1
> Event shrink_inactive_list..shrink_zone 541 us count 1
> Event shrink_inactive_list..shrink_zone 531 us count 1
> Event split_huge_page..add_to_swap 232 us count 1
> Event save_args..call_softirq 36 us count 1
> Event save_args..call_softirq 35 us count 2
> Event __wake_up..__wake_up 1 us count 1
>
> This patch reduces the worst-case IRQs-disabled latencies by releasing the
> lock every SWAP_CLUSTER_MAX pages that are scanned and releasing the CPU if
> necessary. The cost of this is that the processing performing compaction will
> be slower but IRQs being disabled for too long a time has worse consequences
> as the following report shows;
>
> Total sampled time IRQs off (not real total time): 4367
> Event shrink_inactive_list..shrink_zone 881 us count 1
> Event shrink_inactive_list..shrink_zone 875 us count 1
> Event shrink_inactive_list..shrink_zone 868 us count 1
> Event shrink_inactive_list..shrink_zone 555 us count 1
> Event split_huge_page..add_to_swap 495 us count 1
> Event compact_zone..compact_zone_order 269 us count 1
> Event split_huge_page..add_to_swap 266 us count 1
> Event shrink_inactive_list..shrink_zone 85 us count 1
> Event save_args..call_softirq 36 us count 2
> Event __wake_up..__wake_up 1 us count 1
>
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> ---
> mm/compaction.c | 18 ++++++++++++++++++
> 1 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 11d88a2..ec9eb0f 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -279,9 +279,27 @@ static unsigned long isolate_migratepages(struct zone *zone,
> }
>
> /* Time to isolate some pages for migration */
> + cond_resched();
> spin_lock_irq(&zone->lru_lock);
> for (; low_pfn < end_pfn; low_pfn++) {
> struct page *page;
> + bool unlocked = false;
> +
> + /* give a chance to irqs before checking need_resched() */
> + if (!((low_pfn+1) % SWAP_CLUSTER_MAX)) {
> + spin_unlock_irq(&zone->lru_lock);
> + unlocked = true;
> + }
> + if (need_resched() || spin_is_contended(&zone->lru_lock)) {
I am not sure it's good if we release the lock whenever lru->lock was contended
unconditionally? There are many kinds of lru_lock operations(add to lru,
del from lru, isolation, reclaim, activation, deactivation and so on).
Do we really need to release the lock whenever all such operations were contened?
I think what we need is just spin_is_contended_irqcontext.
Otherwise, please write down the comment for justifying for it.
> + if (!unlocked)
> + spin_unlock_irq(&zone->lru_lock);
> + cond_resched();
> + spin_lock_irq(&zone->lru_lock);
> + if (fatal_signal_pending(current))
> + break;
This patch is for reducing for irq latency but do we have to check signal
in irq hold time?
> + } else if (unlocked)
> + spin_lock_irq(&zone->lru_lock);
> +
> if (!pfn_valid_within(low_pfn))
> continue;
> nr_scanned++;
> --
> 1.7.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Kind regards,
Minchan Kim
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
2011-02-28 23:01 ` Minchan Kim
@ 2011-02-28 23:07 ` Andrea Arcangeli
-1 siblings, 0 replies; 46+ messages in thread
From: Andrea Arcangeli @ 2011-02-28 23:07 UTC (permalink / raw)
To: Minchan Kim
Cc: Mel Gorman, Andrew Morton, Arthur Marsh, Clemens Ladisch,
Linux-MM, Linux Kernel Mailing List
On Tue, Mar 01, 2011 at 08:01:31AM +0900, Minchan Kim wrote:
> I am not sure it's good if we release the lock whenever lru->lock was contended
> unconditionally? There are many kinds of lru_lock operations(add to lru,
> del from lru, isolation, reclaim, activation, deactivation and so on).
This is mostly to mirror cond_resched_lock (which actually uses
spin_needbreak but it's ok to have it also when preempt is off). I
doubt it makes a big difference but I tried to mirror
cond_resched_lock.
> Do we really need to release the lock whenever all such operations were contened?
> I think what we need is just spin_is_contended_irqcontext.
> Otherwise, please write down the comment for justifying for it.
What is spin_is_contended_irqcontext?
> This patch is for reducing for irq latency but do we have to check signal
> in irq hold time?
I think it's good idea to check the signal in case the loop is very
long and this is run in direct compaction context.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
@ 2011-02-28 23:07 ` Andrea Arcangeli
0 siblings, 0 replies; 46+ messages in thread
From: Andrea Arcangeli @ 2011-02-28 23:07 UTC (permalink / raw)
To: Minchan Kim
Cc: Mel Gorman, Andrew Morton, Arthur Marsh, Clemens Ladisch,
Linux-MM, Linux Kernel Mailing List
On Tue, Mar 01, 2011 at 08:01:31AM +0900, Minchan Kim wrote:
> I am not sure it's good if we release the lock whenever lru->lock was contended
> unconditionally? There are many kinds of lru_lock operations(add to lru,
> del from lru, isolation, reclaim, activation, deactivation and so on).
This is mostly to mirror cond_resched_lock (which actually uses
spin_needbreak but it's ok to have it also when preempt is off). I
doubt it makes a big difference but I tried to mirror
cond_resched_lock.
> Do we really need to release the lock whenever all such operations were contened?
> I think what we need is just spin_is_contended_irqcontext.
> Otherwise, please write down the comment for justifying for it.
What is spin_is_contended_irqcontext?
> This patch is for reducing for irq latency but do we have to check signal
> in irq hold time?
I think it's good idea to check the signal in case the loop is very
long and this is run in direct compaction context.
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
2011-02-28 23:07 ` Andrea Arcangeli
@ 2011-02-28 23:25 ` Minchan Kim
-1 siblings, 0 replies; 46+ messages in thread
From: Minchan Kim @ 2011-02-28 23:25 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Mel Gorman, Andrew Morton, Arthur Marsh, Clemens Ladisch,
Linux-MM, Linux Kernel Mailing List
On Tue, Mar 01, 2011 at 12:07:12AM +0100, Andrea Arcangeli wrote:
> On Tue, Mar 01, 2011 at 08:01:31AM +0900, Minchan Kim wrote:
> > I am not sure it's good if we release the lock whenever lru->lock was contended
> > unconditionally? There are many kinds of lru_lock operations(add to lru,
> > del from lru, isolation, reclaim, activation, deactivation and so on).
>
> This is mostly to mirror cond_resched_lock (which actually uses
> spin_needbreak but it's ok to have it also when preempt is off). I
> doubt it makes a big difference but I tried to mirror
> cond_resched_lock.
But what's the benefit of releasing lock in here when lock contentionn happen where
activate_page for example?
>
> > Do we really need to release the lock whenever all such operations were contened?
> > I think what we need is just spin_is_contended_irqcontext.
> > Otherwise, please write down the comment for justifying for it.
>
> What is spin_is_contended_irqcontext?
I thought what we need function is to check lock contention happened in only irq context
for short irq latency.
>
> > This patch is for reducing for irq latency but do we have to check signal
> > in irq hold time?
>
> I think it's good idea to check the signal in case the loop is very
> long and this is run in direct compaction context.
I don't oppose the signal check.
I am not sure why we should check by just sign of lru_lock contention.
How about this by coarse-grained?
/* give a chance to irqs before checking need_resched() */
if (!((low_pfn+1) % SWAP_CLUSTER_MAX)) {
if (fatal_signal_pending(current))
break;
spin_unlock_irq(&zone->lru_lock);
unlocked = true;
}
if (need_resched() || spin_is_contended(&zone->lru_lock)) {
if (!unlocked)
spin_unlock_irq(&zone->lru_lock);
cond_resched();
spin_lock_irq(&zone->lru_lock);
} else if (unlocked)
spin_lock_irq(&zone->lru_lock);
>
> --
> 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/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
@ 2011-02-28 23:25 ` Minchan Kim
0 siblings, 0 replies; 46+ messages in thread
From: Minchan Kim @ 2011-02-28 23:25 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Mel Gorman, Andrew Morton, Arthur Marsh, Clemens Ladisch,
Linux-MM, Linux Kernel Mailing List
On Tue, Mar 01, 2011 at 12:07:12AM +0100, Andrea Arcangeli wrote:
> On Tue, Mar 01, 2011 at 08:01:31AM +0900, Minchan Kim wrote:
> > I am not sure it's good if we release the lock whenever lru->lock was contended
> > unconditionally? There are many kinds of lru_lock operations(add to lru,
> > del from lru, isolation, reclaim, activation, deactivation and so on).
>
> This is mostly to mirror cond_resched_lock (which actually uses
> spin_needbreak but it's ok to have it also when preempt is off). I
> doubt it makes a big difference but I tried to mirror
> cond_resched_lock.
But what's the benefit of releasing lock in here when lock contentionn happen where
activate_page for example?
>
> > Do we really need to release the lock whenever all such operations were contened?
> > I think what we need is just spin_is_contended_irqcontext.
> > Otherwise, please write down the comment for justifying for it.
>
> What is spin_is_contended_irqcontext?
I thought what we need function is to check lock contention happened in only irq context
for short irq latency.
>
> > This patch is for reducing for irq latency but do we have to check signal
> > in irq hold time?
>
> I think it's good idea to check the signal in case the loop is very
> long and this is run in direct compaction context.
I don't oppose the signal check.
I am not sure why we should check by just sign of lru_lock contention.
How about this by coarse-grained?
/* give a chance to irqs before checking need_resched() */
if (!((low_pfn+1) % SWAP_CLUSTER_MAX)) {
if (fatal_signal_pending(current))
break;
spin_unlock_irq(&zone->lru_lock);
unlocked = true;
}
if (need_resched() || spin_is_contended(&zone->lru_lock)) {
if (!unlocked)
spin_unlock_irq(&zone->lru_lock);
cond_resched();
spin_lock_irq(&zone->lru_lock);
} else if (unlocked)
spin_lock_irq(&zone->lru_lock);
>
> --
> 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/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
--
Kind regards,
Minchan Kim
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
2011-02-25 20:04 ` Mel Gorman
@ 2011-03-01 22:15 ` Andrew Morton
-1 siblings, 0 replies; 46+ messages in thread
From: Andrew Morton @ 2011-03-01 22:15 UTC (permalink / raw)
To: Mel Gorman
Cc: Arthur Marsh, Clemens Ladisch, Andrea Arcangeli, Linux-MM,
Linux Kernel Mailing List
On Fri, 25 Feb 2011 20:04:59 +0000
Mel Gorman <mel@csn.ul.ie> wrote:
> From: Andrea Arcangeli <aarcange@redhat.com>
>
> compaction_alloc() isolates pages for migration in isolate_migratepages. While
> it's scanning, IRQs are disabled on the mistaken assumption the scanning
> should be short. Tests show this to be true for the most part but
> contention times on the LRU lock can be increased. Before this patch,
> the IRQ disabled times for a simple test looked like
>
> Total sampled time IRQs off (not real total time): 5493
> Event shrink_inactive_list..shrink_zone 1596 us count 1
> Event shrink_inactive_list..shrink_zone 1530 us count 1
> Event shrink_inactive_list..shrink_zone 956 us count 1
> Event shrink_inactive_list..shrink_zone 541 us count 1
> Event shrink_inactive_list..shrink_zone 531 us count 1
> Event split_huge_page..add_to_swap 232 us count 1
> Event save_args..call_softirq 36 us count 1
> Event save_args..call_softirq 35 us count 2
> Event __wake_up..__wake_up 1 us count 1
>
> This patch reduces the worst-case IRQs-disabled latencies by releasing the
> lock every SWAP_CLUSTER_MAX pages that are scanned and releasing the CPU if
> necessary. The cost of this is that the processing performing compaction will
> be slower but IRQs being disabled for too long a time has worse consequences
> as the following report shows;
>
> Total sampled time IRQs off (not real total time): 4367
> Event shrink_inactive_list..shrink_zone 881 us count 1
> Event shrink_inactive_list..shrink_zone 875 us count 1
> Event shrink_inactive_list..shrink_zone 868 us count 1
> Event shrink_inactive_list..shrink_zone 555 us count 1
> Event split_huge_page..add_to_swap 495 us count 1
> Event compact_zone..compact_zone_order 269 us count 1
> Event split_huge_page..add_to_swap 266 us count 1
> Event shrink_inactive_list..shrink_zone 85 us count 1
> Event save_args..call_softirq 36 us count 2
> Event __wake_up..__wake_up 1 us count 1
>
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> ---
> mm/compaction.c | 18 ++++++++++++++++++
> 1 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 11d88a2..ec9eb0f 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -279,9 +279,27 @@ static unsigned long isolate_migratepages(struct zone *zone,
> }
>
> /* Time to isolate some pages for migration */
> + cond_resched();
> spin_lock_irq(&zone->lru_lock);
> for (; low_pfn < end_pfn; low_pfn++) {
> struct page *page;
> + bool unlocked = false;
> +
> + /* give a chance to irqs before checking need_resched() */
> + if (!((low_pfn+1) % SWAP_CLUSTER_MAX)) {
> + spin_unlock_irq(&zone->lru_lock);
> + unlocked = true;
> + }
> + if (need_resched() || spin_is_contended(&zone->lru_lock)) {
> + if (!unlocked)
> + spin_unlock_irq(&zone->lru_lock);
> + cond_resched();
> + spin_lock_irq(&zone->lru_lock);
> + if (fatal_signal_pending(current))
> + break;
> + } else if (unlocked)
> + spin_lock_irq(&zone->lru_lock);
> +
> if (!pfn_valid_within(low_pfn))
> continue;
> nr_scanned++;
That was somewhat ghastly.
I think it can be made only 99% as ghastly by renaming "unlocked" to
"locked" and eliminating the double-negative logic.
--- a/mm/compaction.c~mm-compaction-minimise-the-time-irqs-are-disabled-while-isolating-pages-for-migration
+++ a/mm/compaction.c
@@ -279,9 +279,27 @@ static unsigned long isolate_migratepage
}
/* Time to isolate some pages for migration */
+ cond_resched();
spin_lock_irq(&zone->lru_lock);
for (; low_pfn < end_pfn; low_pfn++) {
struct page *page;
+ bool locked = true;
+
+ /* give a chance to irqs before checking need_resched() */
+ if (!((low_pfn+1) % SWAP_CLUSTER_MAX)) {
+ spin_unlock_irq(&zone->lru_lock);
+ locked = false;
+ }
+ if (need_resched() || spin_is_contended(&zone->lru_lock)) {
+ if (locked)
+ spin_unlock_irq(&zone->lru_lock);
+ cond_resched();
+ spin_lock_irq(&zone->lru_lock);
+ if (fatal_signal_pending(current))
+ break;
+ } else if (!locked)
+ spin_lock_irq(&zone->lru_lock);
+
if (!pfn_valid_within(low_pfn))
continue;
nr_scanned++;
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration
@ 2011-03-01 22:15 ` Andrew Morton
0 siblings, 0 replies; 46+ messages in thread
From: Andrew Morton @ 2011-03-01 22:15 UTC (permalink / raw)
To: Mel Gorman
Cc: Arthur Marsh, Clemens Ladisch, Andrea Arcangeli, Linux-MM,
Linux Kernel Mailing List
On Fri, 25 Feb 2011 20:04:59 +0000
Mel Gorman <mel@csn.ul.ie> wrote:
> From: Andrea Arcangeli <aarcange@redhat.com>
>
> compaction_alloc() isolates pages for migration in isolate_migratepages. While
> it's scanning, IRQs are disabled on the mistaken assumption the scanning
> should be short. Tests show this to be true for the most part but
> contention times on the LRU lock can be increased. Before this patch,
> the IRQ disabled times for a simple test looked like
>
> Total sampled time IRQs off (not real total time): 5493
> Event shrink_inactive_list..shrink_zone 1596 us count 1
> Event shrink_inactive_list..shrink_zone 1530 us count 1
> Event shrink_inactive_list..shrink_zone 956 us count 1
> Event shrink_inactive_list..shrink_zone 541 us count 1
> Event shrink_inactive_list..shrink_zone 531 us count 1
> Event split_huge_page..add_to_swap 232 us count 1
> Event save_args..call_softirq 36 us count 1
> Event save_args..call_softirq 35 us count 2
> Event __wake_up..__wake_up 1 us count 1
>
> This patch reduces the worst-case IRQs-disabled latencies by releasing the
> lock every SWAP_CLUSTER_MAX pages that are scanned and releasing the CPU if
> necessary. The cost of this is that the processing performing compaction will
> be slower but IRQs being disabled for too long a time has worse consequences
> as the following report shows;
>
> Total sampled time IRQs off (not real total time): 4367
> Event shrink_inactive_list..shrink_zone 881 us count 1
> Event shrink_inactive_list..shrink_zone 875 us count 1
> Event shrink_inactive_list..shrink_zone 868 us count 1
> Event shrink_inactive_list..shrink_zone 555 us count 1
> Event split_huge_page..add_to_swap 495 us count 1
> Event compact_zone..compact_zone_order 269 us count 1
> Event split_huge_page..add_to_swap 266 us count 1
> Event shrink_inactive_list..shrink_zone 85 us count 1
> Event save_args..call_softirq 36 us count 2
> Event __wake_up..__wake_up 1 us count 1
>
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> ---
> mm/compaction.c | 18 ++++++++++++++++++
> 1 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 11d88a2..ec9eb0f 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -279,9 +279,27 @@ static unsigned long isolate_migratepages(struct zone *zone,
> }
>
> /* Time to isolate some pages for migration */
> + cond_resched();
> spin_lock_irq(&zone->lru_lock);
> for (; low_pfn < end_pfn; low_pfn++) {
> struct page *page;
> + bool unlocked = false;
> +
> + /* give a chance to irqs before checking need_resched() */
> + if (!((low_pfn+1) % SWAP_CLUSTER_MAX)) {
> + spin_unlock_irq(&zone->lru_lock);
> + unlocked = true;
> + }
> + if (need_resched() || spin_is_contended(&zone->lru_lock)) {
> + if (!unlocked)
> + spin_unlock_irq(&zone->lru_lock);
> + cond_resched();
> + spin_lock_irq(&zone->lru_lock);
> + if (fatal_signal_pending(current))
> + break;
> + } else if (unlocked)
> + spin_lock_irq(&zone->lru_lock);
> +
> if (!pfn_valid_within(low_pfn))
> continue;
> nr_scanned++;
That was somewhat ghastly.
I think it can be made only 99% as ghastly by renaming "unlocked" to
"locked" and eliminating the double-negative logic.
--- a/mm/compaction.c~mm-compaction-minimise-the-time-irqs-are-disabled-while-isolating-pages-for-migration
+++ a/mm/compaction.c
@@ -279,9 +279,27 @@ static unsigned long isolate_migratepage
}
/* Time to isolate some pages for migration */
+ cond_resched();
spin_lock_irq(&zone->lru_lock);
for (; low_pfn < end_pfn; low_pfn++) {
struct page *page;
+ bool locked = true;
+
+ /* give a chance to irqs before checking need_resched() */
+ if (!((low_pfn+1) % SWAP_CLUSTER_MAX)) {
+ spin_unlock_irq(&zone->lru_lock);
+ locked = false;
+ }
+ if (need_resched() || spin_is_contended(&zone->lru_lock)) {
+ if (locked)
+ spin_unlock_irq(&zone->lru_lock);
+ cond_resched();
+ spin_lock_irq(&zone->lru_lock);
+ if (fatal_signal_pending(current))
+ break;
+ } else if (!locked)
+ spin_lock_irq(&zone->lru_lock);
+
if (!pfn_valid_within(low_pfn))
continue;
nr_scanned++;
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 46+ messages in thread