Re: [PATCH 6/6] lib/scatterlist: Drop order argument from sgl_free_n_order
Hi, On 07/03/18 18:30, James Bottomley wrote: On Wed, 2018-03-07 at 12:47 +, Tvrtko Ursulin wrote: From: Tvrtko Ursulin <tvrtko.ursu...@intel.com> Firstly, I don't see any justifiable benefit to churning this API, so why bother? but secondly this: Primarily because I wanted to extend sgl_alloc_order slightly in order to be able to use it from i915. And then in the process noticed a couple of bugs in the implementation, type inconsistencies and unused exported symbols. That gave me a feeling API could actually use a bit of work. We can derive the order from sg->length and so do not need to pass it in explicitly. Is wrong. I can have a length 2 scatterlist that crosses a page boundary, but I can also have one within a single page, so the order cannot be deduced from the length. sgl_alloc_order never does this. However there is a different bug in my patch relating to the last entry which can have shorter length from the rest. So get_order on the last entry is incorrect - I have to store the deduced order and carry it over. In which case it may even make sense to refactor sgl_alloc_order a bit more to avoid wastage on the last entry with high order allocations. Regards, Tvrtko
Re: [PATCH 6/6] lib/scatterlist: Drop order argument from sgl_free_n_order
On 08/03/18 15:56, Bart Van Assche wrote: On Thu, 2018-03-08 at 07:59 +, Tvrtko Ursulin wrote: However there is a different bug in my patch relating to the last entry which can have shorter length from the rest. So get_order on the last entry is incorrect - I have to store the deduced order and carry it over. Will that work if there is only one entry in the list and if it is a short entry? Yeah, needs more work. I especially don't like that case (as in any other with a final short chunk) wasting memory. So it would need more refactoring to make it possible. It did work in my internal tree where sgl_alloc_order was extended to become sgl_alloc_order_min_max, and as such uses a smaller order for smaller chunks. This patch can be dropped for now but the earlier ones are still valid I think. On those one I think we have some opens on how to proceed so if you could reply there, where applicable, that would be great. Regards, Tvrtko
Re: [PATCH 6/6] lib/scatterlist: Drop order argument from sgl_free_n_order
On 07/03/18 16:23, Bart Van Assche wrote: On Wed, 2018-03-07 at 12:47 +, 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
[PATCH 6/6] lib/scatterlist: Drop order argument from sgl_free_n_order
From: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
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.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
Cc: Bart Van Assche <bart.vanass...@wdc.com>
Cc: Hannes Reinecke <h...@suse.com>
Cc: Johannes Thumshirn <jthumsh...@suse.de>
Cc: Jens Axboe <ax...@kernel.dk>
Cc: "Nicholas A. Bellinger" <n...@linux-iscsi.org>
Cc: linux-scsi@vger.kernel.org
Cc: target-de...@vger.kernel.org
---
drivers/target/target_core_transport.c | 2 +-
include/linux/scatterlist.h| 5 ++---
lib/scatterlist.c | 16 ++--
3 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/drivers/target/target_core_transport.c
b/drivers/target/target_core_transport.c
index 4558f2e1fe1b..91e8f4047492 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2303,7 +2303,7 @@ static void target_complete_ok_work(struct work_struct
*work)
void target_free_sgl(struct scatterlist *sgl, int nents)
{
- sgl_free_n_order(sgl, nents, 0);
+ sgl_free_n(sgl, nents);
}
EXPORT_SYMBOL(target_free_sgl);
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 3ffc5f3bf181..3779d1fdd5c4 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -280,8 +280,7 @@ int sg_alloc_table_from_pages(struct sg_table *sgt, struct
page **pages,
struct scatterlist *sgl_alloc_order(unsigned long length, unsigned int order,
bool chainable, gfp_t gfp,
unsigned int *nent_p);
-void sgl_free_n_order(struct scatterlist *sgl, unsigned int nents,
- unsigned int order);
+void sgl_free_n(struct scatterlist *sgl, unsigned int nents);
/**
* sgl_alloc - allocate a scatterlist and its pages
@@ -303,7 +302,7 @@ sgl_alloc(unsigned long length, gfp_t gfp, unsigned int
*nent_p)
*/
static inline void sgl_free(struct scatterlist *sgl)
{
- sgl_free_n_order(sgl, UINT_MAX, 0);
+ sgl_free_n(sgl, UINT_MAX);
}
#endif /* CONFIG_SGL_ALLOC */
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index c637849482d3..76111e91a038 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -493,7 +493,7 @@ struct scatterlist *sgl_alloc_order(unsigned long length,
unsigned int order,
{
unsigned int chunk_len = PAGE_SIZE << order;
struct scatterlist *sgl, *sg;
- unsigned int nent, i;
+ unsigned int nent;
nent = round_up(length, chunk_len) >> (PAGE_SHIFT + order);
@@ -517,12 +517,11 @@ struct scatterlist *sgl_alloc_order(unsigned long length,
unsigned int order,
sg_init_table(sgl, nent);
sg = sgl;
- i = 0;
while (length) {
struct page *page = alloc_pages(gfp, order);
if (!page) {
- sgl_free_n_order(sgl, i, order);
+ sgl_free(sgl);
return NULL;
}
@@ -530,7 +529,6 @@ struct scatterlist *sgl_alloc_order(unsigned long length,
unsigned int order,
sg_set_page(sg, page, chunk_len, 0);
length -= chunk_len;
sg = sg_next(sg);
- i++;
}
WARN_ONCE(length, "length = %ld\n", length);
return sgl;
@@ -538,10 +536,9 @@ struct scatterlist *sgl_alloc_order(unsigned long length,
unsigned int order,
EXPORT_SYMBOL(sgl_alloc_order);
/**
- * sgl_free_n_order - free a scatterlist and its pages
+ * sgl_free_n - free a scatterlist and its pages
* @sgl: Scatterlist with one or more elements
* @nents: Maximum number of elements to free
- * @order: Second argument for __free_pages()
*
* Notes:
* - If several scatterlists have been chained and each chain element is
@@ -550,8 +547,7 @@ EXPORT_SYMBOL(sgl_alloc_order);
* - All pages in a chained scatterlist can be freed at once by setting @nents
* to a high number.
*/
-void sgl_free_n_order(struct scatterlist *sgl, unsigned int nents,
- unsigned int order)
+void sgl_free_n(struct scatterlist *sgl, unsigned int nents)
{
struct scatterlist *sg;
struct page *page;
@@ -562,11 +558,11 @@ void sgl_free_n_order(struct scatterlist *sgl, unsigned
int nents,
break;
page = sg_page(sg);
if (page)
- __free_pages(page, order);
+ __free_pages(page, get_order(sg->length));
}
kfree(sgl);
}
-EXPORT_SYMBOL(sgl_free_n_order);
+EXPORT_SYMBOL(sgl_free_n);
#endif /* CONFIG_SGL_ALLOC */
--
2.14.1
