| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ba152cb1-db38-0d70-08a8-ba3c052b5b4e@nvidia.com>
Date: Fri, 2 Oct 2020 19:11:33 +0300
From: Maor Gottlieb <maorg@...dia.com>
To: Jason Gunthorpe <jgg@...dia.com>, Leon Romanovsky <leon@...nel.org>
CC: Doug Ledford <dledford@...hat.com>,
Maor Gottlieb <maorg@...lanox.com>,
Christoph Hellwig <hch@....de>,
Daniel Vetter <daniel@...ll.ch>,
David Airlie <airlied@...ux.ie>,
<dri-devel@...ts.freedesktop.org>,
<intel-gfx@...ts.freedesktop.org>,
Jani Nikula <jani.nikula@...ux.intel.com>,
Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
<linux-kernel@...r.kernel.org>, <linux-rdma@...r.kernel.org>,
Rodrigo Vivi <rodrigo.vivi@...el.com>,
Roland Scheidegger <sroland@...are.com>,
"Tvrtko Ursulin" <tvrtko.ursulin@...el.com>,
VMware Graphics <linux-graphics-maintainer@...are.com>
Subject: 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);
>
Powered by blists - more mailing lists