diff mbox

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

Message ID 20180307124712.14963-7-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tvrtko Ursulin March 7, 2018, 12:47 p.m. UTC 
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(-)

Comments

Bart Van Assche March 7, 2018, 4:23 p.m. UTC | #1 
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.
Tvrtko Ursulin March 7, 2018, 5:23 p.m. UTC | #2 
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
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche March 7, 2018, 5:33 p.m. UTC | #3 
T24gV2VkLCAyMDE4LTAzLTA3IGF0IDE3OjIzICswMDAwLCBUdnJ0a28gVXJzdWxpbiB3cm90ZToN
Cj4gT2sgSSBndWVzcyBteSBtYWluIHF1ZXN0aW9ucyBhcmUgdGhlIG9uZXMgZnJvbSB0aGUgY292
ZXIgbGV0dGVyIC0gd2hlcmUgDQo+IGlzIHRoaXMgQVBJIGdvaW5nIGFuZCB3aHkgZGlkIGl0IGdl
dCBpbiBhIGJpdCBvZiBhIGZ1bmt5IHN0YXRlPyBCZWNhdXNlIA0KPiBpdCBkb2Vzbid0IGxvb2sg
ZnVsbHkgdGhvdWdodCB0aHJvdWdoIGFuZCB0ZXN0ZWQgdG8gbWUuDQoNCkZ1bmt5IHN0YXRlPyBO
b3QgZnVsbHkgdGVzdGVkPyBFeGNlcHQgZm9yIHRoZSBlcnJvciBwYXRocyBhbmQgdXBwZXIgbGVu
Z3RoDQpsaW1pdHMgdGhlIHNnbCBhbGxvY2F0aW9uIGFuZCBmcmVlaW5nIGZ1bmN0aW9ucyBoYXZl
IGJlZW4gdGVzdGVkIHRob3JvdWdobHkuDQoNCj4gTXkgbW90aXZhdGlvbiBpcyB0aGF0IEkgd291
bGQgbGlrZSB0byBleHRlbmQgaXQgdG8gYWRkIA0KPiBzZ2xfYWxsb2Nfb3JkZXJfbWluX21heCwg
d2hpY2ggdGFrZXMgbWluIG9yZGVyIGFuZCBtYXggb3JkZXIsIGFuZCANCj4gYWxsb2NhdGVzIGFz
IGxhcmdlIGNodW5rcyBhcyBpdCBjYW4gZ2l2ZW4gdGhvc2UgY29uc3RyYWludHMuIFRoaXMgaXMg
DQo+IHNvbWV0aGluZyB3ZSBoYXZlIGluIGk5MTUgYW5kIGNvdWxkIHRoZW4gZHJvcCBvdXIgaW1w
bGVtZW50YXRpb24gYW5kIHVzZSANCj4gdGhlIGxpYnJhcnkgZnVuY3Rpb24uDQoNClRoYXQgc291
bmRzIHVzZWZ1bCB0byBtZS4NCg0KPiBCdXQgSSBhbHNvIHdhbnRlZCB0byByZWZhY3RvciBzZ2xf
YWxsb2Nfb3JkZXIgdG8gYmVuZWZpdCBmcm9tIHRoZSANCj4gZXhpc3RpbmcgY2hhaW5lZCBzdHJ1
Y3Qgc2NhdHRlcmxpc3QgYWxsb2NhdG9yLiBCdXQgU0dMIEFQSSBkb2VzIG5vdCANCj4gZW1iZWQg
aW50byBzdHJ1Y3Qgc2dfdGFibGUsIG5laXRoZXIgaXQgY2FycmllcyBleHBsaWNpdGx5IHRoZSBu
dW1iZXIgb2YgDQo+IG5lbnRzIGFsbG9jYXRlZCwgbWFraW5nIGl0IGltcG9zc2libGUgdG8gY29y
cmVjdGx5IGZyZWUgd2l0aCBleGlzdGluZyANCj4gc2dfZnJlZV90YWJsZS4NCg0KSXQgaXMgb24g
cHVycG9zZSB0aGF0IHNnbF9hbGxvY19vcmRlcigpIHJldHVybnMgYSBzdHJ1Y3Qgc2NhdHRlcmxp
c3QNCmluc3RlYWQgb2YgYW55IHN0cnVjdHVyZSBidWlsdCBvbiB0b3Agb2Ygc3RydWN0IHNjYXR0
ZXJsaXN0LiBJZiB5b3UgaGF2ZQ0KYSBsb29rIGF0IHRoZSBzZ2xfYWxsb2MqKCkgY2FsbGVycyB0
aGVuIHlvdSB3aWxsIHNlZSB0aGF0IG5vbnRyaXZpYWwNCmNoYW5nZXMgaW4gdGhlc2UgY2FsbGVy
cyBhcmUgcmVxdWlyZWQgdG8gbWFrZSB0aGVtIHVzZSBzb21ldGhpbmcgZWxzZSB0aGFuDQphIHN0
cnVjdCBzY2F0dGVybGlzdCBwb2ludGVyLiBCdXQgaWYgeW91IHdvdWxkIGxpa2UgdG8gcmV3b3Jr
IHRob3NlIGNhbGxlcnMNCnRoYXQncyBmaW5lIHdpdGggbWUuIEkgY2FuIGhlbHAgd2l0aCByZXZp
ZXdpbmcgdGhlIGNvZGUgSSdtIGZhbWlsaWFyIHdpdGguDQoNCj4gQWxzbyBJIGFtIG5vdCBzdXJl
IGlmIGEgc2luZ2xlIGdmcCBhcmd1bWVudCB0byBzZ2xfYWxsb2Nfb3JkZXIgaXMgdGhlIA0KPiBy
aWdodCB0aGluZyB0byBkby4gSSBoYXZlIGEgZmVlbGluZyB5b3UgZWl0aGVyIG5lZWQgdG8gaWdu
b3JlIGl0IGZvciANCj4ga21hbGxvY19hcnJheSwgb3IgcGFzcyBpbiB0d28gZ2ZwX3QgYXJndW1l
bnRzIHRvIGJlIHVzZWQgZm9yIG1ldGFkYXRhIA0KPiBhbmQgYmFja2luZyBzdG9yYWdlIHJlc3Bl
Y3RpdmVseS4NCg0KSWYgdGhlcmUgaXMgYSBjYWxsZXIgdGhhdCBuZWVkcyB0aGlzIGZlZWwgZnJl
ZSB0byBtYWtlIHRoaXMgY2hhbmdlLiBCdXQNCnBsZWFzZSBkb24ndCBtYWtlIHRoaXMgY2hhbmdl
IGJlZm9yZSB0aGVyZSBpcyBhIGNhbGxlciB0aGF0IG5lZWRzIGl0Lg0KDQpUaGFua3MsDQoNCkJh
cnQuDQoNCg0K
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Bottomley March 7, 2018, 6:30 p.m. UTC | #4 
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

--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tvrtko Ursulin March 8, 2018, 7:59 a.m. UTC | #5 
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
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche March 8, 2018, 3:56 p.m. UTC | #6 
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.
Tvrtko Ursulin March 8, 2018, 5:06 p.m. UTC | #7 
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
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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 */