| Message ID | 20220727162245.209794-6-p.raghav@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | support non power of 2 zoned device | expand |
> Performance Measurement: > > Device: > zone size = 128M, blocksize=4k > > FIO cmd: > > fio --name=zbc --filename=/dev/nullb0 --direct=1 --zonemode=zbd --size=23G > --io_size=<iosize> --ioengine=io_uring --iodepth=<iod> --rw=<mode> --bs=4k > --loops=4 > > The following results are an average of 4 runs on AMD Ryzen 5 5600X with > 32GB of RAM: > > Sequential Write: > > x-----------------x---------------------------------x---------------------------------x > | IOdepth | 8 | 16 | > x-----------------x---------------------------------x---------------------------------x > | | KIOPS |BW(MiB/s) | Lat(usec) | KIOPS |BW(MiB/s) | Lat(usec) | > x-----------------x---------------------------------x---------------------------------x > | Without patch | 578 | 2257 | 12.80 | 576 | 2248 | 25.78 | > x-----------------x---------------------------------x---------------------------------x > | With patch | 581 | 2268 | 12.74 | 576 | 2248 | 25.85 | > x-----------------x---------------------------------x---------------------------------x > > Sequential read: > > x-----------------x---------------------------------x---------------------------------x > | IOdepth | 8 | 16 | > x-----------------x---------------------------------x---------------------------------x > | | KIOPS |BW(MiB/s) | Lat(usec) | KIOPS |BW(MiB/s) | Lat(usec) | > x-----------------x---------------------------------x---------------------------------x > | Without patch | 667 | 2605 | 11.79 | 675 | 2637 | 23.49 | > x-----------------x---------------------------------x---------------------------------x > | With patch | 667 | 2605 | 11.79 | 675 | 2638 | 23.48 | > x-----------------x---------------------------------x---------------------------------x > > Random read: > > x-----------------x---------------------------------x---------------------------------x > | IOdepth | 8 | 16 | > x-----------------x---------------------------------x---------------------------------x > | | KIOPS |BW(MiB/s) | Lat(usec) | KIOPS |BW(MiB/s) | Lat(usec) | > x-----------------x---------------------------------x---------------------------------x > | Without patch | 522 | 2038 | 15.05 | 514 | 2006 | 30.87 | > x-----------------x---------------------------------x---------------------------------x > | With patch | 522 | 2039 | 15.04 | 523 | 2042 | 30.33 | > x-----------------x---------------------------------x---------------------------------x > > Minor variations are noticed in Sequential write with io depth 8 and > in random read with io depth 16. But overall no noticeable differences > were noticed minor variations in with aspect of the performance ? are these documented somewhere ? move the large table of performance numbers to the cover letter it looks ugly in the git log...
On 7/28/22 01:22, Pankaj Raghav wrote: > Convert the power of 2 based calculation with zone size to be generic in > null_zone_no with optimization for power of 2 based zone sizes. > > The nr_zones calculation in null_init_zoned_dev has been replaced with a > division without special handling for power of 2 based zone sizes as > this function is called only during the initialization and will not > invoked in the hot path. > > Performance Measurement: > > Device: > zone size = 128M, blocksize=4k > > FIO cmd: > > fio --name=zbc --filename=/dev/nullb0 --direct=1 --zonemode=zbd --size=23G > --io_size=<iosize> --ioengine=io_uring --iodepth=<iod> --rw=<mode> --bs=4k > --loops=4 > > The following results are an average of 4 runs on AMD Ryzen 5 5600X with > 32GB of RAM: > > Sequential Write: > > x-----------------x---------------------------------x---------------------------------x > | IOdepth | 8 | 16 | > x-----------------x---------------------------------x---------------------------------x > | | KIOPS |BW(MiB/s) | Lat(usec) | KIOPS |BW(MiB/s) | Lat(usec) | > x-----------------x---------------------------------x---------------------------------x > | Without patch | 578 | 2257 | 12.80 | 576 | 2248 | 25.78 | > x-----------------x---------------------------------x---------------------------------x > | With patch | 581 | 2268 | 12.74 | 576 | 2248 | 25.85 | > x-----------------x---------------------------------x---------------------------------x > > Sequential read: > > x-----------------x---------------------------------x---------------------------------x > | IOdepth | 8 | 16 | > x-----------------x---------------------------------x---------------------------------x > | | KIOPS |BW(MiB/s) | Lat(usec) | KIOPS |BW(MiB/s) | Lat(usec) | > x-----------------x---------------------------------x---------------------------------x > | Without patch | 667 | 2605 | 11.79 | 675 | 2637 | 23.49 | > x-----------------x---------------------------------x---------------------------------x > | With patch | 667 | 2605 | 11.79 | 675 | 2638 | 23.48 | > x-----------------x---------------------------------x---------------------------------x > > Random read: > > x-----------------x---------------------------------x---------------------------------x > | IOdepth | 8 | 16 | > x-----------------x---------------------------------x---------------------------------x > | | KIOPS |BW(MiB/s) | Lat(usec) | KIOPS |BW(MiB/s) | Lat(usec) | > x-----------------x---------------------------------x---------------------------------x > | Without patch | 522 | 2038 | 15.05 | 514 | 2006 | 30.87 | > x-----------------x---------------------------------x---------------------------------x > | With patch | 522 | 2039 | 15.04 | 523 | 2042 | 30.33 | > x-----------------x---------------------------------x---------------------------------x > > Minor variations are noticed in Sequential write with io depth 8 and > in random read with io depth 16. But overall no noticeable differences > were noticed > > Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> > Reviewed by: Adam Manzanares <a.manzanares@samsung.com> > Reviewed-by: Hannes Reinecke <hare@suse.de> > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> > --- > drivers/block/null_blk/main.c | 5 ++--- > drivers/block/null_blk/null_blk.h | 6 ++++++ > drivers/block/null_blk/zoned.c | 18 +++++++++++------- > 3 files changed, 19 insertions(+), 10 deletions(-) > > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c > index c451c477978f..f1e0605dee94 100644 > --- a/drivers/block/null_blk/main.c > +++ b/drivers/block/null_blk/main.c > @@ -1976,9 +1976,8 @@ static int null_validate_conf(struct nullb_device *dev) > if (dev->queue_mode == NULL_Q_BIO) > dev->mbps = 0; > > - if (dev->zoned && > - (!dev->zone_size || !is_power_of_2(dev->zone_size))) { > - pr_err("zone_size must be power-of-two\n"); > + if (dev->zoned && !dev->zone_size) { > + pr_err("Invalid zero zone size\n"); > return -EINVAL; > } > > diff --git a/drivers/block/null_blk/null_blk.h b/drivers/block/null_blk/null_blk.h > index 94ff68052b1e..ece6dded9508 100644 > --- a/drivers/block/null_blk/null_blk.h > +++ b/drivers/block/null_blk/null_blk.h > @@ -83,6 +83,12 @@ struct nullb_device { > unsigned int imp_close_zone_no; > struct nullb_zone *zones; > sector_t zone_size_sects; > + /* > + * zone_size_sects_shift is only useful when the zone size is > + * power of 2. This variable is set to zero when zone size is non > + * power of 2. > + */ Simplify: /* * zone_size_sects_shift is used only when the zone size is a * power of 2 number of sectors. */ But personally, I would simply drop this comment. The name is obvious and the code very clear. > + unsigned int zone_size_sects_shift; > bool need_zone_res_mgmt; > spinlock_t zone_res_lock; > > diff --git a/drivers/block/null_blk/zoned.c b/drivers/block/null_blk/zoned.c > index 55a69e48ef8b..015f6823706c 100644 > --- a/drivers/block/null_blk/zoned.c > +++ b/drivers/block/null_blk/zoned.c > @@ -16,7 +16,10 @@ static inline sector_t mb_to_sects(unsigned long mb) > > static inline unsigned int null_zone_no(struct nullb_device *dev, sector_t sect) > { > - return sect >> ilog2(dev->zone_size_sects); > + if (dev->zone_size_sects_shift) > + return sect >> dev->zone_size_sects_shift; > + > + return div64_u64(sect, dev->zone_size_sects); > } > > static inline void null_lock_zone_res(struct nullb_device *dev) > @@ -65,10 +68,6 @@ int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q) > sector_t sector = 0; > unsigned int i; > > - if (!is_power_of_2(dev->zone_size)) { > - pr_err("zone_size must be power-of-two\n"); > - return -EINVAL; > - } > if (dev->zone_size > dev->size) { > pr_err("Zone size larger than device capacity\n"); > return -EINVAL; > @@ -86,9 +85,14 @@ int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q) > zone_capacity_sects = mb_to_sects(dev->zone_capacity); > dev_capacity_sects = mb_to_sects(dev->size); > dev->zone_size_sects = mb_to_sects(dev->zone_size); > - dev->nr_zones = round_up(dev_capacity_sects, dev->zone_size_sects) > - >> ilog2(dev->zone_size_sects); > > + if (is_power_of_2(dev->zone_size_sects)) > + dev->zone_size_sects_shift = ilog2(dev->zone_size_sects); > + else > + dev->zone_size_sects_shift = 0; > + > + dev->nr_zones = DIV_ROUND_UP_SECTOR_T(dev_capacity_sects, > + dev->zone_size_sects); > dev->zones = kvmalloc_array(dev->nr_zones, sizeof(struct nullb_zone), > GFP_KERNEL | __GFP_ZERO); > if (!dev->zones)
On 2022-07-27 23:59, Chaitanya Kulkarni wrote: >> Sequential Write: >> >> x-----------------x---------------------------------x---------------------------------x >> | IOdepth | 8 | 16 | >> x-----------------x---------------------------------x---------------------------------x >> | | KIOPS |BW(MiB/s) | Lat(usec) | KIOPS |BW(MiB/s) | Lat(usec) | >> x-----------------x---------------------------------x---------------------------------x >> | Without patch | 578 | 2257 | 12.80 | 576 | 2248 | 25.78 | >> x-----------------x---------------------------------x---------------------------------x >> | With patch | 581 | 2268 | 12.74 | 576 | 2248 | 25.85 | >> x-----------------x---------------------------------x---------------------------------x >> >> Sequential read: >> >> x-----------------x---------------------------------x---------------------------------x >> | IOdepth | 8 | 16 | >> x-----------------x---------------------------------x---------------------------------x >> | | KIOPS |BW(MiB/s) | Lat(usec) | KIOPS |BW(MiB/s) | Lat(usec) | >> x-----------------x---------------------------------x---------------------------------x >> | Without patch | 667 | 2605 | 11.79 | 675 | 2637 | 23.49 | >> x-----------------x---------------------------------x---------------------------------x >> | With patch | 667 | 2605 | 11.79 | 675 | 2638 | 23.48 | >> x-----------------x---------------------------------x---------------------------------x >> >> Random read: >> >> x-----------------x---------------------------------x---------------------------------x >> | IOdepth | 8 | 16 | >> x-----------------x---------------------------------x---------------------------------x >> | | KIOPS |BW(MiB/s) | Lat(usec) | KIOPS |BW(MiB/s) | Lat(usec) | >> x-----------------x---------------------------------x---------------------------------x >> | Without patch | 522 | 2038 | 15.05 | 514 | 2006 | 30.87 | >> x-----------------x---------------------------------x---------------------------------x >> | With patch | 522 | 2039 | 15.04 | 523 | 2042 | 30.33 | >> x-----------------x---------------------------------x---------------------------------x >> >> Minor variations are noticed in Sequential write with io depth 8 and >> in random read with io depth 16. But overall no noticeable differences >> were noticed > > minor variations in with aspect of the performance ? > are these documented somewhere ? > The table above documents those minor variations in performance. > move the large table of performance numbers to the cover letter it looks > ugly in the git log... > I could do that. >
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index c451c477978f..f1e0605dee94 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -1976,9 +1976,8 @@ static int null_validate_conf(struct nullb_device *dev) if (dev->queue_mode == NULL_Q_BIO) dev->mbps = 0; - if (dev->zoned && - (!dev->zone_size || !is_power_of_2(dev->zone_size))) { - pr_err("zone_size must be power-of-two\n"); + if (dev->zoned && !dev->zone_size) { + pr_err("Invalid zero zone size\n"); return -EINVAL; } diff --git a/drivers/block/null_blk/null_blk.h b/drivers/block/null_blk/null_blk.h index 94ff68052b1e..ece6dded9508 100644 --- a/drivers/block/null_blk/null_blk.h +++ b/drivers/block/null_blk/null_blk.h @@ -83,6 +83,12 @@ struct nullb_device { unsigned int imp_close_zone_no; struct nullb_zone *zones; sector_t zone_size_sects; + /* + * zone_size_sects_shift is only useful when the zone size is + * power of 2. This variable is set to zero when zone size is non + * power of 2. + */ + unsigned int zone_size_sects_shift; bool need_zone_res_mgmt; spinlock_t zone_res_lock; diff --git a/drivers/block/null_blk/zoned.c b/drivers/block/null_blk/zoned.c index 55a69e48ef8b..015f6823706c 100644 --- a/drivers/block/null_blk/zoned.c +++ b/drivers/block/null_blk/zoned.c @@ -16,7 +16,10 @@ static inline sector_t mb_to_sects(unsigned long mb) static inline unsigned int null_zone_no(struct nullb_device *dev, sector_t sect) { - return sect >> ilog2(dev->zone_size_sects); + if (dev->zone_size_sects_shift) + return sect >> dev->zone_size_sects_shift; + + return div64_u64(sect, dev->zone_size_sects); } static inline void null_lock_zone_res(struct nullb_device *dev) @@ -65,10 +68,6 @@ int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q) sector_t sector = 0; unsigned int i; - if (!is_power_of_2(dev->zone_size)) { - pr_err("zone_size must be power-of-two\n"); - return -EINVAL; - } if (dev->zone_size > dev->size) { pr_err("Zone size larger than device capacity\n"); return -EINVAL; @@ -86,9 +85,14 @@ int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q) zone_capacity_sects = mb_to_sects(dev->zone_capacity); dev_capacity_sects = mb_to_sects(dev->size); dev->zone_size_sects = mb_to_sects(dev->zone_size); - dev->nr_zones = round_up(dev_capacity_sects, dev->zone_size_sects) - >> ilog2(dev->zone_size_sects); + if (is_power_of_2(dev->zone_size_sects)) + dev->zone_size_sects_shift = ilog2(dev->zone_size_sects); + else + dev->zone_size_sects_shift = 0; + + dev->nr_zones = DIV_ROUND_UP_SECTOR_T(dev_capacity_sects, + dev->zone_size_sects); dev->zones = kvmalloc_array(dev->nr_zones, sizeof(struct nullb_zone), GFP_KERNEL | __GFP_ZERO); if (!dev->zones)