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
