mm: Implement Swap Prefetching v23

Here's a respin with Nick's suggestions and a modification to not cost us extra slab on non-numa.

Andrew if it's ok I'd like to see this one get a run in -mm.

This patch implements swap prefetching when the vm is relatively idle and there is free ram available.

Once pages have been added to the swapped list, a timer is started, testing for conditions suitable to prefetch swap pages every 5 seconds. Suitable conditions are defined as lack of swapping out or in any pages, and no watermark tests failing. Significant amounts of dirtied ram and changes in free ram representing disk writes or reads also prevent prefetching.

mm/swap_prefetch.c new file +5 -0

+static int kprefetchd(void *__unused)
+{
+	set_user_nice(current, 19);
+	/* Set ioprio to lowest if supported by i/o scheduler */
+	sys_ioprio_set(IOPRIO_WHO_PROCESS, 0, IOPRIO_CLASS_IDLE);

Quoted 2 lines · Con Kolivas

Here's a respin with Nick's suggestions and a modification to not cost us extra slab on non-numa.

v23? I'm sure we can do better than that.

Quoted 2 lines

This patch implements swap prefetching when the vm is relatively idle and there is free ram available.

I think "free ram available" is the critical thing here. If it doesn't evict anyhing else then OK, it basically uses unutilised disk bandwidth for free.

But where does it put the pages? If it was really "free", they'd go onto the tail of the inactive list.

And what about all those unpaged text pages which the app will need to fault back in?

Quoted 6 lines

Once pages have been added to the swapped list, a timer is started, testing for conditions suitable to prefetch swap pages every 5 seconds. Suitable conditions are defined as lack of swapping out or in any pages, and no watermark tests failing. Significant amounts of dirtied ram and changes in free ram representing disk writes or reads also prevent prefetching.

OK. The has-the-disk-been-used-recently test still isn't there?

mm/page_alloc.c int zone_watermark_ok(struct zone *z, in · line 849

Quoted 7 lines

mm/page_alloc.c +1 -0

@@ -844,6 +844,7 @@ int zone_watermark_ok(struct zone *z, in
 		if (free_pages <= min)
 			return 0;
 	}
+
 	return 1;
 }

?

mm/swap_prefetch.c line 175

Quoted 2 lines

mm/swap_prefetch.c new file +1 -0

+	read_lock(&swapper_space.tree_lock);

That's interesting. We conventionally do read_lock_irq() on an address_space.tree_lock. Because end_page_writeback()->rotate_reclaimable_page()->test_clear_page_writeback( ) needs to take a write_lock from interrupt context. end_swap_bio_write() calls end_page_writeback() so I don't immediately see why we don't have a deadlock here.

Ordinarily we'd just use find_get_page(), but

mm/swap_prefetch.c line 182

Quoted 7 lines

mm/swap_prefetch.c new file +7 -0

+	/* Entry may already exist */
+	page = radix_tree_lookup(&swapper_space.page_tree, entry.val);
+	read_unlock(&swapper_space.tree_lock);
+	if (page) {
+		remove_from_swapped_list(entry.val);
+		goto out;
+	}

you don't take a ref on the page.

Which makes one wonder what happens here if `page' got whipped out of swapcache between the lookup and the remove_from_swapped_list(). Probably nothing much.

Did you consider borrowing swapper_space.tree_lock to provide the list and radix-tree locking throughout here?

mm/swap_prefetch.c line 402

Quoted 6 lines

mm/swap_prefetch.c new file +6 -0

+out:
+	if (mru) {
+		spin_lock(&swapped.lock);
+		swapped.list.next = mru;
+		spin_unlock(&swapped.lock);
+	}

That looks strange. What happens to swapped.list.prev?

mm/swap_prefetch.c line 424

Quoted 1 line

mm/swap_prefetch.c new file +1 -0

+			schedule_timeout_interruptible(MAX_SCHEDULE_TIMEOUT);

	set_current_state(TASK_INTERRUPTIBLE);
	schedule();

Quoted 6 lines · Andrew Morton

Quoted 2 lines · Con Kolivas

Here's a respin with Nick's suggestions and a modification to not cost us extra slab on non-numa.

v23? I'm sure we can do better than that.

:D

Quoted 7 lines

Quoted 2 lines

This patch implements swap prefetching when the vm is relatively idle and there is free ram available.

I think "free ram available" is the critical thing here. If it doesn't evict anyhing else then OK, it basically uses unutilised disk bandwidth for free.

Quoted 2 lines

But where does it put the pages? If it was really "free", they'd go onto the tail of the inactive list.

It puts them in swapcache. This seems to work nicely as a nowhere-land place where they don't have much affect on anything until we need them or need more ram. This has worked well, but I'm open to other suggestions.

Quoted 4 lines · Con Kolivas

Quoted 2 lines · Andrew Morton

But where does it put the pages? If it was really "free", they'd go onto the tail of the inactive list.

Quoted 3 lines

It puts them in swapcache. This seems to work nicely as a nowhere-land place where they don't have much affect on anything until we need them or need more ram. This has worked well, but I'm open to other suggestions.

Well they go on the head of the inactive list and will kick out file backed pagecache. Which was my concern about reducing the usefulness of useful swapping on desktop systems.

Quoted 3 lines · Nick Piggin

Well they go on the head of the inactive list and will kick out file backed pagecache. Which was my concern about reducing the usefulness of useful swapping on desktop systems.

Ok I see. We don't have a way to add to the tail of that list though? Is that a worthwhile addition to this (ever growing) project? That would definitely

Ok I see. We don't have a way to add to the tail of that list though?

Is that
a worthwhile addition to this (ever growing) project? That would definitely have an impact on the other code if not all done within swap_prefetch.c.. which would also be quite a large open coded something.

Quoted 1 line · Con Kolivas

Ok I see. We don't have a way to add to the tail of that list though?

del_page_from_lru() + (new) add_page_to_inactive_list_tail().

Quoted 1 line

Is that a worthwhile addition to this (ever growing) project? That would definitely have an impact on the other code if not all done within swap_prefetch.c.. which would also be quite a large open coded something.

Do both of the above in a new function in swap.c.

Quoted 14 lines · Andrew Morton

Quoted 1 line · Con Kolivas

Ok I see. We don't have a way to add to the tail of that list though?

del_page_from_lru() + (new) add_page_to_inactive_list_tail().

Quoted 4 lines

Is that
a worthwhile addition to this (ever growing) project? That would definitely have an impact on the other code if not all done within swap_prefetch.c.. which would also be quite a large open coded something.

Quoted 2 lines

Do both of the above in a new function in swap.c.

That'll require the caller to do lru locking.

I'd add an lru_cache_add_tail, use it instead of the current lru_cache_add that Con's got now, and just implement it in a simple manner, without pagevecs.

Quoted 18 lines · Nick Piggin

Quoted 15 lines · Andrew Morton

Quoted 1 line · Con Kolivas

Ok I see. We don't have a way to add to the tail of that list though?

del_page_from_lru() + (new) add_page_to_inactive_list_tail().

Quoted 1 line

Is that a worthwhile addition to this (ever growing) project? That would definitely have an impact on the other code if not all done within swap_prefetch.c.. which would also be quite a large open coded something.

Do both of the above in a new function in swap.c.

Quoted 5 lines

That'll require the caller to do lru locking.

I'd add an lru_cache_add_tail, use it instead of the current lru_cache_add that Con's got now, and just implement it in a simple manner, without pagevecs.

umm, that's what I said ;)

Quoted 17 lines · Andrew Morton

Quoted 16 lines · Nick Piggin

Quoted 5 lines · Andrew Morton

Quoted 1 line · Con Kolivas

Ok I see. We don't have a way to add to the tail of that list though?

del_page_from_lru() + (new) add_page_to_inactive_list_tail().

Quoted 7 lines

Quoted 5 lines

Is that
a worthwhile addition to this (ever growing) project? That would definitely have an impact on the other code if not all done within swap_prefetch.c.. which would also be quite a large open coded something.

Do both of the above in a new function in swap.c.

That'll require the caller to do lru locking.

Quoted 5 lines

Quoted 3 lines

I'd add an lru_cache_add_tail, use it instead of the current lru_cache_add that Con's got now, and just implement it in a simple manner, without pagevecs.

umm, that's what I said ;)

You said del_page_from_lru(), which doesn't belong in a function called lru_cache_add_tail.

Just so it's clear I understand, is this what you (both) had in mind? Inline so it's not built for !CONFIG_SWAP_PREFETCH

mm/swap.c +2 -0

+}
+
 static void __lru_add_drain(int cpu)
 {
 	struct pagevec *pvec = &per_cpu(lru_add_pvecs, cpu);

Quoted 2 lines · Con Kolivas

Just so it's clear I understand, is this what you (both) had in mind? Inline so it's not built for !CONFIG_SWAP_PREFETCH

Close...

Quoted 1 line

+inline void lru_cache_add_tail(struct page *page)

Is this inline going to do what you intend?

     spin_lock_irq(&zone->lru_lock);

mm/swap.c void fastcall lru_cache_add_active(struc · line 163

Quoted 1 line

mm/swap.c +1 -0

+	add_page_to_inactive_list_tail(zone, page);

     spin_unlock_irq(&zone->lru_lock);

Quoted 6 lines · Nick Piggin

Quoted 2 lines · Con Kolivas

Just so it's clear I understand, is this what you (both) had in mind? Inline so it's not built for !CONFIG_SWAP_PREFETCH

Close...

Quoted 3 lines

Quoted 1 line

+inline void lru_cache_add_tail(struct page *page)

Is this inline going to do what you intend?

I don't care if it's actually inlined, but the subtleties of compilers is way beyond me. All it positively achieves is silencing the unused function warning so I had hoped it meant that function was not built. I tend to be wrong though...

Quoted 11 lines · Con Kolivas

Quoted 9 lines · Nick Piggin

Quoted 2 lines · Con Kolivas

Just so it's clear I understand, is this what you (both) had in mind? Inline so it's not built for !CONFIG_SWAP_PREFETCH

Close...

Quoted 1 line

+inline void lru_cache_add_tail(struct page *page)

Is this inline going to do what you intend?

Quoted 4 lines

I don't care if it's actually inlined, but the subtleties of compilers is way beyond me. All it positively achieves is silencing the unused function warning so I had hoped it meant that function was not built. I tend to be wrong though...

I don't think it can because it is not used in the same file. You'd have to put it into the header file.

Not sure why it silences the unused function warning.

You didn't
replace a 'static' with the inline? I don't think there is any other way the compiler can know the function isn't used externally.