Skip to contentSkip to conversation

NVMe: Add rw_page support

PATCH New Keith Busch 3 participants 14 messages Nov 14, 2014 1 message not in the archive

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 NVMe: Add rw_page support

This adds the rw_page entry point to the nvme driver so a page can be written/read without going through the block layer and not requiring additional allocations.

Just because we implement this doesn't mean we want to use it. I only see a performance win on some types of work, like swap where I see about 15% reduction in system time (compared to 20% prior to blk-mq when we didn't allocate a request to get a command id). Even then, system time accounts for very little of the real time anyway, and it's only an over-all win if the device has very low-latency. But the driver doesn't know this nor if the expected workload will even benefit from using page io, so I added a queue flag that a user can toggle on/off.

The other benefit besides reduced system time is that we can swap pages in/out without having to allocate anything since everything is preallocated in this path.

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 block/blk-sysfs.c         |    8 +++++
 drivers/block/nvme-core.c |   78 +++++++++++++++++++++++++++++++++++++++++++++
 fs/block_dev.c            |    4 +--
 include/linux/blkdev.h    |    2 ++
 4 files changed, 90 insertions(+), 2 deletions(-)
block/blk-sysfs.c +8 -0
index 1fac434..b29de5f 100644
@@ -221,6 +221,7 @@ queue_store_##name(struct request_queue *q, const char *page, size_t count) \
 QUEUE_SYSFS_BIT_FNS(nonrot, NONROT, 1);
 QUEUE_SYSFS_BIT_FNS(random, ADD_RANDOM, 0);
 QUEUE_SYSFS_BIT_FNS(iostats, IO_STAT, 0);
+QUEUE_SYSFS_BIT_FNS(rw_page, RW_PG, 0);
 #undef QUEUE_SYSFS_BIT_FNS
 
 static ssize_t queue_nomerges_show(struct request_queue *q, char *page)
@@ -404,6 +405,12 @@ static struct queue_sysfs_entry queue_random_entry = {
 	.store = queue_store_random,
 };
 
+static struct queue_sysfs_entry queue_rw_page_entry = {
+	.attr = {.name = "rw_page", .mode = S_IRUGO | S_IWUSR },
+	.show = queue_show_rw_page,
+	.store = queue_store_rw_page,
+};
+
 static struct attribute *default_attrs[] = {
 	&queue_requests_entry.attr,
 	&queue_ra_entry.attr,
@@ -427,6 +434,7 @@ static struct attribute *default_attrs[] = {
 	&queue_rq_affinity_entry.attr,
 	&queue_iostats_entry.attr,
 	&queue_random_entry.attr,
+	&queue_rw_page_entry.attr,
 	NULL,
 };
 
drivers/block/nvme-core.c +78 -0

92 lines not on this page · raw message

fs/block_dev.c +2 -2
index cc9d411..f17f95d 100644
@@ -380,7 +380,7 @@ int bdev_read_page(struct block_device *bdev, sector_t sector,
 			struct page *page)
 {
 	const struct block_device_operations *ops = bdev->bd_disk->fops;
-	if (!ops->rw_page)
+	if (!ops->rw_page || !blk_queue_rw_page(bdev->bd_queue))
 		return -EOPNOTSUPP;
 	return ops->rw_page(bdev, sector + get_start_sect(bdev), page, READ);
 }
@@ -411,7 +411,7 @@ int bdev_write_page(struct block_device *bdev, sector_t sector,
 	int result;
 	int rw = (wbc->sync_mode == WB_SYNC_ALL) ? WRITE_SYNC : WRITE;
 	const struct block_device_operations *ops = bdev->bd_disk->fops;
-	if (!ops->rw_page)
+	if (!ops->rw_page || !blk_queue_rw_page(bdev->bd_queue))
 		return -EOPNOTSUPP;
 	set_page_writeback(page);
 	result = ops->rw_page(bdev, sector + get_start_sect(bdev), page, rw);
include/linux/blkdev.h +2 -0
index 77db6dc..17a6058 100644
@@ -509,6 +509,7 @@ struct request_queue {
 #define QUEUE_FLAG_INIT_DONE   20	/* queue is initialized */
 #define QUEUE_FLAG_NO_SG_MERGE 21	/* don't attempt to merge SG segments*/
 #define QUEUE_FLAG_SG_GAPS     22	/* queue doesn't support SG gaps */
+#define QUEUE_FLAG_RW_PG       23	/* use .rw_page if implemented */
 
 #define QUEUE_FLAG_DEFAULT	((1 << QUEUE_FLAG_IO_STAT) |		\
 				 (1 << QUEUE_FLAG_STACKABLE)	|	\
@@ -596,6 +597,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
 #define blk_queue_discard(q)	test_bit(QUEUE_FLAG_DISCARD, &(q)->queue_flags)
 #define blk_queue_secdiscard(q)	(blk_queue_discard(q) && \
 	test_bit(QUEUE_FLAG_SECDISCARD, &(q)->queue_flags))
+#define blk_queue_rw_page(q)	test_bit(QUEUE_FLAG_RW_PG, &(q)->queue_flags)
 
 #define blk_noretry_request(rq) \
 	((rq)->cmd_flags & (REQ_FAILFAST_DEV|REQ_FAILFAST_TRANSPORT| \
Signature · 1 line
1.7.10.4
maintainer
#1 lore raw

PATCH 1/1 NVMe: Add rw_page support

Quoted 4 lines · Keith Busch

This adds the rw_page entry point to the nvme driver so a page can be written/read without going through the block layer and not requiring additional allocations.

Quoted 8 lines

Just because we implement this doesn't mean we want to use it. I only see a performance win on some types of work, like swap where I see about 15% reduction in system time (compared to 20% prior to blk-mq when we didn't allocate a request to get a command id). Even then, system time accounts for very little of the real time anyway, and it's only an over-all win if the device has very low-latency. But the driver doesn't know this nor if the expected workload will even benefit from using page io, so I added a queue flag that a user can toggle on/off.

Of the reduced system time, where was that spent?

Quoted 3 lines

The other benefit besides reduced system time is that we can swap pages in/out without having to allocate anything since everything is preallocated in this path.

I have an iod prealloc patch that gets rid of the last kmalloc/kfree in the IO path for nvme, for smaller IO (defaults to 8KB, or 2 segments), making the generic IO path contain no allocations/frees for smaller IO. Probably that would get us pretty close to rw_page. For direct/sync IO, we'll go direct to ->queue_rq mostly, too.

The downside I see is that this is an OOB IO path. Once we start adding IO scheduling for those that need that, then this will completely bypass that.

Signature · 1 line
Jens Axboe
#2 lore raw

PATCH 1/1 NVMe: Add rw_page support

Quoted 2 lines · Jens Axboe

The downside I see is that this is an OOB IO path. Once we start adding IO scheduling for those that need that, then this will completely bypass that.

The idea is that you would only enable it for devices that are based on NVM that is of "near-DRAM" speeds, and can complete small I/Os as fast as they are issued. For those kinds of devices, there is absolutely no value to any kind of IO scheduling.

maintainer
#3 lore raw

PATCH 1/1 NVMe: Add rw_page support

Quoted 8 lines · Matthew Wilcox
Quoted 2 lines · Jens Axboe

The downside I see is that this is an OOB IO path. Once we start adding IO scheduling for those that need that, then this will completely bypass that.

The idea is that you would only enable it for devices that are based on NVM that is of "near-DRAM" speeds, and can complete small I/Os as fast as they are issued. For those kinds of devices, there is absolutely no value to any kind of IO scheduling.

I agree, that's not the kind of device that people would generally do scheduling on, and we can't at those rates. But if that's the case, why isn't this a sync interface? "Near DRAM speeds" and interrupt driven seems like a poor choice.

Signature · 1 line
Jens Axboe
#4 lore raw

PATCH 1/1 NVMe: Add rw_page support

Quoted 10 lines · Jens Axboe
Quoted 8 lines · Matthew Wilcox
Quoted 2 lines · Jens Axboe

The downside I see is that this is an OOB IO path. Once we start adding IO scheduling for those that need that, then this will completely bypass that.

The idea is that you would only enable it for devices that are based on NVM that is of "near-DRAM" speeds, and can complete small I/Os as fast as they are issued. For those kinds of devices, there is absolutely no value to any kind of IO scheduling.

Quoted 4 lines

I agree, that's not the kind of device that people would generally do scheduling on, and we can't at those rates. But if that's the case, why isn't this a sync interface? "Near DRAM speeds" and interrupt driven seems like a poor choice.

It could be done as a sync interface; zram and brd do implement it synchronously. But if you look at the callers, mostly they try to send several pages before waiting on each of them to complete, and so we can overlap the work of sending each page with the drive handling the I/O of the previous page. You'll notice that we check the completion queue before returning from nvme_rw_page(), so not waiting for an interrupt to fire for anything that already completed.

The missing piece that I think we need is something like the patch I sent last year to spin instead of sleeping in io_schedule()
(https://lwn.net/Articles/555886/). That will ensure that we pick up the last I/O or two without waiting for an interrupt.

maintainer
#5 lore raw

PATCH 1/1 NVMe: Add rw_page support

Quoted 16 lines · Matthew Wilcox
Quoted 10 lines · Jens Axboe
Quoted 8 lines · Matthew Wilcox
Quoted 2 lines · Jens Axboe

The downside I see is that this is an OOB IO path. Once we start adding IO scheduling for those that need that, then this will completely bypass that.

The idea is that you would only enable it for devices that are based on NVM that is of "near-DRAM" speeds, and can complete small I/Os as fast as they are issued. For those kinds of devices, there is absolutely no value to any kind of IO scheduling.

Quoted 4 lines

I agree, that's not the kind of device that people would generally do scheduling on, and we can't at those rates. But if that's the case, why isn't this a sync interface? "Near DRAM speeds" and interrupt driven seems like a poor choice.

Quoted 7 lines

It could be done as a sync interface; zram and brd do implement it synchronously. But if you look at the callers, mostly they try to send several pages before waiting on each of them to complete, and so we can overlap the work of sending each page with the drive handling the I/O of the previous page. You'll notice that we check the completion queue before returning from nvme_rw_page(), so not waiting for an interrupt to fire for anything that already completed.

For the cases where you do indeed end up submitting multiple, it's even more of a shame to bypass the normal IO path. There are various tricks we can do in there to speed things up, like batched doorbell rings. And if we kill that last alloc/free per IO, then I'd really be curious to know why rw_page is faster. Seems it should be possible to fix that up instead.

Quoted 4 lines

The missing piece that I think we need is something like the patch I sent last year to spin instead of sleeping in io_schedule()
(https://lwn.net/Articles/555886/). That will ensure that we pick up the last I/O or two without waiting for an interrupt.

Yes, we need to look more into that at some point. If we can eliminate a sleep/wakeup cycle, we're way ahead in the game.

Signature · 1 line
Jens Axboe
#6 lore raw

PATCH 1/1 NVMe: Add rw_page support

Quoted 6 lines · Jens Axboe

For the cases where you do indeed end up submitting multiple, it's even more of a shame to bypass the normal IO path. There are various tricks we can do in there to speed things up, like batched doorbell rings. And if we kill that last alloc/free per IO, then I'd really be curious to know why rw_page is faster. Seems it should be possible to fix that up instead.

Here's some perf data of just the kernel from two runs with a simple swap testing program. I'm a novice at interpreting this for comparison, so I'm not sure if this shows what we're looking for. The test ran for the same amount of time in both cases, but perf couted ~16% fewer events when using rw_page.

With rw_page disabled:

      7.33%  swap     [kernel.kallsyms]  [k] page_fault
      5.13%  swap     [kernel.kallsyms]  [k] clear_page_c
      4.46%  swap     [kernel.kallsyms]  [k] __radix_tree_lookup
      4.36%  swap     [kernel.kallsyms]  [k] do_raw_spin_lock
      2.63%  swap     [kernel.kallsyms]  [k] handle_mm_fault
      2.17%  swap     [kernel.kallsyms]  [k] get_page_from_freelist
      1.77%  swap     [kernel.kallsyms]  [k] __swap_duplicate
      1.53%  swap     [nvme]             [k] nvme_queue_rq
      1.38%  swap     [kernel.kallsyms]  [k] intel_pmu_disable_all
      1.37%  swap     [kernel.kallsyms]  [k] put_page_testzero
      1.19%  swap     [kernel.kallsyms]  [k] __do_page_fault
      1.05%  swap     [kernel.kallsyms]  [k] _raw_spin_lock_irqsave
      0.99%  swap     [kernel.kallsyms]  [k] __free_one_page
      0.97%  swap     [kernel.kallsyms]  [k] swap_info_get
      0.90%  swap     [kernel.kallsyms]  [k] __alloc_pages_nodemask
      0.80%  swap     [kernel.kallsyms]  [k] radix_tree_insert
      0.78%  swap     [kernel.kallsyms]  [k] test_and_set_bit.constprop.90
      0.74%  swap     [kernel.kallsyms]  [k] __bt_get
      0.71%  swap     [kernel.kallsyms]  [k] sg_init_table
      0.71%  swap     [kernel.kallsyms]  [k] list_del
      0.70%  swap     [kernel.kallsyms]  [k] ____cache_alloc
      0.67%  swap     [kernel.kallsyms]  [k] __schedule
      0.66%  swap     [kernel.kallsyms]  [k] round_jiffies_common
      0.63%  swap     [kernel.kallsyms]  [k] __wait_on_bit
      0.61%  swap     [kernel.kallsyms]  [k] __rmqueue
      0.60%  swap     [kernel.kallsyms]  [k] vmacache_find
      0.54%  swap     [kernel.kallsyms]  [k] __blk_bios_map_sg
      0.54%  swap     [kernel.kallsyms]  [k] blk_mq_start_request
      0.53%  swap     [kernel.kallsyms]  [k] unmap_single_vma
      0.52%  swap     [kernel.kallsyms]  [k] __update_tg_runnable_avg.isra.23
      0.52%  swap     [kernel.kallsyms]  [k] __blk_mq_alloc_request
      0.51%  swap     [kernel.kallsyms]  [k] swiotlb_map_sg_attrs
      0.49%  swap     [nvme]             [k] nvme_alloc_iod
      0.49%  swap     [kernel.kallsyms]  [k] update_cfs_shares
      0.47%  swap     [kernel.kallsyms]  [k] __add_to_swap_cache
      0.46%  swap     [kernel.kallsyms]  [k] update_curr
      0.46%  swap     [kernel.kallsyms]  [k] swap_entry_free
      0.45%  swap     [kernel.kallsyms]  [k] swapin_readahead
      0.45%  swap     [kernel.kallsyms]  [k] __call_rcu.constprop.62
      0.44%  swap     [kernel.kallsyms]  [k] page_waitqueue
      0.44%  swap     [kernel.kallsyms]  [k] tag_get
      0.43%  swap     [kernel.kallsyms]  [k] next_zones_zonelist
      0.43%  swap     [kernel.kallsyms]  [k] kmem_cache_alloc
      0.42%  swap     [nvme]             [k] nvme_process_cq

With rw_page enabled:

      8.33%  swap     [kernel.kallsyms]  [k] page_fault
      6.36%  swap     [kernel.kallsyms]  [k] clear_page_c
      5.15%  swap     [kernel.kallsyms]  [k] do_raw_spin_lock
      5.10%  swap     [kernel.kallsyms]  [k] __radix_tree_lookup
      3.01%  swap     [kernel.kallsyms]  [k] handle_mm_fault
      2.57%  swap     [kernel.kallsyms]  [k] get_page_from_freelist
      2.06%  swap     [kernel.kallsyms]  [k] __swap_duplicate
      1.57%  swap     [kernel.kallsyms]  [k] put_page_testzero
      1.44%  swap     [kernel.kallsyms]  [k] intel_pmu_disable_all
      1.37%  swap     [kernel.kallsyms]  [k] test_and_set_bit.constprop.90
      1.20%  swap     [kernel.kallsyms]  [k] _raw_spin_lock_irqsave
      1.19%  swap     [kernel.kallsyms]  [k] __free_one_page
      1.15%  swap     [kernel.kallsyms]  [k] radix_tree_insert
      1.15%  swap     [kernel.kallsyms]  [k] __do_page_fault
      1.07%  swap     [kernel.kallsyms]  [k] swap_info_get
      0.89%  swap     [kernel.kallsyms]  [k] __alloc_pages_nodemask
      0.85%  swap     [kernel.kallsyms]  [k] list_del
      0.81%  swap     [kernel.kallsyms]  [k] __bt_get
      0.78%  swap     [nvme]             [k] nvme_rw_page
      0.74%  swap     [kernel.kallsyms]  [k] __rmqueue
      0.74%  swap     [kernel.kallsyms]  [k] __wait_on_bit
      0.69%  swap     [kernel.kallsyms]  [k] __schedule
      0.63%  swap     [kernel.kallsyms]  [k] unmap_single_vma
      0.62%  swap     [kernel.kallsyms]  [k] vmacache_find
      0.60%  swap     [kernel.kallsyms]  [k] update_cfs_shares
      0.59%  swap     [kernel.kallsyms]  [k] tag_get
      0.55%  swap     [kernel.kallsyms]  [k] update_curr
      0.53%  swap     [kernel.kallsyms]  [k] __update_tg_runnable_avg.isra.23
      0.51%  swap     [kernel.kallsyms]  [k] next_zones_zonelist
      0.51%  swap     [kernel.kallsyms]  [k] __radix_tree_create
      0.50%  swap     [kernel.kallsyms]  [k] __blk_mq_alloc_request
      0.50%  swap     [kernel.kallsyms]  [k] __call_rcu.constprop.62
      0.49%  swap     [kernel.kallsyms]  [k] page_waitqueue
      0.48%  swap     [kernel.kallsyms]  [k] swap_entry_free
      0.47%  swap     [kernel.kallsyms]  [k] __add_to_swap_cache
      0.46%  swap     [kernel.kallsyms]  [k] down_read_trylock
      0.44%  swap     [kernel.kallsyms]  [k] up_read
      0.43%  swap     [kernel.kallsyms]  [k] __wake_up_bit
      0.43%  swap     [kernel.kallsyms]  [k] io_schedule
      0.42%  swap     [kernel.kallsyms]  [k] __mod_zone_page_state
      0.42%  swap     [kernel.kallsyms]  [k] do_wp_page
      0.39%  swap     [kernel.kallsyms]  [k] __inc_zone_state
      0.39%  swap     [kernel.kallsyms]  [k] dequeue_task_fair
      0.39%  swap     [kernel.kallsyms]  [k] prepare_to_wait
maintainer
#7 lore raw

PATCH 1/1 NVMe: Add rw_page support

Quoted 101 lines · Keith Busch
Quoted 6 lines · Jens Axboe

For the cases where you do indeed end up submitting multiple, it's even more of a shame to bypass the normal IO path. There are various tricks we can do in there to speed things up, like batched doorbell rings. And if we kill that last alloc/free per IO, then I'd really be curious to know why rw_page is faster. Seems it should be possible to fix that up instead.

Here's some perf data of just the kernel from two runs with a simple swap testing program. I'm a novice at interpreting this for comparison, so I'm not sure if this shows what we're looking for. The test ran for the same amount of time in both cases, but perf couted ~16% fewer events when using rw_page.

With rw_page disabled:

     7.33%  swap     [kernel.kallsyms]  [k] page_fault
     5.13%  swap     [kernel.kallsyms]  [k] clear_page_c
     4.46%  swap     [kernel.kallsyms]  [k] __radix_tree_lookup
     4.36%  swap     [kernel.kallsyms]  [k] do_raw_spin_lock
     2.63%  swap     [kernel.kallsyms]  [k] handle_mm_fault
     2.17%  swap     [kernel.kallsyms]  [k] get_page_from_freelist
     1.77%  swap     [kernel.kallsyms]  [k] __swap_duplicate
     1.53%  swap     [nvme]             [k] nvme_queue_rq
     1.38%  swap     [kernel.kallsyms]  [k] intel_pmu_disable_all
     1.37%  swap     [kernel.kallsyms]  [k] put_page_testzero
     1.19%  swap     [kernel.kallsyms]  [k] __do_page_fault
     1.05%  swap     [kernel.kallsyms]  [k] _raw_spin_lock_irqsave
     0.99%  swap     [kernel.kallsyms]  [k] __free_one_page
     0.97%  swap     [kernel.kallsyms]  [k] swap_info_get
     0.90%  swap     [kernel.kallsyms]  [k] __alloc_pages_nodemask
     0.80%  swap     [kernel.kallsyms]  [k] radix_tree_insert
     0.78%  swap     [kernel.kallsyms]  [k] test_and_set_bit.constprop.90
     0.74%  swap     [kernel.kallsyms]  [k] __bt_get
     0.71%  swap     [kernel.kallsyms]  [k] sg_init_table
     0.71%  swap     [kernel.kallsyms]  [k] list_del
     0.70%  swap     [kernel.kallsyms]  [k] ____cache_alloc
     0.67%  swap     [kernel.kallsyms]  [k] __schedule
     0.66%  swap     [kernel.kallsyms]  [k] round_jiffies_common
     0.63%  swap     [kernel.kallsyms]  [k] __wait_on_bit
     0.61%  swap     [kernel.kallsyms]  [k] __rmqueue
     0.60%  swap     [kernel.kallsyms]  [k] vmacache_find
     0.54%  swap     [kernel.kallsyms]  [k] __blk_bios_map_sg
     0.54%  swap     [kernel.kallsyms]  [k] blk_mq_start_request
     0.53%  swap     [kernel.kallsyms]  [k] unmap_single_vma
     0.52%  swap     [kernel.kallsyms]  [k]
__update_tg_runnable_avg.isra.23
     0.52%  swap     [kernel.kallsyms]  [k] __blk_mq_alloc_request
     0.51%  swap     [kernel.kallsyms]  [k] swiotlb_map_sg_attrs
     0.49%  swap     [nvme]             [k] nvme_alloc_iod
     0.49%  swap     [kernel.kallsyms]  [k] update_cfs_shares
     0.47%  swap     [kernel.kallsyms]  [k] __add_to_swap_cache
     0.46%  swap     [kernel.kallsyms]  [k] update_curr
     0.46%  swap     [kernel.kallsyms]  [k] swap_entry_free
     0.45%  swap     [kernel.kallsyms]  [k] swapin_readahead
     0.45%  swap     [kernel.kallsyms]  [k] __call_rcu.constprop.62
     0.44%  swap     [kernel.kallsyms]  [k] page_waitqueue
     0.44%  swap     [kernel.kallsyms]  [k] tag_get
     0.43%  swap     [kernel.kallsyms]  [k] next_zones_zonelist
     0.43%  swap     [kernel.kallsyms]  [k] kmem_cache_alloc
     0.42%  swap     [nvme]             [k] nvme_process_cq

With rw_page enabled:

     8.33%  swap     [kernel.kallsyms]  [k] page_fault
     6.36%  swap     [kernel.kallsyms]  [k] clear_page_c
     5.15%  swap     [kernel.kallsyms]  [k] do_raw_spin_lock
     5.10%  swap     [kernel.kallsyms]  [k] __radix_tree_lookup
     3.01%  swap     [kernel.kallsyms]  [k] handle_mm_fault
     2.57%  swap     [kernel.kallsyms]  [k] get_page_from_freelist
     2.06%  swap     [kernel.kallsyms]  [k] __swap_duplicate
     1.57%  swap     [kernel.kallsyms]  [k] put_page_testzero
     1.44%  swap     [kernel.kallsyms]  [k] intel_pmu_disable_all
     1.37%  swap     [kernel.kallsyms]  [k] test_and_set_bit.constprop.90
     1.20%  swap     [kernel.kallsyms]  [k] _raw_spin_lock_irqsave
     1.19%  swap     [kernel.kallsyms]  [k] __free_one_page
     1.15%  swap     [kernel.kallsyms]  [k] radix_tree_insert
     1.15%  swap     [kernel.kallsyms]  [k] __do_page_fault
     1.07%  swap     [kernel.kallsyms]  [k] swap_info_get
     0.89%  swap     [kernel.kallsyms]  [k] __alloc_pages_nodemask
     0.85%  swap     [kernel.kallsyms]  [k] list_del
     0.81%  swap     [kernel.kallsyms]  [k] __bt_get
     0.78%  swap     [nvme]             [k] nvme_rw_page
     0.74%  swap     [kernel.kallsyms]  [k] __rmqueue
     0.74%  swap     [kernel.kallsyms]  [k] __wait_on_bit
     0.69%  swap     [kernel.kallsyms]  [k] __schedule
     0.63%  swap     [kernel.kallsyms]  [k] unmap_single_vma
     0.62%  swap     [kernel.kallsyms]  [k] vmacache_find
     0.60%  swap     [kernel.kallsyms]  [k] update_cfs_shares
     0.59%  swap     [kernel.kallsyms]  [k] tag_get
     0.55%  swap     [kernel.kallsyms]  [k] update_curr
     0.53%  swap     [kernel.kallsyms]  [k]
__update_tg_runnable_avg.isra.23
     0.51%  swap     [kernel.kallsyms]  [k] next_zones_zonelist
     0.51%  swap     [kernel.kallsyms]  [k] __radix_tree_create
     0.50%  swap     [kernel.kallsyms]  [k] __blk_mq_alloc_request
     0.50%  swap     [kernel.kallsyms]  [k] __call_rcu.constprop.62
     0.49%  swap     [kernel.kallsyms]  [k] page_waitqueue
     0.48%  swap     [kernel.kallsyms]  [k] swap_entry_free
     0.47%  swap     [kernel.kallsyms]  [k] __add_to_swap_cache
     0.46%  swap     [kernel.kallsyms]  [k] down_read_trylock
Quoted 8 lines
     0.44%  swap     [kernel.kallsyms]  [k] up_read
     0.43%  swap     [kernel.kallsyms]  [k] __wake_up_bit
     0.43%  swap     [kernel.kallsyms]  [k] io_schedule
     0.42%  swap     [kernel.kallsyms]  [k] __mod_zone_page_state
     0.42%  swap     [kernel.kallsyms]  [k] do_wp_page
     0.39%  swap     [kernel.kallsyms]  [k] __inc_zone_state
     0.39%  swap     [kernel.kallsyms]  [k] dequeue_task_fair
     0.39%  swap     [kernel.kallsyms]  [k] prepare_to_wait

It's hard (impossible) to tell from just this, we'd need performance data to go with it, too. The number of events is a very vague hint, I would not put any value into that.

If you can describe your workload, I'd love to just run it and see what happens here!

Signature · 1 line
Jens Axboe
#8 lore raw

PATCH 1/1 NVMe: Add rw_page support

Quoted 3 lines · Jens Axboe

It's hard (impossible) to tell from just this, we'd need performance data to go with it, too. The number of events is a very vague hint, I would not put any value into that.

Oh good. After generating the report I couldn't figure out why I thought it would have been useful in the first place!

Quoted 2 lines

If you can describe your workload, I'd love to just run it and see what happens here!

Sure, I made this program up real quick, so there's probably something better out there. It just allocates millions of pages in an array then writes and reads a byte from each page to force page faults and swapping, then does it all again a few more times.

This is the basic verion. I've another one that does this with lots of threads accessing pages in random order.

It allocates 40GB worth of pages, so that might not be appropriate for all machines.

swap.c:
---
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

int main(int argc, char ** argv)
{
unsigned long i, j, num_pages;
unsigned char **mem;
volatile unsigned char k;

num_pages = (1024 * 1024 * 1024 * 40ULL) / getpagesize(); mem = (unsigned char **)calloc(num_pages, sizeof(*mem)); if (!mem)
return 1;

for (i = 0; i < num_pages; i++) {
mem[i] = malloc(getpagesize());
if (!mem[i])
return 1;
}

for (j = 0; j < 8; j++) {
for (i = 0; i < num_pages; i++)
mem[i][0] = i;
for (i = 0; i < num_pages; i++)
k = mem[i][0];
}
return 0;
}
--

#9 lore raw

PATCH 1/1 NVMe: Add rw_page support

In reply to Keith Busch

drivers/block/nvme-core.c static int nvme_revalidate_disk(struct gendisk *disk) · line 1820

Quoted 10 lines · Keith Busch
drivers/block/nvme-core.c +10 -0
+static void pgrd_completion(struct nvme_queue *nvmeq, void *ctx,
+						struct nvme_completion *cqe)
+{
+	struct request *req = ctx;
+	struct nvme_cmd_info *cmd_rq = blk_mq_rq_to_pdu(req);
+	struct page *page = req->special;
+	u16 status = le16_to_cpup(&cqe->status) >> 1;
+
+	dma_unmap_page(nvmeq->q_dmadev, cmd_rq->dma, PAGE_CACHE_SIZE, DMA_FROM_DEVICE);
+	page_endio(page, READ, status != NVME_SC_SUCCESS);

The third parameter to page_endio() is supposed to be an errno. This will happen to work fine since anything other than 0 and -ENOSPC gets translated to -EIO, but it'd be better to make this line:

	page_endio(page, READ, nvme_error_status(status));

drivers/block/nvme-core.c static int nvme_revalidate_disk(struct gendisk *disk) · line 1833

Quoted 10 lines
drivers/block/nvme-core.c +10 -0
+static void pgwr_completion(struct nvme_queue *nvmeq, void *ctx,
+						struct nvme_completion *cqe)
+{
+	struct request *req = ctx;
+	struct nvme_cmd_info *cmd_rq = blk_mq_rq_to_pdu(req);
+	struct page *page = req->special;
+	u16 status = le16_to_cpup(&cqe->status) >> 1;
+
+	dma_unmap_page(nvmeq->q_dmadev, cmd_rq->dma, PAGE_CACHE_SIZE, DMA_TO_DEVICE);
+	page_endio(page, WRITE, status != NVME_SC_SUCCESS);

Here too.

A message that is not in the archive (sent off-list or not yet received): CANvN+ekQTdNgPe33iaM_9=2Hjrfds2B2R3d3XK06K9n=SY+ZKA@mail.gmail.com

#11 lore raw

PATCH 1/1 NVMe: Add rw_page support

Quoted 10 lines · Andrey Kuzmin
Quoted 9 lines · Keith Busch
(quoted diff) +9 -0
+static void pgwr_completion(struct nvme_queue *nvmeq, void *ctx,
+? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct nvme_completion *cqe)
+{
+? ? ? ?struct request *req = ctx;
+? ? ? ?struct nvme_cmd_info *cmd_rq = blk_mq_rq_to_pdu(req);
+? ? ? ?struct page *page = req->special;
+? ? ? ?u16 status = le16_to_cpup(&cqe->status) >> 1;
+
+? ? ? ?dma_unmap_page(nvmeq->q_dmadev, cmd_rq->dma, PAGE_CACHE_SIZE, DMA_TO_DEVICE);
Quoted 8 lines

Review

Quoted 4 lines

+? ? ? ?page_endio(page, WRITE, status != NVME_SC_SUCCESS); +? ? ? ?blk_put_request(req);

(quoted diff) +2 -0
+}
+

As you have access to the nvme_cmd_info - and thus command opcode - in the completion handler, having separate completion handlers for read and write operations looks like unnecessary code duplication. Just my .02 :).

But nvme_cmd_info does not contain the opcode. I do have the struct request here, and I can certainly pull the data direction from its cmd_flags, so thanks for the suggestion!

Now I wonder if there's something else unused in a request that I can repurpose to save the dma_addr_t in so I don't add it in the nvme_cmd_info...

Quoted 2 lines

Best regards,
Andrey

maintainer
#12 lore raw

PATCH 1/1 NVMe: Add rw_page support

Quoted 24 lines · Keith Busch
Quoted 12 lines · Andrey Kuzmin

Review

Quoted 2 lines · Keith Busch
(quoted diff) +2 -0
+static void pgwr_completion(struct nvme_queue *nvmeq, void *ctx,
+                                               struct

nvme_completion *cqe)

drivers/block/nvme-core.c static int nvme_revalidate_disk(struct gendisk *disk)

Quoted 7 lines
drivers/block/nvme-core.c +7 -0
+{
+       struct request *req = ctx;
+       struct nvme_cmd_info *cmd_rq = blk_mq_rq_to_pdu(req);
+       struct page *page = req->special;
+       u16 status = le16_to_cpup(&cqe->status) >> 1;
+
+       dma_unmap_page(nvmeq->q_dmadev, cmd_rq->dma,

PAGE_CACHE_SIZE, DMA_TO_DEVICE);

Quoted 10 lines

drivers/block/nvme-core.c static int nvme_revalidate_disk(struct gendisk *disk) · line 1836

Quoted 4 lines
drivers/block/nvme-core.c +4 -0
+       page_endio(page, WRITE, status != NVME_SC_SUCCESS);
+       blk_put_request(req);
+}
+

As you have access to the nvme_cmd_info - and thus command opcode - in the
completion handler, having separate completion handlers for read and write
operations looks like unnecessary code duplication. Just my .02 :).

Quoted 7 lines

But nvme_cmd_info does not contain the opcode. I do have the struct request here, and I can certainly pull the data direction from its cmd_flags, so thanks for the suggestion!

Now I wonder if there's something else unused in a request that I can repurpose to save the dma_addr_t in so I don't add it in the nvme_cmd_info...

For the rw_page path, you don't use ->special to store the iod. So you could use that... That might break for 32-bit platforms and 64-bit DMA, though. If you really want to get nasty, ->__cmd is 16 bytes of unused goodness as well, though I do want to make that one dynamically allocated for the queues that need it.

Signature · 1 line
Jens Axboe
#13 lore raw

PATCH 1/1 NVMe: Add rw_page support

Quoted 9 lines · Jens Axboe
Quoted 7 lines · Keith Busch

But nvme_cmd_info does not contain the opcode. I do have the struct request here, and I can certainly pull the data direction from its cmd_flags, so thanks for the suggestion!

Now I wonder if there's something else unused in a request that I can repurpose to save the dma_addr_t in so I don't add it in the nvme_cmd_info...

Quoted 5 lines

For the rw_page path, you don't use ->special to store the iod. So you could use that... That might break for 32-bit platforms and 64-bit DMA, though. If you really want to get nasty, ->__cmd is 16 bytes of unused goodness as well, though I do want to make that one dynamically allocated for the queues that need it.

Ah, but I already got req->special pointing to the page. Maybe we can union another field with a new "special_2" field.

maintainer
#14 lore raw

PATCH 1/1 NVMe: Add rw_page support

Quoted 10 lines · Keith Busch
Quoted 9 lines · Jens Axboe
Quoted 7 lines · Keith Busch

But nvme_cmd_info does not contain the opcode. I do have the struct request here, and I can certainly pull the data direction from its cmd_flags, so thanks for the suggestion!

Now I wonder if there's something else unused in a request that I can repurpose to save the dma_addr_t in so I don't add it in the nvme_cmd_info...

Quoted 8 lines
Quoted 5 lines

For the rw_page path, you don't use ->special to store the iod. So you could use that... That might break for 32-bit platforms and 64-bit DMA, though. If you really want to get nasty, ->__cmd is 16 bytes of unused goodness as well, though I do want to make that one dynamically allocated for the queues that need it.

Ah, but I already got req->special pointing to the page. Maybe we can union another field with a new "special_2" field.

Since you are the request allocator, you can use ->end_io_data as well.

Signature · 1 line
Jens Axboe