compaction: check lock contention first before taking lock

PATCH In review Shaohua Li 4 participants 7 messages Sep 10, 2012 – Sep 13, 2012

Subsystems (2)

Memory management Page allocator

lore

Share & tools

lore

Shaohua Li

author

2012-09-10 01:18 UTC #0 lore raw

PATCH v2 2/2 compaction: check lock contention first before taking lock

isolate_migratepages_range will take zone->lru_lock first and check if the lock is contented, if yes, it will release the lock. This isn't efficient. If the lock is truly contented, a lock/unlock pair will increase the lock contention. We'd better check if the lock is contended first. compact_trylock_irqsave perfectly meets the requirement.

V2:
leave cond_resched() pointed out by Mel.

Signed-off-by Shaohua Li <[email protected]>

---
 mm/compaction.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

mm/compaction.c +3 -2

Index: linux/mm/compaction.c
===================================================================
@@ -295,8 +295,9 @@ isolate_migratepages_range(struct zone *
 
 	/* Time to isolate some pages for migration */
 	cond_resched();
-	spin_lock_irqsave(&zone->lru_lock, flags);
-	locked = true;
+	locked = compact_trylock_irqsave(&zone->lru_lock, &flags, cc);
+	if (!locked)
+		return 0;
 	for (; low_pfn < end_pfn; low_pfn++) {
 		struct page *page;

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

Mel Gorman

Acked-by on v2 2/2

2012-09-10 08:12 UTC #1 lore raw

Quoted 10 lines · Shaohua Li

isolate_migratepages_range will take zone->lru_lock first and check if the lock is contented, if yes, it will release the lock. This isn't efficient. If the lock is truly contented, a lock/unlock pair will increase the lock contention. We'd better check if the lock is contended first. compact_trylock_irqsave perfectly meets the requirement.
V2:
leave cond_resched() pointed out by Mel.
Signed-off-by Shaohua Li <[email protected]>

Acked-by Mel Gorman <[email protected]>

Signature · 8 lines

Mel Gorman
SUSE Labs

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

Minchan Kim

Acked-by on v2 2/2

2012-09-11 01:56 UTC #2 lore raw

In reply to Shaohua Li

Quoted 10 lines · Shaohua Li

isolate_migratepages_range will take zone->lru_lock first and check if the lock is contented, if yes, it will release the lock. This isn't efficient. If the lock is truly contented, a lock/unlock pair will increase the lock contention. We'd better check if the lock is contended first. compact_trylock_irqsave perfectly meets the requirement.
V2:
leave cond_resched() pointed out by Mel.
Signed-off-by Shaohua Li <[email protected]>

Acked-by Minchan Kim <[email protected]>

Signature · 8 lines

Kind regards,
Minchan Kim

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

Andrew Morton

maintainer

2012-09-11 23:43 UTC #3 lore raw

In reply to Shaohua Li

Quoted 19 lines · On Mon, 10 Sep 2012 09:18:50 +0800 Shaohua Li <[email protected]> wrote:

isolate_migratepages_range will take zone->lru_lock first and check if the lock is contented, if yes, it will release the lock. This isn't efficient. If the lock is truly contented, a lock/unlock pair will increase the lock contention. We'd better check if the lock is contended first. compact_trylock_irqsave perfectly meets the requirement.
V2:
leave cond_resched() pointed out by Mel.
Signed-off-by Shaohua Li <[email protected]>
---
 mm/compaction.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Index: linux/mm/compaction.c
===================================================================

mm/compaction.c isolate_migratepages_range(struct zone * · line 302

Quoted 10 lines

mm/compaction.c +3 -2

@@ -295,8 +295,9 @@ isolate_migratepages_range(struct zone *
 
 	/* Time to isolate some pages for migration */
 	cond_resched();
-	spin_lock_irqsave(&zone->lru_lock, flags);
-	locked = true;
+	locked = compact_trylock_irqsave(&zone->lru_lock, &flags, cc);
+	if (!locked)
+		return 0;
 	for (; low_pfn < end_pfn; low_pfn++) {
 		struct page *page;

Geeze that compact_checklock_irqsave stuff is naaaasty.

What happens if a process has need_resched set? It cannot perform compaction? There is no relationship between the concepts "user pressed ^C" and "this device driver or subsystem wants a high-order allocation".

Mel Gorman

2012-09-12 10:55 UTC #4 lore raw

Quoted 23 lines · Andrew Morton

Quoted 20 lines · On Mon, 10 Sep 2012 09:18:50 +0800 Shaohua Li <[email protected]> wrote:
isolate_migratepages_range will take zone->lru_lock first and check if the lock is contented, if yes, it will release the lock. This isn't efficient. If the lock is truly contented, a lock/unlock pair will increase the lock contention. We'd better check if the lock is contended first. compact_trylock_irqsave perfectly meets the requirement.
V2:
leave cond_resched() pointed out by Mel.
Signed-off-by Shaohua Li <[email protected]>
---
 mm/compaction.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Index: linux/mm/compaction.c
===================================================================
mm/compaction.c +0 -0
@@ -295,8 +295,9 @@ isolate_migratepages_range(struct zone *
 

Quoted 12 lines

mm/compaction.c isolate_migratepages_range(struct zone * · line 302
Quoted 9 lines
mm/compaction.c +3 -2
 	/* Time to isolate some pages for migration */
 	cond_resched();
-	spin_lock_irqsave(&zone->lru_lock, flags);
-	locked = true;
+	locked = compact_trylock_irqsave(&zone->lru_lock, &flags, cc);
+	if (!locked)
+		return 0;
 	for (; low_pfn < end_pfn; low_pfn++) {
 		struct page *page;
Geeze that compact_checklock_irqsave stuff is naaaasty.

The intention is to avoid THP allocations getting stuck in compaction.c for ages due to spinlock contention. It's always better for those to fail quickly. If compact_trylock_irqsave is improved it must still be able to do this.

Quoted 1 line

What happens if a process has need_resched set?

On async compaction, it will abort compaction and it's up to the caller
to retry with sync compaction if necessary. Whether compaction is called again comes down to this check in the page allocator.

/*
* If compaction is deferred for high-order allocations, it is because
* sync compaction recently failed. In this is the case and the caller
* requested a movable allocation that does not heavily disrupt the
* system then fail the allocation instead of entering direct
* reclaim.
*/
if ((deferred_compaction || contended_compaction) &&
(gfp_mask & (__GFP_MOVABLE|__GFP_REPEAT)) == __GFP_MOVABLE)
goto nopage;

On sync compaction, it will call cond_resched() and continue compacting

Quoted 2 lines

It cannot perform
compaction?

It can still use sync compaction.

Quoted 4 lines

There is no relationship between the concepts "user pressed ^C" and "this device driver or subsystem wants a high-order allocation".

hmm, I see your point. The fatal signal check is "hidden" but this was to preseve the existing behaviour prior to commit [c67fe375: mm: compaction:
Abort async compaction if locks are contended or taking too long]. The fatal_signal_check could be deleted from compact_trylock_irqsave() but then it should be checked in the isolate_migratepages_range() at the very least. How about this?

---8<---
mm: compaction: Move fatal signal check out of compact_checklock_irqsave

Commit [c67fe3752: mm: compaction: Abort async compaction if locks are contended or taking too long] addressed a lock contention problem in compaction by introducing compact_checklock_irqsave() that effecively aborting async compaction in the event of compaction.

To preserve existing behaviour it also moved a fatal_signal_pending() check into compact_checklock_irqsave() but that is very misleading. It "hides" the check within a locking function but has nothing to do with locking as such. It just happens to work in a desirable fashion.

This patch moves the fatal_signal_pending() check to isolate_migratepages_range() where it belongs. Arguably the same check should also happen when isolating pages for freeing but it's overkill.

Signed-off-by Mel Gorman <[email protected]>

---
 mm/compaction.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

mm/compaction.c +1 -3

index 0fbc6b7..364e12f 100644
@@ -76,8 +76,6 @@ static bool compact_checklock_irqsave(spinlock_t *lock, unsigned long *flags,
 		}
 
 		cond_resched();
-		if (fatal_signal_pending(current))
-			return false;
 	}
 
 	if (!locked)

@@ -364,7 +362,7 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 		/* Check if it is ok to still hold the lock */
 		locked = compact_checklock_irqsave(&zone->lru_lock, &flags,
 								locked, cc);
-		if (!locked)
+		if (!locked || fatal_signal_pending(current))
 			break;
 
 		/*

Andrew Morton

maintainer

2012-09-12 21:59 UTC #5 lore raw

Quoted 37 lines · On Wed, 12 Sep 2012 11:55:35 +0100 Mel Gorman <[email protected]> wrote:

Quoted 23 lines · Andrew Morton
Quoted 20 lines · On Mon, 10 Sep 2012 09:18:50 +0800 Shaohua Li <[email protected]> wrote:
isolate_migratepages_range will take zone->lru_lock first and check if the lock is contented, if yes, it will release the lock. This isn't efficient. If the lock is truly contented, a lock/unlock pair will increase the lock contention. We'd better check if the lock is contended first. compact_trylock_irqsave perfectly meets the requirement.
V2:
leave cond_resched() pointed out by Mel.
Signed-off-by Shaohua Li <[email protected]>
---
 mm/compaction.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Index: linux/mm/compaction.c
===================================================================
mm/compaction.c +0 -0
@@ -295,8 +295,9 @@ isolate_migratepages_range(struct zone *
 
Quoted 12 lines
mm/compaction.c isolate_migratepages_range(struct zone * · line 302
Quoted 9 lines
mm/compaction.c +3 -2
 	/* Time to isolate some pages for migration */
 	cond_resched();
-	spin_lock_irqsave(&zone->lru_lock, flags);
-	locked = true;
+	locked = compact_trylock_irqsave(&zone->lru_lock, &flags, cc);
+	if (!locked)
+		return 0;
 	for (; low_pfn < end_pfn; low_pfn++) {
 		struct page *page;
Geeze that compact_checklock_irqsave stuff is naaaasty.

Quoted 4 lines

The intention is to avoid THP allocations getting stuck in compaction.c for ages due to spinlock contention. It's always better for those to fail quickly. If compact_trylock_irqsave is improved it must still be able to do this.

So there's an implicit two-level prioritization here. But between what and what?

It all sounds a bit hack/bandaidy?

Quoted 6 lines

Quoted 5 lines
There is no relationship between the concepts "user pressed ^C" and "this device driver or subsystem wants a high-order allocation".

Quoted 6 lines

hmm, I see your point. The fatal signal check is "hidden" but this was to preseve the existing behaviour prior to commit [c67fe375: mm: compaction:
Abort async compaction if locks are contended or taking too long]. The fatal_signal_check could be deleted from compact_trylock_irqsave() but then it should be checked in the isolate_migratepages_range() at the very least. How about this?

hm, well, actually, I chose ^C as an example of something which might set need_resched(). How about `There is no relationship between the concepts "this process exceeded its timeslice" and "this device driver or subsystem wants a high-order allocation"'.

Mel Gorman

2012-09-13 08:19 UTC #6 lore raw

Quoted 40 lines · Andrew Morton

Quoted 37 lines · On Wed, 12 Sep 2012 11:55:35 +0100 Mel Gorman <[email protected]> wrote:
Quoted 35 lines · Andrew Morton
Quoted 19 lines · On Mon, 10 Sep 2012 09:18:50 +0800 Shaohua Li <[email protected]> wrote:
isolate_migratepages_range will take zone->lru_lock first and check if the lock is contented, if yes, it will release the lock. This isn't efficient. If the lock is truly contented, a lock/unlock pair will increase the lock contention. We'd better check if the lock is contended first. compact_trylock_irqsave perfectly meets the requirement.
V2:
leave cond_resched() pointed out by Mel.
Signed-off-by Shaohua Li <[email protected]>
---
 mm/compaction.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Index: linux/mm/compaction.c
===================================================================
mm/compaction.c isolate_migratepages_range(struct zone * · line 302
Quoted 10 lines
mm/compaction.c +3 -2
@@ -295,8 +295,9 @@ isolate_migratepages_range(struct zone *
 
 	/* Time to isolate some pages for migration */
 	cond_resched();
-	spin_lock_irqsave(&zone->lru_lock, flags);
-	locked = true;
+	locked = compact_trylock_irqsave(&zone->lru_lock, &flags, cc);
+	if (!locked)
+		return 0;
 	for (; low_pfn < end_pfn; low_pfn++) {
 		struct page *page;
Geeze that compact_checklock_irqsave stuff is naaaasty.

Quoted 8 lines

Quoted 4 lines
The intention is to avoid THP allocations getting stuck in compaction.c for ages due to spinlock contention. It's always better for those to fail quickly. If compact_trylock_irqsave is improved it must still be able to do this.
So there's an implicit two-level prioritization here. But between what and what?

Between two processes trying high-order allocations at the same time. In practice it is expected to be two processes trying to allocate a THP.

Quoted 2 lines

It all sounds a bit hack/bandaidy?

It is but excessive time spent in compaction.c offsets any advantage of using THP. The ideal would be that the zone lock or lru_lock could be fine-grained but I have not designed something suitable. Splitting lru_lock would be particularly problematic.

Quoted 12 lines

Quoted 11 lines
Quoted 4 lines
There is no relationship between the concepts "user pressed ^C" and "this device driver or subsystem wants a high-order allocation".
hmm, I see your point. The fatal signal check is "hidden" but this was to preseve the existing behaviour prior to commit [c67fe375: mm: compaction:
Abort async compaction if locks are contended or taking too long]. The fatal_signal_check could be deleted from compact_trylock_irqsave() but then it should be checked in the isolate_migratepages_range() at the very least. How about this?

Quoted 5 lines

hm, well, actually, I chose ^C as an example of something which might set need_resched(). How about `There is no relationship between the concepts "this process exceeded its timeslice" and "this device driver or subsystem wants a high-order allocation"'.

A device driver or subsystem that wants a high-order allocation will not (or at least are very unlikely) to have specified __GFP_MOVABLE. These will retry with sync compaction and wait for the lock to be acquired.

Signature · 8 lines

Mel Gorman
SUSE Labs

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