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

[Nitesh Shetty <nj.shetty@samsung.com>]

[-- 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 --]



[-- Attachment #3: Type: text/plain, Size: 98 bytes --]

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel