| Message ID | 20220427160255.300418-14-p.raghav@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | support non power of 2 zoned devices | expand |
On Wed, Apr 27, 2022 at 06:02:52PM +0200, 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> > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> > --- > drivers/block/null_blk/main.c | 4 ++-- > drivers/block/null_blk/zoned.c | 14 +++++++------- > 2 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c > index c441a4972064..82a62b543782 100644 > --- a/drivers/block/null_blk/main.c > +++ b/drivers/block/null_blk/main.c > @@ -1931,8 +1931,8 @@ static int null_validate_conf(struct nullb_device *dev) > 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"); > + (!dev->zone_size)) { > + pr_err("zone_size must not be zero\n"); > return -EINVAL; > } > > diff --git a/drivers/block/null_blk/zoned.c b/drivers/block/null_blk/zoned.c > index dae54dd1aeac..00c34e65ef0a 100644 > --- a/drivers/block/null_blk/zoned.c > +++ b/drivers/block/null_blk/zoned.c > @@ -13,7 +13,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 (is_power_of_2(dev->zone_size_sects)) > + return sect >> ilog2(dev->zone_size_sects); > + > + return div64_u64(sect, dev->zone_size_sects); > } > > static inline void null_lock_zone_res(struct nullb_device *dev) > @@ -62,10 +65,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; > @@ -83,8 +82,9 @@ 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); > + dev->nr_zones = > + div64_u64(roundup(dev_capacity_sects, dev->zone_size_sects), > + dev->zone_size_sects); > > dev->zones = kvmalloc_array(dev->nr_zones, sizeof(struct nullb_zone), > GFP_KERNEL | __GFP_ZERO); > -- > 2.25.1 > Looks good. Reviewed by: Adam Manzanares <a.manzanares@samsung.com>
On 4/27/22 09:02, Pankaj Raghav wrote: > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c > index c441a4972064..82a62b543782 100644 > --- a/drivers/block/null_blk/main.c > +++ b/drivers/block/null_blk/main.c > @@ -1931,8 +1931,8 @@ static int null_validate_conf(struct nullb_device *dev) > 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"); > + (!dev->zone_size)) { > + pr_err("zone_size must not be zero\n"); > return -EINVAL; > } Please combine "if (dev->zoned &&" and "(!dev->zone_size)) {" into a single line and leave out the parentheses that became superfluous. Thanks, Bart.
On 4/27/22 09:02, 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> > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> > --- > drivers/block/null_blk/main.c | 4 ++-- > drivers/block/null_blk/zoned.c | 14 +++++++------- > 2 files changed, 9 insertions(+), 9 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index c441a4972064..82a62b543782 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -1931,8 +1931,8 @@ static int null_validate_conf(struct nullb_device *dev) 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"); + (!dev->zone_size)) { + pr_err("zone_size must not be zero\n"); return -EINVAL; } diff --git a/drivers/block/null_blk/zoned.c b/drivers/block/null_blk/zoned.c index dae54dd1aeac..00c34e65ef0a 100644 --- a/drivers/block/null_blk/zoned.c +++ b/drivers/block/null_blk/zoned.c @@ -13,7 +13,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 (is_power_of_2(dev->zone_size_sects)) + return sect >> ilog2(dev->zone_size_sects); + + return div64_u64(sect, dev->zone_size_sects); } static inline void null_lock_zone_res(struct nullb_device *dev) @@ -62,10 +65,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; @@ -83,8 +82,9 @@ 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); + dev->nr_zones = + div64_u64(roundup(dev_capacity_sects, dev->zone_size_sects), + dev->zone_size_sects); dev->zones = kvmalloc_array(dev->nr_zones, sizeof(struct nullb_zone), GFP_KERNEL | __GFP_ZERO);