Re: [PATCH 6/6] lib/scatterlist: Drop order argument from sgl_free_n_order

[Tvrtko Ursulin]

On 07/03/18 16:23, Bart Van Assche wrote:
> On Wed, 2018-03-07 at 12:47 +0000, Tvrtko Ursulin wrote:
> > We can derive the order from sg->length and so do not need to pass it in
> > explicitly. Rename the function to sgl_free_n.
>
> Using get_order() to compute the order looks fine to me but this patch will
> have to rebased in order to address the comments on the previous patches.

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.

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.

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.

Another benefit of using the existing sg allocator would be that for 
large allocation you don't depend on the availability of contiguous 
chunks like you do with kmalloc_array.

For instance if in another reply you mentioned 4GiB allocations are a 
possibility. If you use order 0 that means you need 1M nents, which can 
be something like 32 bytes each and you need a 32MiB kmalloc for the 
nents array and thats quite big. If you would be able to reuse the 
existing sg_alloc_table infrastructure (I have patches which extract it 
if you don't want to deal with struct sg_table), you would benefit from 
PAGE_SIZE allocations.

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.

So I have many questions regarding the current state and future 
direction, but essentially would like to make it usable for other 
drivers, like i915, as well.

Regards,

Tvrtko