swap: add per-partition lock for swapfile

PATCH In review Shaohua Li 3 participants 3 messages Dec 10, 2012 – Jan 17, 2013

Subsystems (6)

Memory management Staging MGLRU Memory mapping +2 more subsystems

lore

Share & tools

lore

Shaohua Li

author

2012-12-10 01:25 UTC #0 lore raw

PATCH 2/2 swap: add per-partition lock for swapfile

swap_lock is heavily contended when I test swap to 3 fast SSD (even slightly slower than swap to 2 such SSD). The main contention comes from swap_info_get(). This patch tries to fix the gap with adding a new per-partition lock.

global data like nr_swapfiles, total_swap_pages, least_priority and swap_list are still protected by swap_lock.

nr_swap_pages is an atomic now, it can be changed without swap_lock. In theory, it's possible get_swap_page() finds no swap pages but actually there are free swap pages. But sounds not a big problem.

accessing partition specific data (like scan_swap_map and so on) is only protected by swap_info_struct.lock.

Changing swap_info_struct.flags need hold swap_lock and swap_info_struct.lock, because scan_scan_map() will check it. read the flags is ok with either the locks hold.

If both swap_lock and swap_info_struct.lock must be hold, we always hold the former first to avoid deadlock.

swap_entry_free() can change swap_list. To delete that code, we add a new highest_priority_index. Whenever get_swap_page() is called, we check it. If it's valid, we use it.

It's a pitty get_swap_page() still holds swap_lock(). But in practice, swap_lock() isn't heavily contended in my test with this patch (or I can say there are other much more heavier bottlenecks like TLB flush). And BTW, looks get_swap_page() doesn't really need the lock. We never free swap_info[] and we check SWAP_WRITEOK flag. The only risk without the lock is we could swapout to some low priority swap, but we can quickly recover after several rounds of swap, so sounds not a big deal to me. But I'd prefer to fix this if it's a real problem.

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

---
 drivers/staging/zcache/zcache-main.c |   23 ++++-
 include/linux/swap.h                 |    6 +
 mm/mmap.c                            |    2 
 mm/swap_state.c                      |    3 
 mm/swapfile.c                        |  138 +++++++++++++++++++++++------------
 mm/vmscan.c                          |    8 +-
 6 files changed, 122 insertions(+), 58 deletions(-)

include/linux/swap.h +4 -2

Index: linux/include/linux/swap.h
===================================================================
@@ -252,6 +252,7 @@ struct swap_info_struct {
 	unsigned long *frontswap_map;	/* frontswap in-use, one bit per page */
 	atomic_t frontswap_pages;	/* frontswap pages in-use counter */
 #endif
+	spinlock_t lock;
 };
 
 struct swap_list_t {

@@ -260,7 +261,8 @@ struct swap_list_t {
 };
 
 /* Swap 50% full? Release swapcache more aggressively.. */
-#define vm_swap_full() (nr_swap_pages*2 < total_swap_pages)
+#define vm_swap_full() \
+	(atomic_long_read(&nr_swap_pages)*2 < total_swap_pages)
 
 /* linux/mm/page_alloc.c */
 extern unsigned long totalram_pages;

@@ -397,7 +399,7 @@ extern struct page *swapin_readahead(swp
 			struct vm_area_struct *vma, unsigned long addr);
 
 /* linux/mm/swapfile.c */
-extern long nr_swap_pages;
+extern atomic_long_t nr_swap_pages;
 extern long total_swap_pages;
 extern void si_swapinfo(struct sysinfo *);
 extern swp_entry_t get_swap_page(void);

mm/swapfile.c +94 ~~-44~~

399 lines not on this page · raw message

mm/vmscan.c +4 -4

Index: linux/mm/vmscan.c
===================================================================
@@ -1664,7 +1664,7 @@ static void get_scan_count(struct lruvec
 		force_scan = true;
 
 	/* If we have no swap space, do not bother scanning anon pages. */
-	if (!sc->may_swap || (nr_swap_pages <= 0)) {
+	if (!sc->may_swap || (atomic_long_read(&nr_swap_pages) <= 0)) {
 		noswap = 1;
 		fraction[0] = 0;
 		fraction[1] = 1;

@@ -1808,7 +1808,7 @@ static inline bool should_continue_recla
 	 */
 	pages_for_compaction = (2UL << sc->order);
 	inactive_lru_pages = get_lru_size(lruvec, LRU_INACTIVE_FILE);
-	if (nr_swap_pages > 0)
+	if (atomic_long_read(&nr_swap_pages) > 0)
 		inactive_lru_pages += get_lru_size(lruvec, LRU_INACTIVE_ANON);
 	if (sc->nr_reclaimed < pages_for_compaction &&
 			inactive_lru_pages > pages_for_compaction)

@@ -3044,7 +3044,7 @@ unsigned long global_reclaimable_pages(v
 	nr = global_page_state(NR_ACTIVE_FILE) +
 	     global_page_state(NR_INACTIVE_FILE);
 
-	if (nr_swap_pages > 0)
+	if (atomic_long_read(&nr_swap_pages) > 0)
 		nr += global_page_state(NR_ACTIVE_ANON) +
 		      global_page_state(NR_INACTIVE_ANON);

@@ -3058,7 +3058,7 @@ unsigned long zone_reclaimable_pages(str
 	nr = zone_page_state(zone, NR_ACTIVE_FILE) +
 	     zone_page_state(zone, NR_INACTIVE_FILE);
 
-	if (nr_swap_pages > 0)
+	if (atomic_long_read(&nr_swap_pages) > 0)
 		nr += zone_page_state(zone, NR_ACTIVE_ANON) +
 		      zone_page_state(zone, NR_INACTIVE_ANON);

mm/mmap.c +1 -1

Index: linux/mm/mmap.c
===================================================================
@@ -128,7 +128,7 @@ int __vm_enough_memory(struct mm_struct
 		 */
 		free -= global_page_state(NR_SHMEM);
 
-		free += nr_swap_pages;
+		free += atomic_long_read(&nr_swap_pages);
 
 		/*
 		 * Any slabs which are created with the

mm/swap_state.c +2 -1

Index: linux/mm/swap_state.c
===================================================================
@@ -69,7 +69,8 @@ void show_swap_cache_info(void)
 	printk("Swap cache stats: add %lu, delete %lu, find %lu/%lu\n",
 		swap_cache_info.add_total, swap_cache_info.del_total,
 		swap_cache_info.find_success, swap_cache_info.find_total);
-	printk("Free swap  = %ldkB\n", nr_swap_pages << (PAGE_SHIFT - 10));
+	printk("Free swap  = %ldkB\n",
+		atomic_long_read(&nr_swap_pages) << (PAGE_SHIFT - 10));
 	printk("Total swap = %lukB\n", total_swap_pages << (PAGE_SHIFT - 10));
 }

drivers/staging/zcache/zcache-main.c +17 -6

Index: linux/drivers/staging/zcache/zcache-main.c
===================================================================
@@ -927,8 +927,8 @@ static struct kobj_attribute zcache_zv_p
  */
 
 /* useful stats not collected by cleancache or frontswap */
-static unsigned long zcache_flush_total;
-static unsigned long zcache_flush_found;
+static atomic_long_t zcache_flush_total;
+static atomic_long_t zcache_flush_found;
 static unsigned long zcache_flobj_total;
 static unsigned long zcache_flobj_found;
 static unsigned long zcache_failed_eph_puts;

@@ -1428,6 +1428,17 @@ static struct notifier_block zcache_cpu_
 		.show = zcache_##_name##_show, \
 	}
 
+#define ZCACHE_SYSFS_RO_ATOMIC_LONG(_name) \
+	static ssize_t zcache_##_name##_show(struct kobject *kobj, \
+				struct kobj_attribute *attr, char *buf) \
+	{ \
+	    return sprintf(buf, "%lu\n", atomic_long_read(&zcache_##_name)); \
+	} \
+	static struct kobj_attribute zcache_##_name##_attr = { \
+		.attr = { .name = __stringify(_name), .mode = 0444 }, \
+		.show = zcache_##_name##_show, \
+	}
+
 #define ZCACHE_SYSFS_RO_CUSTOM(_name, _func) \
 	static ssize_t zcache_##_name##_show(struct kobject *kobj, \
 				struct kobj_attribute *attr, char *buf) \

@@ -1441,8 +1452,8 @@ static struct notifier_block zcache_cpu_
 
 ZCACHE_SYSFS_RO(curr_obj_count_max);
 ZCACHE_SYSFS_RO(curr_objnode_count_max);
-ZCACHE_SYSFS_RO(flush_total);
-ZCACHE_SYSFS_RO(flush_found);
+ZCACHE_SYSFS_RO_ATOMIC_LONG(flush_total);
+ZCACHE_SYSFS_RO_ATOMIC_LONG(flush_found);
 ZCACHE_SYSFS_RO(flobj_total);
 ZCACHE_SYSFS_RO(flobj_found);
 ZCACHE_SYSFS_RO(failed_eph_puts);

@@ -1614,7 +1625,7 @@ static int zcache_flush_page(int cli_id,
 	unsigned long flags;
 
 	local_irq_save(flags);
-	zcache_flush_total++;
+	atomic_long_inc(&zcache_flush_total);
 	pool = zcache_get_pool_by_id(cli_id, pool_id);
 	if (likely(pool != NULL)) {
 		if (atomic_read(&pool->obj_count) > 0)

@@ -1622,7 +1633,7 @@ static int zcache_flush_page(int cli_id,
 		zcache_put_pool(pool);
 	}
 	if (ret >= 0)
-		zcache_flush_found++;
+		atomic_long_inc(&zcache_flush_found);
 	local_irq_restore(flags);
 	return ret;
 }

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

Rik van Riel

Acked-by on v1 2/2

2012-12-13 18:22 UTC #1 lore raw

Quoted 35 lines · Shaohua Li

swap_lock is heavily contended when I test swap to 3 fast SSD (even slightly slower than swap to 2 such SSD). The main contention comes from swap_info_get(). This patch tries to fix the gap with adding a new per-partition lock.
global data like nr_swapfiles, total_swap_pages, least_priority and swap_list are still protected by swap_lock.
nr_swap_pages is an atomic now, it can be changed without swap_lock. In theory, it's possible get_swap_page() finds no swap pages but actually there are free swap pages. But sounds not a big problem.
accessing partition specific data (like scan_swap_map and so on) is only protected by swap_info_struct.lock.
Changing swap_info_struct.flags need hold swap_lock and swap_info_struct.lock, because scan_scan_map() will check it. read the flags is ok with either the locks hold.
If both swap_lock and swap_info_struct.lock must be hold, we always hold the former first to avoid deadlock.
swap_entry_free() can change swap_list. To delete that code, we add a new highest_priority_index. Whenever get_swap_page() is called, we check it. If it's valid, we use it.
It's a pitty get_swap_page() still holds swap_lock(). But in practice, swap_lock() isn't heavily contended in my test with this patch (or I can say there are other much more heavier bottlenecks like TLB flush). And BTW, looks get_swap_page() doesn't really need the lock. We never free swap_info[] and we check SWAP_WRITEOK flag. The only risk without the lock is we could swapout to some low priority swap, but we can quickly recover after several rounds of swap, so sounds not a big deal to me. But I'd prefer to fix this if it's a real problem.

Quoted 1 line

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

Acked-by Rik van Riel <[email protected]>

Andrew Morton

maintainer

2013-01-17 23:24 UTC #2 lore raw

In reply to Shaohua Li

Quoted 40 lines · On Mon, 10 Dec 2012 09:25:10 +0800 Shaohua Li <[email protected]> wrote:

swap_lock is heavily contended when I test swap to 3 fast SSD (even slightly slower than swap to 2 such SSD). The main contention comes from swap_info_get(). This patch tries to fix the gap with adding a new per-partition lock.
global data like nr_swapfiles, total_swap_pages, least_priority and swap_list are still protected by swap_lock.
nr_swap_pages is an atomic now, it can be changed without swap_lock. In theory, it's possible get_swap_page() finds no swap pages but actually there are free swap pages. But sounds not a big problem.
accessing partition specific data (like scan_swap_map and so on) is only protected by swap_info_struct.lock.
Changing swap_info_struct.flags need hold swap_lock and swap_info_struct.lock, because scan_scan_map() will check it. read the flags is ok with either the locks hold.
If both swap_lock and swap_info_struct.lock must be hold, we always hold the former first to avoid deadlock.
swap_entry_free() can change swap_list. To delete that code, we add a new highest_priority_index. Whenever get_swap_page() is called, we check it. If it's valid, we use it.
It's a pitty get_swap_page() still holds swap_lock(). But in practice, swap_lock() isn't heavily contended in my test with this patch (or I can say there are other much more heavier bottlenecks like TLB flush). And BTW, looks get_swap_page() doesn't really need the lock. We never free swap_info[] and we check SWAP_WRITEOK flag. The only risk without the lock is we could swapout to some low priority swap, but we can quickly recover after several rounds of swap, so sounds not a big deal to me. But I'd prefer to fix this if it's a real problem.
...

include/linux/swap.h struct swap_info_struct { · line 256

Quoted 5 lines

include/linux/swap.h +1 -0

@@ -252,6 +252,7 @@ struct swap_info_struct {
 	unsigned long *frontswap_map;	/* frontswap in-use, one bit per page */
 	atomic_t frontswap_pages;	/* frontswap pages in-use counter */
 #endif
+	spinlock_t lock;
 };

Please document the lock. Describe what it protects and its ranking rules.

include/linux/swap.h struct swap_list_t { · line 265

Quoted 8 lines

include/linux/swap.h +2 -1

 struct swap_list_t {

@@ -260,7 +261,8 @@ struct swap_list_t {
 };
 
 /* Swap 50% full? Release swapcache more aggressively.. */
-#define vm_swap_full() (nr_swap_pages*2 < total_swap_pages)
+#define vm_swap_full() \
+	(atomic_long_read(&nr_swap_pages)*2 < total_swap_pages)

May as well turn this into a real C function and move it to mm/internal.h.

Quoted 10 lines

include/linux/swap.h +0 -1

 /* linux/mm/page_alloc.c */
 extern unsigned long totalram_pages;

@@ -397,7 +399,7 @@ extern struct page *swapin_readahead(swp
 
...
 
--- linux.orig/include/linux/swap.h	2012-12-10 09:02:45.029330611 +0800

mm/swapfile.c static sector_t map_swap_entry(swp_entry · line 54

Quoted 9 lines

mm/swapfile.c +3 -1

@@ -47,9 +47,11 @@ static sector_t map_swap_entry(swp_entry
 
 DEFINE_SPINLOCK(swap_lock);
 static unsigned int nr_swapfiles;
-long nr_swap_pages;
+atomic_long_t nr_swap_pages;
+/* protected with swap_lock. reading in vm_swap_full() doesn't need lock */
 long total_swap_pages;
 static int least_priority;
+static atomic_t highest_priority_index = ATOMIC_INIT(-1);

Please document this variable. What does it mean, what does it do.

Quoted 14 lines

mm/swapfile.c +2 -1

 static const char Bad_file[] = "Bad swap file entry ";
 static const char Unused_file[] = "Unused swap file entry ";
 
...

@@ -417,13 +419,31 @@ swp_entry_t get_swap_page(void)
 	pgoff_t offset;
 	int type, next;
 	int wrapped = 0;
+	int hp_index;
 
 	spin_lock(&swap_lock);
-	if (nr_swap_pages <= 0)
+	if (atomic_long_read(&nr_swap_pages) <= 0)

mm/swapfile.c swp_entry_t get_swap_page(void) · line 434

Quoted 10 lines

mm/swapfile.c +6 -1

 		goto noswap;
-	nr_swap_pages--;
+	atomic_long_dec(&nr_swap_pages);
 
 	for (type = swap_list.next; type >= 0 && wrapped < 2; type = next) {
+		hp_index = atomic_xchg(&highest_priority_index, -1);
+		/*
+		 * highest_priority_index isn't protected by swap_lock, so it
+		 * can be an invalid value if the corresponding swap is
+		 * swapoff.

I don't understand this. How can swap be swapoff?

Quoted 19 lines

mm/swapfile.c +15 -0

 		   We double check the flags here. It's even possible
+		 * the swap is swapoff and swapon again and its priority is
+		 * changed. In such rare case, low prority swap might be used,
+		 * but eventually high priority swap will be used after several
+		 * rounds of swap.
+		 */
+		if (hp_index != -1 && hp_index != type &&
+		    swap_info[type]->prio < swap_info[hp_index]->prio &&
+		    (swap_info[hp_index]->flags & SWP_WRITEOK)) {
+			type = hp_index;
+			swap_list.next = type;
+		}
+
 
...
 
+static void set_highest_priority_index(int type)
+{
+	int old_hp_index, new_hp_index;

mm/swapfile.c swp_entry_t get_swap_page(void)

Quoted 10 lines

mm/swapfile.c +10 -0

+
+	do {
+		old_hp_index = atomic_read(&highest_priority_index);
+		if (old_hp_index != -1 &&
+			swap_info[old_hp_index]->prio >= swap_info[type]->prio)
+			break;
+		new_hp_index = type;
+	} while (atomic_cmpxchg(&highest_priority_index,
+		old_hp_index, new_hp_index) != old_hp_index);
+}

Needs a covering comment explaining what it does and why it does it.

Quoted 3 lines

...