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

On 10/2/2020 6:02 PM, Jason Gunthorpe wrote:

On Sun, Sep 27, 2020 at 09:46:44AM +0300, Leon Romanovsky wrote:

...

+struct scatterlist *__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,
  

• struct scatterlist *prv, unsigned int left_pages,
  

• gfp_t gfp_mask)
  

  {

• unsigned int chunks, cur_page, seg_len, i;

• unsigned int chunks, cur_page, seg_len, i, prv_len = 0;
• struct scatterlist *s = prv;
• unsigned int table_size;
• unsigned int tmp_nents; int ret;

• struct scatterlist *s;

  if (WARN_ON(!max_segment || offset_in_page(max_segment)))

• return -EINVAL;
  

• return ERR_PTR(-EINVAL);
  

• if (IS_ENABLED(CONFIG_ARCH_NO_SG_CHAIN) && prv)

• return ERR_PTR(-EOPNOTSUPP);
  

• tmp_nents = prv ? sgt->nents : 0;
• if (prv &&

•    page_to_pfn(sg_page(prv)) + (prv->length >> PAGE_SHIFT) ==
  

This calculation of the end doesn't consider sg->offset

Right, should be fixed.

...

•    page_to_pfn(pages[0]))
  

• prv_len = prv->length;
  
  

  /* compute number of contiguous chunks */ chunks = 1;

@@ -410,13 +461,17 @@ 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;
  

• 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);
  

• }

This is basically redundant right? Now that get_next_sg() can allocate SGs it can just build them one by one, no need to preallocate.

Actually all the changes the the allocation seem like overkill, just allocate a single new array directly in get_next_sg() whenever it needs.

No, only the last allocation could be less than maximum. (as written in the comment). I am preferring to stick with the current implementation and fix the offset.

Something like this:

@@ -365,6 +372,37 @@ int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask) } EXPORT_SYMBOL(sg_alloc_table);

+static struct scatterlist *get_next_sg(struct sg_table *table,

• struct scatterlist *cur, unsigned long needed_sges,
  

• gfp_t gfp_mask)
  

+{

• struct scatterlist *new_sg;
• unsigned int alloc_size;
• if (cur) {

• struct scatterlist *next_sg = sg_next(cur);
  

• /* Check if last entry should be keeped for chainning */
  

• if (!sg_is_last(next_sg) || needed_sges == 1)
  

• 	return next_sg;
  

• }
• alloc_size = min_t(unsigned long, needed_sges, SG_MAX_SINGLE_ALLOC);
• new_sg = sg_kmalloc(alloc_size, gfp_mask);
• if (!new_sg)

• return ERR_PTR(-ENOMEM);
  

• sg_init_table(new_sg, alloc_size);
• if (cur) {

• __sg_chain(cur, new_sg);
  

• table->orig_nents += alloc_size - 1;
  

• } else {

• table->sgl = new_sg;
  

• table->orig_nents = alloc_size;
  

• table->nents = 0;
  

• }
• return new_sg;

+}

• /**
  • __sg_alloc_table_from_pages - Allocate and initialize an sg table from

  • 	         an array of pages
    

@@ -374,29 +412,64 @@ EXPORT_SYMBOL(sg_alloc_table);

• @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)

• • @prv: Last populated sge in sgt
• • @left_pages: Left pages caller have to set after this call
  • @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.

• • Description:
• • If @prv is NULL, allocate and initialize an sg table from a list of pages,
• • else reuse the scatterlist passed in at @prv.
• • Contiguous ranges of the pages are squashed into a single scatterlist
• • entry 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.
  • Returns:

• • 0 on success, negative error on failure

• • Last SGE in sgt on success, PTR_ERR on otherwise.
• • The allocation in @sgt must be released by sg_free_table.
• • 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_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_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,
  

• struct scatterlist *prv, unsigned int left_pages,
  

• gfp_t gfp_mask)
  

  {

• unsigned int chunks, cur_page, seg_len, i;
• int ret;
• struct scatterlist *s;

• unsigned int chunks, cur_page, seg_len, i, prv_len = 0;

• unsigned int added_nents = 0;

• struct scatterlist *s = prv;

  if (WARN_ON(!max_segment || offset_in_page(max_segment)))

• return -EINVAL;
  

• return ERR_PTR(-EINVAL);
  

• if (IS_ENABLED(CONFIG_ARCH_NO_SG_CHAIN) && prv)

• return ERR_PTR(-EOPNOTSUPP);
  

• if (prv) {

• unsigned long paddr = (page_to_pfn(sg_page(prv)) * PAGE_SIZE +
  

• 		       prv->offset + prv->length) /
  

• 		      PAGE_SIZE;
  

• if (WARN_ON(offset))
  

• 	return ERR_PTR(-EINVAL);
  

• /* Merge contiguous pages into the last SG */
  

• prv_len = prv->length;
  

• while (n_pages && page_to_pfn(pages[0]) == paddr) {
  

• 	if (prv->length + PAGE_SIZE > max_segment)
  

• 		break;
  

• 	prv->length += PAGE_SIZE;
  

• 	paddr++;
  

• 	pages++;
  

• 	n_pages--;
  

• }
  

• if (!n_pages) {
  

• 	sg_mark_end(sg_next(prv));
  

• 	return prv;
  

• }
  

• }

  /* compute number of contiguous chunks */ chunks = 1;

@@ -410,13 +483,9 @@ 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;
  

• /* merging chunks and putting them into the scatterlist */ cur_page = 0;
• for_each_sg(sgt->sgl, s, sgt->orig_nents, i) {

• for (i = 0; i < chunks; i++) { unsigned int j, chunk_size;

  /* look for the end of the current chunk */

@@ -429,15 +498,28 @@ int __sg_alloc_table_from_pages(struct sg_table *sgt, struct page **pages, break; }

• /* Pass how many chunks might be 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)
  

• 		prv->length = prv_len;
  

• 	return s;
  

• }
  

  chunk_size = ((j - cur_page) << PAGE_SHIFT) - offset; sg_set_page(s, pages[cur_page], min_t(unsigned long, size, chunk_size), offset);

• added_nents++;
  

  size -= chunk_size; offset = 0; cur_page = j; }

• return 0;

• sgt->nents += added_nents;
• sg_mark_end(s);
• return s; } EXPORT_SYMBOL(__sg_alloc_table_from_pages);