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

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

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@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.ursulin@intel.com>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: "Nicholas A. Bellinger" <nab@linux-iscsi.org>
Cc: linux-scsi@vger.kernel.org
Cc: target-devel@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

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

[Bart Van Assche]

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.

Thanks,

Bart.

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

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

[Bart Van Assche]

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.

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

[James Bottomley]

On Wed, 2018-03-07 at 12:47 +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Firstly, I don't see any justifiable benefit to churning this API, so
why bother? but secondly this:

> 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.

James

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

[Tvrtko Ursulin]

Hi,

On 07/03/18 18:30, James Bottomley wrote:
> On Wed, 2018-03-07 at 12:47 +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@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

[Bart Van Assche]

On Thu, 2018-03-08 at 07:59 +0000, 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?

Bart.

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

[Tvrtko Ursulin]

On 08/03/18 15:56, Bart Van Assche wrote:
> On Thu, 2018-03-08 at 07:59 +0000, 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