Thread (22 messages) 22 messages, 6 authors, 2011-03-01

Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration

From: Minchan Kim <hidden>
Date: 2011-02-28 23:01:40
Also in: lkml

On Fri, Feb 25, 2011 at 08:04:59PM +0000, Mel Gorman wrote:
quoted hunk ↗ jump to hunk
From: Andrea Arcangeli <redacted>

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 <redacted>
Signed-off-by: Mel Gorman <redacted>
---
 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>
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help