scatterlist: add new capabilities
Subsystems (3)
v7 cover scatterlist: add new capabilities
Scatter-gather lists (sgl_s) are frequently used as data carriers in the block layer. For example the SCSI and NVMe subsystems interchange data with the block layer using sgl_s. The sgl API is declared in <linux/scatterlist.h>
This patchset extends the scatterlist API by adding functions to:
- copy a sgl to another sgl, stop after copying n_bytes or when either sgl is exhausted [2/4]
- compare one sgl against another sgl for equality. Stop when either sgl is exhausted, or a miscompare is detected or when n_bytes are compared. Supply a variant function that gives the position of the miscompare [3/4]
- generalize the existing sg_zero_buffer() function with a new sgl_memset function [4/4]
The first patch [1/4] removes a 4 GiB size limitation from the sgl_alloc_order() function.
The author changed the backing store (i.e. ramdisks) behind the scsi_debug driver from using vmalloc() to using the scatterlist API with the above additions. The removal of the 4 GiB size limit allows scsi_debug to mimic a disk of larger size. Being able to copy one sgl to another simplifies implementing SCSI READ and WRITE commands. The sgl_equal_sgl() function both simplifies the SCSI VERIFY(BytChk=1) and COMPARE AND WRITE commands and is a performance win as there is no need for a temporary buffer to hold the data-out transfer associated with these comparison commands.
The target subsystem and NVMe may find these additions to the scatterlist API useful.
Changes since v6 [posted 20210118]:
- re-add sgl_alloc_order() fix to remove its (undocumented) 4 GiB limit
- rebase on lk 5.17.0-rc1
Changes since v5 [posted 20201228]:
- incorporate review requests from Jason Gunthorpe
- replace integer overflow detection code in sgl_alloc_order() with a pre-condition statement
- rebase on lk 5.11.0-rc4
Changes since v4 [posted 20201105]:
- rebase on lk 5.10.0-rc2
Changes since v3 [posted 20201019]:
- re-instate check on integer overflow of nent calculation in sgl_alloc_order(). Do it in such a way as to not limit the overall sgl size to 4 GiB
- introduce sgl_compare_sgl_idx() helper function that, if requested and if a miscompare is detected, will yield the byte index of the first miscompare.
- add Reviewed-by tags from Bodo Stroesser
- rebase on lk 5.10.0-rc2 [was on lk 5.9.0]
Changes since v2 [posted 20201018]:
- remove unneeded lines from sgl_memset() definition.
- change sg_zero_buffer() to call sgl_memset() as the former is a subset.
Changes since v1 [posted 20201016]:
- Bodo Stroesser pointed out a problem with the nesting of kmap_atomic() [called via sg_miter_next()] and kunmap_atomic() calls [called via sg_miter_stop()] and proposed a solution that simplifies the previous code.
- the new implementation of the three functions has shorter periods when pre-emption is disabled (but has more them). This should make operations on large sgl_s more pre-emption "friendly" with a relatively small performance hit.
- sgl_memset return type changed from void to size_t and is the number of bytes actually (over)written. That number is needed anyway internally so may as well return it as it may be useful to the caller.
This patchset is against lk 5.17.0-rc1
*** BLURB HERE ***
Douglas Gilbert (4):
sgl_alloc_order: remove 4 GiB limit
scatterlist: add sgl_copy_sgl() function
scatterlist: add sgl_equal_sgl() function
scatterlist: add sgl_memset()
include/linux/scatterlist.h | 33 ++++- lib/scatterlist.c | 256 +++++++++++++++++++++++++++++++----- 2 files changed, 256 insertions(+), 33 deletions(-)
Signature · 1 line
2.25.1
PATCH v7 1/4 sgl_alloc_order: remove 4 GiB limit
This patch fixes a check done by sgl_alloc_order() before it starts any allocations. The comment in the original said: "Check for integer overflow" but the right hand side of the expression in the condition is resolved as u32 so it can not exceed UINT32_MAX (4 GiB) which means 'length' can not exceed that value.
This function may be used to replace vmalloc(unsigned long) for a large allocation (e.g. a ramdisk). vmalloc has no limit at 4 GiB so it seems unreasonable that sgl_alloc_order() whose length type is unsigned long long should be limited to 4 GB.
In early 2021 there was discussion between Jason Gunthorpe <[email protected]> and Bodo Stroesser <[email protected]> about the way to check for overflow caused by order (an exponent) being too large. Take the solution proposed by Bodo in post dated 20210118 to the linux-scsi and linux-block lists.
An earlier patch fixed a memory leak in sg_alloc_order() due to the misuse of sgl_free(). Take the opportunity to put a one line comment above sgl_free()'s declaration warning that it is not suitable when order > 0 .
--- include/linux/scatterlist.h | 1 + lib/scatterlist.c | 24 +++++++++++++----------- 2 files changed, 14 insertions(+), 11 deletions(-)
include/linux/scatterlist.h +1 -0
@@ -357,6 +357,7 @@ struct scatterlist *sgl_alloc(unsigned long long length, gfp_t gfp, unsigned int *nent_p);void sgl_free_n_order (struct scatterlist *sgl,int nents,int order);void sgl_free_order (struct scatterlist *sgl,int order); +/* Only use sgl_free() when order is 0 */ void sgl_free (struct scatterlist *sgl);#endif /* CONFIG_SGL_ALLOC */
lib/scatterlist.c +13 -11
@@ -585,13 +585,16 @@ EXPORT_SYMBOL(sg_alloc_table_from_pages_segment);#ifdef CONFIG_SGL_ALLOC /** -* sgl_alloc_order - allocate a scatterlist and its pages +* sgl_alloc_order - allocate a scatterlist with equally sized elements each +* of which has 2^@order continuous pages * @length: Length in bytes of the scatterlist. Must be at least one -* @order: Second argument for alloc_pages() +* @order: Second argument for alloc_pages(). Each sgl element size will +* be (PAGE_SIZE*2^@order) bytes. @order must not exceed 16. * @chainable: Whether or not to allocate an extra element in the scatterlist -* for scatterlist chaining purposes +* for scatterlist chaining purposes * @gfp: Memory allocation flags -* @nent_p: [out] Number of entries in the scatterlist that have pages +* @nent_p: [out] Number of entries in the scatterlist that have pages. +* Ignored if @nent_p is NULL. * * Returns: A pointer to an initialized scatterlist or %NULL upon failure. */
@@ -604,16 +607,15 @@ struct scatterlist *sgl_alloc_order(unsigned long long length,unsigned int nent, nalloc;u32 elem_len; - nent =round_up (length,PAGE_SIZE << order) >> (PAGE_SHIFT + order); -/* Check for integer overflow */ -if (length > (nent << (PAGE_SHIFT + order))) +if (length >> (PAGE_SHIFT + order) >=UINT_MAX )return NULL ; - nalloc = nent; + nent =DIV_ROUND_UP (length,PAGE_SIZE << order); +if (chainable) { -/* Check for integer overflow */ -if (nalloc +1 < nalloc) +if (check_add_overflow (nent,1U , &nalloc))return NULL ; - nalloc++; + }else { + nalloc = nent; } sgl =kmalloc_array (nalloc,sizeof (struct scatterlist ), gfp & ~GFP_DMA );
Signature · 1 line
2.25.1
Re: [PATCH v7 1/4] sgl_alloc_order: remove 4 GiB limit
Quoted 69 lines · Douglas Gilbert
This patch fixes a check done by
sgl_alloc_order()before it starts any allocations. The comment in the original said: "Check for integer overflow" but the right hand side of the expression in the condition is resolved as u32 so it can not exceedUINT32_MAX(4 GiB) which means 'length' can not exceed that value.This function may be used to replace vmalloc(unsigned long) for a large allocation (e.g. a ramdisk). vmalloc has no limit at 4 GiB so it seems unreasonable that
sgl_alloc_order()whose length type is unsigned long long should be limited to 4 GB.In early 2021 there was discussion between Jason Gunthorpe <[email protected]> and Bodo Stroesser <[email protected]> about the way to check for overflow caused by order (an exponent) being too large. Take the solution proposed by Bodo in post dated 20210118 to the linux-scsi and linux-block lists.
An earlier patch fixed a memory leak in
sg_alloc_order()due to the misuse ofsgl_free(). Take the opportunity to put a one line comment above sgl_free()'s declaration warning that it is not suitable when order > 0 .Cc Jason Gunthorpe <[email protected]>Reviewed-by Bodo Stroesser <[email protected]>Signed-off-by Douglas Gilbert <[email protected]>--- include/linux/scatterlist.h | 1 + lib/scatterlist.c | 24 +++++++++++++----------- 2 files changed, 14 insertions(+), 11 deletions(-)include/linux/scatterlist.h +1
-0@@ -357,6 +357,7 @@ struct scatterlist *sgl_alloc(unsigned long long length, gfp_t gfp, unsigned int *nent_p);void sgl_free_n_order (struct scatterlist *sgl,int nents,int order);void sgl_free_order (struct scatterlist *sgl,int order); +/* Only use sgl_free() when order is 0 */ void sgl_free (struct scatterlist *sgl);#endif /* CONFIG_SGL_ALLOC */ lib/scatterlist.c +7
-4@@ -585,13 +585,16 @@ EXPORT_SYMBOL(sg_alloc_table_from_pages_segment);#ifdef CONFIG_SGL_ALLOC /** -* sgl_alloc_order - allocate a scatterlist and its pages +* sgl_alloc_order - allocate a scatterlist with equally sized elements each +* of which has 2^@order continuous pages * @length: Length in bytes of the scatterlist. Must be at least one -* @order: Second argument for alloc_pages() +* @order: Second argument for alloc_pages(). Each sgl element size will +* be (PAGE_SIZE*2^@order) bytes. @order must not exceed 16. * @chainable: Whether or not to allocate an extra element in the scatterlist -* for scatterlist chaining purposes +* for scatterlist chaining purposes * @gfp: Memory allocation flags -* @nent_p: [out] Number of entries in the scatterlist that have pages +* @nent_p: [out] Number of entries in the scatterlist that have pages. +* Ignored if @nent_p is NULL. * * Returns: A pointer to an initialized scatterlist or %NULL upon failure. */ @@ -604,16 +607,15 @@ struct scatterlist *sgl_alloc_order(unsigned long long length,unsigned int nent, nalloc;
lib/scatterlist.c struct scatterlist *sgl_alloc_order(unsigned long long length, · line 613
Quoted 10 lines
lib/scatterlist.c +3
-4u32 elem_len; - nent =round_up (length,PAGE_SIZE << order) >> (PAGE_SHIFT + order); -/* Check for integer overflow */ -if (length > (nent << (PAGE_SHIFT + order))) +if (length >> (PAGE_SHIFT + order) >=UINT_MAX )return NULL ; - nalloc = nent; + nent =DIV_ROUND_UP (length,PAGE_SIZE << order); +
This would be clearer to make nent/etc an unsigned long long. Then check if nalloc is > SIZE_MAX before casting it to size_t for the allocation. Avoids the wonky if statement.
Kaspm
kernel test robot
build botRe: [PATCH v7 1/4] sgl_alloc_order: remove 4 GiB limit
In reply to Douglas GilbertThank you for the patch! Yet something to improve:
Hi Douglas,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.17-rc2 next-20220131]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Douglas-Gilbert/scatterlist-add-new-capabilities/20220201-115047 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 26291c54e111ff6ba87a164d85d4a4e134b7315c config: i386-randconfig-a003-20220131 (https://download.01.org/0day-ci/archive/20220201/202202012125.JGVcLupw-lkp(a)intel.com/config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/be1e80a043970c400c00709be739ab26f931331a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Douglas-Gilbert/scatterlist-add-new-capabilities/20220201-115047 git checkout be1e80a043970c400c00709be739ab26f931331a
# save the config file to linux build tree
mkdir build_dir
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
All errors (new ones prefixed by >>):
ld: lib/scatterlist.o: in function `sgl_alloc_order':
>> lib/scatterlist.c:612: undefined reference to `__udivdi3'
vim +612 lib/scatterlist.c
586
587 /**
588 * sgl_alloc_order - allocate a scatterlist with equally sized elements each
589 * of which has 2^@order continuous pages
590 * @length: Length in bytes of the scatterlist. Must be at least one
591 * @order: Second argument for alloc_pages(). Each sgl element size will
592 * be (PAGE_SIZE*2^@order) bytes. @order must not exceed 16.
593 * @chainable: Whether or not to allocate an extra element in the scatterlist
594 * for scatterlist chaining purposes
595 * @gfp: Memory allocation flags
596 * @nent_p: [out] Number of entries in the scatterlist that have pages.
597 * Ignored if @nent_p is NULL.
598 *
599 * Returns: A pointer to an initialized scatterlist or %NULL upon failure.
600 */
601 struct scatterlist *sgl_alloc_order(unsigned long long length,
602 unsigned int order, bool chainable,
603 gfp_t gfp, unsigned int *nent_p)
604 {
605 struct scatterlist *sgl, *sg;
606 struct page *page;
607 unsigned int nent, nalloc;
608 u32 elem_len;
609
610 if (length >> (PAGE_SHIFT + order) >= UINT_MAX)
611 return NULL;
> 612 nent = DIV_ROUND_UP(length, PAGE_SIZE << order);
613
614 if (chainable) {
615 if (check_add_overflow(nent, 1U, &nalloc))
616 return NULL;
617 } else {
618 nalloc = nent;
619 }
620 sgl = kmalloc_array(nalloc, sizeof(struct scatterlist),
621 gfp & ~GFP_DMA);
622 if (!sgl)
623 return NULL;
624
625 sg_init_table(sgl, nalloc);
626 sg = sgl;
627 while (length) {
628 elem_len = min_t(u64, length, PAGE_SIZE << order);
629 page = alloc_pages(gfp, order);
630 if (!page) {
631 sgl_free_order(sgl, order);
632 return NULL;
633 }
634
635 sg_set_page(sg, page, elem_len, 0);
636 length -= elem_len;
637 sg = sg_next(sg);
638 }
639 WARN_ONCE(length, "length = %lld\n", length);
640 if (nent_p)
641 *nent_p = nent;
642 return sgl;
643 }
644 EXPORT_SYMBOL(sgl_alloc_order);
645
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
PATCH v7 2/4 scatterlist: add sgl_copy_sgl() function
In reply to Douglas GilbertBoth the SCSI and NVMe subsystems receive user data from the block layer in scatterlist_s (aka scatter gather lists (sgl) which are often arrays). If drivers in those subsystems represent storage (e.g. a ramdisk) or cache "hot" user data then they may also choose to use scatterlist_s. Currently there are no sgl to sgl operations in the kernel. Start with a sgl to sgl copy. Stops when the first of the number of requested bytes to copy, or the source sgl, or the destination sgl is exhausted. So the destination sgl will _not_ grow.
--- include/linux/scatterlist.h | 4 ++ lib/scatterlist.c | 74 +++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+)
include/linux/scatterlist.h +4 -0
@@ -376,6 +376,10 @@ size_t sg_pcopy_to_buffer(struct scatterlist *sgl, unsigned int nents,size_t sg_zero_buffer (struct scatterlist *sgl,unsigned int nents,size_t buflen,off_t skip); +size_t sgl_copy_sgl (struct scatterlist *d_sgl,unsigned int d_nents,off_t d_skip, +struct scatterlist *s_sgl,unsigned int s_nents,off_t s_skip, +size_t n_bytes); +/* * Maximum number of entries that will be allocated in one piece, if * a list larger than this is required then chaining will be utilized.
lib/scatterlist.c +74 -0
77 lines not on this page · raw message
Signature · 1 line
2.25.1
PATCH v7 3/4 scatterlist: add sgl_equal_sgl() function
In reply to Douglas GilbertAfter enabling copies between scatter gather lists (sgl_s), another storage related operation is to compare two sgl_s for equality. This new function is designed to partially implement NVMe's Compare command and the SCSI VERIFY(BYTCHK=1) command. Like memcmp() this function begins scanning at the start (of each sgl) and returns false on the first miscompare and stops comparing.
The sgl_equal_sgl_idx() function additionally yields the index (i.e. byte position) of the first miscompare. The additional parameter, miscompare_idx, is a pointer. If it is non-NULL and a miscompare is detected (i.e. the function returns false) then the byte index of the first miscompare is written to *miscompare_idx. Knowing the location of the first miscompare is needed to implement properly the SCSI COMPARE AND WRITE command.
--- include/linux/scatterlist.h | 8 +++ lib/scatterlist.c | 110 ++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+)
include/linux/scatterlist.h +8 -0
@@ -380,6 +380,14 @@ size_t sgl_copy_sgl(struct scatterlist *d_sgl, unsigned int d_nents, off_t d_ski struct scatterlist *s_sgl, unsigned int s_nents, off_t s_skip, size_t n_bytes); +bool sgl_equal_sgl (struct scatterlist *x_sgl,unsigned int x_nents,off_t x_skip, +struct scatterlist *y_sgl,unsigned int y_nents,off_t y_skip, +size_t n_bytes); + +bool sgl_equal_sgl_idx (struct scatterlist *x_sgl,unsigned int x_nents,off_t x_skip, +struct scatterlist *y_sgl,unsigned int y_nents,off_t y_skip, +size_t n_bytes,size_t *miscompare_idx); +/* * Maximum number of entries that will be allocated in one piece, if * a list larger than this is required then chaining will be utilized.
lib/scatterlist.c +110 -0
113 lines not on this page · raw message
Signature · 1 line
2.25.1
PATCH v7 4/4 scatterlist: add sgl_memset()
In reply to Douglas GilbertThe existing sg_zero_buffer() function is a bit restrictive. For example protection information (PI) blocks are usually initialized to 0xff bytes. As its name suggests sgl_memset() is modelled on memset(). One difference is the type of the val argument which is u8 rather than int. Plus it returns the number of bytes (over)written.
Change implementation of sg_zero_buffer() to call this new function.
--- include/linux/scatterlist.h | 20 +++++++++- lib/scatterlist.c | 78 ++++++++++++++++++++----------------- 2 files changed, 61 insertions(+), 37 deletions(-)
include/linux/scatterlist.h +18 -2
@@ -373,8 +373,6 @@ size_t sg_pcopy_from_buffer(struct scatterlist *sgl, unsigned int nents, const void *buf, size_t buflen, off_t skip);size_t sg_pcopy_to_buffer (struct scatterlist *sgl,unsigned int nents,void *buf,size_t buflen,off_t skip); -size_t sg_zero_buffer (struct scatterlist *sgl,unsigned int nents, -size_t buflen,off_t skip); size_t sgl_copy_sgl(struct scatterlist *d_sgl, unsigned int d_nents, off_t d_skip, struct scatterlist *s_sgl, unsigned int s_nents, off_t s_skip,
@@ -388,6 +386,24 @@ bool sgl_equal_sgl_idx(struct scatterlist *x_sgl, unsigned int x_nents, off_t x_ struct scatterlist *y_sgl, unsigned int y_nents, off_t y_skip, size_t n_bytes, size_t *miscompare_idx); +size_t sgl_memset (struct scatterlist *sgl,unsigned int nents,off_t skip, +u8 val,size_t n_bytes); + +/** +* sg_zero_buffer - Zero-out a part of a SG list +* @sgl: The SG list +* @nents: Number of SG entries +* @buflen: The number of bytes to zero out +* @skip: Number of bytes to skip before zeroing +* +* Returns the number of bytes zeroed. +**/ +static inline size_t sg_zero_buffer (struct scatterlist *sgl,unsigned int nents, +size_t buflen,off_t skip) +{ +return sgl_memset (sgl, nents, skip,0 , buflen); +} +/* * Maximum number of entries that will be allocated in one piece, if * a list larger than this is required then chaining will be utilized.
lib/scatterlist.c +43 -35
87 lines not on this page · raw message
Signature · 1 line
2.25.1