Messages in this thread | | | Subject | Re: [PATCH rdma-next 3/4] lib/scatterlist: Add support in dynamic allocation of SG table from pages | From | Maor Gottlieb <> | Date | Mon, 7 Sep 2020 15:44:08 +0300 |
| |
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
| |