On Wed, 2018-03-07 at 17:23 +0000, Tvrtko Ursulin wrote: > Ok I guess my main questions are the ones from the cover letter - where > is this API going and why did it get in a bit of a funky state? Because > it doesn't look fully thought through and tested to me. Funky state? Not fully tested? Except for the error paths and upper length limits the sgl allocation and freeing functions have been tested thoroughly. > My motivation is that I would like to extend it to add > sgl_alloc_order_min_max, which takes min order and max order, and > allocates as large chunks as it can given those constraints. This is > something we have in i915 and could then drop our implementation and use > the library function. That sounds useful to me. > But I also wanted to refactor sgl_alloc_order to benefit from the > existing chained struct scatterlist allocator. But SGL API does not > embed into struct sg_table, neither it carries explicitly the number of > nents allocated, making it impossible to correctly free with existing > sg_free_table. It is on purpose that sgl_alloc_order() returns a struct scatterlist instead of any structure built on top of struct scatterlist. If you have a look at the sgl_alloc*() callers then you will see that nontrivial changes in these callers are required to make them use something else than a struct scatterlist pointer. But if you would like to rework those callers that's fine with me. I can help with reviewing the code I'm familiar with. > Also I am not sure if a single gfp argument to sgl_alloc_order is the > right thing to do. I have a feeling you either need to ignore it for > kmalloc_array, or pass in two gfp_t arguments to be used for metadata > and backing storage respectively. If there is a caller that needs this feel free to make this change. But please don't make this change before there is a caller that needs it. Thanks, Bart.
