Messages in this thread | | | Subject | Re: [PATCH rdma-next v4 1/4] lib/scatterlist: Add support in dynamic allocation of SG table from pages | From | Maor Gottlieb <> | Date | Fri, 2 Oct 2020 19:11:33 +0300 |
| |
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); >
| |