remove rw_page() from brd, pmem and btt

Dan Williams and Christoph Hellwig have recently expressed doubt about whether the rw_page() interface made sense for synchronous memory drivers [1][2]. It's unclear whether this interface has any performance benefit for these drivers, but as we continue to fix bugs it is clear that it does have a maintenance burden. This series removes the rw_page() implementations in brd, pmem and btt to relieve this burden.

Quoted 6 lines · Ross Zwisler

Dan Williams and Christoph Hellwig have recently expressed doubt about whether the rw_page() interface made sense for synchronous memory drivers [1][2]. It's unclear whether this interface has any performance benefit for these drivers, but as we continue to fix bugs it is clear that it does have a maintenance burden. This series removes the rw_page() implementations in brd, pmem and btt to relieve this burden.

Why don't you measure whether it has performance benefits? I don't understand why zram would see performance benefits and not other drivers. If it's going to be removed, then the whole interface should be removed, not just have the implementations removed from some drivers.

Quoted 8 lines · On Fri, 28 Jul 2017 10:31:43 -0700 Matthew Wilcox <[email protected]> wrote:

Quoted 6 lines · Ross Zwisler

Dan Williams and Christoph Hellwig have recently expressed doubt about whether the rw_page() interface made sense for synchronous memory drivers [1][2]. It's unclear whether this interface has any performance benefit for these drivers, but as we continue to fix bugs it is clear that it does have a maintenance burden. This series removes the rw_page() implementations in brd, pmem and btt to relieve this burden.

Quoted 4 lines

Why don't you measure whether it has performance benefits? I don't understand why zram would see performance benefits and not other drivers. If it's going to be removed, then the whole interface should be removed, not just have the implementations removed from some drivers.

Yes please. Minchan, could you please take a look sometime?

Hi Andrew,

Quoted 10 lines · Andrew Morton

Quoted 8 lines · On Fri, 28 Jul 2017 10:31:43 -0700 Matthew Wilcox <[email protected]> wrote:

Quoted 6 lines · Ross Zwisler

Dan Williams and Christoph Hellwig have recently expressed doubt about whether the rw_page() interface made sense for synchronous memory drivers [1][2]. It's unclear whether this interface has any performance benefit for these drivers, but as we continue to fix bugs it is clear that it does have a maintenance burden. This series removes the rw_page() implementations in brd, pmem and btt to relieve this burden.

Quoted 6 lines

Quoted 4 lines

Why don't you measure whether it has performance benefits? I don't understand why zram would see performance benefits and not other drivers. If it's going to be removed, then the whole interface should be removed, not just have the implementations removed from some drivers.

Yes please. Minchan, could you please take a look sometime?

rw_page's gain is reducing of dynamic allocation in swap path as well as performance gain thorugh avoiding bio allocation. And it would be important in memory pressure situation.

I guess it comes from bio_alloc mempool. Usually, zram-swap works in high memory pressure so mempool would be exahusted easily. It means that mempool wait and repeated alloc would consume the overhead.

Actually, at that time although Karam reported the gain is 2.4%, I got a report from production team that the gain in corner case (e.g., animation playing is smooth) would be much higher than expected.

rw_page's gain is reducing of dynamic allocation in swap path as well as performance gain thorugh avoiding bio allocation. And it would be important in memory pressure situation.

Quoted 3 lines · Minchan Kim

rw_page's gain is reducing of dynamic allocation in swap path as well as performance gain thorugh avoiding bio allocation. And it would be important in memory pressure situation.

There is no need for any dynamic allocation when using the bio path. Take a look at __blkdev_direct_IO_simple for an example that doesn't do any allocations.

Do you suggest define something special flag(e.g., SWP_INMEMORY) for in-memory swap to swap_info_struct when swapon time manually or from bdi_queue_someting automatically?
And depending the flag of swap_info_struct, use the onstack bio instead of dynamic allocation if the swap device is in-memory?

Quoted 5 lines · Minchan Kim

Do you suggest define something special flag(e.g., SWP_INMEMORY) for in-memory swap to swap_info_struct when swapon time manually or from bdi_queue_someting automatically?
And depending the flag of swap_info_struct, use the onstack bio instead of dynamic allocation if the swap device is in-memory?

Currently swap always just does I/O on a single page as far as I can tell, so it can always just use an on-stack bio and biovec.

Quoted 7 lines · Christoph Hellwig

Quoted 5 lines · Minchan Kim

Do you suggest define something special flag(e.g., SWP_INMEMORY) for in-memory swap to swap_info_struct when swapon time manually or from bdi_queue_someting automatically?
And depending the flag of swap_info_struct, use the onstack bio instead of dynamic allocation if the swap device is in-memory?

Quoted 3 lines

Currently swap always just does I/O on a single page as far as I can tell, so it can always just use an on-stack bio and biovec.

That's for synchronous I/O, aka reads of course. For writes you'll need to do a dynamic allocation if they are asynchronous. But yes, if we want to force certain devices to be synchronous we'll need a flag for that.

Quoted 6 lines · Ross Zwisler

Dan Williams and Christoph Hellwig have recently expressed doubt about whether the rw_page() interface made sense for synchronous memory drivers [1][2]. It's unclear whether this interface has any performance benefit for these drivers, but as we continue to fix bugs it is clear that it does have a maintenance burden. This series removes the rw_page() implementations in brd, pmem and btt to relieve this burden.

Why don't you measure whether it has performance benefits? I don't understand why zram would see performance benefits and not other drivers. If it's going to be removed, then the whole interface should be removed, not just have the implementations removed from some drivers.

Quoted 8 lines · Matthew Wilcox

Quoted 6 lines · Ross Zwisler

Dan Williams and Christoph Hellwig have recently expressed doubt about whether the rw_page() interface made sense for synchronous memory drivers [1][2]. It's unclear whether this interface has any performance benefit for these drivers, but as we continue to fix bugs it is clear that it does have a maintenance burden. This series removes the rw_page() implementations in brd, pmem and btt to relieve this burden.

Quoted 4 lines

Why don't you measure whether it has performance benefits? I don't understand why zram would see performance benefits and not other drivers. If it's going to be removed, then the whole interface should be removed, not just have the implementations removed from some drivers.

Okay, I've run a bunch of performance tests with the PMEM and with BTT entry points for rw_pages() in a swap workload, and in all cases I do see an improvement over the code when rw_pages() is removed. Here are the results from my random lab box:

Average latency of swap_writepage()

+------+------------+---------+-------------+
|      | no rw_page | rw_page | Improvement |
+-------------------------------------------+
| PMEM |  5.0 us    |  4.7 us |     6%      |
+-------------------------------------------+
|  BTT |  6.8 us    |  6.1 us |    10%      |
+------+------------+---------+-------------+

Average latency of swap_readpage()

+------+------------+---------+-------------+
|      | no rw_page | rw_page | Improvement |
+-------------------------------------------+
| PMEM |  3.3 us    |  2.9 us |    12%      |
+-------------------------------------------+
|  BTT |  3.7 us    |  3.4 us |     8%      |
+------+------------+---------+-------------+

The workload was pmbench, a memory benchmark, run on a system where I had severely restricted the amount of memory in the system with the 'mem' kernel command line parameter. The benchmark was set up to test more memory than I allowed the OS to have so it spilled over into swap.

The PMEM or BTT device was set up as my swap device, and during the test I got a few hundred thousand samples of each of swap_writepage() and swap_writepage(). The PMEM/BTT device was just memory reserved with the memmap kernel command line parameter.

Thanks, Matthew, for asking for performance data. It looks like removing this code would have been a mistake.

Hi Ross,

Quoted 42 lines · Ross Zwisler

Quoted 8 lines · Matthew Wilcox

Quoted 6 lines · Ross Zwisler

Dan Williams and Christoph Hellwig have recently expressed doubt about whether the rw_page() interface made sense for synchronous memory drivers [1][2]. It's unclear whether this interface has any performance benefit for these drivers, but as we continue to fix bugs it is clear that it does have a maintenance burden. This series removes the rw_page() implementations in brd, pmem and btt to relieve this burden.

Quoted 4 lines

Why don't you measure whether it has performance benefits? I don't understand why zram would see performance benefits and not other drivers. If it's going to be removed, then the whole interface should be removed, not just have the implementations removed from some drivers.

Okay, I've run a bunch of performance tests with the PMEM and with BTT entry points for rw_pages() in a swap workload, and in all cases I do see an improvement over the code when rw_pages() is removed. Here are the results from my random lab box:

Average latency of swap_writepage()

+------+------------+---------+-------------+
|      | no rw_page | rw_page | Improvement |
+-------------------------------------------+
| PMEM |  5.0 us    |  4.7 us |     6%      |
+-------------------------------------------+
|  BTT |  6.8 us    |  6.1 us |    10%      |
+------+------------+---------+-------------+

Average latency of swap_readpage()

+------+------------+---------+-------------+
|      | no rw_page | rw_page | Improvement |
+-------------------------------------------+
| PMEM |  3.3 us    |  2.9 us |    12%      |
+-------------------------------------------+
|  BTT |  3.7 us    |  3.4 us |     8%      |
+------+------------+---------+-------------+

The workload was pmbench, a memory benchmark, run on a system where I had severely restricted the amount of memory in the system with the 'mem' kernel command line parameter. The benchmark was set up to test more memory than I allowed the OS to have so it spilled over into swap.

Quoted 7 lines

The PMEM or BTT device was set up as my swap device, and during the test I got a few hundred thousand samples of each of swap_writepage() and swap_writepage(). The PMEM/BTT device was just memory reserved with the memmap kernel command line parameter.

Thanks, Matthew, for asking for performance data. It looks like removing this code would have been a mistake.

By suggestion of Christoph Hellwig, I made a quick patch which does IO without dynamic bio allocation for swap IO. Actually, it's not formal patch to be worth to send mainline yet but I believe it's enough to test the improvement.

Could you test patchset on pmem and btt without rw_page?

For working the patch, block drivers need to declare it's synchronous IO device via BDI_CAP_SYNC but if it's hard, you can just make every swap IO comes from (sis->flags & SWP_SYNC_IO) with removing condition check

if (!(sis->flags & SWP_SYNC_IO)) in swap_[read|write]page.

Patchset is based on 4.13-rc3.

drivers/block/zram/zram_drv.c +3 -3

index 856d5dc02451..b1c5e9bf3ad5 100644
@@ -125,9 +125,9 @@ static inline bool is_partial_io(struct bio_vec *bvec)
 static void zram_revalidate_disk(struct zram *zram)
 {
        revalidate_disk(zram->disk);
-       /* revalidate_disk reset the BDI_CAP_STABLE_WRITES so set again */
+       /* revalidate_disk reset the BDI capability so set again */
        zram->disk->queue->backing_dev_info->capabilities |=
-               BDI_CAP_STABLE_WRITES;
+               (BDI_CAP_STABLE_WRITES|BDI_CAP_SYNC);
 }
 
 /*

@@ -1096,7 +1096,7 @@ static int zram_open(struct block_device *bdev, fmode_t mode)
 static const struct block_device_operations zram_devops = {
        .open = zram_open,
        .swap_slot_free_notify = zram_slot_free_notify,
-       .rw_page = zram_rw_page,
+       // .rw_page = zram_rw_page,
        .owner = THIS_MODULE
 };
 

include/linux/backing-dev.h +6 -0

index 854e1bdd0b2a..05eee145d964 100644
@@ -130,6 +130,7 @@ int bdi_set_max_ratio(struct backing_dev_info *bdi, unsigned int max_ratio);
 #define BDI_CAP_STABLE_WRITES  0x00000008
 #define BDI_CAP_STRICTLIMIT    0x00000010
 #define BDI_CAP_CGROUP_WRITEBACK 0x00000020
+#define BDI_CAP_SYNC           0x00000040
 
 #define BDI_CAP_NO_ACCT_AND_WRITEBACK \
        (BDI_CAP_NO_WRITEBACK | BDI_CAP_NO_ACCT_DIRTY | BDI_CAP_NO_ACCT_WB)

@@ -177,6 +178,11 @@ long wait_iff_congested(struct pglist_data *pgdat, int sync, long timeout);
 int pdflush_proc_obsolete(struct ctl_table *table, int write,
                void __user *buffer, size_t *lenp, loff_t *ppos);
 
+static inline bool bdi_cap_sync_io_required(struct backing_dev_info *bdi)
+{
+       return bdi->capabilities & BDI_CAP_SYNC;
+}
+
 static inline bool bdi_cap_stable_pages_required(struct backing_dev_info *bdi)
 {
        return bdi->capabilities & BDI_CAP_STABLE_WRITES;

include/linux/swap.h +2 -1

index d83d28e53e62..86457dbfd300 100644
@@ -152,8 +152,9 @@ enum {
        SWP_AREA_DISCARD = (1 << 8),    /* single-time swap area discards */
        SWP_PAGE_DISCARD = (1 << 9),    /* freed swap page-cluster discards */
        SWP_STABLE_WRITES = (1 << 10),  /* no overwrite PG_writeback pages */
+       SWP_SYNC_IO     = (1 << 11),
                                        /* add others here before... */
-       SWP_SCANNING    = (1 << 11),    /* refcount in scan_swap_map */
+       SWP_SCANNING    = (1 << 12),    /* refcount in scan_swap_map */
 };
 
 #define SWAP_CLUSTER_MAX 32UL

mm/page_io.c +71 -33

index b6c4ac388209..2c85e5182364 100644
@@ -263,7 +263,6 @@ static sector_t swap_page_sector(struct page *page)
 int __swap_writepage(struct page *page, struct writeback_control *wbc,
                bio_end_io_t end_write_func)
 {
-       struct bio *bio;
        int ret;
        struct swap_info_struct *sis = page_swap_info(page);
 

@@ -316,25 +315,44 @@ int __swap_writepage(struct page *page, struct writeback_control *wbc,
        }
 
        ret = 0;
-       bio = get_swap_bio(GFP_NOIO, page, end_write_func);
-       if (bio == NULL) {
-               set_page_dirty(page);
+       count_vm_event(PSWPOUT);
+
+       if (!(sis->flags & SWP_SYNC_IO)) {
+               struct bio *bio;
+
+               bio = get_swap_bio(GFP_NOIO, page, end_write_func);
+               if (bio == NULL) {
+                       set_page_dirty(page);
+                       unlock_page(page);
+                       ret = -ENOMEM;
+                       goto out;
+               }
+               bio->bi_opf = REQ_OP_WRITE | wbc_to_write_flags(wbc);
+               set_page_writeback(page);
                unlock_page(page);
-               ret = -ENOMEM;
-               goto out;
+               submit_bio(bio);
+       } else {
+               struct bio bio;
+               struct bio_vec bvec;
+
+               bio_init(&bio, &bvec, 1);
+
+               bio.bi_iter.bi_sector = map_swap_page(page, &bio.bi_bdev);
+               bio.bi_iter.bi_sector <<= PAGE_SHIFT - 9;
+               bio.bi_end_io = end_write_func;
+               bio_add_page(&bio, page, PAGE_SIZE, 0);
+               bio.bi_opf = REQ_OP_WRITE | wbc_to_write_flags(wbc);
+               bio_get(&bio);
+               set_page_writeback(page);
+               unlock_page(page);
+               submit_bio(&bio);
        }
-       bio->bi_opf = REQ_OP_WRITE | wbc_to_write_flags(wbc);
-       count_vm_event(PSWPOUT);
-       set_page_writeback(page);
-       unlock_page(page);
-       submit_bio(bio);
 out:
        return ret;
 }
 
 int swap_readpage(struct page *page, bool do_poll)
 {
-       struct bio *bio;
        int ret = 0;
        struct swap_info_struct *sis = page_swap_info(page);
        blk_qc_t qc;

@@ -371,29 +389,49 @@ int swap_readpage(struct page *page, bool do_poll)
        }
 
        ret = 0;
-       bio = get_swap_bio(GFP_KERNEL, page, end_swap_bio_read);
-       if (bio == NULL) {
-               unlock_page(page);
-               ret = -ENOMEM;
-               goto out;
-       }
-       bdev = bio->bi_bdev;
-       bio->bi_private = current;
-       bio_set_op_attrs(bio, REQ_OP_READ, 0);
-       count_vm_event(PSWPIN);
-       bio_get(bio);
-       qc = submit_bio(bio);
-       while (do_poll) {
-               set_current_state(TASK_UNINTERRUPTIBLE);
-               if (!READ_ONCE(bio->bi_private))
-                       break;
-
-               if (!blk_mq_poll(bdev_get_queue(bdev), qc))
-                       break;
+       if (!(sis->flags & SWP_SYNC_IO)) {
+               struct bio *bio;
+
+               bio = get_swap_bio(GFP_KERNEL, page, end_swap_bio_read);
+               if (bio == NULL) {
+                       unlock_page(page);
+                       ret = -ENOMEM;
+                       goto out;
+               }
+               bdev = bio->bi_bdev;
+               bio->bi_private = current;
+               bio_set_op_attrs(bio, REQ_OP_READ, 0);
+               bio_get(bio);
+               qc = submit_bio(bio);
+               while (do_poll) {
+                       set_current_state(TASK_UNINTERRUPTIBLE);
+                       if (!READ_ONCE(bio->bi_private))
+                               break;
+
+                       if (!blk_mq_poll(bdev_get_queue(bdev), qc))
+                               break;
+               }
+               __set_current_state(TASK_RUNNING);
+               bio_put(bio);
+       } else {
+               struct bio bio;
+               struct bio_vec bvec;
+
+               bio_init(&bio, &bvec, 1);
+
+               bio.bi_iter.bi_sector = map_swap_page(page, &bio.bi_bdev);
+               bio.bi_iter.bi_sector <<= PAGE_SHIFT - 9;
+               bio.bi_end_io = end_swap_bio_read;
+               bio_add_page(&bio, page, PAGE_SIZE, 0);
+               bio.bi_private = current;
+               BUG_ON(bio.bi_iter.bi_size != PAGE_SIZE);
+               bio_set_op_attrs(&bio, REQ_OP_READ, 0);
+               /* end_swap_bio_read calls bio_put unconditionally */
+               bio_get(&bio);
+               submit_bio(&bio);
        }
-       __set_current_state(TASK_RUNNING);
-       bio_put(bio);
 
+       count_vm_event(PSWPIN);
 out:
        return ret;
 }

mm/swapfile.c +0 -0

index 6ba4aab2db0b..855d50eeeaf9 100644

FYI, for the read side we should use the on-stack bio unconditionally, as it will always be a win (or not show up at all).

Hi Ross,

Quoted 42 lines · Ross Zwisler

Quoted 8 lines · Matthew Wilcox

Quoted 6 lines · Ross Zwisler

Dan Williams and Christoph Hellwig have recently expressed doubt about whether the rw_page() interface made sense for synchronous memory drivers [1][2]. It's unclear whether this interface has any performance benefit for these drivers, but as we continue to fix bugs it is clear that it does have a maintenance burden. This series removes the rw_page() implementations in brd, pmem and btt to relieve this burden.

Quoted 4 lines

Why don't you measure whether it has performance benefits? I don't understand why zram would see performance benefits and not other drivers. If it's going to be removed, then the whole interface should be removed, not just have the implementations removed from some drivers.

Okay, I've run a bunch of performance tests with the PMEM and with BTT entry points for rw_pages() in a swap workload, and in all cases I do see an improvement over the code when rw_pages() is removed. Here are the results from my random lab box:

Average latency of swap_writepage()

+------+------------+---------+-------------+
|      | no rw_page | rw_page | Improvement |
+-------------------------------------------+
| PMEM |  5.0 us    |  4.7 us |     6%      |
+-------------------------------------------+
|  BTT |  6.8 us    |  6.1 us |    10%      |
+------+------------+---------+-------------+

Average latency of swap_readpage()

+------+------------+---------+-------------+
|      | no rw_page | rw_page | Improvement |
+-------------------------------------------+
| PMEM |  3.3 us    |  2.9 us |    12%      |
+-------------------------------------------+
|  BTT |  3.7 us    |  3.4 us |     8%      |
+------+------------+---------+-------------+

The workload was pmbench, a memory benchmark, run on a system where I had severely restricted the amount of memory in the system with the 'mem' kernel command line parameter. The benchmark was set up to test more memory than I allowed the OS to have so it spilled over into swap.

Quoted 7 lines

The PMEM or BTT device was set up as my swap device, and during the test I got a few hundred thousand samples of each of swap_writepage() and swap_writepage(). The PMEM/BTT device was just memory reserved with the memmap kernel command line parameter.

Thanks, Matthew, for asking for performance data. It looks like removing this code would have been a mistake.

By suggestion of Christoph Hellwig, I made a quick patch which does IO without dynamic bio allocation for swap IO. Actually, it's not formal patch to be worth to send mainline yet but I believe it's enough to test the improvement.

Could you test patchset on pmem and btt without rw_page?

For working the patch, block drivers need to declare it's synchronous IO device via BDI_CAP_SYNC but if it's hard, you can just make every swap IO comes from (sis->flags & SWP_SYNC_IO) with removing condition check

if (!(sis->flags & SWP_SYNC_IO)) in swap_[read|write]page.

Patchset is based on 4.13-rc3.

Quoted 59 lines · Minchan Kim

Hi Ross,

Quoted 42 lines · Ross Zwisler

Quoted 8 lines · Matthew Wilcox

Quoted 6 lines · Ross Zwisler

Dan Williams and Christoph Hellwig have recently expressed doubt about whether the rw_page() interface made sense for synchronous memory drivers [1][2]. It's unclear whether this interface has any performance benefit for these drivers, but as we continue to fix bugs it is clear that it does have a maintenance burden. This series removes the rw_page() implementations in brd, pmem and btt to relieve this burden.

Quoted 4 lines

Why don't you measure whether it has performance benefits? I don't understand why zram would see performance benefits and not other drivers. If it's going to be removed, then the whole interface should be removed, not just have the implementations removed from some drivers.

Okay, I've run a bunch of performance tests with the PMEM and with BTT entry points for rw_pages() in a swap workload, and in all cases I do see an improvement over the code when rw_pages() is removed. Here are the results from my random lab box:

Average latency of swap_writepage()

+------+------------+---------+-------------+
|      | no rw_page | rw_page | Improvement |
+-------------------------------------------+
| PMEM |  5.0 us    |  4.7 us |     6%      |
+-------------------------------------------+
|  BTT |  6.8 us    |  6.1 us |    10%      |
+------+------------+---------+-------------+

Average latency of swap_readpage()

+------+------------+---------+-------------+
|      | no rw_page | rw_page | Improvement |
+-------------------------------------------+
| PMEM |  3.3 us    |  2.9 us |    12%      |
+-------------------------------------------+
|  BTT |  3.7 us    |  3.4 us |     8%      |
+------+------------+---------+-------------+

The workload was pmbench, a memory benchmark, run on a system where I had severely restricted the amount of memory in the system with the 'mem' kernel command line parameter. The benchmark was set up to test more memory than I allowed the OS to have so it spilled over into swap.

Quoted 7 lines

The PMEM or BTT device was set up as my swap device, and during the test I got a few hundred thousand samples of each of swap_writepage() and swap_writepage(). The PMEM/BTT device was just memory reserved with the memmap kernel command line parameter.

Thanks, Matthew, for asking for performance data. It looks like removing this code would have been a mistake.

By suggestion of Christoph Hellwig, I made a quick patch which does IO without dynamic bio allocation for swap IO. Actually, it's not formal patch to be worth to send mainline yet but I believe it's enough to test the improvement.

Could you test patchset on pmem and btt without rw_page?

Quoted 7 lines

For working the patch, block drivers need to declare it's synchronous IO device via BDI_CAP_SYNC but if it's hard, you can just make every swap IO comes from (sis->flags & SWP_SYNC_IO) with removing condition check

if (!(sis->flags & SWP_SYNC_IO)) in swap_[read|write]page.

Patchset is based on 4.13-rc3.

Thanks for the patch, here are the updated results from my test box:

Average latency of swap_writepage()

+------+------------+---------+---------+
|      | no rw_page | minchan | rw_page |
+----------------------------------------
| PMEM |  5.0 us    | 4.98 us |  4.7 us |
+----------------------------------------
|  BTT |  6.8 us    | 6.3 us  |  6.1 us |
+------+------------+---------+---------+

Average latency of swap_readpage()

+------+------------+---------+---------+
|      | no rw_page | minchan | rw_page |
+----------------------------------------
| PMEM |  3.3 us    | 3.27 us |  2.9 us |
+----------------------------------------
|  BTT |  3.7 us    | 3.44 us |  3.4 us |
+------+------------+---------+---------+

I've added another digit in precision in some cases to help differentiate the various results.

In all cases your patches did perform better than with the regularly allocated BIO, but again for all cases the rw_page() path was the fastest, even if only marginally.

Quoted 59 lines · Minchan Kim

Hi Ross,

Quoted 42 lines · Ross Zwisler

Quoted 8 lines · Matthew Wilcox

Quoted 6 lines · Ross Zwisler

Dan Williams and Christoph Hellwig have recently expressed doubt about whether the rw_page() interface made sense for synchronous memory drivers [1][2]. It's unclear whether this interface has any performance benefit for these drivers, but as we continue to fix bugs it is clear that it does have a maintenance burden. This series removes the rw_page() implementations in brd, pmem and btt to relieve this burden.

Quoted 4 lines

Why don't you measure whether it has performance benefits? I don't understand why zram would see performance benefits and not other drivers. If it's going to be removed, then the whole interface should be removed, not just have the implementations removed from some drivers.

Okay, I've run a bunch of performance tests with the PMEM and with BTT entry points for rw_pages() in a swap workload, and in all cases I do see an improvement over the code when rw_pages() is removed. Here are the results from my random lab box:

Average latency of swap_writepage()

+------+------------+---------+-------------+
|      | no rw_page | rw_page | Improvement |
+-------------------------------------------+
| PMEM |  5.0 us    |  4.7 us |     6%      |
+-------------------------------------------+
|  BTT |  6.8 us    |  6.1 us |    10%      |
+------+------------+---------+-------------+

Average latency of swap_readpage()

+------+------------+---------+-------------+
|      | no rw_page | rw_page | Improvement |
+-------------------------------------------+
| PMEM |  3.3 us    |  2.9 us |    12%      |
+-------------------------------------------+
|  BTT |  3.7 us    |  3.4 us |     8%      |
+------+------------+---------+-------------+

The workload was pmbench, a memory benchmark, run on a system where I had severely restricted the amount of memory in the system with the 'mem' kernel command line parameter. The benchmark was set up to test more memory than I allowed the OS to have so it spilled over into swap.

Quoted 7 lines

The PMEM or BTT device was set up as my swap device, and during the test I got a few hundred thousand samples of each of swap_writepage() and swap_writepage(). The PMEM/BTT device was just memory reserved with the memmap kernel command line parameter.

Thanks, Matthew, for asking for performance data. It looks like removing this code would have been a mistake.

By suggestion of Christoph Hellwig, I made a quick patch which does IO without dynamic bio allocation for swap IO. Actually, it's not formal patch to be worth to send mainline yet but I believe it's enough to test the improvement.

Could you test patchset on pmem and btt without rw_page?

Quoted 7 lines

For working the patch, block drivers need to declare it's synchronous IO device via BDI_CAP_SYNC but if it's hard, you can just make every swap IO comes from (sis->flags & SWP_SYNC_IO) with removing condition check

if (!(sis->flags & SWP_SYNC_IO)) in swap_[read|write]page.

Patchset is based on 4.13-rc3.

Thanks for the patch, here are the updated results from my test box:

Average latency of swap_writepage()

+------+------------+---------+---------+
|      | no rw_page | minchan | rw_page |
+----------------------------------------
| PMEM |  5.0 us    | 4.98 us |  4.7 us |
+----------------------------------------
|  BTT |  6.8 us    | 6.3 us  |  6.1 us |
+------+------------+---------+---------+

Average latency of swap_readpage()

+------+------------+---------+---------+
|      | no rw_page | minchan | rw_page |
+----------------------------------------
| PMEM |  3.3 us    | 3.27 us |  2.9 us |
+----------------------------------------
|  BTT |  3.7 us    | 3.44 us |  3.4 us |
+------+------------+---------+---------+

I've added another digit in precision in some cases to help differentiate the various results.

In all cases your patches did perform better than with the regularly allocated BIO, but again for all cases the rw_page() path was the fastest, even if only marginally.

Quoted 88 lines · Ross Zwisler

Quoted 59 lines · Minchan Kim

Hi Ross,

Quoted 42 lines · Ross Zwisler

Quoted 8 lines · Matthew Wilcox

Quoted 6 lines · Ross Zwisler

Dan Williams and Christoph Hellwig have recently expressed doubt about whether the rw_page() interface made sense for synchronous memory drivers [1][2]. It's unclear whether this interface has any performance benefit for these drivers, but as we continue to fix bugs it is clear that it does have a maintenance burden. This series removes the rw_page() implementations in brd, pmem and btt to relieve this burden.

Quoted 4 lines

Why don't you measure whether it has performance benefits? I don't understand why zram would see performance benefits and not other drivers. If it's going to be removed, then the whole interface should be removed, not just have the implementations removed from some drivers.

Okay, I've run a bunch of performance tests with the PMEM and with BTT entry points for rw_pages() in a swap workload, and in all cases I do see an improvement over the code when rw_pages() is removed. Here are the results from my random lab box:

Average latency of swap_writepage()

+------+------------+---------+-------------+
|      | no rw_page | rw_page | Improvement |
+-------------------------------------------+
| PMEM |  5.0 us    |  4.7 us |     6%      |
+-------------------------------------------+
|  BTT |  6.8 us    |  6.1 us |    10%      |
+------+------------+---------+-------------+

Average latency of swap_readpage()

+------+------------+---------+-------------+
|      | no rw_page | rw_page | Improvement |
+-------------------------------------------+
| PMEM |  3.3 us    |  2.9 us |    12%      |
+-------------------------------------------+
|  BTT |  3.7 us    |  3.4 us |     8%      |
+------+------------+---------+-------------+

The workload was pmbench, a memory benchmark, run on a system where I had severely restricted the amount of memory in the system with the 'mem' kernel command line parameter. The benchmark was set up to test more memory than I allowed the OS to have so it spilled over into swap.

Quoted 7 lines

The PMEM or BTT device was set up as my swap device, and during the test I got a few hundred thousand samples of each of swap_writepage() and swap_writepage(). The PMEM/BTT device was just memory reserved with the memmap kernel command line parameter.

Thanks, Matthew, for asking for performance data. It looks like removing this code would have been a mistake.

By suggestion of Christoph Hellwig, I made a quick patch which does IO without dynamic bio allocation for swap IO. Actually, it's not formal patch to be worth to send mainline yet but I believe it's enough to test the improvement.

Could you test patchset on pmem and btt without rw_page?

Quoted 7 lines

For working the patch, block drivers need to declare it's synchronous IO device via BDI_CAP_SYNC but if it's hard, you can just make every swap IO comes from (sis->flags & SWP_SYNC_IO) with removing condition check

if (!(sis->flags & SWP_SYNC_IO)) in swap_[read|write]page.

Patchset is based on 4.13-rc3.

Thanks for the patch, here are the updated results from my test box:

Average latency of swap_writepage()

+------+------------+---------+---------+
|      | no rw_page | minchan | rw_page |
+----------------------------------------
| PMEM |  5.0 us    | 4.98 us |  4.7 us |
+----------------------------------------
|  BTT |  6.8 us    | 6.3 us  |  6.1 us |
+------+------------+---------+---------+

Average latency of swap_readpage()

+------+------------+---------+---------+
|      | no rw_page | minchan | rw_page |
+----------------------------------------
| PMEM |  3.3 us    | 3.27 us |  2.9 us |
+----------------------------------------
|  BTT |  3.7 us    | 3.44 us |  3.4 us |
+------+------------+---------+---------+

Quoted 6 lines

I've added another digit in precision in some cases to help differentiate the various results.

In all cases your patches did perform better than with the regularly allocated BIO, but again for all cases the rw_page() path was the fastest, even if only marginally.

Thanks for the testing. Your testing number is within noise level?

I cannot understand why PMEM doesn't have enough gain while BTT is significant win(8%). I guess no rw_page with BTT testing had more chances to wait bio dynamic allocation and mine and rw_page testing reduced it significantly. However, in no rw_page with pmem, there wasn't many cases to wait bio allocations due to the device is so fast so the number comes from purely the number of instructions has done. At a quick glance of bio init/submit, it's not trivial so indeed, i understand where the 12% enhancement comes from but I'm not sure it's really big difference in real practice at the cost of maintaince burden.

On Fri, Aug 04, 2017 at 12:54:41PM +0900, Minchan Kim wrote:

Quoted 10 lines

Thanks for the testing. Your testing number is within noise level?

I cannot understand why PMEM doesn't have enough gain while BTT is significant win(8%). I guess no rw_page with BTT testing had more chances to wait bio dynamic allocation and mine and rw_page testing reduced it significantly. However, in no rw_page with pmem, there wasn't many cases to wait bio allocations due to the device is so fast so the number comes from purely the number of instructions has done. At a quick glance of bio init/submit, it's not trivial so indeed, i understand where the 12% enhancement comes from but I'm not sure it's really big difference in real practice at the cost of maintaince burden.

I tested pmbench 10 times in my local machine(4 core) with zram-swap. In my machine, even, on-stack bio is faster than rw_page. Unbelievable.

I guess it's really hard to get stable result in severe memory pressure. It would be a result within noise level(see below stddev). So, I think it's hard to conclude rw_page is far faster than onstack-bio.

rw_page

avg     5.54us
stddev  8.89%
max     6.02us
min     4.20us

onstack bio

avg     5.27us
stddev  13.03%
max     5.96us
min     3.55us

[ adding Dave who is working on a blk-mq + dma offload version of the pmem driver ]

Quoted 1 line · Minchan Kim

On Fri, Aug 04, 2017 at 12:54:41PM +0900, Minchan Kim wrote:

[..]

Quoted 24 lines

Quoted 10 lines

Thanks for the testing. Your testing number is within noise level?

I cannot understand why PMEM doesn't have enough gain while BTT is significant win(8%). I guess no rw_page with BTT testing had more chances to wait bio dynamic allocation and mine and rw_page testing reduced it significantly. However, in no rw_page with pmem, there wasn't many cases to wait bio allocations due to the device is so fast so the number comes from purely the number of instructions has done. At a quick glance of bio init/submit, it's not trivial so indeed, i understand where the 12% enhancement comes from but I'm not sure it's really big difference in real practice at the cost of maintaince burden.

I tested pmbench 10 times in my local machine(4 core) with zram-swap. In my machine, even, on-stack bio is faster than rw_page. Unbelievable.

I guess it's really hard to get stable result in severe memory pressure. It would be a result within noise level(see below stddev). So, I think it's hard to conclude rw_page is far faster than onstack-bio.

rw_page

avg     5.54us
stddev  8.89%
max     6.02us
min     4.20us

Quoted 5 lines

onstack bio

avg     5.27us
stddev  13.03%
max     5.96us
min     3.55us

The maintenance burden of having alternative submission paths is significant especially as we consider the pmem driver ising more services of the core block layer. Ideally, I'd want to complete the rw_page removal work before we look at the blk-mq + dma offload reworks.

The change to introduce BDI_CAP_SYNC is interesting because we might have use for switching between dma offload and cpu copy based on whether the I/O is synchronous or otherwise hinted to be a low latency request. Right now the dma offload patches are using "bio_segments() > 1" as the gate for selecting offload vs cpu copy which seem inadequate.

Quoted 42 lines · Dan Williams

[ adding Dave who is working on a blk-mq + dma offload version of the pmem driver ]

Quoted 1 line · Minchan Kim

On Fri, Aug 04, 2017 at 12:54:41PM +0900, Minchan Kim wrote:

[..]

Quoted 24 lines

Quoted 10 lines

Thanks for the testing. Your testing number is within noise level?

I cannot understand why PMEM doesn't have enough gain while BTT is significant win(8%). I guess no rw_page with BTT testing had more chances to wait bio dynamic allocation and mine and rw_page testing reduced it significantly. However, in no rw_page with pmem, there wasn't many cases to wait bio allocations due to the device is so fast so the number comes from purely the number of instructions has done. At a quick glance of bio init/submit, it's not trivial so indeed, i understand where the 12% enhancement comes from but I'm not sure it's really big difference in real practice at the cost of maintaince burden.

I tested pmbench 10 times in my local machine(4 core) with zram-swap. In my machine, even, on-stack bio is faster than rw_page. Unbelievable.

I guess it's really hard to get stable result in severe memory pressure. It would be a result within noise level(see below stddev). So, I think it's hard to conclude rw_page is far faster than onstack-bio.

rw_page

avg     5.54us
stddev  8.89%
max     6.02us
min     4.20us

Quoted 5 lines

onstack bio

avg     5.27us
stddev  13.03%
max     5.96us
min     3.55us

The maintenance burden of having alternative submission paths is significant especially as we consider the pmem driver ising more services of the core block layer. Ideally, I'd want to complete the rw_page removal work before we look at the blk-mq + dma offload reworks.

Quoted 6 lines

The change to introduce BDI_CAP_SYNC is interesting because we might have use for switching between dma offload and cpu copy based on whether the I/O is synchronous or otherwise hinted to be a low latency request. Right now the dma offload patches are using "bio_segments() > 1" as the gate for selecting offload vs cpu copy which seem inadequate.

Okay, so based on the feedback above and from Jens[1], it sounds like we want to go forward with removing the rw_page() interface, and instead optimize the regular I/O path via on-stack BIOS and dma offload, correct?

If so, I'll prepare patches that fully remove the rw_page() code, and let Minchan and Dave work on their optimizations.

Quoted 54 lines · Ross Zwisler

Quoted 42 lines · Dan Williams

[ adding Dave who is working on a blk-mq + dma offload version of the pmem driver ]

Quoted 1 line · Minchan Kim

On Fri, Aug 04, 2017 at 12:54:41PM +0900, Minchan Kim wrote:

[..]

Quoted 24 lines

Quoted 10 lines

Thanks for the testing. Your testing number is within noise level?

I cannot understand why PMEM doesn't have enough gain while BTT is significant win(8%). I guess no rw_page with BTT testing had more chances to wait bio dynamic allocation and mine and rw_page testing reduced it significantly. However, in no rw_page with pmem, there wasn't many cases to wait bio allocations due to the device is so fast so the number comes from purely the number of instructions has done. At a quick glance of bio init/submit, it's not trivial so indeed, i understand where the 12% enhancement comes from but I'm not sure it's really big difference in real practice at the cost of maintaince burden.

I tested pmbench 10 times in my local machine(4 core) with zram-swap. In my machine, even, on-stack bio is faster than rw_page. Unbelievable.

I guess it's really hard to get stable result in severe memory pressure. It would be a result within noise level(see below stddev). So, I think it's hard to conclude rw_page is far faster than onstack-bio.

rw_page

avg     5.54us
stddev  8.89%
max     6.02us
min     4.20us

Quoted 5 lines

onstack bio

avg     5.27us
stddev  13.03%
max     5.96us
min     3.55us

The maintenance burden of having alternative submission paths is significant especially as we consider the pmem driver ising more services of the core block layer. Ideally, I'd want to complete the rw_page removal work before we look at the blk-mq + dma offload reworks.

Quoted 6 lines

The change to introduce BDI_CAP_SYNC is interesting because we might have use for switching between dma offload and cpu copy based on whether the I/O is synchronous or otherwise hinted to be a low latency request. Right now the dma offload patches are using "bio_segments() > 1" as the gate for selecting offload vs cpu copy which seem inadequate.

Okay, so based on the feedback above and from Jens[1], it sounds like we want to go forward with removing the rw_page() interface, and instead optimize the regular I/O path via on-stack BIOS and dma offload, correct?

Quoted 2 lines

If so, I'll prepare patches that fully remove the rw_page() code, and let Minchan and Dave work on their optimizations.

I think the conversion to on-stack-bio should be done in the same patchset that removes rw_page. We don't want to leave a known performance regression while the on-stack-bio work is in-flight.