mm: swap prefetch more improvements

PATCH In review Con Kolivas 4 participants 8 messages May 14, 2007 – May 15, 2007 Memory management

lore

Share & tools

lore

Con Kolivas

author

2007-05-14 00:50 UTC #0 lore raw

PATCH 1/1 mm: swap prefetch more improvements

akpm, please queue on top of "mm: swap prefetch improvements"

---
Failed radix_tree_insert wasn't being handled leaving stale kmem.

The list should be iterated over in the reverse order when prefetching.

Make the yield within kprefetchd stronger through the use of cond_resched.

Check that the pos entry hasn't been removed while unlocked.

Signed-off-by Con Kolivas <[email protected]>

---
 mm/swap_prefetch.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

mm/swap_prefetch.c +10 -9

Index: linux-2.6.21-ck1/mm/swap_prefetch.c
===================================================================
@@ -117,7 +117,8 @@ void add_to_swapped_list(struct page *pa
 	if (likely(!radix_tree_insert(&swapped.swap_tree, index, entry))) {
 		list_add(&entry->swapped_list, &swapped.list);
 		swapped.count++;
-	}
+	} else
+		kmem_cache_free(swapped.cache, entry);
 
 out_locked:
 	spin_unlock_irqrestore(&swapped.lock, flags);

@@ -427,7 +428,7 @@ out:
 static enum trickle_return trickle_swap(void)
 {
 	enum trickle_return ret = TRICKLE_DELAY;
-	struct list_head *p, *next;
+	struct swapped_entry *pos, *n;
 	unsigned long flags;
 
 	if (!prefetch_enabled())

@@ -440,19 +441,19 @@ static enum trickle_return trickle_swap(
 		return TRICKLE_FAILED;
 
 	spin_lock_irqsave(&swapped.lock, flags);
-	list_for_each_safe(p, next, &swapped.list) {
-		struct swapped_entry *entry;
+	list_for_each_entry_safe_reverse(pos, n, &swapped.list, swapped_list) {
 		swp_entry_t swp_entry;
 		int node;
 
 		spin_unlock_irqrestore(&swapped.lock, flags);
-		might_sleep();
-		if (!prefetch_suitable())
+		/* Yield to anything else running */
+		if (cond_resched() || !prefetch_suitable())
 			goto out_unlocked;
 
 		spin_lock_irqsave(&swapped.lock, flags);
-		entry = list_entry(p, struct swapped_entry, swapped_list);
-		node = get_swap_entry_node(entry);
+		if (unlikely(!pos))
+			continue;
+		node = get_swap_entry_node(pos);
 		if (!node_isset(node, sp_stat.prefetch_nodes)) {
 			/*
 			 * We found an entry that belongs to a node that is

@@ -460,7 +461,7 @@ static enum trickle_return trickle_swap(
 			 */
 			continue;
 		}
-		swp_entry = entry->swp_entry;
+		swp_entry = pos->swp_entry;
 		spin_unlock_irqrestore(&swapped.lock, flags);
 
 		if (trickle_swap_cache_async(swp_entry, node) == TRICKLE_DELAY)

Signature · 7 lines

-ck

--
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

2007-05-14 22:00 UTC #1 lore raw

Quoted 8 lines · On Mon, 14 May 2007 10:50:54 +1000 Con Kolivas <[email protected]> wrote:

akpm, please queue on top of "mm: swap prefetch improvements"
---
Failed radix_tree_insert wasn't being handled leaving stale kmem.
The list should be iterated over in the reverse order when prefetching.
Make the yield within kprefetchd stronger through the use of cond_resched.

hm.

mm/swap_prefetch.c static enum trickle_return trickle_swap( · line 451

Quoted 6 lines

mm/swap_prefetch.c +2 -2

-		might_sleep();
-		if (!prefetch_suitable())
+		/* Yield to anything else running */
+		if (cond_resched() || !prefetch_suitable())
 			goto out_unlocked;

So if cond_resched() happened to schedule away, we terminate this swap-tricking attempt. It's not possible to determine the reasons for this from the code or from the changelog (==bad).

How come?

--
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>

Con Kolivas

author

2007-05-14 22:43 UTC #2 lore raw

Quoted 25 lines · Andrew Morton

On Mon, 14 May 2007 10:50:54 +1000
Quoted 9 lines · Con Kolivas
akpm, please queue on top of "mm: swap prefetch improvements"
---
Failed radix_tree_insert wasn't being handled leaving stale kmem.
The list should be iterated over in the reverse order when prefetching.
Make the yield within kprefetchd stronger through the use of cond_resched.
hm.
mm/swap_prefetch.c static enum trickle_return trickle_swap( · line 451
Quoted 5 lines
mm/swap_prefetch.c +2 -2
-		might_sleep();
-		if (!prefetch_suitable())
+		/* Yield to anything else running */
+		if (cond_resched() || !prefetch_suitable())
 			goto out_unlocked;
So if cond_resched() happened to schedule away, we terminate this swap-tricking attempt. It's not possible to determine the reasons for this from the code or from the changelog (==bad).

Quoted 1 line

How come?

Hmm I thought the line above that says "yield to anything else running" was explicit enough. The idea is kprefetchd shouldn't run if any other real activity is happening just about anywhere, and a positive cond_resched would indicate likely activity so we just put kprefetchd back to sleep.

Signature · 7 lines

-ck

--
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

2007-05-14 23:01 UTC #3 lore raw

Quoted 26 lines · On Tue, 15 May 2007 08:43:35 +1000 Con Kolivas <[email protected]> wrote:

Quoted 25 lines · Andrew Morton
On Mon, 14 May 2007 10:50:54 +1000
Quoted 9 lines · Con Kolivas
akpm, please queue on top of "mm: swap prefetch improvements"
---
Failed radix_tree_insert wasn't being handled leaving stale kmem.
The list should be iterated over in the reverse order when prefetching.
Make the yield within kprefetchd stronger through the use of cond_resched.
hm.
mm/swap_prefetch.c static enum trickle_return trickle_swap( · line 451
Quoted 5 lines
mm/swap_prefetch.c +2 -2
-		might_sleep();
-		if (!prefetch_suitable())
+		/* Yield to anything else running */
+		if (cond_resched() || !prefetch_suitable())
 			goto out_unlocked;
So if cond_resched() happened to schedule away, we terminate this swap-tricking attempt. It's not possible to determine the reasons for this from the code or from the changelog (==bad).

Quoted 6 lines

Quoted 1 line
How come?
Hmm I thought the line above that says "yield to anything else running" was explicit enough. The idea is kprefetchd shouldn't run if any other real activity is happening just about anywhere, and a positive cond_resched would indicate likely activity so we just put kprefetchd back to sleep.

But kprefetchd runs as SCHED_BATCH. Doesn't that mean that some low-prio background thing (seti?) will disable swap-prefetch?

I mean, if swap-prefetch is actually useful, then it'll still be useful if the machine happens to be doing some computational work. It's not obvious to me that there is linkage between "doing CPU work" and "prefetching is presently undesirable".

Con Kolivas

author

2007-05-14 23:24 UTC #4 lore raw

Quoted 38 lines · Andrew Morton

On Tue, 15 May 2007 08:43:35 +1000
Quoted 27 lines · Con Kolivas
Quoted 26 lines · Andrew Morton
On Mon, 14 May 2007 10:50:54 +1000
Quoted 10 lines · Con Kolivas
akpm, please queue on top of "mm: swap prefetch improvements"
---
Failed radix_tree_insert wasn't being handled leaving stale kmem.
The list should be iterated over in the reverse order when prefetching.
Make the yield within kprefetchd stronger through the use of cond_resched.
hm.
mm/swap_prefetch.c static enum trickle_return trickle_swap( · line 451
Quoted 5 lines
mm/swap_prefetch.c +2 -2
-		might_sleep();
-		if (!prefetch_suitable())
+		/* Yield to anything else running */
+		if (cond_resched() || !prefetch_suitable())
 			goto out_unlocked;
So if cond_resched() happened to schedule away, we terminate this swap-tricking attempt. It's not possible to determine the reasons for this from the code or from the changelog (==bad).
Quoted 7 lines
Quoted 1 line
How come?
Hmm I thought the line above that says "yield to anything else running" was explicit enough. The idea is kprefetchd shouldn't run if any other real activity is happening just about anywhere, and a positive cond_resched would indicate likely activity so we just put kprefetchd back to sleep.

Quoted 7 lines

But kprefetchd runs as SCHED_BATCH. Doesn't that mean that some low-prio background thing (seti?) will disable swap-prefetch?
I mean, if swap-prefetch is actually useful, then it'll still be useful if the machine happens to be doing some computational work. It's not obvious to me that there is linkage between "doing CPU work" and "prefetching is presently undesirable".

set_tsk_need_resched which is the trigger for a cond_resched occurring won't be set just by a cpu bound task constantly running in the background as far as I can see. It's only if some wakeup has triggered a set_tsk_need_resched. ie prefetching still happens here with setiathome or equivalent running in my testing. It might be overkill but from what I can see here it is no impediment to prefetching occurring. I'll think about it some more and do more testing but it seems ok to me.

Signature · 7 lines

-ck

--
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>

Michael Chang

2007-05-14 23:37 UTC #5 lore raw

In reply to Andrew Morton

Quoted 36 lines · Andrew Morton

Quoted 26 lines · On Tue, 15 May 2007 08:43:35 +1000 Con Kolivas <[email protected]> wrote:
Quoted 25 lines · Andrew Morton
On Mon, 14 May 2007 10:50:54 +1000
Quoted 9 lines · Con Kolivas
akpm, please queue on top of "mm: swap prefetch improvements"
---
Failed radix_tree_insert wasn't being handled leaving stale kmem.
The list should be iterated over in the reverse order when prefetching.
Make the yield within kprefetchd stronger through the use of cond_resched.
hm.
mm/swap_prefetch.c static enum trickle_return trickle_swap( · line 451
Quoted 5 lines
mm/swap_prefetch.c +2 -2
-         might_sleep();
-         if (!prefetch_suitable())
+         /* Yield to anything else running */
+         if (cond_resched() || !prefetch_suitable())
                  goto out_unlocked;
So if cond_resched() happened to schedule away, we terminate this swap-tricking attempt. It's not possible to determine the reasons for this from the code or from the changelog (==bad).
Quoted 6 lines
Quoted 1 line
How come?
Hmm I thought the line above that says "yield to anything else running" was explicit enough. The idea is kprefetchd shouldn't run if any other real activity is happening just about anywhere, and a positive cond_resched would indicate likely activity so we just put kprefetchd back to sleep.

Quoted 4 lines

I mean, if swap-prefetch is actually useful, then it'll still be useful if the machine happens to be doing some computational work. It's not obvious to me that there is linkage between "doing CPU work" and "prefetching is presently undesirable".

That may be true, but I believe Con is attempting to err on the side of caution in saying that swap prefetch should have practically no negative impact if _anything_ is running. (The whole premise, for now, anyways, is that swap prefetch should provide... "something for (almost) nothing", if I'm interpreting this right.)

That said, there are probably some cases (seti) where swap prefetch during the run of that batch program would help. On the flip side, there are also some cases where batch processes (various other seti-like "boinc" apps) which use a good deal of memory, meaning that performing the prefetch at that time is futile, unhelpful, or otherwise unwanted.

Would it be better to be less yield-y in this circumstance?

Signature · 11 lines

Michael Chang

Please avoid sending me Word or PowerPoint attachments.
See http://www.gnu.org/philosophy/no-word-attachments.html
Thank you.

--
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>

Peter Zijlstra

2007-05-15 09:58 UTC #6 lore raw

In reply to Andrew Morton

Quoted 25 lines · Andrew Morton

Quoted 8 lines · On Mon, 14 May 2007 10:50:54 +1000 Con Kolivas <[email protected]> wrote:
akpm, please queue on top of "mm: swap prefetch improvements"
---
Failed radix_tree_insert wasn't being handled leaving stale kmem.
The list should be iterated over in the reverse order when prefetching.
Make the yield within kprefetchd stronger through the use of cond_resched.
hm.
mm/swap_prefetch.c static enum trickle_return trickle_swap( · line 451
Quoted 6 lines
mm/swap_prefetch.c +2 -2
-		might_sleep();
-		if (!prefetch_suitable())
+		/* Yield to anything else running */
+		if (cond_resched() || !prefetch_suitable())
 			goto out_unlocked;
So if cond_resched() happened to schedule away, we terminate this swap-tricking attempt. It's not possible to determine the reasons for this from the code or from the changelog (==bad).

Quoted 1 line

How come?

I think Con meant need_resched(). That would indicate someone else wants to use the CPU and and has higher priority than kprefetchd.

Con Kolivas

author

2007-05-15 12:41 UTC #7 lore raw

Quoted 26 lines · Peter Zijlstra

Quoted 25 lines · Andrew Morton
On Mon, 14 May 2007 10:50:54 +1000
Quoted 9 lines · Con Kolivas
akpm, please queue on top of "mm: swap prefetch improvements"
---
Failed radix_tree_insert wasn't being handled leaving stale kmem.
The list should be iterated over in the reverse order when prefetching.
Make the yield within kprefetchd stronger through the use of cond_resched.
hm.
mm/swap_prefetch.c static enum trickle_return trickle_swap( · line 451
Quoted 5 lines
mm/swap_prefetch.c +2 -2
-		might_sleep();
-		if (!prefetch_suitable())
+		/* Yield to anything else running */
+		if (cond_resched() || !prefetch_suitable())
 			goto out_unlocked;
So if cond_resched() happened to schedule away, we terminate this swap-tricking attempt. It's not possible to determine the reasons for this from the code or from the changelog (==bad).

Quoted 4 lines

Quoted 1 line
How come?
I think Con meant need_resched(). That would indicate someone else wants to use the CPU and and has higher priority than kprefetchd.

It may well be that need_resched is what I was trying to do... I don't need it to do the resched and _then_ break out of swap prefetch.

Signature · 1 line

-ck