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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux