From: Maor Gottlieb <maorg@nvidia.com>
To: Christoph Hellwig <hch@lst.de>, Leon Romanovsky <leon@kernel.org>
Cc: Doug Ledford <dledford@redhat.com>,
Jason Gunthorpe <jgg@nvidia.com>, <linux-kernel@vger.kernel.org>,
<linux-rdma@vger.kernel.org>
Subject: Re: [PATCH rdma-next 3/4] lib/scatterlist: Add support in dynamic allocation of SG table from pages
Date: Mon, 7 Sep 2020 15:44:08 +0300 [thread overview]
Message-ID: <15552707-c9ae-b76b-f6ff-7fedd5b02aed@nvidia.com> (raw)
In-Reply-To: <20200907072921.GC19875@lst.de>
On 9/7/2020 10:29 AM, Christoph Hellwig wrote:
> On Thu, Sep 03, 2020 at 03:18:52PM +0300, Leon Romanovsky wrote:
>> +struct sg_append {
>> + struct scatterlist *prv; /* Previous entry to append */
>> + unsigned int left_pages; /* Left pages to add to table */
>> +};
> I don't really see the point in this structure. Either pass it as
> two separate arguments, or switch sg_alloc_table_append and the
> internal helper to pass all arguments as a struct.
I did it to avoid more than 8 arguments of this function, will change it
to be 9 if it's fine for you.
>
>> + * 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
> This adds a few pointles > 80 char lines.
Will fix.
>
>> +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 sg_append *append)
>> +{
>> +#ifdef CONFIG_ARCH_NO_SG_CHAIN
>> + if (append->left_pages)
>> + return ERR_PTR(-EOPNOTSUPP);
>> +#endif
> Which makes this API entirely useless for !CONFIG_ARCH_NO_SG_CHAIN,
> doesn't it? Wouldn't it make more sense to not provide it for that
> case and add an explicitl dependency in the callers?
Current implementation allow us to support small memory registration
which not require chaining. I am not aware which archs has the SG_CHAIN
support and I don't want to break it so I can't add it to as dependency
to the Kconfig. Another option is to do the logic in the caller, but it
isn't clean.
>
>> + return alloc_from_pages_common(sgt, pages, n_pages, offset, size,
>> + max_segment, gfp_mask, append);
> And if we somehow manage to sort that out we can merge
> sg_alloc_table_append and alloc_from_pages_common, reducing the amount
> of wrappers that just make it too hard to follow the code.
>
>> +EXPORT_SYMBOL(sg_alloc_table_append);
> EXPORT_SYMBOL_GPL, please.
Sure
next prev parent reply other threads:[~2020-09-07 12:50 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-03 12:18 [PATCH rdma-next 0/4] scatterlist: add sg_alloc_table_append function Leon Romanovsky
2020-09-03 12:18 ` [PATCH rdma-next 2/4] lib/scatterlist: Add support in dynamically allocation of SG entries Leon Romanovsky
2020-09-07 7:29 ` Christoph Hellwig
2020-09-07 12:34 ` Maor Gottlieb
2020-09-03 12:18 ` [PATCH rdma-next 3/4] lib/scatterlist: Add support in dynamic allocation of SG table from pages Leon Romanovsky
2020-09-07 7:29 ` Christoph Hellwig
2020-09-07 12:44 ` Maor Gottlieb [this message]
2020-09-08 15:54 ` Christoph Hellwig
2020-09-08 16:13 ` Jason Gunthorpe
2020-09-03 12:18 ` [PATCH rdma-next 4/4] RDMA/umem: Move to allocate " Leon Romanovsky
2020-09-07 7:29 ` Christoph Hellwig
2020-09-08 14:10 ` Jason Gunthorpe
2020-09-03 15:32 ` [PATCH rdma-next 0/4] scatterlist: add sg_alloc_table_append function Christoph Hellwig
2020-09-03 15:55 ` Leon Romanovsky
2020-09-03 15:54 ` [PATCH rdma-next 1/4] lib/scatterlist: Refactor sg_alloc_table_from_pages Leon Romanovsky
2020-09-07 7:29 ` Christoph Hellwig
2020-09-07 12:32 ` Maor Gottlieb
2020-09-08 15:52 ` Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=15552707-c9ae-b76b-f6ff-7fedd5b02aed@nvidia.com \
--to=maorg@nvidia.com \
--cc=dledford@redhat.com \
--cc=hch@lst.de \
--cc=jgg@nvidia.com \
--cc=leon@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).