Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1698950 > unrolled thread

[PATCH 0/3] remove rw_page() from brd, pmem and btt

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] remove rw_page() from brd, pmem and btt Ross Zwisler <ross.zwisler@linux.intel.com> - 2017-07-28 19:00 +0200
    [PATCH 1/3] btt: remove btt_rw_page() Ross Zwisler <ross.zwisler@linux.intel.com> - 2017-07-28 19:00 +0200
    [PATCH 2/3] pmem: remove pmem_rw_page() Ross Zwisler <ross.zwisler@linux.intel.com> - 2017-07-28 19:10 +0200
    [PATCH 3/3] brd: remove brd_rw_page() Ross Zwisler <ross.zwisler@linux.intel.com> - 2017-07-28 19:10 +0200
    Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Matthew Wilcox <willy@infradead.org> - 2017-07-28 19:40 +0200
      Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Andrew Morton <akpm@linux-foundation.org> - 2017-07-28 23:30 +0200
        Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Minchan Kim <minchan@kernel.org> - 2017-07-31 00:20 +0200
          Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Minchan Kim <minchan@kernel.org> - 2017-07-31 00:40 +0200
          Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Christoph Hellwig <hch@lst.de> - 2017-07-31 09:20 +0200
            Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Minchan Kim <minchan@kernel.org> - 2017-07-31 09:50 +0200
              Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Christoph Hellwig <hch@lst.de> - 2017-07-31 09:50 +0200
                Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Minchan Kim <minchan@kernel.org> - 2017-08-01 08:30 +0200
              Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Christoph Hellwig <hch@lst.de> - 2017-07-31 09:50 +0200
      Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Ross Zwisler <ross.zwisler@linux.intel.com> - 2017-08-03 00:20 +0200
        Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Minchan Kim <minchan@kernel.org> - 2017-08-03 02:20 +0200
          Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Dan Williams <dan.j.williams@intel.com> - 2017-08-03 02:40 +0200
          Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Christoph Hellwig <hch@lst.de> - 2017-08-03 10:10 +0200
            Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Minchan Kim <minchan@kernel.org> - 2017-08-04 03:00 +0200
          Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Ross Zwisler <ross.zwisler@linux.intel.com> - 2017-08-03 23:20 +0200
            Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Jens Axboe <axboe@kernel.dk> - 2017-08-03 23:20 +0200
            Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Minchan Kim <minchan@kernel.org> - 2017-08-04 06:00 +0200
              Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Minchan Kim <minchan@kernel.org> - 2017-08-04 10:20 +0200
                Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Dan Williams <dan.j.williams@intel.com> - 2017-08-04 20:10 +0200
                  Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Ross Zwisler <ross.zwisler@linux.intel.com> - 2017-08-04 20:30 +0200
                    Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Dan Williams <dan.j.williams@intel.com> - 2017-08-04 20:30 +0200
                      Re: [PATCH 0/3] remove rw_page() from brd, pmem and btt Minchan Kim <minchan@kernel.org> - 2017-08-07 10:30 +0200

Page 1 of 2  [1] 2  Next page →

#1698950 — [PATCH 0/3] 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.

The last existing user of the rw_page interface is the zram driver, and
according to the changelog for the patch that added zram_rw_page() that
driver does see a clear performance gain:

  I implemented the feature in zram and tested it.  Test bed was the G2, LG
  electronic mobile device, whtich has msm8974 processor and 2GB memory.

  With a memory allocation test program consuming memory, the system
  generates swap.

  Operating time of swap_write_page() was measured.

  --------------------------------------------------
  |             |   operating time   | improvement |
  |             |  (20 runs average) |             |
  --------------------------------------------------
  |with patch   |    1061.15 us      |    +2.4%    |
  --------------------------------------------------
  |without patch|    1087.35 us      |             |
  --------------------------------------------------

  Each test(with paged_io,with BIO) result set shows normal distribution
  and has equal variance.  I mean the two values are valid result to
  compare.  I can say operation with paged I/O(without BIO) is faster 2.4%
  with confidence level 95%.

These patches have passed ext4 and XFS xfstest regression testing with
a memory mode pmem driver (without DAX), with pmem + btt and with brd.

These patches apply cleanly to the current v4.13-rc2 based linux/master.

[1] https://lists.01.org/pipermail/linux-nvdimm/2017-July/011389.html
[2] https://www.mail-archive.com/linux-block@vger.kernel.org/msg11170.html

Ross Zwisler (3):
  btt: remove btt_rw_page()
  pmem: remove pmem_rw_page()
  brd: remove brd_rw_page()

 drivers/block/brd.c   | 10 ----------
 drivers/nvdimm/btt.c  | 15 ---------------
 drivers/nvdimm/pmem.c | 21 ---------------------
 3 files changed, 46 deletions(-)

-- 
2.9.4

[toc] | [next] | [standalone]

#1698953 — [PATCH 1/3] btt: remove btt_rw_page()

The rw_page() interface doesn't provide a clear performance benefit for the
BTT and has had a nonzero maintenance burden, so remove it.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Suggested-by: Christoph Hellwig <hch@infradead.org>
Cc: Matthew Wilcox <willy@infradead.org>
---
 drivers/nvdimm/btt.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
index 14323fa..e10d330 100644
--- a/drivers/nvdimm/btt.c
+++ b/drivers/nvdimm/btt.c
@@ -1236,20 +1236,6 @@ static blk_qc_t btt_make_request(struct request_queue *q, struct bio *bio)
 	return BLK_QC_T_NONE;
 }
 
-static int btt_rw_page(struct block_device *bdev, sector_t sector,
-		struct page *page, bool is_write)
-{
-	struct btt *btt = bdev->bd_disk->private_data;
-	int rc;
-
-	rc = btt_do_bvec(btt, NULL, page, PAGE_SIZE, 0, is_write, sector);
-	if (rc == 0)
-		page_endio(page, is_write, 0);
-
-	return rc;
-}
-
-
 static int btt_getgeo(struct block_device *bd, struct hd_geometry *geo)
 {
 	/* some standard values */
@@ -1261,7 +1247,6 @@ static int btt_getgeo(struct block_device *bd, struct hd_geometry *geo)
 
 static const struct block_device_operations btt_fops = {
 	.owner =		THIS_MODULE,
-	.rw_page =		btt_rw_page,
 	.getgeo =		btt_getgeo,
 	.revalidate_disk =	nvdimm_revalidate_disk,
 };
-- 
2.9.4

[toc] | [prev] | [next] | [standalone]

#1698958 — [PATCH 2/3] pmem: remove pmem_rw_page()

The rw_page() interface doesn't provide a clear performance benefit for
PMEM and has had a nonzero maintenance burden, so remove it.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Suggested-by: Christoph Hellwig <hch@infradead.org>
Cc: Matthew Wilcox <willy@infradead.org>
---
 drivers/nvdimm/pmem.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index f7099ada..f23c82d 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -182,26 +182,6 @@ static blk_qc_t pmem_make_request(struct request_queue *q, struct bio *bio)
 	return BLK_QC_T_NONE;
 }
 
-static int pmem_rw_page(struct block_device *bdev, sector_t sector,
-		       struct page *page, bool is_write)
-{
-	struct pmem_device *pmem = bdev->bd_queue->queuedata;
-	blk_status_t rc;
-
-	rc = pmem_do_bvec(pmem, page, PAGE_SIZE, 0, is_write, sector);
-
-	/*
-	 * The ->rw_page interface is subtle and tricky.  The core
-	 * retries on any error, so we can only invoke page_endio() in
-	 * the successful completion case.  Otherwise, we'll see crashes
-	 * caused by double completion.
-	 */
-	if (rc == 0)
-		page_endio(page, is_write, 0);
-
-	return blk_status_to_errno(rc);
-}
-
 /* see "strong" declaration in tools/testing/nvdimm/pmem-dax.c */
 __weak long __pmem_direct_access(struct pmem_device *pmem, pgoff_t pgoff,
 		long nr_pages, void **kaddr, pfn_t *pfn)
@@ -225,7 +205,6 @@ __weak long __pmem_direct_access(struct pmem_device *pmem, pgoff_t pgoff,
 
 static const struct block_device_operations pmem_fops = {
 	.owner =		THIS_MODULE,
-	.rw_page =		pmem_rw_page,
 	.revalidate_disk =	nvdimm_revalidate_disk,
 };
 
-- 
2.9.4

[toc] | [prev] | [next] | [standalone]

#1698963 — [PATCH 3/3] brd: remove brd_rw_page()

The rw_page() interface doesn't provide a clear performance benefit for
BRD and has had a nonzero maintenance burden, so remove it.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Suggested-by: Christoph Hellwig <hch@infradead.org>
Cc: Matthew Wilcox <willy@infradead.org>
---
 drivers/block/brd.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index 104b71c..29325058 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -322,15 +322,6 @@ static blk_qc_t brd_make_request(struct request_queue *q, struct bio *bio)
 	return BLK_QC_T_NONE;
 }
 
-static int brd_rw_page(struct block_device *bdev, sector_t sector,
-		       struct page *page, bool is_write)
-{
-	struct brd_device *brd = bdev->bd_disk->private_data;
-	int err = brd_do_bvec(brd, page, PAGE_SIZE, 0, is_write, sector);
-	page_endio(page, is_write, err);
-	return err;
-}
-
 #ifdef CONFIG_BLK_DEV_RAM_DAX
 static long __brd_direct_access(struct brd_device *brd, pgoff_t pgoff,
 		long nr_pages, void **kaddr, pfn_t *pfn)
@@ -370,7 +361,6 @@ static const struct dax_operations brd_dax_ops = {
 
 static const struct block_device_operations brd_fops = {
 	.owner =		THIS_MODULE,
-	.rw_page =		brd_rw_page,
 };
 
 /*
-- 
2.9.4

[toc] | [prev] | [next] | [standalone]

#1698976

On Fri, Jul 28, 2017 at 10:56:01AM -0600, Ross Zwisler wrote:
> 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.

[toc] | [prev] | [next] | [standalone]

#1699121

On Fri, 28 Jul 2017 10:31:43 -0700 Matthew Wilcox <willy@infradead.org> wrote:

> On Fri, Jul 28, 2017 at 10:56:01AM -0600, Ross Zwisler wrote:
> > 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.

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

[toc] | [prev] | [next] | [standalone]

#1699626

Hi Andrew,

On Fri, Jul 28, 2017 at 02:21:23PM -0700, Andrew Morton wrote:
> On Fri, 28 Jul 2017 10:31:43 -0700 Matthew Wilcox <willy@infradead.org> wrote:
> 
> > On Fri, Jul 28, 2017 at 10:56:01AM -0600, Ross Zwisler wrote:
> > > 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.
> 
> 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.

[toc] | [prev] | [next] | [standalone]

#1699628

On Mon, Jul 31, 2017 at 07:16:59AM +0900, Minchan Kim wrote:
> Hi Andrew,
> 
> On Fri, Jul 28, 2017 at 02:21:23PM -0700, Andrew Morton wrote:
> > On Fri, 28 Jul 2017 10:31:43 -0700 Matthew Wilcox <willy@infradead.org> wrote:
> > 
> > > On Fri, Jul 28, 2017 at 10:56:01AM -0600, Ross Zwisler wrote:
> > > > 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.
> > 
> > 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.

One of the idea is to create bioset only for swap without sharing
with FS so bio allocation for swap doesn't need to wait returning
bio from FS side which does slow nand IO to mempool.

[toc] | [prev] | [next] | [standalone]

#1699753

On Mon, Jul 31, 2017 at 07:16:59AM +0900, Minchan Kim wrote:
> 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.

[toc] | [prev] | [next] | [standalone]

#1699780

On Mon, Jul 31, 2017 at 09:17:07AM +0200, Christoph Hellwig wrote:
> On Mon, Jul 31, 2017 at 07:16:59AM +0900, Minchan Kim wrote:
> > 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?

[toc] | [prev] | [next] | [standalone]

#1699783

On Mon, Jul 31, 2017 at 09:42:06AM +0200, Christoph Hellwig wrote:
> On Mon, Jul 31, 2017 at 04:36:47PM +0900, Minchan Kim wrote:
> > 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.

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.

[toc] | [prev] | [next] | [standalone]

#1700663

On Mon, Jul 31, 2017 at 09:44:04AM +0200, Christoph Hellwig wrote:
> On Mon, Jul 31, 2017 at 09:42:06AM +0200, Christoph Hellwig wrote:
> > On Mon, Jul 31, 2017 at 04:36:47PM +0900, Minchan Kim wrote:
> > > 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.
> 
> 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.

Okay, I will look into that.
Thanks for the suggestion, Christoph.

[toc] | [prev] | [next] | [standalone]

#1699785

On Mon, Jul 31, 2017 at 04:36:47PM +0900, Minchan Kim wrote:
> 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.

[toc] | [prev] | [next] | [standalone]

#1702530

On Fri, Jul 28, 2017 at 10:31:43AM -0700, Matthew Wilcox wrote:
> On Fri, Jul 28, 2017 at 10:56:01AM -0600, Ross Zwisler wrote:
> > 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.

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.

[toc] | [prev] | [next] | [standalone]

#1702586

Hi Ross,

On Wed, Aug 02, 2017 at 04:13:59PM -0600, Ross Zwisler wrote:
> On Fri, Jul 28, 2017 at 10:31:43AM -0700, Matthew Wilcox wrote:
> > On Fri, Jul 28, 2017 at 10:56:01AM -0600, Ross Zwisler wrote:
> > > 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.
> 
> 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.

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.


diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 856d5dc02451..b1c5e9bf3ad5 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -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
 };
 
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index 854e1bdd0b2a..05eee145d964 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -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;
diff --git a/include/linux/swap.h b/include/linux/swap.h
index d83d28e53e62..86457dbfd300 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -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
diff --git a/mm/page_io.c b/mm/page_io.c
index b6c4ac388209..2c85e5182364 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c
@@ -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;
 }
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 6ba4aab2db0b..855d50eeeaf9 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -2931,6 +2931,9 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
 	if (bdi_cap_stable_pages_required(inode_to_bdi(inode)))
 		p->flags |= SWP_STABLE_WRITES;
 
+	if (bdi_cap_sync_io_required(inode_to_bdi(inode)))
+		p->flags |= SWP_SYNC_IO;
+
 	if (p->bdev && blk_queue_nonrot(bdev_get_queue(p->bdev))) {
 		int cpu;
 		unsigned long ci, nr_cluster;

[toc] | [prev] | [next] | [standalone]

#1702591

[ adding Tim and Ying who have also been looking at swap optimization
and rw_page interactions ]

On Wed, Aug 2, 2017 at 5:13 PM, Minchan Kim <minchan@kernel.org> wrote:
> Hi Ross,
>
> On Wed, Aug 02, 2017 at 04:13:59PM -0600, Ross Zwisler wrote:
>> On Fri, Jul 28, 2017 at 10:31:43AM -0700, Matthew Wilcox wrote:
>> > On Fri, Jul 28, 2017 at 10:56:01AM -0600, Ross Zwisler wrote:
>> > > 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.
>>
>> 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.
>
> 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.
>
>
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 856d5dc02451..b1c5e9bf3ad5 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -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
>  };
>
> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index 854e1bdd0b2a..05eee145d964 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -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;
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index d83d28e53e62..86457dbfd300 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -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
> diff --git a/mm/page_io.c b/mm/page_io.c
> index b6c4ac388209..2c85e5182364 100644
> --- a/mm/page_io.c
> +++ b/mm/page_io.c
> @@ -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;
>  }
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 6ba4aab2db0b..855d50eeeaf9 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -2931,6 +2931,9 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
>         if (bdi_cap_stable_pages_required(inode_to_bdi(inode)))
>                 p->flags |= SWP_STABLE_WRITES;
>
> +       if (bdi_cap_sync_io_required(inode_to_bdi(inode)))
> +               p->flags |= SWP_SYNC_IO;
> +
>         if (p->bdev && blk_queue_nonrot(bdev_get_queue(p->bdev))) {
>                 int cpu;
>                 unsigned long ci, nr_cluster;

[toc] | [prev] | [next] | [standalone]

#1702750

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

[toc] | [prev] | [next] | [standalone]

#1703571

On Thu, Aug 03, 2017 at 10:05:44AM +0200, Christoph Hellwig wrote:
> 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).

Think about readahead. Unconditional on-stack bio to read around pages
with faulted address will cause latency peek. So, I want to use that
synchronous IO only if device says "Hey, I'm a synchronous".

[toc] | [prev] | [next] | [standalone]

#1703419

On Thu, Aug 03, 2017 at 09:13:15AM +0900, Minchan Kim wrote:
> Hi Ross,
> 
> On Wed, Aug 02, 2017 at 04:13:59PM -0600, Ross Zwisler wrote:
> > On Fri, Jul 28, 2017 at 10:31:43AM -0700, Matthew Wilcox wrote:
> > > On Fri, Jul 28, 2017 at 10:56:01AM -0600, Ross Zwisler wrote:
> > > > 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.
> > 
> > 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.
> 
> 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.

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.

- Ross

[toc] | [prev] | [next] | [standalone]

#1703421

On 08/03/2017 03:13 PM, Ross Zwisler wrote:
> On Thu, Aug 03, 2017 at 09:13:15AM +0900, Minchan Kim wrote:
>> Hi Ross,
>>
>> On Wed, Aug 02, 2017 at 04:13:59PM -0600, Ross Zwisler wrote:
>>> On Fri, Jul 28, 2017 at 10:31:43AM -0700, Matthew Wilcox wrote:
>>>> On Fri, Jul 28, 2017 at 10:56:01AM -0600, Ross Zwisler wrote:
>>>>> 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.
>>>
>>> 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.
>>
>> 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.
> 
> 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.

IMHO, the win needs to be pretty substantial to justify keeping a
parallel read/write path in the kernel. The recent work of making
O_DIRECT faster is exactly the same as what Minchan did here for sync
IO. I would greatly prefer one fast path, instead of one fast and one
that's just a little faster for some things. It's much better to get
everyone behind one path/stack, and make that as fast as it can be.

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web