Re: [PATCH v4 01/10] block: Introduce queue limits for copy-offload support

Nitesh Shetty
2022-04-27T18:09:09Z

[-- Attachment #1: Type: text/plain, Size: 12544 bytes --]

On Wed, Apr 27, 2022 at 10:59:01AM +0900, Damien Le Moal wrote:
> On 4/26/22 19:12, Nitesh Shetty wrote:
> > Add device limits as sysfs entries,
> > - copy_offload (RW)
> > - copy_max_bytes (RW)
> > - copy_max_hw_bytes (RO)
> > - copy_max_range_bytes (RW)
> > - copy_max_range_hw_bytes (RO)
> > - copy_max_nr_ranges (RW)
> > - copy_max_nr_ranges_hw (RO)
> >
> > Above limits help to split the copy payload in block layer.
> > copy_offload, used for setting copy offload(1) or emulation(0).
> > copy_max_bytes: maximum total length of copy in single payload.
> > copy_max_range_bytes: maximum length in a single entry.
> > copy_max_nr_ranges: maximum number of entries in a payload.
> > copy_max_*_hw_*: Reflects the device supported maximum limits.
> >
> > Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
> > Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
> > Signed-off-by: Arnav Dawn <arnav.dawn@samsung.com>
> > ---
> > Documentation/ABI/stable/sysfs-block | 83 ++++++++++++++++
> > block/blk-settings.c | 59 ++++++++++++
> > block/blk-sysfs.c | 138 +++++++++++++++++++++++++++
> > include/linux/blkdev.h | 13 +++
> > 4 files changed, 293 insertions(+)
> >
> > diff --git a/Documentation/ABI/stable/sysfs-block b/Documentation/ABI/stable/sysfs-block
> > index e8797cd09aff..65e64b5a0105 100644
> > --- a/Documentation/ABI/stable/sysfs-block
> > +++ b/Documentation/ABI/stable/sysfs-block
> > @@ -155,6 +155,89 @@ Description:
> > last zone of the device which may be smaller.
> >
> >
> > +What: /sys/block/<disk>/queue/copy_offload
> > +Date: April 2022
> > +Contact: linux-block@vger.kernel.org
> > +Description:
> > + [RW] When read, this file shows whether offloading copy to
> > + device is enabled (1) or disabled (0). Writing '0' to this
> > + file will disable offloading copies for this device.
> > + Writing any '1' value will enable this feature.
> > +
> > +
> > +What: /sys/block/<disk>/queue/copy_max_bytes
> > +Date: April 2022
> > +Contact: linux-block@vger.kernel.org
> > +Description:
> > + [RW] While 'copy_max_hw_bytes' is the hardware limit for the
> > + device, 'copy_max_bytes' setting is the software limit.
> > + Setting this value lower will make Linux issue smaller size
> > + copies.
> > +
> > +
> > +What: /sys/block/<disk>/queue/copy_max_hw_bytes
> > +Date: April 2022
> > +Contact: linux-block@vger.kernel.org
> > +Description:
> > + [RO] Devices that support offloading copy functionality may have
> > + internal limits on the number of bytes that can be offloaded
> > + in a single operation. The `copy_max_hw_bytes`
> > + parameter is set by the device driver to the maximum number of
> > + bytes that can be copied in a single operation. Copy
> > + requests issued to the device must not exceed this limit.
> > + A value of 0 means that the device does not
> > + support copy offload.
> > +
> > +
> > +What: /sys/block/<disk>/queue/copy_max_nr_ranges
> > +Date: April 2022
> > +Contact: linux-block@vger.kernel.org
> > +Description:
> > + [RW] While 'copy_max_nr_ranges_hw' is the hardware limit for the
> > + device, 'copy_max_nr_ranges' setting is the software limit.
> > +
> > +
> > +What: /sys/block/<disk>/queue/copy_max_nr_ranges_hw
> > +Date: April 2022
> > +Contact: linux-block@vger.kernel.org
> > +Description:
> > + [RO] Devices that support offloading copy functionality may have
> > + internal limits on the number of ranges in single copy operation
> > + that can be offloaded in a single operation.
> > + A range is tuple of source, destination and length of data
> > + to be copied. The `copy_max_nr_ranges_hw` parameter is set by
> > + the device driver to the maximum number of ranges that can be
> > + copied in a single operation. Copy requests issued to the device
> > + must not exceed this limit. A value of 0 means that the device
> > + does not support copy offload.
> > +
> > +
> > +What: /sys/block/<disk>/queue/copy_max_range_bytes
> > +Date: April 2022
> > +Contact: linux-block@vger.kernel.org
> > +Description:
> > + [RW] While 'copy_max_range_hw_bytes' is the hardware limit for
> > + the device, 'copy_max_range_bytes' setting is the software
> > + limit.
> > +
> > +
> > +What: /sys/block/<disk>/queue/copy_max_range_hw_bytes
> > +Date: April 2022
> > +Contact: linux-block@vger.kernel.org
> > +Description:
> > + [RO] Devices that support offloading copy functionality may have
> > + internal limits on the size of data, that can be copied in a
> > + single range within a single copy operation.
> > + A range is tuple of source, destination and length of data to be
> > + copied. The `copy_max_range_hw_bytes` parameter is set by the
> > + device driver to set the maximum length in bytes of a range
> > + that can be copied in an operation.
> > + Copy requests issued to the device must not exceed this limit.
> > + Sum of sizes of all ranges in a single opeartion should not
> > + exceed 'copy_max_hw_bytes'. A value of 0 means that the device
> > + does not support copy offload.
> > +
> > +
> > What: /sys/block/<disk>/queue/crypto/
> > Date: February 2022
> > Contact: linux-block@vger.kernel.org
> > diff --git a/block/blk-settings.c b/block/blk-settings.c
> > index 6ccceb421ed2..70167aee3bf7 100644
> > --- a/block/blk-settings.c
> > +++ b/block/blk-settings.c
> > @@ -57,6 +57,12 @@ void blk_set_default_limits(struct queue_limits *lim)
> > lim->misaligned = 0;
> > lim->zoned = BLK_ZONED_NONE;
> > lim->zone_write_granularity = 0;
> > + lim->max_hw_copy_sectors = 0;
>
> For readability, I would keep "hw" next to sectors/nr_ranges:
>
> max_copy_hw_sectors
> max_copy_sectors
> max_copy_hw_nr_ranges
> max_copy_nr_ranges
> max_copy_range_hw_sectors
> max_copy_range_sectors
>

acked

> > + lim->max_copy_sectors = 0;
> > + lim->max_hw_copy_nr_ranges = 0;
> > + lim->max_copy_nr_ranges = 0;
> > + lim->max_hw_copy_range_sectors = 0;
> > + lim->max_copy_range_sectors = 0;
> > }
> > EXPORT_SYMBOL(blk_set_default_limits);
> >
> > @@ -81,6 +87,12 @@ void blk_set_stacking_limits(struct queue_limits *lim)
> > lim->max_dev_sectors = UINT_MAX;
> > lim->max_write_zeroes_sectors = UINT_MAX;
> > lim->max_zone_append_sectors = UINT_MAX;
> > + lim->max_hw_copy_sectors = ULONG_MAX;
> > + lim->max_copy_sectors = ULONG_MAX;
> > + lim->max_hw_copy_range_sectors = UINT_MAX;
> > + lim->max_copy_range_sectors = UINT_MAX;
> > + lim->max_hw_copy_nr_ranges = USHRT_MAX;
> > + lim->max_copy_nr_ranges = USHRT_MAX;
> > }
> > EXPORT_SYMBOL(blk_set_stacking_limits);
> >
> > @@ -177,6 +189,45 @@ void blk_queue_max_discard_sectors(struct request_queue *q,
> > }
> > EXPORT_SYMBOL(blk_queue_max_discard_sectors);
> >
> > +/**
> > + * blk_queue_max_copy_sectors - set max sectors for a single copy payload
> > + * @q: the request queue for the device
> > + * @max_copy_sectors: maximum number of sectors to copy
> > + **/
> > +void blk_queue_max_copy_sectors(struct request_queue *q,
>
> This should be blk_queue_max_copy_hw_sectors().
>

acked. Reasoning being, this function is used only by driver once for setting hw
limits ?

> > + unsigned int max_copy_sectors)
> > +{
> > + q->limits.max_hw_copy_sectors = max_copy_sectors;
> > + q->limits.max_copy_sectors = max_copy_sectors;
> > +}
> > +EXPORT_SYMBOL_GPL(blk_queue_max_copy_sectors);
> > +
> > +/**
> > + * blk_queue_max_copy_range_sectors - set max sectors for a single range, in a copy payload
> > + * @q: the request queue for the device
> > + * @max_copy_range_sectors: maximum number of sectors to copy in a single range
> > + **/
> > +void blk_queue_max_copy_range_sectors(struct request_queue *q,
>
> And this should be blk_queue_max_copy_range_hw_sectors(). Etc for the
> other ones below.
>

acked

> > + unsigned int max_copy_range_sectors)
> > +{
> > + q->limits.max_hw_copy_range_sectors = max_copy_range_sectors;
> > + q->limits.max_copy_range_sectors = max_copy_range_sectors;
> > +}
> > +EXPORT_SYMBOL_GPL(blk_queue_max_copy_range_sectors);
> > +
> > +/**
> > + * blk_queue_max_copy_nr_ranges - set max number of ranges, in a copy payload
> > + * @q: the request queue for the device
> > + * @max_copy_nr_ranges: maximum number of ranges
> > + **/
> > +void blk_queue_max_copy_nr_ranges(struct request_queue *q,
> > + unsigned int max_copy_nr_ranges)
> > +{
> > + q->limits.max_hw_copy_nr_ranges = max_copy_nr_ranges;
> > + q->limits.max_copy_nr_ranges = max_copy_nr_ranges;
> > +}
> > +EXPORT_SYMBOL_GPL(blk_queue_max_copy_nr_ranges);
> > +
> > /**
> > * blk_queue_max_secure_erase_sectors - set max sectors for a secure erase
> > * @q: the request queue for the device
> > @@ -572,6 +623,14 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
> > t->max_segment_size = min_not_zero(t->max_segment_size,
> > b->max_segment_size);
> >
> > + t->max_copy_sectors = min(t->max_copy_sectors, b->max_copy_sectors);
> > + t->max_hw_copy_sectors = min(t->max_hw_copy_sectors, b->max_hw_copy_sectors);
> > + t->max_copy_range_sectors = min(t->max_copy_range_sectors, b->max_copy_range_sectors);
> > + t->max_hw_copy_range_sectors = min(t->max_hw_copy_range_sectors,
> > + b->max_hw_copy_range_sectors);
> > + t->max_copy_nr_ranges = min(t->max_copy_nr_ranges, b->max_copy_nr_ranges);
> > + t->max_hw_copy_nr_ranges = min(t->max_hw_copy_nr_ranges, b->max_hw_copy_nr_ranges);
> > +
> > t->misaligned |= b->misaligned;
> >
> > alignment = queue_limit_alignment_offset(b, start);
> > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> > index 88bd41d4cb59..bae987c10f7f 100644
> > --- a/block/blk-sysfs.c
> > +++ b/block/blk-sysfs.c
> > @@ -212,6 +212,129 @@ static ssize_t queue_discard_zeroes_data_show(struct request_queue *q, char *pag
> > return queue_var_show(0, page);
> > }
> >
> > +static ssize_t queue_copy_offload_show(struct request_queue *q, char *page)
> > +{
> > + return queue_var_show(blk_queue_copy(q), page);
> > +}
> > +
> > +static ssize_t queue_copy_offload_store(struct request_queue *q,
> > + const char *page, size_t count)
> > +{
> > + unsigned long copy_offload;
> > + ssize_t ret = queue_var_store(&copy_offload, page, count);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (copy_offload && !q->limits.max_hw_copy_sectors)
> > + return -EINVAL;
> > +
> > + if (copy_offload)
> > + blk_queue_flag_set(QUEUE_FLAG_COPY, q);
> > + else
> > + blk_queue_flag_clear(QUEUE_FLAG_COPY, q);
> > +
> > + return ret;
> > +}
> > +
> > +static ssize_t queue_copy_max_hw_show(struct request_queue *q, char *page)
> > +{
> > + return sprintf(page, "%llu\n",
> > + (unsigned long long)q->limits.max_hw_copy_sectors << 9);
> > +}
> > +
> > +static ssize_t queue_copy_max_show(struct request_queue *q, char *page> +{
> > + return sprintf(page, "%llu\n",
> > + (unsigned long long)q->limits.max_copy_sectors << 9);
> > +}
> > +
> > +static ssize_t queue_copy_max_store(struct request_queue *q,
> > + const char *page, size_t count)
> > +{
> > + unsigned long max_copy;
> > + ssize_t ret = queue_var_store(&max_copy, page, count);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (max_copy & (queue_logical_block_size(q) - 1))
> > + return -EINVAL;
> > +
> > + max_copy >>= 9;
> > + if (max_copy > q->limits.max_hw_copy_sectors)
> > + max_copy = q->limits.max_hw_copy_sectors;
> > +
> > + q->limits.max_copy_sectors = max_copy;
> > + return ret;
> > +}
> > +
> > +static ssize_t queue_copy_range_max_hw_show(struct request_queue *q, char *page)
> > +{
> > + return sprintf(page, "%llu\n",
> > + (unsigned long long)q->limits.max_hw_copy_range_sectors << 9);
> > +}
> > +
> > +static ssize_t queue_copy_range_max_show(struct request_queue *q,
> > + char *page)
> > +{
> > + return sprintf(page, "%llu\n",
> > + (unsigned long long)q->limits.max_copy_range_sectors << 9);
> > +}
> > +
> > +static ssize_t queue_copy_range_max_store(struct request_queue *q,
> > + const char *page, size_t count)
> > +{
> > + unsigned long max_copy;
> > + ssize_t ret = queue_var_store(&max_copy, page, count);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (max_copy & (queue_logical_block_size(q) - 1))
> > + return -EINVAL;
> > +
> > + max_copy >>= 9;
> > + if (max_copy > UINT_MAX)
>
> On 32-bits arch, unsigned long and unsigned int are the same so this test
> is useless for these arch. Better have max_copy declared as unsigned long
> long.
>

acked

--
Nitesh Shetty

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]