[PATCH rdma-next v1 0/4] scatterlist: add sg_alloc_table_append function

[PATCH rdma-next v1 0/4] scatterlist: add sg_alloc_table_append function

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Changelog:
v1:
 * Changed _sg_chain to be __sg_chain
 * Added dependency on ARCH_NO_SG_CHAIN
 * Removed struct sg_append
v0:
 * https://lore.kernel.org/lkml/20200903121853.1145976-1-leon@kernel.org

--------------------------------------------------------------------------
From Maor:

This series adds a new constructor for a scatter gather table. Like
sg_alloc_table_from_pages function, this function merges all contiguous
chunks of the pages a into single scatter gather entry.

In contrast to sg_alloc_table_from_pages, the new API allows chaining of
new pages to already initialized SG table.

This allows drivers to utilize the optimization of merging contiguous
pages without a need to pre allocate all the pages and hold them in
a very large temporary buffer prior to the call to SG table initialization.

The first two patches refactor the code of sg_alloc_table_from_pages
in order to have code sharing and add sg_alloc_next function to allow
dynamic allocation of more entries in the SG table.

The third patch introduces the new API.

The last patch changes the Infiniband driver to use the new API. It
removes duplicate functionality from the code and benefits the
optimization of allocating dynamic SG table from pages.

In huge pages system of 2MB page size, without this change, the SG table
would contain x512 SG entries.
E.g. for 100GB memory registration:

             Number of entries      Size
    Before        26214400          600.0MB
    After            51200            1.2MB

Thanks

Maor Gottlieb (4):
  lib/scatterlist: Refactor sg_alloc_table_from_pages
  lib/scatterlist: Add support in dynamically allocation of SG entries
  lib/scatterlist: Add support in dynamic allocation of SG table from
    pages
  RDMA/umem: Move to allocate SG table from pages

 drivers/infiniband/Kconfig     |   1 +
 drivers/infiniband/core/umem.c |  90 ++--------
 include/linux/scatterlist.h    |  35 ++--
 lib/scatterlist.c              | 292 +++++++++++++++++++++++++--------
 4 files changed, 255 insertions(+), 163 deletions(-)

--
2.26.2

[PATCH rdma-next v1 1/4] lib/scatterlist: Refactor sg_alloc_table_from_pages

[Leon Romanovsky]

From: Maor Gottlieb <maorg@nvidia.com>

Currently, sg_alloc_table_from_pages doesn't support dynamic chaining of
SG entries. Therefore it requires from user to allocate all the pages in
advance and hold them in a large buffer. Such a buffer consumes a lot of
temporary memory in HPC systems which do a very large memory registration.

The next patches introduce API for dynamically allocation from pages and
it requires us to do the following:
 * Extract the code to alloc_from_pages_common.
 * Change the build of the table to iterate on the chunks and not on the
   SGEs. It will allow dynamic allocation of more SGEs.

Since sg_alloc_table_from_pages allocate exactly the number of chunks,
therefore chunks are equal to the number of SG entries.

Signed-off-by: Maor Gottlieb <maorg@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 lib/scatterlist.c | 75 ++++++++++++++++++++++++++++-------------------
 1 file changed, 45 insertions(+), 30 deletions(-)

diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 5d63a8857f36..292e785d21ee 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -365,38 +365,18 @@ int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask)
 }
 EXPORT_SYMBOL(sg_alloc_table);

-/**
- * __sg_alloc_table_from_pages - Allocate and initialize an sg table from
- *			         an array of pages
- * @sgt:	 The sg table header to use
- * @pages:	 Pointer to an array of page pointers
- * @n_pages:	 Number of pages in the pages array
- * @offset:      Offset from start of the first page to the start of a buffer
- * @size:        Number of valid bytes in the buffer (after offset)
- * @max_segment: Maximum size of a scatterlist node in bytes (page aligned)
- * @gfp_mask:	 GFP allocation mask
- *
- *  Description:
- *    Allocate and initialize an sg table from a list of pages. Contiguous
- *    ranges of the pages are squashed into a single scatterlist node up to the
- *    maximum size specified in @max_segment. An user may provide an offset at a
- *    start and a size of valid data in a buffer specified by the page array.
- *    The returned sg table is released by sg_free_table.
- *
- * Returns:
- *   0 on success, negative error on failure
- */
-int __sg_alloc_table_from_pages(struct sg_table *sgt, struct page **pages,
-				unsigned int n_pages, unsigned int offset,
-				unsigned long size, unsigned int max_segment,
-				gfp_t gfp_mask)
+static struct scatterlist *
+alloc_from_pages_common(struct sg_table *sgt, struct page **pages,
+			unsigned int n_pages, unsigned int offset,
+			unsigned long size, unsigned int max_segment,
+			gfp_t gfp_mask)
 {
 	unsigned int chunks, cur_page, seg_len, i;
+	struct scatterlist *prv, *s = NULL;
 	int ret;
-	struct scatterlist *s;

 	if (WARN_ON(!max_segment || offset_in_page(max_segment)))
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);

 	/* compute number of contiguous chunks */
 	chunks = 1;
@@ -412,11 +392,12 @@ int __sg_alloc_table_from_pages(struct sg_table *sgt, struct page **pages,

 	ret = sg_alloc_table(sgt, chunks, gfp_mask);
 	if (unlikely(ret))
-		return ret;
+		return ERR_PTR(ret);

 	/* merging chunks and putting them into the scatterlist */
 	cur_page = 0;
-	for_each_sg(sgt->sgl, s, sgt->orig_nents, i) {
+	s = sgt->sgl;
+	for (i = 0; i < chunks; i++) {
 		unsigned int j, chunk_size;

 		/* look for the end of the current chunk */
@@ -435,9 +416,43 @@ int __sg_alloc_table_from_pages(struct sg_table *sgt, struct page **pages,
 		size -= chunk_size;
 		offset = 0;
 		cur_page = j;
+		prv = s;
+		s = sg_next(s);
 	}
+	return prv;
+}

-	return 0;
+/**
+ * __sg_alloc_table_from_pages - Allocate and initialize an sg table from
+ *			         an array of pages
+ * @sgt:	 The sg table header to use
+ * @pages:	 Pointer to an array of page pointers
+ * @n_pages:	 Number of pages in the pages array
+ * @offset:      Offset from start of the first page to the start of a buffer
+ * @size:        Number of valid bytes in the buffer (after offset)
+ * @max_segment: Maximum size of a scatterlist node in bytes (page aligned)
+ * @gfp_mask:	 GFP allocation mask
+ *
+ *  Description:
+ *    Allocate and initialize an sg table from a list of pages. Contiguous
+ *    ranges of the pages are squashed into a single scatterlist node up to the
+ *    maximum size specified in @max_segment. A user may provide an offset at a
+ *    start and a size of valid data in a buffer specified by the page array.
+ *    The returned sg table is released by sg_free_table.
+ *
+ * Returns:
+ *   0 on success, negative error on failure
+ */
+int __sg_alloc_table_from_pages(struct sg_table *sgt, struct page **pages,
+				unsigned int n_pages, unsigned int offset,
+				unsigned long size, unsigned int max_segment,
+				gfp_t gfp_mask)
+{
+	struct scatterlist *sg;
+
+	sg = alloc_from_pages_common(sgt, pages, n_pages, offset, size,
+				     max_segment, gfp_mask);
+	return PTR_ERR_OR_ZERO(sg);
 }
 EXPORT_SYMBOL(__sg_alloc_table_from_pages);

--
2.26.2

Re: [PATCH rdma-next v1 1/4] lib/scatterlist: Refactor sg_alloc_table_from_pages

[Christoph Hellwig]

On Thu, Sep 10, 2020 at 04:42:56PM +0300, Leon Romanovsky wrote:
> From: Maor Gottlieb <maorg@nvidia.com>
> 
> Currently, sg_alloc_table_from_pages doesn't support dynamic chaining of
> SG entries. Therefore it requires from user to allocate all the pages in
> advance and hold them in a large buffer. Such a buffer consumes a lot of
> temporary memory in HPC systems which do a very large memory registration.
> 
> The next patches introduce API for dynamically allocation from pages and
> it requires us to do the following:
>  * Extract the code to alloc_from_pages_common.
>  * Change the build of the table to iterate on the chunks and not on the
>    SGEs. It will allow dynamic allocation of more SGEs.
> 
> Since sg_alloc_table_from_pages allocate exactly the number of chunks,
> therefore chunks are equal to the number of SG entries.
> 
> Signed-off-by: Maor Gottlieb <maorg@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>

I really don't think this refactoring on its own adds any value,
it just makes reading the rest of the series harder.

(functionally it looks correct, though)

Re: [PATCH rdma-next v1 1/4] lib/scatterlist: Refactor sg_alloc_table_from_pages

[Leon Romanovsky]

On Tue, Sep 15, 2020 at 06:16:43PM +0200, Christoph Hellwig wrote:
> On Thu, Sep 10, 2020 at 04:42:56PM +0300, Leon Romanovsky wrote:
> > From: Maor Gottlieb <maorg@nvidia.com>
> >
> > Currently, sg_alloc_table_from_pages doesn't support dynamic chaining of
> > SG entries. Therefore it requires from user to allocate all the pages in
> > advance and hold them in a large buffer. Such a buffer consumes a lot of
> > temporary memory in HPC systems which do a very large memory registration.
> >
> > The next patches introduce API for dynamically allocation from pages and
> > it requires us to do the following:
> >  * Extract the code to alloc_from_pages_common.
> >  * Change the build of the table to iterate on the chunks and not on the
> >    SGEs. It will allow dynamic allocation of more SGEs.
> >
> > Since sg_alloc_table_from_pages allocate exactly the number of chunks,
> > therefore chunks are equal to the number of SG entries.
> >
> > Signed-off-by: Maor Gottlieb <maorg@nvidia.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>
> I really don't think this refactoring on its own adds any value,
> it just makes reading the rest of the series harder.

We expected this type of the comment, but decided to send "splitted"
version because it is much easier to squash patches instead of splitting
them later.

We will squash and resubmit.

>
> (functionally it looks correct, though)

Thanks

[PATCH rdma-next v1 2/4] lib/scatterlist: Add support in dynamically allocation of SG entries

[Leon Romanovsky]

From: Maor Gottlieb <maorg@nvidia.com>

In order to support dynamic allocation of SG table, this patch
introduces sg_alloc_next. This function should be called to add more
entries to the table. In order to share the code, we will do the
following:
 * Extract the allocation code from __sg_alloc_table to sg_alloc.
 * Add a function to chain SGE to the next page.

Signed-off-by: Maor Gottlieb <maorg@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 include/linux/scatterlist.h |  29 ++++++----
 lib/scatterlist.c           | 110 ++++++++++++++++++++++++------------
 2 files changed, 91 insertions(+), 48 deletions(-)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 45cf7b69d852..9d13004334aa 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -165,6 +165,22 @@ static inline void sg_set_buf(struct scatterlist *sg, const void *buf,
 #define for_each_sgtable_dma_sg(sgt, sg, i)	\
 	for_each_sg((sgt)->sgl, sg, (sgt)->nents, i)

+static inline void __sg_chain(struct scatterlist *chain_sg,
+			      struct scatterlist *sgl)
+{
+	/*
+	 * offset and length are unused for chain entry. Clear them.
+	 */
+	chain_sg->offset = 0;
+	chain_sg->length = 0;
+
+	/*
+	 * Set lowest bit to indicate a link pointer, and make sure to clear
+	 * the termination bit if it happens to be set.
+	 */
+	chain_sg->page_link = ((unsigned long) sgl | SG_CHAIN) & ~SG_END;
+}
+
 /**
  * sg_chain - Chain two sglists together
  * @prv:	First scatterlist
@@ -178,18 +194,7 @@ static inline void sg_set_buf(struct scatterlist *sg, const void *buf,
 static inline void sg_chain(struct scatterlist *prv, unsigned int prv_nents,
 			    struct scatterlist *sgl)
 {
-	/*
-	 * offset and length are unused for chain entry.  Clear them.
-	 */
-	prv[prv_nents - 1].offset = 0;
-	prv[prv_nents - 1].length = 0;
-
-	/*
-	 * Set lowest bit to indicate a link pointer, and make sure to clear
-	 * the termination bit if it happens to be set.
-	 */
-	prv[prv_nents - 1].page_link = ((unsigned long) sgl | SG_CHAIN)
-					& ~SG_END;
+	__sg_chain(&prv[prv_nents - 1], sgl);
 }

 /**
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 292e785d21ee..ade5c4a6fbf9 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -242,38 +242,15 @@ void sg_free_table(struct sg_table *table)
 }
 EXPORT_SYMBOL(sg_free_table);

-/**
- * __sg_alloc_table - Allocate and initialize an sg table with given allocator
- * @table:	The sg table header to use
- * @nents:	Number of entries in sg list
- * @max_ents:	The maximum number of entries the allocator returns per call
- * @nents_first_chunk: Number of entries int the (preallocated) first
- * 	scatterlist chunk, 0 means no such preallocated chunk provided by user
- * @gfp_mask:	GFP allocation mask
- * @alloc_fn:	Allocator to use
- *
- * Description:
- *   This function returns a @table @nents long. The allocator is
- *   defined to return scatterlist chunks of maximum size @max_ents.
- *   Thus if @nents is bigger than @max_ents, the scatterlists will be
- *   chained in units of @max_ents.
- *
- * Notes:
- *   If this function returns non-0 (eg failure), the caller must call
- *   __sg_free_table() to cleanup any leftover allocations.
- *
- **/
-int __sg_alloc_table(struct sg_table *table, unsigned int nents,
-		     unsigned int max_ents, struct scatterlist *first_chunk,
-		     unsigned int nents_first_chunk, gfp_t gfp_mask,
-		     sg_alloc_fn *alloc_fn)
+static int sg_alloc(struct sg_table *table, struct scatterlist *prv,
+		    unsigned int nents, unsigned int max_ents,
+		    struct scatterlist *first_chunk,
+		    unsigned int nents_first_chunk,
+		    gfp_t gfp_mask, sg_alloc_fn *alloc_fn)
 {
-	struct scatterlist *sg, *prv;
-	unsigned int left;
-	unsigned curr_max_ents = nents_first_chunk ?: max_ents;
-	unsigned prv_max_ents;
-
-	memset(table, 0, sizeof(*table));
+	unsigned int curr_max_ents = nents_first_chunk ?: max_ents;
+	unsigned int left, prv_max_ents = 0;
+	struct scatterlist *sg;

 	if (nents == 0)
 		return -EINVAL;
@@ -283,7 +260,6 @@ int __sg_alloc_table(struct sg_table *table, unsigned int nents,
 #endif

 	left = nents;
-	prv = NULL;
 	do {
 		unsigned int sg_size, alloc_size = left;

@@ -308,7 +284,7 @@ int __sg_alloc_table(struct sg_table *table, unsigned int nents,
 			 * linkage.  Without this, sg_kfree() may get
 			 * confused.
 			 */
-			if (prv)
+			if (prv_max_ents)
 				table->nents = ++table->orig_nents;

 			return -ENOMEM;
@@ -321,10 +297,17 @@ int __sg_alloc_table(struct sg_table *table, unsigned int nents,
 		 * If this is the first mapping, assign the sg table header.
 		 * If this is not the first mapping, chain previous part.
 		 */
-		if (prv)
-			sg_chain(prv, prv_max_ents, sg);
-		else
+		if (!prv)
 			table->sgl = sg;
+		else if (prv_max_ents)
+			sg_chain(prv, prv_max_ents, sg);
+		else {
+			__sg_chain(prv, sg);
+			/* We decrease one since the prvious last sge in used to
+			 * chainning.
+			 */
+			table->nents = table->orig_nents -= 1;
+		}

 		/*
 		 * If no more entries after this one, mark the end
@@ -339,6 +322,61 @@ int __sg_alloc_table(struct sg_table *table, unsigned int nents,

 	return 0;
 }
+
+/**
+ * sg_alloc_next - Allocate and initialize new entries in the sg table
+ * @table:	The sg table header to use
+ * @last:	The last scatter list entry in the table
+ * @nents:	Number of entries in sg list
+ * @max_ents:	The maximum number of entries the allocator returns per call
+ * @gfp_mask:	GFP allocation mask
+ * @alloc_fn:	Allocator to use
+ *
+ * Description:
+ *   This function extend @table with @nents long. The allocator is
+ *   defined to return scatterlist chunks of maximum size @max_ents.
+ *   Thus if @nents is bigger than @max_ents, the scatterlists will be
+ *   chained in units of @max_ents.
+ *
+ **/
+static int sg_alloc_next(struct sg_table *table, struct scatterlist *last,
+			 unsigned int nents, unsigned int max_ents,
+			 gfp_t gfp_mask)
+{
+	return sg_alloc(table, last, nents, max_ents, NULL, 0, gfp_mask,
+			sg_kmalloc);
+}
+
+/**
+ * __sg_alloc_table - Allocate and initialize an sg table with given allocator
+ * @table:	The sg table header to use
+ * @nents:	Number of entries in sg list
+ * @max_ents:	The maximum number of entries the allocator returns per call
+ * @nents_first_chunk: Number of entries int the (preallocated) first
+ * scatterlist chunk, 0 means no such preallocated chunk provided by user
+ * @gfp_mask:	GFP allocation mask
+ * @alloc_fn:	Allocator to use
+ *
+ * Description:
+ *   This function returns a @table @nents long. The allocator is
+ *   defined to return scatterlist chunks of maximum size @max_ents.
+ *   Thus if @nents is bigger than @max_ents, the scatterlists will be
+ *   chained in units of @max_ents.
+ *
+ * Notes:
+ *   If this function returns non-0 (eg failure), the caller must call
+ *   __sg_free_table() to cleanup any leftover allocations.
+ *
+ **/
+int __sg_alloc_table(struct sg_table *table, unsigned int nents,
+		     unsigned int max_ents, struct scatterlist *first_chunk,
+		     unsigned int nents_first_chunk, gfp_t gfp_mask,
+		     sg_alloc_fn *alloc_fn)
+{
+	memset(table, 0, sizeof(*table));
+	return sg_alloc(table, NULL, nents, max_ents, first_chunk,
+			nents_first_chunk, gfp_mask, alloc_fn);
+}
 EXPORT_SYMBOL(__sg_alloc_table);

 /**
--
2.26.2

Re: [PATCH rdma-next v1 2/4] lib/scatterlist: Add support in dynamically allocation of SG entries

[Christoph Hellwig]

> +			/* We decrease one since the prvious last sge in used to
> +			 * chainning.
> +			 */

The normal style would be:

			/*
			 * We decrease one since the prvious last sge in used to
			 * chain the chunks together.
			 */

(also fixing up what I think it should be saying while I'm at it)

> + *   Thus if @nents is bigger than @max_ents, the scatterlists will be
> + *   chained in units of @max_ents.
> + *
> + **/
> +static int sg_alloc_next(struct sg_table *table, struct scatterlist *last,
> +			 unsigned int nents, unsigned int max_ents,
> +			 gfp_t gfp_mask)
> +{
> +	return sg_alloc(table, last, nents, max_ents, NULL, 0, gfp_mask,
> +			sg_kmalloc);
> +}

This helper seems unused in this patch.  For bisection you probably
want to move it into the next patch with the user.

In fact I'm not even sure there is much of a point in splitting out
this patch either.

Re: [PATCH rdma-next v1 2/4] lib/scatterlist: Add support in dynamically allocation of SG entries

[Leon Romanovsky]

On Tue, Sep 15, 2020 at 06:19:26PM +0200, Christoph Hellwig wrote:
> > +			/* We decrease one since the prvious last sge in used to
> > +			 * chainning.
> > +			 */
>
> The normal style would be:

It is netdev style of formatting, will change.

>
> 			/*
> 			 * We decrease one since the prvious last sge in used to
> 			 * chain the chunks together.
> 			 */
>
> (also fixing up what I think it should be saying while I'm at it)
>
> > + *   Thus if @nents is bigger than @max_ents, the scatterlists will be
> > + *   chained in units of @max_ents.
> > + *
> > + **/
> > +static int sg_alloc_next(struct sg_table *table, struct scatterlist *last,
> > +			 unsigned int nents, unsigned int max_ents,
> > +			 gfp_t gfp_mask)
> > +{
> > +	return sg_alloc(table, last, nents, max_ents, NULL, 0, gfp_mask,
> > +			sg_kmalloc);
> > +}
>
> This helper seems unused in this patch.  For bisection you probably
> want to move it into the next patch with the user.
>
> In fact I'm not even sure there is much of a point in splitting out
> this patch either.

We will squash.

[PATCH rdma-next v1 3/4] lib/scatterlist: Add support in dynamic allocation of SG table from pages

[Leon Romanovsky]

From: Maor Gottlieb <maorg@nvidia.com>

Add an API that supports dynamic allocation of the SG table from pages,
such function should be used by drivers that can't supply all the pages
at one time.

This function returns the last populated sge in the table. Users should
pass it as an argument to the function from the second call and forward.
As for sg_alloc_table_from_pages, nents will be equal to the number of
populated SGEs (chunks).

With this new API, drivers can benefit the optimization of merging
contiguous pages without a need to allocate all pages in advance and
hold them in a large buffer.

E.g. with the Infiniband driver that allocates a single page for hold the
pages. For 1TB memory registration, the temporary buffer would consume only
4KB, instead of 2GB.

Signed-off-by: Maor Gottlieb <maorg@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 include/linux/scatterlist.h |   6 ++
 lib/scatterlist.c           | 131 +++++++++++++++++++++++++++++++-----
 2 files changed, 119 insertions(+), 18 deletions(-)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 9d13004334aa..9fcc5783dfbc 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -291,6 +291,12 @@ void sg_free_table(struct sg_table *);
 int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int,
 		     struct scatterlist *, unsigned int, gfp_t, sg_alloc_fn *);
 int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);
+#ifndef CONFIG_ARCH_NO_SG_CHAIN
+struct scatterlist *sg_alloc_table_append(
+	struct sg_table *sgt, struct page **pages, unsigned int n_pages,
+	unsigned int offset, unsigned long size, unsigned int max_segment,
+	gfp_t gfp_mask, struct scatterlist *prv, unsigned int left_pages);
+#endif
 int __sg_alloc_table_from_pages(struct sg_table *sgt, struct page **pages,
 				unsigned int n_pages, unsigned int offset,
 				unsigned long size, unsigned int max_segment,
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index ade5c4a6fbf9..8249a6764efe 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -403,19 +403,51 @@ int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask)
 }
 EXPORT_SYMBOL(sg_alloc_table);

-static struct scatterlist *
-alloc_from_pages_common(struct sg_table *sgt, struct page **pages,
-			unsigned int n_pages, unsigned int offset,
-			unsigned long size, unsigned int max_segment,
-			gfp_t gfp_mask)
+static struct scatterlist *get_next_sg(struct sg_table *table,
+				       struct scatterlist *prv,
+				       unsigned long left_npages,
+				       gfp_t gfp_mask)
 {
-	unsigned int chunks, cur_page, seg_len, i;
-	struct scatterlist *prv, *s = NULL;
+	struct scatterlist *next_sg;
+	int ret;
+
+	/* If table was just allocated */
+	if (!prv)
+		return table->sgl;
+
+	/* Check if last entry should be keeped for chainning */
+	next_sg = sg_next(prv);
+	if (!sg_is_last(next_sg) || left_npages == 1)
+		return next_sg;
+
+	ret = sg_alloc_next(table, next_sg,
+			    min_t(unsigned long, left_npages,
+				  SG_MAX_SINGLE_ALLOC),
+			    SG_MAX_SINGLE_ALLOC, gfp_mask);
+	if (ret)
+		return ERR_PTR(ret);
+	return sg_next(prv);
+}
+
+static struct scatterlist *alloc_from_pages_common(
+	struct sg_table *sgt, struct page **pages, unsigned int n_pages,
+	unsigned int offset, unsigned long size, unsigned int max_segment,
+	gfp_t gfp_mask, struct scatterlist *prv, unsigned int left_pages)
+{
+	unsigned int chunks, cur_page, seg_len, i, prv_len = 0;
+	unsigned int tmp_nents = sgt->nents;
+	struct scatterlist *s = prv;
+	unsigned int table_size;
 	int ret;

 	if (WARN_ON(!max_segment || offset_in_page(max_segment)))
 		return ERR_PTR(-EINVAL);

+	if (prv &&
+	    page_to_pfn(sg_page(prv)) + (prv->length >> PAGE_SHIFT) ==
+	    page_to_pfn(pages[0]))
+		prv_len = prv->length;
+
 	/* compute number of contiguous chunks */
 	chunks = 1;
 	seg_len = 0;
@@ -428,13 +460,16 @@ alloc_from_pages_common(struct sg_table *sgt, struct page **pages,
 		}
 	}

-	ret = sg_alloc_table(sgt, chunks, gfp_mask);
-	if (unlikely(ret))
-		return ERR_PTR(ret);
+	if (!prv) {
+		/* Only the last allocation could be less than the maximum */
+		table_size = left_pages ? SG_MAX_SINGLE_ALLOC : chunks;
+		ret = sg_alloc_table(sgt, table_size, gfp_mask);
+		if (unlikely(ret))
+			return ERR_PTR(ret);
+	}

 	/* merging chunks and putting them into the scatterlist */
 	cur_page = 0;
-	s = sgt->sgl;
 	for (i = 0; i < chunks; i++) {
 		unsigned int j, chunk_size;

@@ -444,22 +479,82 @@ alloc_from_pages_common(struct sg_table *sgt, struct page **pages,
 			seg_len += PAGE_SIZE;
 			if (seg_len >= max_segment ||
 			    page_to_pfn(pages[j]) !=
-			    page_to_pfn(pages[j - 1]) + 1)
+				    page_to_pfn(pages[j - 1]) + 1)
 				break;
 		}

 		chunk_size = ((j - cur_page) << PAGE_SHIFT) - offset;
-		sg_set_page(s, pages[cur_page],
-			    min_t(unsigned long, size, chunk_size), offset);
+		chunk_size = min_t(unsigned long, size, chunk_size);
+		if (!i && prv_len) {
+			if (max_segment - prv->length >= chunk_size) {
+				sg_set_page(s, sg_page(s),
+					    s->length + chunk_size, s->offset);
+				goto next;
+			}
+		}
+
+		/* Pass how many chunks might left */
+		s = get_next_sg(sgt, s, chunks - i + left_pages, gfp_mask);
+		if (IS_ERR(s)) {
+			/* Adjust entry length to be as before function was
+			 * called
+			 */
+			if (prv_len)
+				prv->length = prv_len;
+			goto out;
+		}
+		sg_set_page(s, pages[cur_page], chunk_size, offset);
+		tmp_nents++;
+next:
 		size -= chunk_size;
 		offset = 0;
 		cur_page = j;
-		prv = s;
-		s = sg_next(s);
 	}
-	return prv;
+	sgt->nents = tmp_nents;
+out:
+	return s;
 }

+#ifndef CONFIG_ARCH_NO_SG_CHAIN
+/**
+ * sg_alloc_table_append - Allocate and initialize an sg table from
+ *                         an array of pages
+ * @sgt:	 The sg table header to use
+ * @pages:	 Pointer to an array of page pointers
+ * @n_pages:	 Number of pages in the pages array
+ * @offset:      Offset from start of the first page to the start of a buffer
+ * @size:        Number of valid bytes in the buffer (after offset)
+ * @max_segment: Maximum size of a scatterlist node in bytes (page aligned)
+ * @gfp_mask:	 GFP allocation mask
+ * @prv:	 Last populated sge in sgt
+ * @left_pages:  Left pages caller have to set after this call
+ *
+ *  Description:
+ *    If @prv is NULL, it allocates and initialize an sg table from a list of
+ *    pages. Contiguous ranges of the pages are squashed into a single
+ *    scatterlist node up to the maximum size specified in @max_segment. A user
+ *    may provide an offset at a start and a size of valid data in a buffer
+ *    specified by the page array. A user may provide @append to chain pages
+ *    to last entry in sgt. The returned sg table is released by sg_free_table.
+ *
+ * Returns:
+ *   Last SGE in sgt on success, negative error on failure.
+ *
+ * Notes:
+ *   If this function returns non-0 (eg failure), the caller must call
+ *   sg_free_table() to cleanup any leftover allocations.
+ */
+struct scatterlist *sg_alloc_table_append(
+	struct sg_table *sgt, struct page **pages, unsigned int n_pages,
+	unsigned int offset, unsigned long size, unsigned int max_segment,
+	gfp_t gfp_mask, struct scatterlist *prv, unsigned int left_pages)
+{
+	return alloc_from_pages_common(sgt, pages, n_pages, offset, size,
+				       max_segment, gfp_mask, prv, left_pages);
+}
+EXPORT_SYMBOL_GPL(sg_alloc_table_append);
+#endif
+
 /**
  * __sg_alloc_table_from_pages - Allocate and initialize an sg table from
  *			         an array of pages
@@ -489,7 +584,7 @@ int __sg_alloc_table_from_pages(struct sg_table *sgt, struct page **pages,
 	struct scatterlist *sg;

 	sg = alloc_from_pages_common(sgt, pages, n_pages, offset, size,
-				     max_segment, gfp_mask);
+				     max_segment, gfp_mask, NULL, 0);
 	return PTR_ERR_OR_ZERO(sg);
 }
 EXPORT_SYMBOL(__sg_alloc_table_from_pages);
--
2.26.2

Re: [PATCH rdma-next v1 3/4] lib/scatterlist: Add support in dynamic allocation of SG table from pages

[Christoph Hellwig]

> +#ifndef CONFIG_ARCH_NO_SG_CHAIN
> +struct scatterlist *sg_alloc_table_append(
> +	struct sg_table *sgt, struct page **pages, unsigned int n_pages,
> +	unsigned int offset, unsigned long size, unsigned int max_segment,
> +	gfp_t gfp_mask, struct scatterlist *prv, unsigned int left_pages);
> +#endif

Odd indentation here, we either do two tabs (my preference) or aligned
to the opening brace (what you seem to be doing elsewhere in the series).

> +	/* Check if last entry should be keeped for chainning */
> +	next_sg = sg_next(prv);
> +	if (!sg_is_last(next_sg) || left_npages == 1)
> +		return next_sg;
> +
> +	ret = sg_alloc_next(table, next_sg,
> +			    min_t(unsigned long, left_npages,
> +				  SG_MAX_SINGLE_ALLOC),
> +			    SG_MAX_SINGLE_ALLOC, gfp_mask);

Do we even need the sg_alloc_next helper added in the last patch,
given that this fairly simple function is the only caller?

> +static struct scatterlist *alloc_from_pages_common(
> +	struct sg_table *sgt, struct page **pages, unsigned int n_pages,
> +	unsigned int offset, unsigned long size, unsigned int max_segment,
> +	gfp_t gfp_mask, struct scatterlist *prv, unsigned int left_pages)

Same strange one tab indent as above.

> +#ifndef CONFIG_ARCH_NO_SG_CHAIN
> +/**
> + * sg_alloc_table_append - Allocate and initialize an sg table from
> + *                         an array of pages
> + * @sgt:	 The sg table header to use
> + * @pages:	 Pointer to an array of page pointers
> + * @n_pages:	 Number of pages in the pages array
> + * @offset:      Offset from start of the first page to the start of a buffer
> + * @size:        Number of valid bytes in the buffer (after offset)
> + * @max_segment: Maximum size of a scatterlist node in bytes (page aligned)
> + * @gfp_mask:	 GFP allocation mask
> + * @prv:	 Last populated sge in sgt
> + * @left_pages:  Left pages caller have to set after this call
> + *
> + *  Description:
> + *    If @prv is NULL, it allocates and initialize an sg table from a list of
> + *    pages. Contiguous ranges of the pages are squashed into a single
> + *    scatterlist node up to the maximum size specified in @max_segment. A user
> + *    may provide an offset at a start and a size of valid data in a buffer
> + *    specified by the page array. A user may provide @append to chain pages
> + *    to last entry in sgt. The returned sg table is released by sg_free_table.
> + *
> + * Returns:
> + *   Last SGE in sgt on success, negative error on failure.
> + *
> + * Notes:
> + *   If this function returns non-0 (eg failure), the caller must call
> + *   sg_free_table() to cleanup any leftover allocations.
> + */
> +struct scatterlist *sg_alloc_table_append(
> +	struct sg_table *sgt, struct page **pages, unsigned int n_pages,
> +	unsigned int offset, unsigned long size, unsigned int max_segment,
> +	gfp_t gfp_mask, struct scatterlist *prv, unsigned int left_pages)

One-tab indent again.

> +{
> +	return alloc_from_pages_common(sgt, pages, n_pages, offset, size,
> +				       max_segment, gfp_mask, prv, left_pages);
> +}
> +EXPORT_SYMBOL_GPL(sg_alloc_table_append);
> +#endif

So there reason I suggested to not provide sg_alloc_table_append
if CONFIG_ARCH_NO_SG_CHAIN was set was to avoid the extra
alloc_from_pages_common helper.  It might be better to move to your
run-time check and just make it conditіtional on a non-NULL prv pointer,
which would allow us to merge alloc_from_pages_common into
sg_alloc_table_append.  Sorry for leading you down this path.

Re: [PATCH rdma-next v1 3/4] lib/scatterlist: Add support in dynamic allocation of SG table from pages

[Leon Romanovsky]

On Tue, Sep 15, 2020 at 06:23:39PM +0200, Christoph Hellwig wrote:
> > +#ifndef CONFIG_ARCH_NO_SG_CHAIN
> > +struct scatterlist *sg_alloc_table_append(
> > +	struct sg_table *sgt, struct page **pages, unsigned int n_pages,
> > +	unsigned int offset, unsigned long size, unsigned int max_segment,
> > +	gfp_t gfp_mask, struct scatterlist *prv, unsigned int left_pages);
> > +#endif
>
> Odd indentation here, we either do two tabs (my preference) or aligned
> to the opening brace (what you seem to be doing elsewhere in the series).

All indentation came from clang-formatter, we will change.

Thanks

[PATCH rdma-next v1 4/4] RDMA/umem: Move to allocate SG table from pages

[Leon Romanovsky]

From: Maor Gottlieb <maorg@nvidia.com>

Remove the implementation of ib_umem_add_sg_table and instead
call to sg_alloc_table_append which already has the logic to
merge contiguous pages.

Besides that it removes duplicated functionality, it reduces the
memory consumption of the SG table significantly. Prior to this
patch, the SG table was allocated in advance regardless consideration
of contiguous pages.

In huge pages system of 2MB page size, without this change, the SG table
would contain x512 SG entries.
E.g. for 100GB memory registration:

	 Number of entries	Size
Before 	      26214400          600.0MB
After            51200		  1.2MB

Signed-off-by: Maor Gottlieb <maorg@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/Kconfig     |  1 +
 drivers/infiniband/core/umem.c | 90 +++++-----------------------------
 2 files changed, 12 insertions(+), 79 deletions(-)

diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
index 91b023341b77..2f299c0be2e5 100644
--- a/drivers/infiniband/Kconfig
+++ b/drivers/infiniband/Kconfig
@@ -6,6 +6,7 @@ menuconfig INFINIBAND
 	depends on INET
 	depends on m || IPV6 != m
 	depends on !ALPHA
+	depends on !ARCH_NO_SG_CHAIN
 	select IRQ_POLL
 	select DIMLIB
 	help
diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
index 01b680b62846..690c303bcc8c 100644
--- a/drivers/infiniband/core/umem.c
+++ b/drivers/infiniband/core/umem.c
@@ -63,73 +63,6 @@ static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int d
 	sg_free_table(&umem->sg_head);
 }

-/* ib_umem_add_sg_table - Add N contiguous pages to scatter table
- *
- * sg: current scatterlist entry
- * page_list: array of npage struct page pointers
- * npages: number of pages in page_list
- * max_seg_sz: maximum segment size in bytes
- * nents: [out] number of entries in the scatterlist
- *
- * Return new end of scatterlist
- */
-static struct scatterlist *ib_umem_add_sg_table(struct scatterlist *sg,
-						struct page **page_list,
-						unsigned long npages,
-						unsigned int max_seg_sz,
-						int *nents)
-{
-	unsigned long first_pfn;
-	unsigned long i = 0;
-	bool update_cur_sg = false;
-	bool first = !sg_page(sg);
-
-	/* Check if new page_list is contiguous with end of previous page_list.
-	 * sg->length here is a multiple of PAGE_SIZE and sg->offset is 0.
-	 */
-	if (!first && (page_to_pfn(sg_page(sg)) + (sg->length >> PAGE_SHIFT) ==
-		       page_to_pfn(page_list[0])))
-		update_cur_sg = true;
-
-	while (i != npages) {
-		unsigned long len;
-		struct page *first_page = page_list[i];
-
-		first_pfn = page_to_pfn(first_page);
-
-		/* Compute the number of contiguous pages we have starting
-		 * at i
-		 */
-		for (len = 0; i != npages &&
-			      first_pfn + len == page_to_pfn(page_list[i]) &&
-			      len < (max_seg_sz >> PAGE_SHIFT);
-		     len++)
-			i++;
-
-		/* Squash N contiguous pages from page_list into current sge */
-		if (update_cur_sg) {
-			if ((max_seg_sz - sg->length) >= (len << PAGE_SHIFT)) {
-				sg_set_page(sg, sg_page(sg),
-					    sg->length + (len << PAGE_SHIFT),
-					    0);
-				update_cur_sg = false;
-				continue;
-			}
-			update_cur_sg = false;
-		}
-
-		/* Squash N contiguous pages into next sge or first sge */
-		if (!first)
-			sg = sg_next(sg);
-
-		(*nents)++;
-		sg_set_page(sg, first_page, len << PAGE_SHIFT, 0);
-		first = false;
-	}
-
-	return sg;
-}
-
 /**
  * ib_umem_find_best_pgsz - Find best HW page size to use for this MR
  *
@@ -221,7 +154,7 @@ static struct ib_umem *__ib_umem_get(struct ib_device *device,
 	struct mm_struct *mm;
 	unsigned long npages;
 	int ret;
-	struct scatterlist *sg;
+	struct scatterlist *sg = NULL;
 	unsigned int gup_flags = FOLL_WRITE;

 	/*
@@ -276,15 +209,9 @@ static struct ib_umem *__ib_umem_get(struct ib_device *device,

 	cur_base = addr & PAGE_MASK;

-	ret = sg_alloc_table(&umem->sg_head, npages, GFP_KERNEL);
-	if (ret)
-		goto vma;
-
 	if (!umem->writable)
 		gup_flags |= FOLL_FORCE;

-	sg = umem->sg_head.sgl;
-
 	while (npages) {
 		cond_resched();
 		ret = pin_user_pages_fast(cur_base,
@@ -297,10 +224,16 @@ static struct ib_umem *__ib_umem_get(struct ib_device *device,

 		cur_base += ret * PAGE_SIZE;
 		npages   -= ret;
-
-		sg = ib_umem_add_sg_table(sg, page_list, ret,
-			dma_get_max_seg_size(device->dma_device),
-			&umem->sg_nents);
+		sg = sg_alloc_table_append(&umem->sg_head, page_list, ret, 0,
+					   ret << PAGE_SHIFT,
+					   dma_get_max_seg_size(device->dma_device),
+					   GFP_KERNEL, sg, npages);
+		umem->sg_nents = umem->sg_head.nents;
+		if (IS_ERR(sg)) {
+			unpin_user_pages_dirty_lock(page_list, ret, 0);
+			ret = PTR_ERR(sg);
+			goto umem_release;
+		}
 	}

 	sg_mark_end(sg);
@@ -322,7 +255,6 @@ static struct ib_umem *__ib_umem_get(struct ib_device *device,

 umem_release:
 	__ib_umem_release(device, umem, 0);
-vma:
 	atomic64_sub(ib_umem_num_pages(umem), &mm->pinned_vm);
 out:
 	free_page((unsigned long) page_list);
--
2.26.2