dm-multipath low performance with blk-mq

Hi All,

I've recently tried out dm-multipath over a "super-fast" nvme device and noticed a serious lock contention in dm-multipath that requires some extra attention. The nvme device is a simple loopback device emulation backed by null_blk device.

With this I've seen dm-multipath pushing around ~470K IOPs while the native (loopback) nvme performance can easily push up to 1500K+ IOPs.

perf output [1] reveals a huge lock contention on the multipath lock which is a per-dm_target contention point which seem to defeat the purpose of blk-mq i/O path.

The two current bottlenecks seem to come from multipath_busy and __multipath_map. Would it make better sense to move to a percpu_ref model with freeze/unfreeze logic for updates similar to what blk-mq is doing?

Thoughts?

[1]:
- 23.67% fio [kernel.kallsyms] [k]
queued_spin_lock_slowpath
- queued_spin_lock_slowpath
- 51.40% _raw_spin_lock_irqsave
- 99.98% multipath_busy
dm_mq_queue_rq
__blk_mq_run_hw_queue
blk_mq_run_hw_queue
blk_mq_insert_requests
blk_mq_flush_plug_list
blk_flush_plug_list
blk_finish_plug
do_io_submit
SyS_io_submit
entry_SYSCALL_64_fastpath
+ io_submit
- 48.05% _raw_spin_lock_irq
- 100.00% __multipath_map
multipath_clone_and_map
target_message
dispatch_io
__blk_mq_run_hw_queue
blk_mq_run_hw_queue
blk_mq_insert_requests
blk_mq_flush_plug_list
blk_flush_plug_list
blk_finish_plug
do_io_submit
SyS_io_submit
entry_SYSCALL_64_fastpath
+ io_submit

(quoted diff) +6 -0

+   1.70%              fio  [kernel.kallsyms]    [k] __blk_mq_run_hw_queue
+   1.56%              fio  fio                  [.] get_io_u
+   1.06%              fio  [kernel.kallsyms]    [k] blk_account_io_start
+   0.92%              fio  fio                  [.] do_io
+   0.82%              fio  [kernel.kallsyms]    [k] do_blockdev_direct_IO
+   0.81%              fio  [kernel.kallsyms]    [k]

blk_mq_hctx_mark_pending

(quoted diff) +3 -0

+   0.75%              fio  [kernel.kallsyms]    [k] __blk_mq_alloc_request
+   0.75%              fio  [kernel.kallsyms]    [k] __bt_get
+   0.69%              fio  [kernel.kallsyms]    [k] do_direct_IO

Hi All,

I've recently tried out dm-multipath over a "super-fast" nvme device and noticed a serious lock contention in dm-multipath that requires some extra attention. The nvme device is a simple loopback device emulation backed by null_blk device.

With this I've seen dm-multipath pushing around ~470K IOPs while the native (loopback) nvme performance can easily push up to 1500K+ IOPs.

perf output [1] reveals a huge lock contention on the multipath lock which is a per-dm_target contention point which seem to defeat the purpose of blk-mq i/O path.

The two current bottlenecks seem to come from multipath_busy and __multipath_map. Would it make better sense to move to a percpu_ref model with freeze/unfreeze logic for updates similar to what blk-mq is doing?

Thoughts?

Quoted 14 lines · Sagi Grimberg

Hi All,

I've recently tried out dm-multipath over a "super-fast" nvme device and noticed a serious lock contention in dm-multipath that requires some extra attention. The nvme device is a simple loopback device emulation backed by null_blk device.

With this I've seen dm-multipath pushing around ~470K IOPs while the native (loopback) nvme performance can easily push up to 1500K+ IOPs.

perf output [1] reveals a huge lock contention on the multipath lock which is a per-dm_target contention point which seem to defeat the purpose of blk-mq i/O path.

Quoted 6 lines

The two current bottlenecks seem to come from multipath_busy and __multipath_map. Would it make better sense to move to a percpu_ref model with freeze/unfreeze logic for updates similar to what blk-mq is doing?

Thoughts?

Your perf output clearly does identify the 'struct multipath' spinlock as a bottleneck.

Is it fair to assume that implied in your test is that you increased md->tag_set.nr_hw_queues to > 1 in dm_init_request_based_blk_mq_queue()?

I'd like to start by replicating your testbed. So I'll see about setting up the nvme loop driver you referenced in earlier mail. Can you share your fio job file and fio commandline for your test?

On Tue, Jan 19 2016 at 5:45P -0500,
Mike Snitzer <[email protected]> wrote:

Quoted 28 lines

Quoted 14 lines · Sagi Grimberg

Hi All,

I've recently tried out dm-multipath over a "super-fast" nvme device and noticed a serious lock contention in dm-multipath that requires some extra attention. The nvme device is a simple loopback device emulation backed by null_blk device.

With this I've seen dm-multipath pushing around ~470K IOPs while the native (loopback) nvme performance can easily push up to 1500K+ IOPs.

perf output [1] reveals a huge lock contention on the multipath lock which is a per-dm_target contention point which seem to defeat the purpose of blk-mq i/O path.

Quoted 6 lines

The two current bottlenecks seem to come from multipath_busy and __multipath_map. Would it make better sense to move to a percpu_ref model with freeze/unfreeze logic for updates similar to what blk-mq is doing?

Thoughts?

Your perf output clearly does identify the 'struct multipath' spinlock as a bottleneck.

Quoted 6 lines

Is it fair to assume that implied in your test is that you increased md->tag_set.nr_hw_queues to > 1 in dm_init_request_based_blk_mq_queue()?

I'd like to start by replicating your testbed. So I'll see about setting up the nvme loop driver you referenced in earlier mail. Can you share your fio job file and fio commandline for your test?

Would still appreciate answers to my 2 questions above (did you modify md->tag_set.nr_hw_queues and can you share your fio job?)

I've yet to reproduce your config (using hch's nvme loop driver) or test to verify your findings but I did develop a patch that switches from spinlock_t to rwlock_t. I've only compile tested this but I'll try to reproduce your setup and then test this patch to see if it helps.

Your worst offenders (multipath_busy and __multipath_map) are now using a read lock in the fast path.

 drivers/md/dm-mpath.c | 127 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 83 insertions(+), 44 deletions(-)

drivers/md/dm-mpath.c +83 -45

index cfa29f5..34aadb1 100644
@@ -11,6 +11,7 @@
 #include "dm-path-selector.h"
 #include "dm-uevent.h"
 
+#include <linux/rwlock_types.h>
 #include <linux/blkdev.h>
 #include <linux/ctype.h>
 #include <linux/init.h>

@@ -67,7 +68,7 @@ struct multipath {
 	const char *hw_handler_name;
 	char *hw_handler_params;
 
-	spinlock_t lock;
+	rwlock_t lock;
 
 	unsigned nr_priority_groups;
 	struct list_head priority_groups;

@@ -189,7 +190,7 @@ static struct multipath *alloc_multipath(struct dm_target *ti)
 	m = kzalloc(sizeof(*m), GFP_KERNEL);
 	if (m) {
 		INIT_LIST_HEAD(&m->priority_groups);
-		spin_lock_init(&m->lock);
+		rwlock_init(&m->lock);
 		m->queue_io = 1;
 		m->pg_init_delay_msecs = DM_PG_INIT_DELAY_DEFAULT;
 		INIT_WORK(&m->trigger_event, trigger_event);

@@ -386,12 +387,24 @@ static int __multipath_map(struct dm_target *ti, struct request *clone,
 	struct pgpath *pgpath;
 	struct block_device *bdev;
 	struct dm_mpath_io *mpio;
+	bool use_write_lock = false;
 
-	spin_lock_irq(&m->lock);
+retry:
+	if (!use_write_lock)
+		read_lock_irq(&m->lock);
+	else
+		write_lock_irq(&m->lock);
 
 	/* Do we need to select a new pgpath? */
-	if (!m->current_pgpath ||
-	    (!m->queue_io && (m->repeat_count && --m->repeat_count == 0)))
+	if (!use_write_lock) {
+		if (!m->current_pgpath ||
+		    (!m->queue_io && (m->repeat_count && m->repeat_count == 1))) {
+			use_write_lock = true;
+			read_unlock_irq(&m->lock);
+			goto retry;
+		}
+	} else if (!m->current_pgpath ||
+		   (!m->queue_io && (m->repeat_count && --m->repeat_count == 0)))
 		__choose_pgpath(m, nr_bytes);
 
 	pgpath = m->current_pgpath;

@@ -401,13 +414,23 @@ static int __multipath_map(struct dm_target *ti, struct request *clone,
 			r = -EIO;	/* Failed */
 		goto out_unlock;
 	} else if (m->queue_io || m->pg_init_required) {
+		if (!use_write_lock) {
+			use_write_lock = true;
+			read_unlock_irq(&m->lock);
+			goto retry;
+		}
 		__pg_init_all_paths(m);
 		goto out_unlock;
 	}
 
+	if (!use_write_lock)
+		read_unlock_irq(&m->lock);
+	else
+		write_unlock_irq(&m->lock);
+
 	if (set_mapinfo(m, map_context) < 0)
 		/* ENOMEM, requeue */
-		goto out_unlock;
+		return r;
 
 	mpio = map_context->ptr;
 	mpio->pgpath = pgpath;

@@ -415,8 +438,6 @@ static int __multipath_map(struct dm_target *ti, struct request *clone,
 
 	bdev = pgpath->path.dev->bdev;
 
-	spin_unlock_irq(&m->lock);
-
 	if (clone) {
 		/* Old request-based interface: allocated clone is passed in */
 		clone->q = bdev_get_queue(bdev);

@@ -443,7 +464,10 @@ static int __multipath_map(struct dm_target *ti, struct request *clone,
 	return DM_MAPIO_REMAPPED;
 
 out_unlock:
-	spin_unlock_irq(&m->lock);
+	if (!use_write_lock)
+		read_unlock_irq(&m->lock);
+	else
+		write_unlock_irq(&m->lock);
 
 	return r;
 }

@@ -474,14 +498,15 @@ static int queue_if_no_path(struct multipath *m, unsigned queue_if_no_path,
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&m->lock, flags);
+	write_lock_irqsave(&m->lock, flags);
 
 	if (save_old_value)
 		m->saved_queue_if_no_path = m->queue_if_no_path;
 	else
 		m->saved_queue_if_no_path = queue_if_no_path;
 	m->queue_if_no_path = queue_if_no_path;
-	spin_unlock_irqrestore(&m->lock, flags);
+
+	write_unlock_irqrestore(&m->lock, flags);
 
 	if (!queue_if_no_path)
 		dm_table_run_md_queue_async(m->ti->table);

@@ -898,12 +923,12 @@ static void multipath_wait_for_pg_init_completion(struct multipath *m)
 	while (1) {
 		set_current_state(TASK_UNINTERRUPTIBLE);
 
-		spin_lock_irqsave(&m->lock, flags);
+		read_lock_irqsave(&m->lock, flags);
 		if (!m->pg_init_in_progress) {
-			spin_unlock_irqrestore(&m->lock, flags);
+			read_unlock_irqrestore(&m->lock, flags);
 			break;
 		}
-		spin_unlock_irqrestore(&m->lock, flags);
+		read_unlock_irqrestore(&m->lock, flags);
 
 		io_schedule();
 	}

@@ -916,18 +941,18 @@ static void flush_multipath_work(struct multipath *m)
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&m->lock, flags);
+	write_lock_irqsave(&m->lock, flags);
 	m->pg_init_disabled = 1;
-	spin_unlock_irqrestore(&m->lock, flags);
+	write_unlock_irqrestore(&m->lock, flags);
 
 	flush_workqueue(kmpath_handlerd);
 	multipath_wait_for_pg_init_completion(m);
 	flush_workqueue(kmultipathd);
 	flush_work(&m->trigger_event);
 
-	spin_lock_irqsave(&m->lock, flags);
+	write_lock_irqsave(&m->lock, flags);
 	m->pg_init_disabled = 0;
-	spin_unlock_irqrestore(&m->lock, flags);
+	write_unlock_irqrestore(&m->lock, flags);
 }
 
 static void multipath_dtr(struct dm_target *ti)

@@ -946,7 +971,7 @@ static int fail_path(struct pgpath *pgpath)
 	unsigned long flags;
 	struct multipath *m = pgpath->pg->m;
 
-	spin_lock_irqsave(&m->lock, flags);
+	write_lock_irqsave(&m->lock, flags);
 
 	if (!pgpath->is_active)
 		goto out;

@@ -968,7 +993,7 @@ static int fail_path(struct pgpath *pgpath)
 	schedule_work(&m->trigger_event);
 
 out:
-	spin_unlock_irqrestore(&m->lock, flags);
+	write_unlock_irqrestore(&m->lock, flags);
 
 	return 0;
 }

@@ -982,7 +1007,7 @@ static int reinstate_path(struct pgpath *pgpath)
 	unsigned long flags;
 	struct multipath *m = pgpath->pg->m;
 
-	spin_lock_irqsave(&m->lock, flags);
+	write_lock_irqsave(&m->lock, flags);
 
 	if (pgpath->is_active)
 		goto out;

@@ -1014,7 +1039,7 @@ static int reinstate_path(struct pgpath *pgpath)
 	schedule_work(&m->trigger_event);
 
 out:
-	spin_unlock_irqrestore(&m->lock, flags);
+	write_unlock_irqrestore(&m->lock, flags);
 	if (run_queue)
 		dm_table_run_md_queue_async(m->ti->table);
 

@@ -1049,13 +1074,13 @@ static void bypass_pg(struct multipath *m, struct priority_group *pg,
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&m->lock, flags);
+	write_lock_irqsave(&m->lock, flags);
 
 	pg->bypassed = bypassed;
 	m->current_pgpath = NULL;
 	m->current_pg = NULL;
 
-	spin_unlock_irqrestore(&m->lock, flags);
+	write_unlock_irqrestore(&m->lock, flags);
 
 	schedule_work(&m->trigger_event);
 }

@@ -1076,7 +1101,7 @@ static int switch_pg_num(struct multipath *m, const char *pgstr)
 		return -EINVAL;
 	}
 
-	spin_lock_irqsave(&m->lock, flags);
+	write_lock_irqsave(&m->lock, flags);
 	list_for_each_entry(pg, &m->priority_groups, list) {
 		pg->bypassed = 0;
 		if (--pgnum)

@@ -1086,7 +1111,7 @@ static int switch_pg_num(struct multipath *m, const char *pgstr)
 		m->current_pg = NULL;
 		m->next_pg = pg;
 	}
-	spin_unlock_irqrestore(&m->lock, flags);
+	write_unlock_irqrestore(&m->lock, flags);
 
 	schedule_work(&m->trigger_event);
 	return 0;

@@ -1125,14 +1150,14 @@ static int pg_init_limit_reached(struct multipath *m, struct pgpath *pgpath)
 	unsigned long flags;
 	int limit_reached = 0;
 
-	spin_lock_irqsave(&m->lock, flags);
+	write_lock_irqsave(&m->lock, flags);
 
 	if (m->pg_init_count <= m->pg_init_retries && !m->pg_init_disabled)
 		m->pg_init_required = 1;
 	else
 		limit_reached = 1;
 
-	spin_unlock_irqrestore(&m->lock, flags);
+	write_unlock_irqrestore(&m->lock, flags);
 
 	return limit_reached;
 }

@@ -1186,7 +1211,7 @@ static void pg_init_done(void *data, int errors)
 		fail_path(pgpath);
 	}
 
-	spin_lock_irqsave(&m->lock, flags);
+	write_lock_irqsave(&m->lock, flags);
 	if (errors) {
 		if (pgpath == m->current_pgpath) {
 			DMERR("Could not failover device. Error %d.", errors);

@@ -1213,7 +1238,7 @@ static void pg_init_done(void *data, int errors)
 	wake_up(&m->pg_init_wait);
 
 out:
-	spin_unlock_irqrestore(&m->lock, flags);
+	write_unlock_irqrestore(&m->lock, flags);
 }
 
 static void activate_path(struct work_struct *work)

@@ -1272,7 +1297,7 @@ static int do_end_io(struct multipath *m, struct request *clone,
 	if (mpio->pgpath)
 		fail_path(mpio->pgpath);
 
-	spin_lock_irqsave(&m->lock, flags);
+	read_lock_irqsave(&m->lock, flags);
 	if (!m->nr_valid_paths) {
 		if (!m->queue_if_no_path) {
 			if (!__must_push_back(m))

@@ -1282,7 +1307,7 @@ static int do_end_io(struct multipath *m, struct request *clone,
 				r = error;
 		}
 	}
-	spin_unlock_irqrestore(&m->lock, flags);
+	read_unlock_irqrestore(&m->lock, flags);
 
 	return r;
 }

@@ -1340,9 +1365,9 @@ static void multipath_resume(struct dm_target *ti)
 	struct multipath *m = (struct multipath *) ti->private;
 	unsigned long flags;
 
-	spin_lock_irqsave(&m->lock, flags);
+	write_lock_irqsave(&m->lock, flags);
 	m->queue_if_no_path = m->saved_queue_if_no_path;
-	spin_unlock_irqrestore(&m->lock, flags);
+	write_unlock_irqrestore(&m->lock, flags);
 }
 
 /*

@@ -1372,7 +1397,7 @@ static void multipath_status(struct dm_target *ti, status_type_t type,
 	unsigned pg_num;
 	char state;
 
-	spin_lock_irqsave(&m->lock, flags);
+	read_lock_irqsave(&m->lock, flags);
 
 	/* Features */
 	if (type == STATUSTYPE_INFO)

@@ -1467,7 +1492,7 @@ static void multipath_status(struct dm_target *ti, status_type_t type,
 		break;
 	}
 
-	spin_unlock_irqrestore(&m->lock, flags);
+	read_unlock_irqrestore(&m->lock, flags);
 }
 
 static int multipath_message(struct dm_target *ti, unsigned argc, char **argv)

@@ -1534,16 +1559,27 @@ out:
 }
 
 static int multipath_prepare_ioctl(struct dm_target *ti,
-		struct block_device **bdev, fmode_t *mode)
+				   struct block_device **bdev, fmode_t *mode)
 {
 	struct multipath *m = ti->private;
 	unsigned long flags;
 	int r;
+	bool use_write_lock = false;
 
-	spin_lock_irqsave(&m->lock, flags);
+retry:
+	if (!use_write_lock)
+		read_lock_irqsave(&m->lock, flags);
+	else
+		write_lock_irqsave(&m->lock, flags);
 
-	if (!m->current_pgpath)
+	if (!m->current_pgpath) {
+		if (!use_write_lock) {
+			use_write_lock = true;
+			read_unlock_irqrestore(&m->lock, flags);
+			goto retry;
+		}
 		__choose_pgpath(m, 0);
+	}
 
 	if (m->current_pgpath) {
 		if (!m->queue_io) {

@@ -1562,17 +1598,20 @@ static int multipath_prepare_ioctl(struct dm_target *ti,
 			r = -EIO;
 	}
 
-	spin_unlock_irqrestore(&m->lock, flags);
+	if (!use_write_lock)
+		read_unlock_irqrestore(&m->lock, flags);
+	else
+		write_unlock_irqrestore(&m->lock, flags);
 
 	if (r == -ENOTCONN) {
-		spin_lock_irqsave(&m->lock, flags);
+		write_lock_irqsave(&m->lock, flags);
 		if (!m->current_pg) {
 			/* Path status changed, redo selection */
 			__choose_pgpath(m, 0);
 		}
 		if (m->pg_init_required)
 			__pg_init_all_paths(m);
-		spin_unlock_irqrestore(&m->lock, flags);
+		write_unlock_irqrestore(&m->lock, flags);
 		dm_table_run_md_queue_async(m->ti->table);
 	}
 

@@ -1627,7 +1666,7 @@ static int multipath_busy(struct dm_target *ti)
 	struct pgpath *pgpath;
 	unsigned long flags;
 
-	spin_lock_irqsave(&m->lock, flags);
+	read_lock_irqsave(&m->lock, flags);
 
 	/* pg_init in progress or no paths available */
 	if (m->pg_init_in_progress ||

@@ -1674,7 +1713,7 @@ static int multipath_busy(struct dm_target *ti)
 		busy = 0;
 
 out:
-	spin_unlock_irqrestore(&m->lock, flags);
+	read_unlock_irqrestore(&m->lock, flags);
 
 	return busy;
 }
 
--

dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

Quoted 2 lines · Mike Snitzer

On Tue, Jan 19 2016 at 5:45P -0500,
Mike Snitzer <[email protected]> wrote:

I don't think this is going to help __multipath_map() without some configuration changes. Now that we're running on already merged requests instead of bios, the m->repeat_count is almost always set to 1, so we call the path_selector every time, which means that we'll always need the write lock. Bumping up the number of IOs we send before calling the path selector again will give this patch a change to do some good here.

To do that you need to set:

rr_min_io_rq <something_bigger_than_one>

in the defaults section of /etc/multipath.conf and then reload the multipathd service.

The patch should hopefully help in multipath_busy() regardless of the the rr_min_io_rq setting.

Quoted 14 lines · Benjamin Marzinski

Quoted 2 lines · Mike Snitzer

On Tue, Jan 19 2016 at 5:45P -0500,
Mike Snitzer <[email protected]> wrote:

I don't think this is going to help __multipath_map() without some configuration changes. Now that we're running on already merged requests instead of bios, the m->repeat_count is almost always set to 1, so we call the path_selector every time, which means that we'll always need the write lock. Bumping up the number of IOs we send before calling the path selector again will give this patch a change to do some good here.

To do that you need to set:

Quoted 7 lines

rr_min_io_rq <something_bigger_than_one>

in the defaults section of /etc/multipath.conf and then reload the multipathd service.

The patch should hopefully help in multipath_busy() regardless of the the rr_min_io_rq setting.

This patch, while generic, is meant to help the blk-mq case. A blk-mq request_queue doesn't have an elevator so the requests will not have seen merging.

But yes, implied in the patch is the requirement to increase m->repeat_count via multipathd's rr_min_io_rq (I'll backfill a proper header once it is tested).

Quoted 29 lines · Mike Snitzer

Quoted 14 lines · Benjamin Marzinski

Quoted 2 lines · Mike Snitzer

On Tue, Jan 19 2016 at 5:45P -0500,
Mike Snitzer <[email protected]> wrote:

I don't think this is going to help __multipath_map() without some configuration changes. Now that we're running on already merged requests instead of bios, the m->repeat_count is almost always set to 1, so we call the path_selector every time, which means that we'll always need the write lock. Bumping up the number of IOs we send before calling the path selector again will give this patch a change to do some good here.

To do that you need to set:

Quoted 7 lines

rr_min_io_rq <something_bigger_than_one>

in the defaults section of /etc/multipath.conf and then reload the multipathd service.

The patch should hopefully help in multipath_busy() regardless of the the rr_min_io_rq setting.

This patch, while generic, is meant to help the blk-mq case. A blk-mq request_queue doesn't have an elevator so the requests will not have seen merging.

Quoted 4 lines

But yes, implied in the patch is the requirement to increase m->repeat_count via multipathd's rr_min_io_rq (I'll backfill a proper header once it is tested).

But that would defeat load balancing, would it not?
IE when you want to do load balancing you would constantly change paths, thereby always taking the write lock.
Which would render the patch pointless.

I was rather wondering if we could expose all active paths as hardware contexts and let blk-mq do the I/O mapping. That way we would only have to take the write lock if we have to choose a new pgpath/priority group ie in the case the active priority group goes down.

[you're killing me.. you nuked all CCs again]

Quoted 1 line · Mike Snitzer

[you're killing me.. you nuked all CCs again]

I think that's the reply-to defaults for dm-devel. They try to do that to me all that time, but fortunately I configure mutt to not blindly follow that bad advice but ask me instead.

Quoted 29 lines · Mike Snitzer

Quoted 14 lines · Benjamin Marzinski

Quoted 2 lines · Mike Snitzer

On Tue, Jan 19 2016 at 5:45P -0500,
Mike Snitzer <[email protected]> wrote:

I don't think this is going to help __multipath_map() without some configuration changes. Now that we're running on already merged requests instead of bios, the m->repeat_count is almost always set to 1, so we call the path_selector every time, which means that we'll always need the write lock. Bumping up the number of IOs we send before calling the path selector again will give this patch a change to do some good here.

To do that you need to set:

Quoted 7 lines

rr_min_io_rq <something_bigger_than_one>

in the defaults section of /etc/multipath.conf and then reload the multipathd service.

The patch should hopefully help in multipath_busy() regardless of the the rr_min_io_rq setting.

This patch, while generic, is meant to help the blk-mq case. A blk-mq request_queue doesn't have an elevator so the requests will not have seen merging.

Quoted 4 lines

But yes, implied in the patch is the requirement to increase m->repeat_count via multipathd's rr_min_io_rq (I'll backfill a proper header once it is tested).

But that would defeat load balancing, would it not?
IE when you want to do load balancing you would constantly change paths, thereby always taking the write lock.
Which would render the patch pointless.

I was rather wondering if we could expose all active paths as hardware contexts and let blk-mq do the I/O mapping. That way we would only have to take the write lock if we have to choose a new pgpath/priority group ie in the case the active priority group goes down.

Cheers,

Hannes

Signature · 10 lines

Dr. Hannes Reinecke		   Teamlead Storage & Networking
[email protected]			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

Quoted 11 lines

I don't think this is going to help __multipath_map() without some configuration changes. Now that we're running on already merged requests instead of bios, the m->repeat_count is almost always set to 1, so we call the path_selector every time, which means that we'll always need the write lock. Bumping up the number of IOs we send before calling the path selector again will give this patch a change to do some good here.

To do that you need to set:

Quoted 7 lines

rr_min_io_rq <something_bigger_than_one>

in the defaults section of /etc/multipath.conf and then reload the multipathd service.

The patch should hopefully help in multipath_busy() regardless of the the rr_min_io_rq setting.

This patch, while generic, is meant to help the blk-mq case. A blk-mq request_queue doesn't have an elevator so the requests will not have seen merging.

But yes, implied in the patch is the requirement to increase m->repeat_count via multipathd's rr_min_io_rq (I'll backfill a proper header once it is tested).

Quoted 23 lines

Quoted 11 lines

I don't think this is going to help __multipath_map() without some configuration changes. Now that we're running on already merged requests instead of bios, the m->repeat_count is almost always set to 1, so we call the path_selector every time, which means that we'll always need the write lock. Bumping up the number of IOs we send before calling the path selector again will give this patch a change to do some good here.

To do that you need to set:

Quoted 7 lines

rr_min_io_rq <something_bigger_than_one>

in the defaults section of /etc/multipath.conf and then reload the multipathd service.

The patch should hopefully help in multipath_busy() regardless of the the rr_min_io_rq setting.

This patch, while generic, is meant to help the blk-mq case. A blk-mq request_queue doesn't have an elevator so the requests will not have seen merging.

Quoted 3 lines

But yes, implied in the patch is the requirement to increase m->repeat_count via multipathd's rr_min_io_rq (I'll backfill a proper header once it is tested).

I'll test it once I get some spare time (hopefully soon...)

Quoted 23 lines · Sagi Grimberg

Quoted 23 lines

Quoted 11 lines

I don't think this is going to help __multipath_map() without some configuration changes. Now that we're running on already merged requests instead of bios, the m->repeat_count is almost always set to 1, so we call the path_selector every time, which means that we'll always need the write lock. Bumping up the number of IOs we send before calling the path selector again will give this patch a change to do some good here.

To do that you need to set:

Quoted 7 lines

rr_min_io_rq <something_bigger_than_one>

in the defaults section of /etc/multipath.conf and then reload the multipathd service.

The patch should hopefully help in multipath_busy() regardless of the the rr_min_io_rq setting.

This patch, while generic, is meant to help the blk-mq case. A blk-mq request_queue doesn't have an elevator so the requests will not have seen merging.

Quoted 5 lines

Quoted 3 lines

But yes, implied in the patch is the requirement to increase m->repeat_count via multipathd's rr_min_io_rq (I'll backfill a proper header once it is tested).

I'll test it once I get some spare time (hopefully soon...)

OK thanks.

BTW, I _cannot_ get null_blk to come even close to your reported 1500K+ IOPs on 2 "fast" systems I have access to. Which arguments are you loading the null_blk module with?

I've been using:
modprobe null_blk gb=4 bs=4096 nr_devices=1 queue_mode=2 submit_queues=12

On my 1 system is a 12 core single socket, single NUMA node with 12G of memory, I can only get ~500K read IOPs and ~85K write IOPs.

On another much larger system with 72 cores and 4 NUMA nodes with 128G of memory, I can only get ~310K read IOPs and ~175K write IOPs.

Quoted 8 lines · Mike Snitzer

BTW, I _cannot_ get null_blk to come even close to your reported 1500K+ IOPs on 2 "fast" systems I have access to. Which arguments are you loading the null_blk module with?

I've been using:
modprobe null_blk gb=4 bs=4096 nr_devices=1 queue_mode=2 submit_queues=12

Quoted 5 lines

On my 1 system is a 12 core single socket, single NUMA node with 12G of memory, I can only get ~500K read IOPs and ~85K write IOPs.

On another much larger system with 72 cores and 4 NUMA nodes with 128G of memory, I can only get ~310K read IOPs and ~175K write IOPs.

Look at the completion method (irqmode) and completion time (completion_nsec).

Quoted 9 lines · Jens Axboe

Quoted 8 lines · Mike Snitzer

BTW, I _cannot_ get null_blk to come even close to your reported 1500K+ IOPs on 2 "fast" systems I have access to. Which arguments are you loading the null_blk module with?

I've been using:
modprobe null_blk gb=4 bs=4096 nr_devices=1 queue_mode=2 submit_queues=12

Quoted 8 lines

Quoted 5 lines

On my 1 system is a 12 core single socket, single NUMA node with 12G of memory, I can only get ~500K read IOPs and ~85K write IOPs.

On another much larger system with 72 cores and 4 NUMA nodes with 128G of memory, I can only get ~310K read IOPs and ~175K write IOPs.

Look at the completion method (irqmode) and completion time (completion_nsec).

OK, I found that queue_mode=0 (bio-based) is _much_ faster than blk-mq (2, the default). Improving to ~950K read IOPs and ~675K write IOPs (on the single numa node system).

Default for irqmode is 1 (softirq). 2 (timer) yields poor results. 0 (none) seems slightly slower than 1.

And if I use completion_nsec=1 I can bump up to ~990K read IOPs.

Seems the best, for IOPs, so far on this system is with:
modprobe null_blk gb=4 bs=4096 nr_devices=1 queue_mode=0 irqmode=1 completion_nsec=1 submit_queues=4

Quoted 28 lines · Mike Snitzer

Quoted 9 lines · Jens Axboe

Quoted 8 lines · Mike Snitzer

BTW, I _cannot_ get null_blk to come even close to your reported 1500K+ IOPs on 2 "fast" systems I have access to. Which arguments are you loading the null_blk module with?

I've been using:
modprobe null_blk gb=4 bs=4096 nr_devices=1 queue_mode=2 submit_queues=12

Quoted 8 lines

Quoted 5 lines

On my 1 system is a 12 core single socket, single NUMA node with 12G of memory, I can only get ~500K read IOPs and ~85K write IOPs.

On another much larger system with 72 cores and 4 NUMA nodes with 128G of memory, I can only get ~310K read IOPs and ~175K write IOPs.

Look at the completion method (irqmode) and completion time (completion_nsec).

OK, I found that queue_mode=0 (bio-based) is _much_ faster than blk-mq (2, the default). Improving to ~950K read IOPs and ~675K write IOPs (on the single numa node system).

Default for irqmode is 1 (softirq). 2 (timer) yields poor results. 0 (none) seems slightly slower than 1.

Quoted 4 lines

And if I use completion_nsec=1 I can bump up to ~990K read IOPs.

Seems the best, for IOPs, so far on this system is with:
modprobe null_blk gb=4 bs=4096 nr_devices=1 queue_mode=0 irqmode=1 completion_nsec=1 submit_queues=4

That sounds a bit odd. queue_mode=0 will always be a bit faster, depending on how many threads, etc. But from 310K to 950K, that sounds very suspicious.

And 500K/85K read/write is very low. Just did a quickie on a 2 node box I have here, single thread performance with queue_mode=2 is around 500K/500K read/write.

Yeah, I was using some crap ioping test to get those 500K/85K results. I've now switched to the fio test Sagi shared [...]

Quoted 23 lines · Sagi Grimberg

Quoted 23 lines

Quoted 11 lines

I don't think this is going to help __multipath_map() without some configuration changes. Now that we're running on already merged requests instead of bios, the m->repeat_count is almost always set to 1, so we call the path_selector every time, which means that we'll always need the write lock. Bumping up the number of IOs we send before calling the path selector again will give this patch a change to do some good here.

To do that you need to set:

Quoted 7 lines

rr_min_io_rq <something_bigger_than_one>

in the defaults section of /etc/multipath.conf and then reload the multipathd service.

The patch should hopefully help in multipath_busy() regardless of the the rr_min_io_rq setting.

This patch, while generic, is meant to help the blk-mq case. A blk-mq request_queue doesn't have an elevator so the requests will not have seen merging.

Quoted 5 lines

Quoted 3 lines

But yes, implied in the patch is the requirement to increase m->repeat_count via multipathd's rr_min_io_rq (I'll backfill a proper header once it is tested).

I'll test it once I get some spare time (hopefully soon...)

OK thanks.

BTW, I _cannot_ get null_blk to come even close to your reported 1500K+ IOPs on 2 "fast" systems I have access to. Which arguments are you loading the null_blk module with?

I've been using:
modprobe null_blk gb=4 bs=4096 nr_devices=1 queue_mode=2 submit_queues=12

Quoted 7 lines · Mike Snitzer

BTW, I _cannot_ get null_blk to come even close to your reported 1500K+ IOPs on 2 "fast" systems I have access to. Which arguments are you loading the null_blk module with?

I've been using:
modprobe null_blk gb=4 bs=4096 nr_devices=1 queue_mode=2 submit_queues=12

$ for f in /sys/module/null_blk/parameters/*; do echo $f; cat $f; done
/sys/module/null_blk/parameters/bs
512
/sys/module/null_blk/parameters/completion_nsec
10000
/sys/module/null_blk/parameters/gb
250
/sys/module/null_blk/parameters/home_node
-1
/sys/module/null_blk/parameters/hw_queue_depth
64
/sys/module/null_blk/parameters/irqmode
1
/sys/module/null_blk/parameters/nr_devices
2
/sys/module/null_blk/parameters/queue_mode
2
/sys/module/null_blk/parameters/submit_queues
24
/sys/module/null_blk/parameters/use_lightnvm
N
/sys/module/null_blk/parameters/use_per_node_hctx
N

$ fio --group_reporting --rw=randread --bs=4k --numjobs=24
--iodepth=32 --runtime=99999999 --time_based --loops=1
--ioengine=libaio --direct=1 --invalidate=1 --randrepeat=1
--norandommap --exitall --name task_nullb0 --filename=/dev/nullb0
task_nullb0: (g=0): rw=randread, bs=4K-4K/4K-4K/4K-4K,
ioengine=libaio, iodepth=32
...
fio-2.1.10
Starting 24 processes
Jobs: 24 (f=24): [rrrrrrrrrrrrrrrrrrrrrrrr] [0.0% done]
[7234MB/0KB/0KB /s] [1852K/0/0 iops] [eta 1157d:09h:46m:22s]

Quoted 35 lines · Sagi Grimberg

Quoted 7 lines · Mike Snitzer

BTW, I _cannot_ get null_blk to come even close to your reported 1500K+ IOPs on 2 "fast" systems I have access to. Which arguments are you loading the null_blk module with?

I've been using:
modprobe null_blk gb=4 bs=4096 nr_devices=1 queue_mode=2 submit_queues=12

$ for f in /sys/module/null_blk/parameters/*; do echo $f; cat $f; done
/sys/module/null_blk/parameters/bs
512
/sys/module/null_blk/parameters/completion_nsec
10000
/sys/module/null_blk/parameters/gb
250
/sys/module/null_blk/parameters/home_node
-1
/sys/module/null_blk/parameters/hw_queue_depth
64
/sys/module/null_blk/parameters/irqmode
1
/sys/module/null_blk/parameters/nr_devices
2
/sys/module/null_blk/parameters/queue_mode
2
/sys/module/null_blk/parameters/submit_queues
24
/sys/module/null_blk/parameters/use_lightnvm
N
/sys/module/null_blk/parameters/use_per_node_hctx
N

Quoted 11 lines

$ fio --group_reporting --rw=randread --bs=4k --numjobs=24
--iodepth=32 --runtime=99999999 --time_based --loops=1
--ioengine=libaio --direct=1 --invalidate=1 --randrepeat=1
--norandommap --exitall --name task_nullb0 --filename=/dev/nullb0
task_nullb0: (g=0): rw=randread, bs=4K-4K/4K-4K/4K-4K,
ioengine=libaio, iodepth=32
...
fio-2.1.10
Starting 24 processes
Jobs: 24 (f=24): [rrrrrrrrrrrrrrrrrrrrrrrr] [0.0% done]
[7234MB/0KB/0KB /s] [1852K/0/0 iops] [eta 1157d:09h:46m:22s]

Thanks, the number of fio threads was pretty important. I'm still seeing better IOPs with queue_mode=0 (bio-based).

Jobs: 24 (f=24): [r(24)] [11.7% done] [11073MB/0KB/0KB /s] [2835K/0/0 iops] [eta 14m:42s]

(with queue_mode=2 I get ~1930K IOPs.. which I need to use to stack request-based DM multipath ontop)

Now I can focus on why dm-multipath is slow...

Quoted 53 lines · Mike Snitzer

Quoted 35 lines · Sagi Grimberg

Quoted 7 lines · Mike Snitzer

BTW, I _cannot_ get null_blk to come even close to your reported 1500K+ IOPs on 2 "fast" systems I have access to. Which arguments are you loading the null_blk module with?

I've been using:
modprobe null_blk gb=4 bs=4096 nr_devices=1 queue_mode=2 submit_queues=12

$ for f in /sys/module/null_blk/parameters/*; do echo $f; cat $f; done
/sys/module/null_blk/parameters/bs
512
/sys/module/null_blk/parameters/completion_nsec
10000
/sys/module/null_blk/parameters/gb
250
/sys/module/null_blk/parameters/home_node
-1
/sys/module/null_blk/parameters/hw_queue_depth
64
/sys/module/null_blk/parameters/irqmode
1
/sys/module/null_blk/parameters/nr_devices
2
/sys/module/null_blk/parameters/queue_mode
2
/sys/module/null_blk/parameters/submit_queues
24
/sys/module/null_blk/parameters/use_lightnvm
N
/sys/module/null_blk/parameters/use_per_node_hctx
N

Quoted 11 lines

$ fio --group_reporting --rw=randread --bs=4k --numjobs=24
--iodepth=32 --runtime=99999999 --time_based --loops=1
--ioengine=libaio --direct=1 --invalidate=1 --randrepeat=1
--norandommap --exitall --name task_nullb0 --filename=/dev/nullb0
task_nullb0: (g=0): rw=randread, bs=4K-4K/4K-4K/4K-4K,
ioengine=libaio, iodepth=32
...
fio-2.1.10
Starting 24 processes
Jobs: 24 (f=24): [rrrrrrrrrrrrrrrrrrrrrrrr] [0.0% done]
[7234MB/0KB/0KB /s] [1852K/0/0 iops] [eta 1157d:09h:46m:22s]

Thanks, the number of fio threads was pretty important. I'm still seeing better IOPs with queue_mode=0 (bio-based).

Quoted 6 lines

Jobs: 24 (f=24): [r(24)] [11.7% done] [11073MB/0KB/0KB /s] [2835K/0/0 iops] [eta 14m:42s]

(with queue_mode=2 I get ~1930K IOPs.. which I need to use to stack request-based DM multipath ontop)

Now I can focus on why dm-multipath is slow...

queue_mode=0 doesn't do a whole lot, once you add the required bits for a normal driver, that will eat up a bit of overhead. The point was to retain scaling, and to avoid drivers having to built support for the required functionality from scratch. If we do that once and fast/correct, then all mq drivers get it.

That said, your jump is a big one. Some of that is support functionality, and some of it (I bet) is just doing io stats. If you disable io stats with queue_mode=2, the performance will most likely increase. Other things we just can't disable or don't want to disable, if we are going to keep this as an indication of what a real driver could do through the mq stack.

Now, if queue_mode=1 is faster, then there's certainly an issue!

Quoted 32 lines · Mike Snitzer

Quoted 23 lines · Sagi Grimberg

Quoted 23 lines

Quoted 11 lines

I don't think this is going to help __multipath_map() without some configuration changes. Now that we're running on already merged requests instead of bios, the m->repeat_count is almost always set to 1, so we call the path_selector every time, which means that we'll always need the write lock. Bumping up the number of IOs we send before calling the path selector again will give this patch a change to do some good here.

To do that you need to set:

Quoted 7 lines

rr_min_io_rq <something_bigger_than_one>

in the defaults section of /etc/multipath.conf and then reload the multipathd service.

The patch should hopefully help in multipath_busy() regardless of the the rr_min_io_rq setting.

This patch, while generic, is meant to help the blk-mq case. A blk-mq request_queue doesn't have an elevator so the requests will not have seen merging.

Quoted 5 lines

Quoted 3 lines

But yes, implied in the patch is the requirement to increase m->repeat_count via multipathd's rr_min_io_rq (I'll backfill a proper header once it is tested).

I'll test it once I get some spare time (hopefully soon...)

Quoted 8 lines

OK thanks.

BTW, I _cannot_ get null_blk to come even close to your reported 1500K+ IOPs on 2 "fast" systems I have access to. Which arguments are you loading the null_blk module with?

I've been using:
modprobe null_blk gb=4 bs=4096 nr_devices=1 queue_mode=2 submit_queues=12

$ for f in /sys/module/null_blk/parameters/*; do echo $f; cat $f; done
/sys/module/null_blk/parameters/bs
512
/sys/module/null_blk/parameters/completion_nsec
10000
/sys/module/null_blk/parameters/gb
250
/sys/module/null_blk/parameters/home_node
-1
/sys/module/null_blk/parameters/hw_queue_depth
64
/sys/module/null_blk/parameters/irqmode
1
/sys/module/null_blk/parameters/nr_devices
2
/sys/module/null_blk/parameters/queue_mode
2
/sys/module/null_blk/parameters/submit_queues
24
/sys/module/null_blk/parameters/use_lightnvm
N
/sys/module/null_blk/parameters/use_per_node_hctx
N

$ fio --group_reporting --rw=randread --bs=4k --numjobs=24
--iodepth=32 --runtime=99999999 --time_based --loops=1
--ioengine=libaio --direct=1 --invalidate=1 --randrepeat=1
--norandommap --exitall --name task_nullb0 --filename=/dev/nullb0
task_nullb0: (g=0): rw=randread, bs=4K-4K/4K-4K/4K-4K,
ioengine=libaio, iodepth=32
...
fio-2.1.10
Starting 24 processes
Jobs: 24 (f=24): [rrrrrrrrrrrrrrrrrrrrrrrr] [0.0% done]
[7234MB/0KB/0KB /s] [1852K/0/0 iops] [eta 1157d:09h:46m:22s]

Quoted 68 lines · Sagi Grimberg

Quoted 32 lines · Mike Snitzer

Quoted 23 lines · Sagi Grimberg

Quoted 23 lines

Quoted 11 lines

I don't think this is going to help __multipath_map() without some configuration changes. Now that we're running on already merged requests instead of bios, the m->repeat_count is almost always set to 1, so we call the path_selector every time, which means that we'll always need the write lock. Bumping up the number of IOs we send before calling the path selector again will give this patch a change to do some good here.

To do that you need to set:

Quoted 7 lines

rr_min_io_rq <something_bigger_than_one>

in the defaults section of /etc/multipath.conf and then reload the multipathd service.

The patch should hopefully help in multipath_busy() regardless of the the rr_min_io_rq setting.

This patch, while generic, is meant to help the blk-mq case. A blk-mq request_queue doesn't have an elevator so the requests will not have seen merging.

Quoted 5 lines

Quoted 3 lines

But yes, implied in the patch is the requirement to increase m->repeat_count via multipathd's rr_min_io_rq (I'll backfill a proper header once it is tested).

I'll test it once I get some spare time (hopefully soon...)

Quoted 8 lines

OK thanks.

BTW, I _cannot_ get null_blk to come even close to your reported 1500K+ IOPs on 2 "fast" systems I have access to. Which arguments are you loading the null_blk module with?

I've been using:
modprobe null_blk gb=4 bs=4096 nr_devices=1 queue_mode=2 submit_queues=12

$ for f in /sys/module/null_blk/parameters/*; do echo $f; cat $f; done
/sys/module/null_blk/parameters/bs
512
/sys/module/null_blk/parameters/completion_nsec
10000
/sys/module/null_blk/parameters/gb
250
/sys/module/null_blk/parameters/home_node
-1
/sys/module/null_blk/parameters/hw_queue_depth
64
/sys/module/null_blk/parameters/irqmode
1
/sys/module/null_blk/parameters/nr_devices
2
/sys/module/null_blk/parameters/queue_mode
2
/sys/module/null_blk/parameters/submit_queues
24
/sys/module/null_blk/parameters/use_lightnvm
N
/sys/module/null_blk/parameters/use_per_node_hctx
N

Quoted 11 lines

$ fio --group_reporting --rw=randread --bs=4k --numjobs=24
--iodepth=32 --runtime=99999999 --time_based --loops=1
--ioengine=libaio --direct=1 --invalidate=1 --randrepeat=1
--norandommap --exitall --name task_nullb0 --filename=/dev/nullb0
task_nullb0: (g=0): rw=randread, bs=4K-4K/4K-4K/4K-4K,
ioengine=libaio, iodepth=32
...
fio-2.1.10
Starting 24 processes
Jobs: 24 (f=24): [rrrrrrrrrrrrrrrrrrrrrrrr] [0.0% done]
[7234MB/0KB/0KB /s] [1852K/0/0 iops] [eta 1157d:09h:46m:22s]

Your test above is prone to exhaust the dm-mpath blk-mq tags (128) because 24 threads * 32 easily exceeds 128 (by a factor of 6).

I found that we were context switching (via bt_get's io_schedule) waiting for tags to become available.

This is embarassing but, until Jens told me today, I was oblivious to the fact that the number of blk-mq's tags per hw_queue was defined by tag_set.queue_depth.

Previously request-based DM's blk-mq support had:
md->tag_set.queue_depth = BLKDEV_MAX_RQ; (again: 128)

Now I have a patch that allows tuning queue_depth via dm_mod module parameter. And I'll likely bump the default to 4096 or something (doing so eliminated blocking in bt_get).

But eliminating the tags bottleneck only raised my read IOPs from ~600K to ~800K (using 1 hw_queue for both null_blk and dm-mpath).

When I raise nr_hw_queues to 4 for null_blk (keeping dm-mq at 1) I see a whole lot more context switching due to request-based DM's use of ksoftirqd (and kworkers) for request completion.

So I'm moving on to optimizing the completion path. But at least some progress was made, more to come...

Quoted 25 lines · Mike Snitzer

Your test above is prone to exhaust the dm-mpath blk-mq tags (128) because 24 threads * 32 easily exceeds 128 (by a factor of 6).

I found that we were context switching (via bt_get's io_schedule) waiting for tags to become available.

This is embarassing but, until Jens told me today, I was oblivious to the fact that the number of blk-mq's tags per hw_queue was defined by tag_set.queue_depth.

Previously request-based DM's blk-mq support had:
md->tag_set.queue_depth = BLKDEV_MAX_RQ; (again: 128)

Now I have a patch that allows tuning queue_depth via dm_mod module parameter. And I'll likely bump the default to 4096 or something (doing so eliminated blocking in bt_get).

But eliminating the tags bottleneck only raised my read IOPs from ~600K to ~800K (using 1 hw_queue for both null_blk and dm-mpath).

When I raise nr_hw_queues to 4 for null_blk (keeping dm-mq at 1) I see a whole lot more context switching due to request-based DM's use of ksoftirqd (and kworkers) for request completion.

Quoted 3 lines

So I'm moving on to optimizing the completion path. But at least some progress was made, more to come...

Would you mind sharing your patches?

We're currently doing tests with a high-performance FC setup (16G FC with all-flash storage), and are still 20% short of the announced backend performance.

Just as a side note: we're currently getting 550k IOPs. With unpatched dm-mpath.

So nearly on par with your null-blk setup. but with real hardware. (Which in itself is pretty cool. You should get faster RAM :-)

Quoted 26 lines · Hannes Reinecke

Quoted 25 lines · Mike Snitzer

Your test above is prone to exhaust the dm-mpath blk-mq tags (128) because 24 threads * 32 easily exceeds 128 (by a factor of 6).

I found that we were context switching (via bt_get's io_schedule) waiting for tags to become available.

This is embarassing but, until Jens told me today, I was oblivious to the fact that the number of blk-mq's tags per hw_queue was defined by tag_set.queue_depth.

Previously request-based DM's blk-mq support had:
md->tag_set.queue_depth = BLKDEV_MAX_RQ; (again: 128)

Now I have a patch that allows tuning queue_depth via dm_mod module parameter. And I'll likely bump the default to 4096 or something (doing so eliminated blocking in bt_get).

But eliminating the tags bottleneck only raised my read IOPs from ~600K to ~800K (using 1 hw_queue for both null_blk and dm-mpath).

When I raise nr_hw_queues to 4 for null_blk (keeping dm-mq at 1) I see a whole lot more context switching due to request-based DM's use of ksoftirqd (and kworkers) for request completion.

Quoted 5 lines

Quoted 3 lines

So I'm moving on to optimizing the completion path. But at least some progress was made, more to come...

Would you mind sharing your patches?

I'm still working through this. I'll hopefully have a handful of RFC-level changes by end of day Monday. But could take longer.

One change that I already shared in a previous mail is:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=devel2&id=99ebcaf36d9d1fa3acec98492c36664d57ba8fbd

Quoted 4 lines

We're currently doing tests with a high-performance FC setup (16G FC with all-flash storage), and are still 20% short of the announced backend performance.

Quoted 2 lines

Just as a side note: we're currently getting 550k IOPs. With unpatched dm-mpath.

What is your test workload? If you can share I'll be sure to factor it into my testing.

Quoted 2 lines

So nearly on par with your null-blk setup. but with real hardware. (Which in itself is pretty cool. You should get faster RAM :-)

You've misunderstood what I said my null_blk (RAM) performance is.

My null_blk test gets ~1900K read IOPs. But dm-mpath ontop only gets between 600K and 1000K IOPs depending on $FIO_QUEUE_DEPTH and if I use multiple $NULL_BLK_HW_QUEUES.

Quoted 12 lines · Mike Snitzer

Quoted 3 lines · Hannes Reinecke

So nearly on par with your null-blk setup. but with real hardware. (Which in itself is pretty cool. You should get faster RAM :-)

You've misunderstood what I said my null_blk (RAM) performance is.

My null_blk test gets ~1900K read IOPs. But dm-mpath ontop only gets between 600K and 1000K IOPs depending on $FIO_QUEUE_DEPTH and if I use multiple $NULL_BLK_HW_QUEUES.

Right.
We're using two 16G FC links, each talking to 4 LUNs. With dm-mpath on top. The FC HBAs have a hardware queue depth of roughly 2000, so we might need to tweak the queue depth of the multipath devices, too.

Will be having a look at your patches.

I'm still not clear on where the considerable performance loss is coming from (on null_blk device I see ~1900K read IOPs but I'm still only seeing ~1000K read IOPs when blk-mq DM-multipath is layered ontop). What is very much apparent is: layering dm-mq multipath ontop of null_blk results in a HUGE amount of additional context switches. I can only infer that the request completion for this stacked device (blk-mq queue ontop of blk-mq queue, with 2 completions: 1 for clone completing on underlying device and 1 for original request completing) is the reason for all the extra context switches.

Here are pictures of 'perf report' for perf datat collected using 'perf record -ag -e cs'.

Against null_blk:
http://people.redhat.com/msnitzer/perf-report-cs-null_blk.png

Against dm-mpath ontop of the same null_blk:
http://people.redhat.com/msnitzer/perf-report-cs-dm_mq.png

Quoted 5 lines

Here are pictures of 'perf report' for perf datat collected using 'perf record -ag -e cs'.

Against null_blk:
http://people.redhat.com/msnitzer/perf-report-cs-null_blk.png

if dm-mq nr_hw_queues=1 and null_blk nr_hw_queues=1
cpu : usr=25.53%, sys=74.40%, ctx=1970, majf=0, minf=474
if dm-mq nr_hw_queues=1 and null_blk nr_hw_queues=4
cpu : usr=26.79%, sys=73.15%, ctx=2067, majf=0, minf=479

Quoted 2 lines

Against dm-mpath ontop of the same null_blk:
http://people.redhat.com/msnitzer/perf-report-cs-dm_mq.png

if dm-mq nr_hw_queues=1 and null_blk nr_hw_queues=1
cpu : usr=11.07%, sys=33.90%, ctx=667784, majf=0, minf=466
if dm-mq nr_hw_queues=1 and null_blk nr_hw_queues=4
cpu : usr=15.22%, sys=48.44%, ctx=2314901, majf=0, minf=466

Quoted 9 lines · Mike Snitzer

I'm still not clear on where the considerable performance loss is coming from (on null_blk device I see ~1900K read IOPs but I'm still only seeing ~1000K read IOPs when blk-mq DM-multipath is layered ontop). What is very much apparent is: layering dm-mq multipath ontop of null_blk results in a HUGE amount of additional context switches. I can only infer that the request completion for this stacked device (blk-mq queue ontop of blk-mq queue, with 2 completions: 1 for clone completing on underlying device and 1 for original request completing) is the reason for all the extra context switches.

Starts to explain, certainly not the "reason"; that is still very much TBD...

Quoted 5 lines

Here are pictures of 'perf report' for perf datat collected using 'perf record -ag -e cs'.

Against null_blk:
http://people.redhat.com/msnitzer/perf-report-cs-null_blk.png

if dm-mq nr_hw_queues=1 and null_blk nr_hw_queues=1
cpu : usr=25.53%, sys=74.40%, ctx=1970, majf=0, minf=474
if dm-mq nr_hw_queues=1 and null_blk nr_hw_queues=4
cpu : usr=26.79%, sys=73.15%, ctx=2067, majf=0, minf=479

Quoted 2 lines

Against dm-mpath ontop of the same null_blk:
http://people.redhat.com/msnitzer/perf-report-cs-dm_mq.png

if dm-mq nr_hw_queues=1 and null_blk nr_hw_queues=1
cpu : usr=11.07%, sys=33.90%, ctx=667784, majf=0, minf=466
if dm-mq nr_hw_queues=1 and null_blk nr_hw_queues=4
cpu : usr=15.22%, sys=48.44%, ctx=2314901, majf=0, minf=466

So yeah, the percentages reflected in these respective images didn't do the huge increase in context switches justice... we _must_ figure out why we're seeing so many context switches with dm-mq.

Quoted 35 lines · Mike Snitzer

Quoted 9 lines · Mike Snitzer

I'm still not clear on where the considerable performance loss is coming from (on null_blk device I see ~1900K read IOPs but I'm still only seeing ~1000K read IOPs when blk-mq DM-multipath is layered ontop). What is very much apparent is: layering dm-mq multipath ontop of null_blk results in a HUGE amount of additional context switches. I can only infer that the request completion for this stacked device (blk-mq queue ontop of blk-mq queue, with 2 completions: 1 for clone completing on underlying device and 1 for original request completing) is the reason for all the extra context switches.

Starts to explain, certainly not the "reason"; that is still very much TBD...

Quoted 5 lines

Here are pictures of 'perf report' for perf datat collected using 'perf record -ag -e cs'.

Against null_blk:
http://people.redhat.com/msnitzer/perf-report-cs-null_blk.png

if dm-mq nr_hw_queues=1 and null_blk nr_hw_queues=1
cpu : usr=25.53%, sys=74.40%, ctx=1970, majf=0, minf=474
if dm-mq nr_hw_queues=1 and null_blk nr_hw_queues=4
cpu : usr=26.79%, sys=73.15%, ctx=2067, majf=0, minf=479

Quoted 2 lines

Against dm-mpath ontop of the same null_blk:
http://people.redhat.com/msnitzer/perf-report-cs-dm_mq.png

if dm-mq nr_hw_queues=1 and null_blk nr_hw_queues=1
cpu : usr=11.07%, sys=33.90%, ctx=667784, majf=0, minf=466
if dm-mq nr_hw_queues=1 and null_blk nr_hw_queues=4
cpu : usr=15.22%, sys=48.44%, ctx=2314901, majf=0, minf=466

Quoted 4 lines

So yeah, the percentages reflected in these respective images didn't do the huge increase in context switches justice... we _must_ figure out why we're seeing so many context switches with dm-mq.

Well, the most obvious one being that you're using 1 dm-mq queue vs 4 null_blk queues.
So you will have have to do an additional context switch for 75% of the total I/Os submitted.

Have you tested with 4 dm-mq hw queues?

To avoid context switches we would have to align the dm-mq queues to the underlying blk-mq layout for the paths.

And we need to look at making the main submission path lockless; I was wondering if we really need to take the lock if we don't switch priority groups; maybe we can establish a similar algorithm blk-mq does; if we were to have a queue per valid path in any given priority group we should be able to run lockless and only take the lock if we need to switch priority groups.

But anyway, I'll be looking at your patches.

On Thu, Feb 04 2016 at 1:54am -0500,
Hannes Reinecke <[email protected]> wrote:

Quoted 1 line

But anyway, I'll be looking at your patches.

Thanks, sadly none of the patches are going to fix the performance problems but I do think they are a step forward.

Quoted 3 lines · Mike Snitzer

On Thu, Feb 04 2016 at 1:54am -0500,
Hannes Reinecke <[email protected]> wrote:

[ .. ]

Quoted 2 lines

Quoted 1 line

But anyway, I'll be looking at your patches.

Quoted 3 lines

Thanks, sadly none of the patches are going to fix the performance problems but I do think they are a step forward.

Hmm. I've got a slew of patches converting dm-mpath to use atomic_t and bitops; with that we should be able to move to rcu for path lookup and do away with most of the locking.
Quite raw, though; drop me a mail if you're interested.

Quoted 7 lines · Hannes Reinecke

Quoted 3 lines · Mike Snitzer

On Thu, Feb 04 2016 at 1:54am -0500,
Hannes Reinecke <[email protected]> wrote:

[ .. ]

Quoted 2 lines

Quoted 1 line

But anyway, I'll be looking at your patches.

Quoted 7 lines

Quoted 3 lines

Thanks, sadly none of the patches are going to fix the performance problems but I do think they are a step forward.

Hmm. I've got a slew of patches converting dm-mpath to use atomic_t and bitops; with that we should be able to move to rcu for path lookup and do away with most of the locking.
Quite raw, though; drop me a mail if you're interested.

Hmm, ok I just switched m->lock from spinlock_t to rwlock_t, see:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.6&id=a5226e23a6958ac9b7ade13a983604c43d232c7d

So any patch you'd have in this area would need rebasing. I'll gladly look at what you have (even if it isn't rebased). So yes please share.

(it could be that there isn't a big enough win associated with switching to rwlock_t -- that we could get away without doing that particular churn.. open to that if you think rwlock_t pointless given we'll take the write lock after repeat_count drops to 0)

Quoted 24 lines · Mike Snitzer

Quoted 7 lines · Hannes Reinecke

Quoted 3 lines · Mike Snitzer

On Thu, Feb 04 2016 at 1:54am -0500,
Hannes Reinecke <[email protected]> wrote:

[ .. ]

Quoted 2 lines

Quoted 1 line

But anyway, I'll be looking at your patches.

Quoted 7 lines

Quoted 3 lines

Thanks, sadly none of the patches are going to fix the performance problems but I do think they are a step forward.

Hmm. I've got a slew of patches converting dm-mpath to use atomic_t and bitops; with that we should be able to move to rcu for path lookup and do away with most of the locking.
Quite raw, though; drop me a mail if you're interested.

Hmm, ok I just switched m->lock from spinlock_t to rwlock_t, see:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.6&id=a5226e23a6958ac9b7ade13a983604c43d232c7d

So any patch you'd have in this area would need rebasing. I'll gladly look at what you have (even if it isn't rebased). So yes please share.

Quoted 5 lines

(it could be that there isn't a big enough win associated with switching to rwlock_t -- that we could get away without doing that particular churn.. open to that if you think rwlock_t pointless given we'll take the write lock after repeat_count drops to 0)

personally, I don't think the switching to a rwlock_t will buy us anything; for a decent performance you have to set rq_min_io to 1 anyway, thereby defeating the purpose of the rwlock.

My thinking was rather a different direction:
Move the crucial bits of the multipath structure to atomics, and split off the path selection code into one bit for selecting the path within a path group, and another which switches the path groups. When we do that we could use rcus for the paths themselves, and would only need to take the spinlock if we need to switch path groups. Which should be okay as switching path groups is (potentially) a rather slow operation anyway.

Quoted 36 lines · Hannes Reinecke

Quoted 35 lines · Mike Snitzer

Quoted 9 lines · Mike Snitzer

I'm still not clear on where the considerable performance loss is coming from (on null_blk device I see ~1900K read IOPs but I'm still only seeing ~1000K read IOPs when blk-mq DM-multipath is layered ontop). What is very much apparent is: layering dm-mq multipath ontop of null_blk results in a HUGE amount of additional context switches. I can only infer that the request completion for this stacked device (blk-mq queue ontop of blk-mq queue, with 2 completions: 1 for clone completing on underlying device and 1 for original request completing) is the reason for all the extra context switches.

Starts to explain, certainly not the "reason"; that is still very much TBD...

Quoted 5 lines

Here are pictures of 'perf report' for perf datat collected using 'perf record -ag -e cs'.

Against null_blk:
http://people.redhat.com/msnitzer/perf-report-cs-null_blk.png

if dm-mq nr_hw_queues=1 and null_blk nr_hw_queues=1
cpu : usr=25.53%, sys=74.40%, ctx=1970, majf=0, minf=474
if dm-mq nr_hw_queues=1 and null_blk nr_hw_queues=4
cpu : usr=26.79%, sys=73.15%, ctx=2067, majf=0, minf=479

Quoted 2 lines

Against dm-mpath ontop of the same null_blk:
http://people.redhat.com/msnitzer/perf-report-cs-dm_mq.png

if dm-mq nr_hw_queues=1 and null_blk nr_hw_queues=1
cpu : usr=11.07%, sys=33.90%, ctx=667784, majf=0, minf=466
if dm-mq nr_hw_queues=1 and null_blk nr_hw_queues=4
cpu : usr=15.22%, sys=48.44%, ctx=2314901, majf=0, minf=466

Quoted 8 lines

Quoted 4 lines

So yeah, the percentages reflected in these respective images didn't do the huge increase in context switches justice... we _must_ figure out why we're seeing so many context switches with dm-mq.

Well, the most obvious one being that you're using 1 dm-mq queue vs 4 null_blk queues.
So you will have have to do an additional context switch for 75% of the total I/Os submitted.

Right, that case is certainly prone to more context switches. But I'm initially most concerned about the case where both only have 1 queue.

Quoted 1 line

Have you tested with 4 dm-mq hw queues?

Yes, it makes performance worse. This is likely rooted in dm-mpath IO path not being lockless. But I also have concern about whether the clone, sent to the underlying path, is completing on a different cpu than dm-mq's original request.

I'll be using ftrace to try to dig into the various aspects of this (perf, as I know how to use it, isn't giving me enough precision in its reporting).

Quoted 2 lines

To avoid context switches we would have to align the dm-mq queues to the underlying blk-mq layout for the paths.

Right, we need to take more care (how remains TBD). But for now I'm just going to focus on the case where both dm-mq and null_blk have 1 for nr_hw_queues. As you can see even in that config the number of context switches goes from 1970 to 667784 (and there is a huge loss of system cpu utilization) once dm-mq w/ 1 hw_queue is stacked ontop on the null_blk device.

Once we understand the source of all the additional context switching for this more simplistic stacked configuration we can look closer at scaling as we add more underlying paths.

Following is RFC because it really speaks to dm-mq _needing_ a variant of blk_mq_complete_request() that supports partial completions. Not supporting partial completions really isn't an option for DM multipath.

From: Mike Snitzer <[email protected]>
Date: Fri, 5 Feb 2016 08:49:01 -0500
Subject: [RFC PATCH] dm: fix excessive dm-mq context switching

Request-based DM's blk-mq support (dm-mq) was reported to be 50% slower than if an underlying null_blk device were used directly. This biggest reason for this drop in performance is that blk_insert_clone_request() was calling blk_mq_insert_request() with @async=true. This forced the use of kblockd_schedule_delayed_work_on() to run the queues which ushered in ping-ponging between process context (fio in this case) and kblockd's kworker to submit the cloned request. The ftrace function_graph tracer showed:

kworker-2013 => fio-12190
fio-12190 => kworker-2013
...
kworker-2013 => fio-12190
fio-12190 => kworker-2013
...

Fixing blk_mq_insert_request() to _not_ use kblockd to submit the cloned requests isn't enough to fix eliminated the oberved context switches.

In addition to this dm-mq specific blk-core fix, there were 2 DM core fixes to dm-mq that (when paired with the blk-core fix) completely eliminate the observed context switching:

1) don't blk_mq_run_hw_queues in blk-mq request completion

Motivated by desire to reduce overhead of dm-mq, punting to kblockd just increases context switches.

In my testing against a really fast null_blk device there was no benefit to running blk_mq_run_hw_queues() on completion (and no other blk-mq driver does this). So hopefully this change doesn't induce the need for yet another revert like commit 621739b00e16ca2d !

2) use blk_mq_complete_request() in dm_complete_request()

blk_complete_request() doesn't offer the traditional q->mq_ops vs .request_fn branching pattern that other historic block interfaces do (e.g. blk_get_request). Using blk_mq_complete_request() for blk-mq requests is important for performance but it doesn't handle partial completions -- which is a pretty big problem given the potential for partial completions with DM multipath due to path failure(s). As such this makes this entire patch only RFC-worthy.

drivers/md/dm.c +0 -0

index c683f6d..a618477 100644
@@ -1344,7 +1340,10 @@ static void dm_complete_request(struct request *rq, int error)
 	struct dm_rq_target_io *tio = tio_from_request(rq);

Quoted 57 lines · Mike Snitzer

Following is RFC because it really speaks to dm-mq _needing_ a variant of blk_mq_complete_request() that supports partial completions. Not supporting partial completions really isn't an option for DM multipath.

From: Mike Snitzer <[email protected]>
Date: Fri, 5 Feb 2016 08:49:01 -0500
Subject: [RFC PATCH] dm: fix excessive dm-mq context switching

Request-based DM's blk-mq support (dm-mq) was reported to be 50% slower than if an underlying null_blk device were used directly. This biggest reason for this drop in performance is that blk_insert_clone_request() was calling blk_mq_insert_request() with @async=true. This forced the use of kblockd_schedule_delayed_work_on() to run the queues which ushered in ping-ponging between process context (fio in this case) and kblockd's kworker to submit the cloned request. The ftrace function_graph tracer showed:

kworker-2013 => fio-12190
fio-12190 => kworker-2013
...
kworker-2013 => fio-12190
fio-12190 => kworker-2013
...

Fixing blk_mq_insert_request() to _not_ use kblockd to submit the cloned requests isn't enough to fix eliminated the oberved context switches.

In addition to this dm-mq specific blk-core fix, there were 2 DM core fixes to dm-mq that (when paired with the blk-core fix) completely eliminate the observed context switching:

1) don't blk_mq_run_hw_queues in blk-mq request completion

Motivated by desire to reduce overhead of dm-mq, punting to kblockd just increases context switches.

In my testing against a really fast null_blk device there was no benefit to running blk_mq_run_hw_queues() on completion (and no other blk-mq driver does this). So hopefully this change doesn't induce the need for yet another revert like commit 621739b00e16ca2d !

2) use blk_mq_complete_request() in dm_complete_request()

blk_complete_request() doesn't offer the traditional q->mq_ops vs .request_fn branching pattern that other historic block interfaces do (e.g. blk_get_request). Using blk_mq_complete_request() for blk-mq requests is important for performance but it doesn't handle partial completions -- which is a pretty big problem given the potential for partial completions with DM multipath due to path failure(s). As such this makes this entire patch only RFC-worthy.

drivers/md/dm.c +0 -0

index c683f6d..a618477 100644
@@ -1344,7 +1340,10 @@ static void dm_complete_request(struct request *rq, int error)
 	struct dm_rq_target_io *tio = tio_from_request(rq);

drivers/md/dm.c static void dm_complete_request(struct request *rq, int error) · line 1349

Quoted 10 lines

drivers/md/dm.c +4 -1

 
 	tio->error = error;
-	blk_complete_request(rq);
+	if (!rq->q->mq_ops)
+		blk_complete_request(rq);
+	else
+		blk_mq_complete_request(rq, rq->errors);
 }
 
 /*

Looking closer, DM is very likely OK just using blk_mq_complete_request.

blk_complete_request() also doesn't provide native partial completion support (it relies on the driver to do it, which DM core does):

/**
* blk_complete_request - end I/O on a request
* @req: the request being processed
*
* Description:

 *     Ends all I/O on a request. It does not handle partial completions,
 *     unless the driver actually implements this in its completion callback
 *     through requeueing. The actual completion happens out-of-order,
 *     through a softirq handler. The user must have registered a completion
 *     callback through blk_queue_softirq_done().

**/

blk_mq_complete_request() is effectively implemented in a comparable fashion to blk_complete_request(). Given that DM core is providing partial completion support by dm.c:end_clone_bio() triggering requeueing of the request via dm-mpath.c:multipath_end_io()'s return of DM_ENDIO_REQUEUE.

So I'm thinking I can drop the "RFC" for this patch and run with it.. once I get Jens' feedback (hopefully) confirming my understanding.

Jens, please advise. If you're comfortable providing your Acked-by I can get this fix in for 4.5-rc4 or so...

FYI, here is the latest revised patch:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.6&id=a5b835282422ec41991c1dbdb88daa4af7d166d2

(revised patch header and fixed a thinko in the dm.c:rq_completed() change from the RFC patch I posted earlier)

Quoted 6 lines

FYI, here is the latest revised patch:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.6&id=a5b835282422ec41991c1dbdb88daa4af7d166d2

(revised patch header and fixed a thinko in the dm.c:rq_completed() change from the RFC patch I posted earlier)

Hi Mike,

So I gave your patches a go (dm-4.6) but I still don't see the improvement you reported (while I do see a minor improvement).

null_blk queue_mode=2 submit_queues=24
dm_mod blk_mq_nr_hw_queues=24 blk_mq_queue_depth=4096 use_blk_mq=Y

I see 620K IOPs on dm_mq vs. 1750K IOPs on raw nullb0.

Is there something I'm missing?

Quoted 6 lines

Hi Mike,

So I gave your patches a go (dm-4.6) but I still don't see the improvement you reported (while I do see a minor improvement).

Quoted 4 lines

null_blk queue_mode=2 submit_queues=24
dm_mod blk_mq_nr_hw_queues=24 blk_mq_queue_depth=4096 use_blk_mq=Y

I see 620K IOPs on dm_mq vs. 1750K IOPs on raw nullb0.

blk_mq_nr_hw_queues=24 isn't likely to help you (but with these patches, the first being the most important, it shouldn't hurt either provided you have 24 cpus).

Could be you have multiple NUMA nodes and are seeing problems from that?

I have 12 cpus (in the same physical cpu) and only a single NUMA node. I get the same results as blk_mq_nr_hw_queues=12 with blk_mq_nr_hw_queues=4 (same goes for null_blk submit_queues). I've seen my IOPs go from ~950K to ~1400K. The peak null_blk can get on my setup is ~1950K. So I'm still seeing a ~25% drop with dm-mq (but that is much better than the over 50% drop I saw seeing).

Quoted 1 line

Is there something I'm missing?

Not sure, I just emailed out all my patches (and cc'd you). Please verify you're using the latest here (same as 'dm-4.6' branch):
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/log/?h=for-next

I rebased a couple times... so please diff what you have tested against this latest 'dm-4.6' branch.

Quoted 6 lines

FYI, here is the latest revised patch:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.6&id=a5b835282422ec41991c1dbdb88daa4af7d166d2

(revised patch header and fixed a thinko in the dm.c:rq_completed() change from the RFC patch I posted earlier)

Hi Mike,

So I gave your patches a go (dm-4.6) but I still don't see the improvement you reported (while I do see a minor improvement).

null_blk queue_mode=2 submit_queues=24
dm_mod blk_mq_nr_hw_queues=24 blk_mq_queue_depth=4096 use_blk_mq=Y

I see 620K IOPs on dm_mq vs. 1750K IOPs on raw nullb0.

Is there something I'm missing?

Hello Sagi,

Did you run your test on a NUMA system ?

If so, can you check with e.g.
perf record -ags -e LLC-load-misses sleep 10 && perf report whether this workload triggers perhaps lock contention ? What you need to look for in the perf output is whether any functions occupy more than 10% CPU time.

Quoted 2 lines

Hello Sagi,

Hey Bart,

Quoted 1 line

Did you run your test on a NUMA system ?

I did.

Quoted 4 lines

If so, can you check with e.g.
perf record -ags -e LLC-load-misses sleep 10 && perf report whether this workload triggers perhaps lock contention ? What you need to look for in the perf output is whether any functions occupy more than 10% CPU time.

I will, thanks for the tip!

Quoted 5 lines

If so, can you check with e.g.
perf record -ags -e LLC-load-misses sleep 10 && perf report whether this workload triggers perhaps lock contention ? What you need to look for in the perf output is whether any functions occupy more than 10% CPU time.

I will, thanks for the tip!

Quoted 6 lines

Quoted 5 lines

If so, can you check with e.g.
perf record -ags -e LLC-load-misses sleep 10 && perf report whether this workload triggers perhaps lock contention ? What you need to look for in the perf output is whether any functions occupy more than 10% CPU time.

Quoted 1 line

I will, thanks for the tip!

The perf report is very similar to the one that started this effort..

I'm afraid we'll need to resolve the per-target m->lock in order to scale with NUMA...

Quoted 5 lines

The perf report is very similar to the one that started this effort..

I'm afraid we'll need to resolve the per-target m->lock in order to scale with NUMA...

Could be. Just for testing, you can try the 2 topmost commits I've put here (once applied both __multipath_map and multipath_busy won't have _any_ locking.. again, very much test-only):

http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=devel2

Quoted 10 lines

Quoted 5 lines

The perf report is very similar to the one that started this effort..

I'm afraid we'll need to resolve the per-target m->lock in order to scale with NUMA...

Could be. Just for testing, you can try the 2 topmost commits I've put here (once applied both __multipath_map and multipath_busy won't have _any_ locking.. again, very much test-only):

Quoted 1 line

http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=devel2

Hi Mike,

So I still don't see the IOPs scale like I expected. With these two patches applied I see ~670K IOPs while the perf output is different and does not indicate a clear lock contention.

- 2.07% ksoftirqd/6 [kernel.kallsyms] [k]
blk_mq_run_hw_queues
- blk_mq_run_hw_queues
- 99.70% rq_completed
dm_done
dm_softirq_done
blk_done_softirq
+ __do_softirq

Quoted 10 lines · Sagi Grimberg

Quoted 7 lines

Quoted 5 lines

If so, can you check with e.g.
perf record -ags -e LLC-load-misses sleep 10 && perf report whether this workload triggers perhaps lock contention ? What you need to look for in the perf output is whether any functions occupy more than 10% CPU time.

I will, thanks for the tip!

The perf report is very similar to the one that started this effort..

Quoted 2 lines

I'm afraid we'll need to resolve the per-target m->lock in order to scale with NUMA...

Could be. Just for testing, you can try the 2 topmost commits I've put here (once applied both __multipath_map and multipath_busy won't have _any_ locking.. again, very much test-only):

http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=devel2

Quoted 20 lines · Mike Snitzer

Quoted 6 lines · Sagi Grimberg

Quoted 6 lines

Quoted 5 lines

If so, can you check with e.g.
perf record -ags -e LLC-load-misses sleep 10 && perf report whether this workload triggers perhaps lock contention ? What you need to look for in the perf output is whether any functions occupy more than 10% CPU time.

Quoted 6 lines

Quoted 1 line

I will, thanks for the tip!

The perf report is very similar to the one that started this effort..

I'm afraid we'll need to resolve the per-target m->lock in order to scale with NUMA...

Could be. Just for testing, you can try the 2 topmost commits I've put here (once applied both __multipath_map and multipath_busy won't have _any_ locking.. again, very much test-only):

Quoted 2 lines

http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=devel2

So, I gave those patches a spin.
Sad to say, they do _not_ resolve the issue fully.

My testbed (2 paths per LUN, 40 CPUs, 4 cores) yields 505k IOPs with those patches.

Using a single path (without those patches, but still running multipath on top of that path) the same testbed yields 550k IOPs. Which very much smells like a lock contention ...
We do get a slight improvement, though; without those patches I could only get about 350k IOPs. But still, I would somehow expect 2 paths to be faster than just one ..

Quoted 21 lines · Hannes Reinecke

Quoted 20 lines · Mike Snitzer

Quoted 6 lines · Sagi Grimberg

Quoted 6 lines

Quoted 5 lines

If so, can you check with e.g.
perf record -ags -e LLC-load-misses sleep 10 && perf report whether this workload triggers perhaps lock contention ? What you need to look for in the perf output is whether any functions occupy more than 10% CPU time.

Quoted 6 lines

Quoted 1 line

I will, thanks for the tip!

The perf report is very similar to the one that started this effort..

I'm afraid we'll need to resolve the per-target m->lock in order to scale with NUMA...

Could be. Just for testing, you can try the 2 topmost commits I've put here (once applied both __multipath_map and multipath_busy won't have _any_ locking.. again, very much test-only):

Quoted 7 lines

Quoted 2 lines

http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=devel2

So, I gave those patches a spin.
Sad to say, they do _not_ resolve the issue fully.

My testbed (2 paths per LUN, 40 CPUs, 4 cores) yields 505k IOPs with those patches.

That isn't a surprise. We knew the m->lock spinlock contention to be a problem. And NUMA makes it even worse.

Quoted 6 lines

Using a single path (without those patches, but still running multipath on top of that path) the same testbed yields 550k IOPs. Which very much smells like a lock contention ...
We do get a slight improvement, though; without those patches I could only get about 350k IOPs. But still, I would somehow expect 2 paths to be faster than just one ..

https://www.redhat.com/archives/dm-devel/2016-February/msg00036.html

hint hint...

Quoted 44 lines · Mike Snitzer

Quoted 21 lines · Hannes Reinecke

Quoted 20 lines · Mike Snitzer

Quoted 6 lines · Sagi Grimberg

Quoted 6 lines

Quoted 5 lines

If so, can you check with e.g.
perf record -ags -e LLC-load-misses sleep 10 && perf report whether this workload triggers perhaps lock contention ? What you need to look for in the perf output is whether any functions occupy more than 10% CPU time.

Quoted 6 lines

Quoted 1 line

I will, thanks for the tip!

The perf report is very similar to the one that started this effort..

I'm afraid we'll need to resolve the per-target m->lock in order to scale with NUMA...

Could be. Just for testing, you can try the 2 topmost commits I've put here (once applied both __multipath_map and multipath_busy won't have _any_ locking.. again, very much test-only):

Quoted 7 lines

Quoted 2 lines

http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=devel2

So, I gave those patches a spin.
Sad to say, they do _not_ resolve the issue fully.

My testbed (2 paths per LUN, 40 CPUs, 4 cores) yields 505k IOPs with those patches.

That isn't a surprise. We knew the m->lock spinlock contention to be a problem. And NUMA makes it even worse.

Quoted 6 lines

Using a single path (without those patches, but still running multipath on top of that path) the same testbed yields 550k IOPs. Which very much smells like a lock contention ...
We do get a slight improvement, though; without those patches I could only get about 350k IOPs. But still, I would somehow expect 2 paths to be faster than just one ..

https://www.redhat.com/archives/dm-devel/2016-February/msg00036.html

Quoted 2 lines

hint hint...

I hoped they wouldn't be needed with your patches. Plus perf revealed that I first need to address a spinlock contention in the lpfc driver before that even would make sense.

So more debugging to follow.

OK, I took a crack at embracing RCU. Only slightly better performance on my single NUMA node testbed. (But I'll have to track down a system with multiple NUMA nodes to do any justice to the next wave of this optimization effort)

This RCU work is very heavy-handed and way too fiddley (there could easily be bugs). Anyway, please see:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=devel2&id=d80a7e4f8b5be9c81e4d452137623b003fa64745

But this might give you something to build on to arrive at something more scalable?

Quoted 6 lines · Mike Snitzer

OK, I took a crack at embracing RCU. Only slightly better performance on my single NUMA node testbed. (But I'll have to track down a system with multiple NUMA nodes to do any justice to the next wave of this optimization effort)

Quoted 6 lines

This RCU work is very heavy-handed and way too fiddley (there could easily be bugs). Anyway, please see:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=devel2&id=d80a7e4f8b5be9c81e4d452137623b003fa64745

But this might give you something to build on to arrive at something more scalable?

I've a bit more polished version of this work (broken up into multiple commits, with some fixes, etc) here:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=devel3

Hannes and/or Sagi, if you get a chance to try this on your NUMA system please let me know how it goes.

Quoted 6 lines · Mike Snitzer

OK, I took a crack at embracing RCU. Only slightly better performance on my single NUMA node testbed. (But I'll have to track down a system with multiple NUMA nodes to do any justice to the next wave of this optimization effort)

Quoted 6 lines

This RCU work is very heavy-handed and way too fiddley (there could easily be bugs). Anyway, please see:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=devel2&id=d80a7e4f8b5be9c81e4d452137623b003fa64745

But this might give you something to build on to arrive at something more scalable?

I've a bit more polished version of this work (broken up into multiple commits, with some fixes, etc) here:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=devel3

Hannes and/or Sagi, if you get a chance to try this on your NUMA system please let me know how it goes.

Quoted 20 lines · Mike Snitzer

Quoted 6 lines · Mike Snitzer

OK, I took a crack at embracing RCU. Only slightly better performance on my single NUMA node testbed. (But I'll have to track down a system with multiple NUMA nodes to do any justice to the next wave of this optimization effort)

Quoted 6 lines

This RCU work is very heavy-handed and way too fiddley (there could easily be bugs). Anyway, please see:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=devel2&id=d80a7e4f8b5be9c81e4d452137623b003fa64745

But this might give you something to build on to arrive at something more scalable?

I've a bit more polished version of this work (broken up into multiple commits, with some fixes, etc) here:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=devel3

Quoted 2 lines

Hannes and/or Sagi, if you get a chance to try this on your NUMA system please let me know how it goes.

Initial review has uncovered some locking problems with the current code (nothing that caused crashes or hangs in my testing but...) so please hold off on testing until you hear from me (hopefully tomorrow).

Quoted 23 lines · Mike Snitzer

Quoted 20 lines · Mike Snitzer

Quoted 6 lines · Mike Snitzer

OK, I took a crack at embracing RCU. Only slightly better performance on my single NUMA node testbed. (But I'll have to track down a system with multiple NUMA nodes to do any justice to the next wave of this optimization effort)

Quoted 6 lines

This RCU work is very heavy-handed and way too fiddley (there could easily be bugs). Anyway, please see:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=devel2&id=d80a7e4f8b5be9c81e4d452137623b003fa64745

But this might give you something to build on to arrive at something more scalable?

I've a bit more polished version of this work (broken up into multiple commits, with some fixes, etc) here:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=devel3

Quoted 7 lines

Quoted 2 lines

Hannes and/or Sagi, if you get a chance to try this on your NUMA system please let me know how it goes.

Initial review has uncovered some locking problems with the current code (nothing that caused crashes or hangs in my testing but...) so please hold off on testing until you hear from me (hopefully tomorrow).

Good news is that I've managed to hit the roof for my array with the devel2 version of those patches. (And a _heavily_ patched-up lpfc driver :-)
So from that perspective everything's fine now; we've reached the hardware limit for my setup.
Which in itself is quite impressive; beating Intel P3700 with 16FC is not bad methinks :-)

So thanks for all your work here.

Quoted 39 lines · Hannes Reinecke

Quoted 23 lines · Mike Snitzer

Quoted 20 lines · Mike Snitzer

Quoted 6 lines · Mike Snitzer

OK, I took a crack at embracing RCU. Only slightly better performance on my single NUMA node testbed. (But I'll have to track down a system with multiple NUMA nodes to do any justice to the next wave of this optimization effort)

Quoted 6 lines

This RCU work is very heavy-handed and way too fiddley (there could easily be bugs). Anyway, please see:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=devel2&id=d80a7e4f8b5be9c81e4d452137623b003fa64745

But this might give you something to build on to arrive at something more scalable?

I've a bit more polished version of this work (broken up into multiple commits, with some fixes, etc) here:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=devel3

Quoted 7 lines

Quoted 2 lines

Hannes and/or Sagi, if you get a chance to try this on your NUMA system please let me know how it goes.

Initial review has uncovered some locking problems with the current code (nothing that caused crashes or hangs in my testing but...) so please hold off on testing until you hear from me (hopefully tomorrow).

Good news is that I've managed to hit the roof for my array with the devel2 version of those patches. (And a _heavily_ patched-up lpfc driver :-)
So from that perspective everything's fine now; we've reached the hardware limit for my setup.
Which in itself is quite impressive; beating Intel P3700 with 16FC is not bad methinks :-)

Quoted 1 line

So thanks for all your work here.

Ah, that's really good news! But devel2 is definitely _not_ destined for upstream. 'devel3' is much closer to "ready". But your testing and review would really push it forward.

Please see/test:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=devel3

Also, please read this header:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=devel3&id=65a01b76502dd68e8ca298ee6614c0151b677f4a

Even with devel2 I hacked it such that repeat_count > 1 is effectively broken. I'm now _seriously_ considering deprecating repeat_count completely (adding a DMWARN that will inform the user. e.g.:
"repeat_count > 1 is no longer supported"). I see no point going to great lengths to maintain a dm-mpath feature that was only a hack for when dm-mpath was bio-based. What do you think?

Quoted 23 lines · Mike Snitzer

Quoted 10 lines · Hannes Reinecke

Quoted 1 line

Good news is that I've managed to hit the roof for my array with the devel2 version of those patches. (And a _heavily_ patched-up lpfc driver :-)
So from that perspective everything's fine now; we've reached the hardware limit for my setup.
Which in itself is quite impressive; beating Intel P3700 with 16FC is not bad methinks :-)

So thanks for all your work here.

Ah, that's really good news! But devel2 is definitely _not_ destined for upstream. 'devel3' is much closer to "ready". But your testing and review would really push it forward.

Please see/test:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=devel3

Also, please read this header:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=devel3&id=65a01b76502dd68e8ca298ee6614c0151b677f4a

Quoted 6 lines

Even with devel2 I hacked it such that repeat_count > 1 is effectively broken. I'm now _seriously_ considering deprecating repeat_count completely (adding a DMWARN that will inform the user. e.g.:
"repeat_count > 1 is no longer supported"). I see no point going to great lengths to maintain a dm-mpath feature that was only a hack for when dm-mpath was bio-based. What do you think?

Drop it, and make setting of which a no-op.
Never liked it anyway, and these decisions should really be delegated to the path selector.

Quoted 31 lines · Hannes Reinecke

Quoted 29 lines · Mike Snitzer

Quoted 10 lines · Hannes Reinecke

Quoted 1 line

Good news is that I've managed to hit the roof for my array with the devel2 version of those patches. (And a _heavily_ patched-up lpfc driver :-)
So from that perspective everything's fine now; we've reached the hardware limit for my setup.
Which in itself is quite impressive; beating Intel P3700 with 16FC is not bad methinks :-)

So thanks for all your work here.

Ah, that's really good news! But devel2 is definitely _not_ destined for upstream. 'devel3' is much closer to "ready". But your testing and review would really push it forward.

Please see/test:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=devel3

Also, please read this header:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=devel3&id=65a01b76502dd68e8ca298ee6614c0151b677f4a

Even with devel2 I hacked it such that repeat_count > 1 is effectively broken. I'm now _seriously_ considering deprecating repeat_count completely (adding a DMWARN that will inform the user. e.g.:
"repeat_count > 1 is no longer supported"). I see no point going to great lengths to maintain a dm-mpath feature that was only a hack for when dm-mpath was bio-based. What do you think?

Quoted 3 lines

Drop it, and make setting of which a no-op.
Never liked it anyway, and these decisions should really be delegated to the path selector.

Sure, but my point is DM-mpath will no longer be able to provide the ability to properly handle repeat_count > 1 (because updating the ->current_pgpath crushes the write-side of the RCU).

As such I've rebased 'devel3' to impose repeat_count = 1 (both in dm-mpath.c and the defaults in each path selector).

Quoted 2 lines · Mike Snitzer

On Mon, Jan 18 2016 at 7:04am -0500,
Sagi Grimberg <[email protected]> wrote:

Would still appreciate answers to my 2 questions above (did you modify md->tag_set.nr_hw_queues and can you share your fio job?)

Quoted 30 lines · Mike Snitzer

Quoted 14 lines · Sagi Grimberg

Hi All,

I've recently tried out dm-multipath over a "super-fast" nvme device and noticed a serious lock contention in dm-multipath that requires some extra attention. The nvme device is a simple loopback device emulation backed by null_blk device.

With this I've seen dm-multipath pushing around ~470K IOPs while the native (loopback) nvme performance can easily push up to 1500K+ IOPs.

perf output [1] reveals a huge lock contention on the multipath lock which is a per-dm_target contention point which seem to defeat the purpose of blk-mq i/O path.

Quoted 6 lines

The two current bottlenecks seem to come from multipath_busy and __multipath_map. Would it make better sense to move to a percpu_ref model with freeze/unfreeze logic for updates similar to what blk-mq is doing?

Thoughts?

Your perf output clearly does identify the 'struct multipath' spinlock as a bottleneck.

Is it fair to assume that implied in your test is that you increased md->tag_set.nr_hw_queues to > 1 in dm_init_request_based_blk_mq_queue()?

Quoted 3 lines

I'd like to start by replicating your testbed. So I'll see about setting up the nvme loop driver you referenced in earlier mail. Can you share your fio job file and fio commandline for your test?

Would still appreciate answers to my 2 questions above (did you modify md->tag_set.nr_hw_queues and can you share your fio job?)

I've yet to reproduce your config (using hch's nvme loop driver) or

Christoph, any chance you could rebase your 'nvme-loop.2' on v4.5-rc1?

Quoted 1 line · Mike Snitzer

Christoph, any chance you could rebase your 'nvme-loop.2' on v4.5-rc1?

Don't think I have time at the moment, sorry..

Quoted 2 lines

Don't think I have time at the moment, sorry..

No problem, I pulled the latest DM changes ontop of 'nvme-loop.2', see:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=devel

I was able to use nvme-loop like was documented here:
http://git.infradead.org/users/hch/block.git/commit/47d8d5b1db270463b5bd7b6a68cd89bd8762840d

But I'm not too sure how Sagi used the resulting nvme device with multipath.. maybe he just manually pushed down a DM multipath table? And only used the single /dev/nvme0n1 device? Sagi?

Christoph, any chance you could rebase your 'nvme-loop.2' on v4.5-rc1?

Or point me to a branch that is more current...

--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel