Skip to contentSkip to conversation

swap: make swap discard async

PATCH Superseded Shaohua Li 2 participants 3 messages Jul 15, 2013 – Jul 18, 2013

Subsystems (2)
Memory management MM swap
lore
Share & tools
lore

Shaohua Li

author
#0 lore raw

PATCH v6 2/4 swap: make swap discard async

swap can do cluster discard for SSD, which is good, but there are some problems here:
1. swap do the discard just before page reclaim gets a swap entry and writes the disk sectors. This is useless for high end SSD, because an overwrite to a sector implies a discard to original sector too. A discard + overwrite == overwrite.
2. the purpose of doing discard is to improve SSD firmware garbage collection. Idealy we should send discard as early as possible, so firmware can do something smart. Sending discard just after swap entry is freed is considered early compared to sending discard before write. Of course, if workload is already bound to gc speed, sending discard earlier or later doesn't make difference.
3. block discard is a sync API, which will delay scan_swap_map() significantly.
4. Write and discard command can be executed parallel in PCIe SSD. Making swap discard async can make execution more efficiently.

This patch makes swap discard async and move discard to where swap entry is freed. Discard and write have no dependence now, so above issues can be avoided. Idealy we should do discard for any freed sectors, but some SSD discard is very slow. This patch still does discard for a whole cluster.

My test does a several round of 'mmap, write, unmap', which will trigger a lot of swap discard. In a fusionio card, with this patch, the test runtime is reduced to 18% of the time without it, so around 5.5x faster.

Signed-off-by Shaohua Li <[email protected]>
---
 include/linux/swap.h |    5 -
 mm/swapfile.c        |  183 +++++++++++++++++++++++++++++----------------------
 2 files changed, 108 insertions(+), 80 deletions(-)
include/linux/swap.h +3 -2
Index: linux/include/linux/swap.h
===================================================================
@@ -211,8 +211,6 @@ struct swap_info_struct {
 	unsigned int inuse_pages;	/* number of those currently in use */
 	unsigned int cluster_next;	/* likely index for next allocation */
 	unsigned int cluster_nr;	/* countdown to next cluster search */
-	unsigned int lowest_alloc;	/* while preparing discard cluster */
-	unsigned int highest_alloc;	/* while preparing discard cluster */
 	struct swap_extent *curr_swap_extent;
 	struct swap_extent first_swap_extent;
 	struct block_device *bdev;	/* swap device or bdev of swap file */

@@ -234,6 +232,9 @@ struct swap_info_struct {
 					 * swap_lock. If both locks need hold,
 					 * hold swap_lock first.
 					 */
+	struct work_struct discard_work; /* discard worker */
+	struct swap_cluster_info discard_cluster_head; /* list head of discard clusters */
+	struct swap_cluster_info discard_cluster_tail; /* list tail of discard clusters */
 };
 
 struct swap_list_t {
mm/swapfile.c +105 -78

268 lines not on this page · raw message

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

maintainer
#1 lore raw
Quoted 40 lines · On Tue, 16 Jul 2013 04:43:41 +0800 Shaohua Li <[email protected]> wrote:

swap can do cluster discard for SSD, which is good, but there are some problems here:
1. swap do the discard just before page reclaim gets a swap entry and writes the disk sectors. This is useless for high end SSD, because an overwrite to a sector implies a discard to original sector too. A discard + overwrite == overwrite.
2. the purpose of doing discard is to improve SSD firmware garbage collection. Idealy we should send discard as early as possible, so firmware can do something smart. Sending discard just after swap entry is freed is considered early compared to sending discard before write. Of course, if workload is already bound to gc speed, sending discard earlier or later doesn't make difference.
3. block discard is a sync API, which will delay scan_swap_map() significantly.
4. Write and discard command can be executed parallel in PCIe SSD. Making swap discard async can make execution more efficiently.

This patch makes swap discard async and move discard to where swap entry is freed. Discard and write have no dependence now, so above issues can be avoided. Idealy we should do discard for any freed sectors, but some SSD discard is very slow. This patch still does discard for a whole cluster.

My test does a several round of 'mmap, write, unmap', which will trigger a lot of swap discard. In a fusionio card, with this patch, the test runtime is reduced to 18% of the time without it, so around 5.5x faster.

...

mm/swapfile.c +13 -0
+static void swap_do_scheduled_discard(struct swap_info_struct *si)
+{
+	struct swap_cluster_info *info;
+	unsigned int idx;
+
+	info = si->cluster_info;
+
+	while (!cluster_is_null(&si->discard_cluster_head)) {
+		idx = cluster_next(&si->discard_cluster_head);
+
+		cluster_set_next_flag(&si->discard_cluster_head,
+						cluster_next(&info[idx]), 0);
+		if (cluster_next(&si->discard_cluster_tail) == idx) {

mm/swapfile.c static inline void cluster_set_null(stru · line 289

Quoted 10 lines
mm/swapfile.c +10 -0
+			cluster_set_null(&si->discard_cluster_head);
+			cluster_set_null(&si->discard_cluster_tail);
+		}
+		spin_unlock(&si->lock);
+
+		discard_swap_cluster(si, idx * SWAPFILE_CLUSTER,
+				SWAPFILE_CLUSTER);
+
+		spin_lock(&si->lock);
+		cluster_set_flag(&info[idx], CLUSTER_FLAG_FREE);

Wait. How can we do this? We dropped the spinlock, so `idx' is now invalid.

mm/swapfile.c static inline void cluster_set_null(stru · line 301

Quoted 12 lines
mm/swapfile.c +12 -0
+		if (cluster_is_null(&si->free_cluster_head)) {
+			cluster_set_next_flag(&si->free_cluster_head,
+						idx, 0);
+			cluster_set_next_flag(&si->free_cluster_tail,
+						idx, 0);
+		} else {
+			unsigned int next;
+
+			next = cluster_next(&si->free_cluster_tail);
+			cluster_set_next(&info[next], idx);
+			cluster_set_next_flag(&si->free_cluster_tail,
+						idx, 0);

ditto.

mm/swapfile.c static inline void cluster_set_null(stru · line 304

Quoted 3 lines
mm/swapfile.c +3 -0
+		}
+		memset(si->swap_map + idx * SWAPFILE_CLUSTER,
+				0, SWAPFILE_CLUSTER);

again.

Quoted 30 lines
mm/swapfile.c +3 -13
+	}
+}
+
 
...

@@ -331,19 +414,6 @@ static unsigned long scan_swap_map(struc
 			si->cluster_nr = SWAPFILE_CLUSTER - 1;
 			goto checks;
 		}
-		if (si->flags & SWP_PAGE_DISCARD) {
-			/*
-			 * Start range check on racing allocations, in case
-			 * they overlap the cluster we eventually decide on
-			 * (we scan without swap_lock to allow preemption).
-			 * It's hardly conceivable that cluster_nr could be
-			 * wrapped during our scan, but don't depend on it.
-			 */
-			if (si->lowest_alloc)
-				goto checks;
-			si->lowest_alloc = si->max;
-			si->highest_alloc = 0;
-		}
 check_cluster:
 		if (!cluster_is_null(&si->free_cluster_head)) {
 			offset = cluster_next(&si->free_cluster_head) *

@@ -351,15 +421,22 @@ check_cluster:
 			last_in_cluster = offset + SWAPFILE_CLUSTER - 1;
 			si->cluster_next = offset;
 			si->cluster_nr = SWAPFILE_CLUSTER - 1;

mm/swapfile.c check_cluster: · line 432

Quoted 10 lines
mm/swapfile.c +6 -1
-			found_free_cluster = 1;
 			goto checks;
 		} else if (si->cluster_info) {
 			/*
+			 * we don't have free cluster but have some clusters in
+			 * discarding, do discard now and reclaim them
+			 */
+			if (!cluster_is_null(&si->discard_cluster_head)) {
+				swap_do_scheduled_discard(si);
+				goto check_cluster;

Again, swap_do_scheduled_discard() might have dropped the lock. The state which scan_swap_map() has copied in from the swap_info_struct is now invalidated. scan_base' and offset' might have changed. si->cluster_nr may have changed.

mm/swapfile.c check_cluster: · line 442

Quoted 13 lines
mm/swapfile.c +3 -1
+			}
+
+			/*
 			 * Checking free cluster is fast enough, we can do the
 			 * check every time
 			 */
 			si->cluster_nr = 0;
-			si->lowest_alloc = 0;
 			goto checks;
 		}
 
...

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

author
#2 lore raw
Quoted 47 lines · Andrew Morton
Quoted 45 lines · On Tue, 16 Jul 2013 04:43:41 +0800 Shaohua Li <[email protected]> wrote:

swap can do cluster discard for SSD, which is good, but there are some problems here:
1. swap do the discard just before page reclaim gets a swap entry and writes the disk sectors. This is useless for high end SSD, because an overwrite to a sector implies a discard to original sector too. A discard + overwrite == overwrite.
2. the purpose of doing discard is to improve SSD firmware garbage collection. Idealy we should send discard as early as possible, so firmware can do something smart. Sending discard just after swap entry is freed is considered early compared to sending discard before write. Of course, if workload is already bound to gc speed, sending discard earlier or later doesn't make difference.
3. block discard is a sync API, which will delay scan_swap_map() significantly.
4. Write and discard command can be executed parallel in PCIe SSD. Making swap discard async can make execution more efficiently.

This patch makes swap discard async and move discard to where swap entry is freed. Discard and write have no dependence now, so above issues can be avoided. Idealy we should do discard for any freed sectors, but some SSD discard is very slow. This patch still does discard for a whole cluster.

My test does a several round of 'mmap, write, unmap', which will trigger a lot of swap discard. In a fusionio card, with this patch, the test runtime is reduced to 18% of the time without it, so around 5.5x faster.

...

mm/swapfile.c +18 -0
+static void swap_do_scheduled_discard(struct swap_info_struct *si)
+{
+	struct swap_cluster_info *info;
+	unsigned int idx;
+
+	info = si->cluster_info;
+
+	while (!cluster_is_null(&si->discard_cluster_head)) {
+		idx = cluster_next(&si->discard_cluster_head);
+
+		cluster_set_next_flag(&si->discard_cluster_head,
+						cluster_next(&info[idx]), 0);
+		if (cluster_next(&si->discard_cluster_tail) == idx) {
+			cluster_set_null(&si->discard_cluster_head);
+			cluster_set_null(&si->discard_cluster_tail);
+		}
+		spin_unlock(&si->lock);
+
Quoted 8 lines

mm/swapfile.c static inline void cluster_set_null(stru · line 289

Quoted 5 lines
mm/swapfile.c +5 -0
+		discard_swap_cluster(si, idx * SWAPFILE_CLUSTER,
+				SWAPFILE_CLUSTER);
+
+		spin_lock(&si->lock);
+		cluster_set_flag(&info[idx], CLUSTER_FLAG_FREE);

Wait. How can we do this? We dropped the spinlock, so `idx' is now invalid.

idx is the current cluster (an index of array) we are discarding, it's not invalid after we drop lock.

Quoted 58 lines

mm/swapfile.c static inline void cluster_set_null(stru · line 301

Quoted 12 lines
mm/swapfile.c +12 -0
+		if (cluster_is_null(&si->free_cluster_head)) {
+			cluster_set_next_flag(&si->free_cluster_head,
+						idx, 0);
+			cluster_set_next_flag(&si->free_cluster_tail,
+						idx, 0);
+		} else {
+			unsigned int next;
+
+			next = cluster_next(&si->free_cluster_tail);
+			cluster_set_next(&info[next], idx);
+			cluster_set_next_flag(&si->free_cluster_tail,
+						idx, 0);

ditto.

mm/swapfile.c static inline void cluster_set_null(stru · line 304

Quoted 3 lines
mm/swapfile.c +3 -0
+		}
+		memset(si->swap_map + idx * SWAPFILE_CLUSTER,
+				0, SWAPFILE_CLUSTER);

again.

Quoted 37 lines
mm/swapfile.c +6 -14
+	}
+}
+
 
...

@@ -331,19 +414,6 @@ static unsigned long scan_swap_map(struc
 			si->cluster_nr = SWAPFILE_CLUSTER - 1;
 			goto checks;
 		}
-		if (si->flags & SWP_PAGE_DISCARD) {
-			/*
-			 * Start range check on racing allocations, in case
-			 * they overlap the cluster we eventually decide on
-			 * (we scan without swap_lock to allow preemption).
-			 * It's hardly conceivable that cluster_nr could be
-			 * wrapped during our scan, but don't depend on it.
-			 */
-			if (si->lowest_alloc)
-				goto checks;
-			si->lowest_alloc = si->max;
-			si->highest_alloc = 0;
-		}
 check_cluster:
 		if (!cluster_is_null(&si->free_cluster_head)) {
 			offset = cluster_next(&si->free_cluster_head) *

@@ -351,15 +421,22 @@ check_cluster:
 			last_in_cluster = offset + SWAPFILE_CLUSTER - 1;
 			si->cluster_next = offset;
 			si->cluster_nr = SWAPFILE_CLUSTER - 1;
-			found_free_cluster = 1;
 			goto checks;
 		} else if (si->cluster_info) {
 			/*
+			 * we don't have free cluster but have some clusters in
+			 * discarding, do discard now and reclaim them
+			 */
Quoted 8 lines

mm/swapfile.c check_cluster: · line 432

Quoted 3 lines
mm/swapfile.c +3 -0
+			if (!cluster_is_null(&si->discard_cluster_head)) {
+				swap_do_scheduled_discard(si);
+				goto check_cluster;

Again, swap_do_scheduled_discard() might have dropped the lock. The state which scan_swap_map() has copied in from the swap_info_struct is now invalidated. scan_base' and offset' might have changed. si->cluster_nr may have changed.

it doesn't matter actually. With it, we might scan to different position, but it doesn't break anthing. But I agree this is confusion. I'll move the goto above.

Thanks,
Shaohua

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