[PATCH 0/6] lib/scatterlist: Small SGL tidy

[PATCH 0/6] lib/scatterlist: Small SGL tidy

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

I spotted a few small issues in the recent added SGL code so am sending some
patches to tidy this.

My motivation was looking at sgl_alloc_order to potentially use from the i915
driver, with a small addition to support fall-back to smaller order allocation
if so was requested.

But then I realized scatterlist table allocated with sgl_alloc_order is not
compatible with being put into the sg_table container due different allocator
being used.

If it had used the standard chained scatterlist allocation, it would benefit
from less memory pressure for small-order large length allocations (in other
words large nent - as it stands it can request really large kmalloc allocations
for those cases), but to convert it to use that looks to would require some
refactoring of the SGL API users. So I wasn't sure how feasible would that be.

There were some other unclear bits to me in the SGL API, like why so much API
some of which is unused, so I tried to trim that as well.

So don't know - comments are welcome.

Tvrtko Ursulin (6):
  lib/scatterlist: Tidy types and fix overflow checking in
    sgl_alloc_order
  lib/scatterlist: Skip requesting zeroed allocations in sgl_alloc_order
  lib/scatterlist: Do not leak pages when high-order allocation fails
  lib/scatterlist: Unexport some trivial wrappers
  lib/scatterlist: Drop unused sgl_free_order
  lib/scatterlist: Drop order argument from sgl_free_n_order

 drivers/target/target_core_transport.c |  2 +-
 include/linux/scatterlist.h            | 36 +++++++++++---
 lib/scatterlist.c                      | 91 +++++++++++-----------------------
 3 files changed, 57 insertions(+), 72 deletions(-)

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

-- 
2.14.1

[PATCH 1/6] lib/scatterlist: Tidy types and fix overflow checking in sgl_alloc_order

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

There are several issues in sgl_alloc_order and accompanying APIs:

1.

sgl_alloc_order explicitly takes a 64-bit length (unsigned long long) but
then rejects it in overflow checking if greater than 4GiB allocation was
requested. This is a consequence of using unsigned int for the right hand
side condition which then natuarally overflows when shifted left, earlier
than nent otherwise would.

Fix is to promote the right hand side of the conditional to unsigned long.

It is also not useful to allow for 64-bit lenght on 32-bit platforms so
I have changed this type to a natural unsigned long. Like this it changes
size naturally depending on the architecture.

2.

elem_len should not be explicitly sized u32 but unsigned int, to match
the underlying struct scatterlist nents type. Same for the nent_p output
parameter type.

I renamed this to chunk_len and consolidated its use throughout the
function.

3.

Eliminated nalloc local by re-arranging the code a bit.

4.

Tidied types in other helpers, replacing int with unsigned int when
passing in back the nents returned from sgl_alloc_order. Again, this is to
match the struct scatterlist underlying types.

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>
---
 include/linux/scatterlist.h | 13 ++++++-----
 lib/scatterlist.c           | 54 ++++++++++++++++++++++++---------------------
 2 files changed, 36 insertions(+), 31 deletions(-)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 22b2131bcdcd..2144de41ee04 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -277,13 +277,14 @@ int sg_alloc_table_from_pages(struct sg_table *sgt, struct page **pages,
 			      unsigned long size, gfp_t gfp_mask);
 
 #ifdef CONFIG_SGL_ALLOC
-struct scatterlist *sgl_alloc_order(unsigned long long length,
-				    unsigned int order, bool chainable,
-				    gfp_t gfp, unsigned int *nent_p);
-struct scatterlist *sgl_alloc(unsigned long long length, gfp_t gfp,
+struct scatterlist *sgl_alloc_order(unsigned long length, unsigned int order,
+				    bool chainable, gfp_t gfp,
+				    unsigned int *nent_p);
+struct scatterlist *sgl_alloc(unsigned long length, gfp_t gfp,
 			      unsigned int *nent_p);
-void sgl_free_n_order(struct scatterlist *sgl, int nents, int order);
-void sgl_free_order(struct scatterlist *sgl, int order);
+void sgl_free_n_order(struct scatterlist *sgl, unsigned int nents,
+		      unsigned int order);
+void sgl_free_order(struct scatterlist *sgl, unsigned int order);
 void sgl_free(struct scatterlist *sgl);
 #endif /* CONFIG_SGL_ALLOC */
 
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 53728d391d3a..d61c025e38b4 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -487,48 +487,51 @@ EXPORT_SYMBOL(sg_alloc_table_from_pages);
  *
  * Returns: A pointer to an initialized scatterlist or %NULL upon failure.
  */
-struct scatterlist *sgl_alloc_order(unsigned long long length,
-				    unsigned int order, bool chainable,
-				    gfp_t gfp, unsigned int *nent_p)
+struct scatterlist *sgl_alloc_order(unsigned long length, unsigned int order,
+				    bool chainable, gfp_t gfp,
+				    unsigned int *nent_p)
 {
+	unsigned int chunk_len = PAGE_SIZE << order;
 	struct scatterlist *sgl, *sg;
-	struct page *page;
-	unsigned int nent, nalloc;
-	u32 elem_len;
+	unsigned int nent;
+
+	nent = round_up(length, chunk_len) >> (PAGE_SHIFT + order);
 
-	nent = round_up(length, PAGE_SIZE << order) >> (PAGE_SHIFT + order);
 	/* Check for integer overflow */
-	if (length > (nent << (PAGE_SHIFT + order)))
+	if (length > ((unsigned long)nent << (PAGE_SHIFT + order)))
 		return NULL;
-	nalloc = nent;
+
+	if (nent_p)
+		*nent_p = nent;
+
 	if (chainable) {
 		/* Check for integer overflow */
-		if (nalloc + 1 < nalloc)
+		if (nent == UINT_MAX)
 			return NULL;
-		nalloc++;
+		nent++;
 	}
-	sgl = kmalloc_array(nalloc, sizeof(struct scatterlist),
+
+	sgl = kmalloc_array(nent, sizeof(struct scatterlist),
 			    (gfp & ~GFP_DMA) | __GFP_ZERO);
 	if (!sgl)
 		return NULL;
 
-	sg_init_table(sgl, nalloc);
+	sg_init_table(sgl, nent);
 	sg = sgl;
 	while (length) {
-		elem_len = min_t(u64, length, PAGE_SIZE << order);
-		page = alloc_pages(gfp, order);
+		struct page *page = alloc_pages(gfp, order);
+
 		if (!page) {
 			sgl_free(sgl);
 			return NULL;
 		}
 
-		sg_set_page(sg, page, elem_len, 0);
-		length -= elem_len;
+		chunk_len = min_t(unsigned long, length, chunk_len);
+		sg_set_page(sg, page, chunk_len, 0);
+		length -= chunk_len;
 		sg = sg_next(sg);
 	}
-	WARN_ONCE(length, "length = %lld\n", length);
-	if (nent_p)
-		*nent_p = nent;
+	WARN_ONCE(length, "length = %ld\n", length);
 	return sgl;
 }
 EXPORT_SYMBOL(sgl_alloc_order);
@@ -541,7 +544,7 @@ EXPORT_SYMBOL(sgl_alloc_order);
  *
  * Returns: A pointer to an initialized scatterlist or %NULL upon failure.
  */
-struct scatterlist *sgl_alloc(unsigned long long length, gfp_t gfp,
+struct scatterlist *sgl_alloc(unsigned long length, gfp_t gfp,
 			      unsigned int *nent_p)
 {
 	return sgl_alloc_order(length, 0, false, gfp, nent_p);
@@ -561,11 +564,12 @@ EXPORT_SYMBOL(sgl_alloc);
  * - 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, int nents, int order)
+void sgl_free_n_order(struct scatterlist *sgl, unsigned int nents,
+		      unsigned int order)
 {
 	struct scatterlist *sg;
 	struct page *page;
-	int i;
+	unsigned int i;
 
 	for_each_sg(sgl, sg, nents, i) {
 		if (!sg)
@@ -583,9 +587,9 @@ EXPORT_SYMBOL(sgl_free_n_order);
  * @sgl: Scatterlist with one or more elements
  * @order: Second argument for __free_pages()
  */
-void sgl_free_order(struct scatterlist *sgl, int order)
+void sgl_free_order(struct scatterlist *sgl, unsigned int order)
 {
-	sgl_free_n_order(sgl, INT_MAX, order);
+	sgl_free_n_order(sgl, UINT_MAX, order);
 }
 EXPORT_SYMBOL(sgl_free_order);
 
-- 
2.14.1

[PATCH 2/6] lib/scatterlist: Skip requesting zeroed allocations in sgl_alloc_order

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

sg_init_table will clear the allocated block so requesting zeroed memory
from the allocator is redundant.

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>
---
 lib/scatterlist.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index d61c025e38b4..9884be50a2c0 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -511,8 +511,7 @@ struct scatterlist *sgl_alloc_order(unsigned long length, unsigned int order,
 		nent++;
 	}
 
-	sgl = kmalloc_array(nent, sizeof(struct scatterlist),
-			    (gfp & ~GFP_DMA) | __GFP_ZERO);
+	sgl = kmalloc_array(nent, sizeof(struct scatterlist), (gfp & ~GFP_DMA));
 	if (!sgl)
 		return NULL;
 
-- 
2.14.1

[PATCH 3/6] lib/scatterlist: Do not leak pages when high-order allocation fails

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

If a higher-order allocation fails, the existing abort and cleanup path
would consider all segments allocated so far as 0-order page allocations
and would therefore leak memory.

Fix this by cleaning up using sgl_free_n_order which allows the correct
page order to be passed in.

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>
---
 lib/scatterlist.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 9884be50a2c0..e13a759c5c49 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;
+	unsigned int nent, i;
 
 	nent = round_up(length, chunk_len) >> (PAGE_SHIFT + order);
 
@@ -517,11 +517,12 @@ 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(sgl);
+			sgl_free_n_order(sgl, i, order);
 			return NULL;
 		}
 
@@ -529,6 +530,7 @@ 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;
-- 
2.14.1

[PATCH 4/6] lib/scatterlist: Unexport some trivial wrappers

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Save some kernel size by moving trivial wrappers to header as static
inline instead of exporting symbols for them.

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>
---
 include/linux/scatterlist.h | 38 ++++++++++++++++++++++++++++++++++----
 lib/scatterlist.c           | 36 ------------------------------------
 2 files changed, 34 insertions(+), 40 deletions(-)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 2144de41ee04..f665a278011a 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -280,12 +280,42 @@ 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);
-struct scatterlist *sgl_alloc(unsigned long length, gfp_t gfp,
-			      unsigned int *nent_p);
 void sgl_free_n_order(struct scatterlist *sgl, unsigned int nents,
 		      unsigned int order);
-void sgl_free_order(struct scatterlist *sgl, unsigned int order);
-void sgl_free(struct scatterlist *sgl);
+
+/**
+ * sgl_alloc - allocate a scatterlist and its pages
+ * @length: Length in bytes of the scatterlist
+ * @gfp: Memory allocation flags
+ * @nent_p: [out] Number of entries in the scatterlist
+ *
+ * Returns: A pointer to an initialized scatterlist or %NULL upon failure.
+ */
+static inline struct scatterlist *
+sgl_alloc(unsigned long length, gfp_t gfp, unsigned int *nent_p)
+{
+	return sgl_alloc_order(length, 0, false, gfp, nent_p);
+}
+
+/**
+ * sgl_free_order - free a scatterlist and its pages
+ * @sgl: Scatterlist with one or more elements
+ * @order: Second argument for __free_pages()
+ */
+static inline void sgl_free_order(struct scatterlist *sgl, unsigned int order)
+{
+	sgl_free_n_order(sgl, UINT_MAX, order);
+}
+
+/**
+ * sgl_free - free a scatterlist and its pages
+ * @sgl: Scatterlist with one or more elements
+ */
+static inline void sgl_free(struct scatterlist *sgl)
+{
+	sgl_free_order(sgl, 0);
+}
+
 #endif /* CONFIG_SGL_ALLOC */
 
 size_t sg_copy_buffer(struct scatterlist *sgl, unsigned int nents, void *buf,
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index e13a759c5c49..c637849482d3 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -537,21 +537,6 @@ struct scatterlist *sgl_alloc_order(unsigned long length, unsigned int order,
 }
 EXPORT_SYMBOL(sgl_alloc_order);
 
-/**
- * sgl_alloc - allocate a scatterlist and its pages
- * @length: Length in bytes of the scatterlist
- * @gfp: Memory allocation flags
- * @nent_p: [out] Number of entries in the scatterlist
- *
- * Returns: A pointer to an initialized scatterlist or %NULL upon failure.
- */
-struct scatterlist *sgl_alloc(unsigned long length, gfp_t gfp,
-			      unsigned int *nent_p)
-{
-	return sgl_alloc_order(length, 0, false, gfp, nent_p);
-}
-EXPORT_SYMBOL(sgl_alloc);
-
 /**
  * sgl_free_n_order - free a scatterlist and its pages
  * @sgl: Scatterlist with one or more elements
@@ -583,27 +568,6 @@ void sgl_free_n_order(struct scatterlist *sgl, unsigned int nents,
 }
 EXPORT_SYMBOL(sgl_free_n_order);
 
-/**
- * sgl_free_order - free a scatterlist and its pages
- * @sgl: Scatterlist with one or more elements
- * @order: Second argument for __free_pages()
- */
-void sgl_free_order(struct scatterlist *sgl, unsigned int order)
-{
-	sgl_free_n_order(sgl, UINT_MAX, order);
-}
-EXPORT_SYMBOL(sgl_free_order);
-
-/**
- * sgl_free - free a scatterlist and its pages
- * @sgl: Scatterlist with one or more elements
- */
-void sgl_free(struct scatterlist *sgl)
-{
-	sgl_free_order(sgl, 0);
-}
-EXPORT_SYMBOL(sgl_free);
-
 #endif /* CONFIG_SGL_ALLOC */
 
 void __sg_page_iter_start(struct sg_page_iter *piter,
-- 
2.14.1

[PATCH 5/6] lib/scatterlist: Drop unused sgl_free_order

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

No code calls it so remove it.

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>
---
 include/linux/scatterlist.h | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index f665a278011a..3ffc5f3bf181 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -297,23 +297,13 @@ sgl_alloc(unsigned long length, gfp_t gfp, unsigned int *nent_p)
 	return sgl_alloc_order(length, 0, false, gfp, nent_p);
 }
 
-/**
- * sgl_free_order - free a scatterlist and its pages
- * @sgl: Scatterlist with one or more elements
- * @order: Second argument for __free_pages()
- */
-static inline void sgl_free_order(struct scatterlist *sgl, unsigned int order)
-{
-	sgl_free_n_order(sgl, UINT_MAX, order);
-}
-
 /**
  * sgl_free - free a scatterlist and its pages
  * @sgl: Scatterlist with one or more elements
  */
 static inline void sgl_free(struct scatterlist *sgl)
 {
-	sgl_free_order(sgl, 0);
+	sgl_free_n_order(sgl, UINT_MAX, 0);
 }
 
 #endif /* CONFIG_SGL_ALLOC */
-- 
2.14.1

[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

[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 2/6] lib/scatterlist: Skip requesting zeroed allocations in sgl_alloc_order

[Andy Shevchenko]

On Wed, Mar 7, 2018 at 2:47 PM, Tvrtko Ursulin <tursulin@ursulin.net> wrote:

> +       sgl = kmalloc_array(nent, sizeof(struct scatterlist), (gfp & ~GFP_DMA));

The parens now become redundant.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] lib/scatterlist: Tidy types and fix overflow checking in sgl_alloc_order

[Bart Van Assche]

On Wed, 2018-03-07 at 12:47 +0000, Tvrtko Ursulin wrote:
> sgl_alloc_order explicitly takes a 64-bit length (unsigned long long) but
> then rejects it in overflow checking if greater than 4GiB allocation was
> requested. This is a consequence of using unsigned int for the right hand
> side condition which then natuarally overflows when shifted left, earlier
> than nent otherwise would.
> 
> Fix is to promote the right hand side of the conditional to unsigned long.

Agreed.

> It is also not useful to allow for 64-bit lenght on 32-bit platforms so
> I have changed this type to a natural unsigned long. Like this it changes
> size naturally depending on the architecture.

I do not agree. Although uncommon, it is possible that e.g. a SCSI initiator
sends a transfer of more than 4 GB to a target system and that that transfer
must not be split. Since this code is used by the SCSI target, I think that's
an example of an application where it is useful to allow allocations of more
than 4 GB at once on a 32-bit system.

> 2.
> 
> elem_len should not be explicitly sized u32 but unsigned int, to match
> the underlying struct scatterlist nents type. Same for the nent_p output
> parameter type.

Are you sure it is useful to support allocations with an order that exceeds
(31 - PAGE_SHIFT)? Since memory gets fragmented easily in the Linux kernel I
think that it's unlikely that such allocations will succeed.

> I renamed this to chunk_len and consolidated its use throughout the
> function.

Please undo this change such that the diff remains as short as possible.

> -void sgl_free_n_order(struct scatterlist *sgl, int nents, int order)
> +void sgl_free_n_order(struct scatterlist *sgl, unsigned int nents,
> +		      unsigned int order)
>  {
>  	struct scatterlist *sg;
>  	struct page *page;
> -	int i;
> +	unsigned int i;
>  
>  	for_each_sg(sgl, sg, nents, i) {
>  		if (!sg)
> @@ -583,9 +587,9 @@ EXPORT_SYMBOL(sgl_free_n_order);
>   * @sgl: Scatterlist with one or more elements
>   * @order: Second argument for __free_pages()
>   */
> -void sgl_free_order(struct scatterlist *sgl, int order)
> +void sgl_free_order(struct scatterlist *sgl, unsigned int order)
>  {
> -	sgl_free_n_order(sgl, INT_MAX, order);
> +	sgl_free_n_order(sgl, UINT_MAX, order);
>  }
>  EXPORT_SYMBOL(sgl_free_order);

Do you have an application that calls these functions to allocate more than
INT_MAX * PAGE_SIZE bytes at once? If not, please leave these changes out.

Thanks,

Bart.

Re: [PATCH 3/6] lib/scatterlist: Do not leak pages when high-order allocation fails

[Bart Van Assche]

On Wed, 2018-03-07 at 12:47 +0000, Tvrtko Ursulin wrote:
> diff --git a/lib/scatterlist.c b/lib/scatterlist.c
> index 9884be50a2c0..e13a759c5c49 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;
> +	unsigned int nent, i;
>  
>  	nent = round_up(length, chunk_len) >> (PAGE_SHIFT + order);
>  
> @@ -517,11 +517,12 @@ 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(sgl);
> +			sgl_free_n_order(sgl, i, order);
>  			return NULL;
>  		}
>  
> @@ -529,6 +530,7 @@ 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++;
>  	}

Since the entire sg-list is zero-initialized before this loop starts, since
the sg-list is not chained onto another sg-list before this function returns
and since sgl_free_n_order() checks whether or not each page pointer is NULL
before freeing it I think we don't need the new loop variable 'i' and that
we can call sgl_free_order() instead of sgl_free_n_order().

Bart.

Re: [PATCH 4/6] lib/scatterlist: Unexport some trivial wrappers

[Bart Van Assche]

On Wed, 2018-03-07 at 12:47 +0000, Tvrtko Ursulin wrote:
> Save some kernel size by moving trivial wrappers to header as static
> inline instead of exporting symbols for them.

Something that you may be unaware of is that the introduction of the sgl
helper functions is only a first step. The next step will be to introduce
a caching allocator for sg-lists. So for small sg-lists inlining won't
help performance. But moving these definitions from a .c file into a .h
file will (slightly) slow down kernel compilation. So I'd prefer that you
drop this patch.

Thanks,

Bart.

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

[Bart Van Assche]

T24gV2VkLCAyMDE4LTAzLTA3IGF0IDEyOjQ3ICswMDAwLCBUdnJ0a28gVXJzdWxpbiB3cm90ZToN
Cj4gV2UgY2FuIGRlcml2ZSB0aGUgb3JkZXIgZnJvbSBzZy0+bGVuZ3RoIGFuZCBzbyBkbyBub3Qg
bmVlZCB0byBwYXNzIGl0IGluDQo+IGV4cGxpY2l0bHkuIFJlbmFtZSB0aGUgZnVuY3Rpb24gdG8g
c2dsX2ZyZWVfbi4NCg0KVXNpbmcgZ2V0X29yZGVyKCkgdG8gY29tcHV0ZSB0aGUgb3JkZXIgbG9v
a3MgZmluZSB0byBtZSBidXQgdGhpcyBwYXRjaCB3aWxsDQpoYXZlIHRvIHJlYmFzZWQgaW4gb3Jk
ZXIgdG8gYWRkcmVzcyB0aGUgY29tbWVudHMgb24gdGhlIHByZXZpb3VzIHBhdGNoZXMuDQoNClRo
YW5rcywNCg0KQmFydC4NCg0KDQo

Re: [PATCH 1/6] lib/scatterlist: Tidy types and fix overflow checking in sgl_alloc_order

[Tvrtko Ursulin]

On 07/03/18 16:10, Bart Van Assche wrote:
> On Wed, 2018-03-07 at 12:47 +0000, Tvrtko Ursulin wrote:
>> sgl_alloc_order explicitly takes a 64-bit length (unsigned long long) but
>> then rejects it in overflow checking if greater than 4GiB allocation was
>> requested. This is a consequence of using unsigned int for the right hand
>> side condition which then natuarally overflows when shifted left, earlier
>> than nent otherwise would.
>>
>> Fix is to promote the right hand side of the conditional to unsigned long.
> 
> Agreed.
> 
>> It is also not useful to allow for 64-bit lenght on 32-bit platforms so
>> I have changed this type to a natural unsigned long. Like this it changes
>> size naturally depending on the architecture.
> 
> I do not agree. Although uncommon, it is possible that e.g. a SCSI initiator
> sends a transfer of more than 4 GB to a target system and that that transfer
> must not be split. Since this code is used by the SCSI target, I think that's
> an example of an application where it is useful to allow allocations of more
> than 4 GB at once on a 32-bit system.

If it can work on 32-bit (it can DMA from highmem or what?) and 
allocation can realistically succeed then  I'm happy to defer to storage 
experts on this one.

> 
>> 2.
>>
>> elem_len should not be explicitly sized u32 but unsigned int, to match
>> the underlying struct scatterlist nents type. Same for the nent_p output
>> parameter type.
> 
> Are you sure it is useful to support allocations with an order that exceeds
> (31 - PAGE_SHIFT)? Since memory gets fragmented easily in the Linux kernel I
> think that it's unlikely that such allocations will succeed.

Not sure what you are getting at here.

There are not explicit width types anywhere in the SGL API apart this 
u32 elem_lem.

So I changed it to unsigned int not to confuse. It gets passed in to 
sg_set_page which takes unsigned int. So no reason for it to be u32.

> 
>> I renamed this to chunk_len and consolidated its use throughout the
>> function.
> 
> Please undo this change such that the diff remains as short as possible.

Name change only? Yeah can do that. Even though chunk as a term is 
somewhat established elsewhere in lib/scatterlist.c.

>> -void sgl_free_n_order(struct scatterlist *sgl, int nents, int order)
>> +void sgl_free_n_order(struct scatterlist *sgl, unsigned int nents,
>> +		      unsigned int order)
>>   {
>>   	struct scatterlist *sg;
>>   	struct page *page;
>> -	int i;
>> +	unsigned int i;
>>   
>>   	for_each_sg(sgl, sg, nents, i) {
>>   		if (!sg)
>> @@ -583,9 +587,9 @@ EXPORT_SYMBOL(sgl_free_n_order);
>>    * @sgl: Scatterlist with one or more elements
>>    * @order: Second argument for __free_pages()
>>    */
>> -void sgl_free_order(struct scatterlist *sgl, int order)
>> +void sgl_free_order(struct scatterlist *sgl, unsigned int order)
>>   {
>> -	sgl_free_n_order(sgl, INT_MAX, order);
>> +	sgl_free_n_order(sgl, UINT_MAX, order);
>>   }
>>   EXPORT_SYMBOL(sgl_free_order);
> 
> Do you have an application that calls these functions to allocate more than
> INT_MAX * PAGE_SIZE bytes at once? If not, please leave these changes out.

There is no reason to used signed int here and it is even inconsistent 
with itself because sgl_alloc_order returns you nents in an unsigned 
int. And sg_init_table takes unsigned int for nents. So really I see no 
reason to have signed types for nents on sgl_free side of the API.

Regards,

Tvrtko

Re: [PATCH 3/6] lib/scatterlist: Do not leak pages when high-order allocation fails

[Tvrtko Ursulin]

On 07/03/18 16:16, Bart Van Assche wrote:
> On Wed, 2018-03-07 at 12:47 +0000, Tvrtko Ursulin wrote:
>> diff --git a/lib/scatterlist.c b/lib/scatterlist.c
>> index 9884be50a2c0..e13a759c5c49 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;
>> +	unsigned int nent, i;
>>   
>>   	nent = round_up(length, chunk_len) >> (PAGE_SHIFT + order);
>>   
>> @@ -517,11 +517,12 @@ 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(sgl);
>> +			sgl_free_n_order(sgl, i, order);
>>   			return NULL;
>>   		}
>>   
>> @@ -529,6 +530,7 @@ 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++;
>>   	}
> 
> Since the entire sg-list is zero-initialized before this loop starts, since
> the sg-list is not chained onto another sg-list before this function returns
> and since sgl_free_n_order() checks whether or not each page pointer is NULL
> before freeing it I think we don't need the new loop variable 'i' and that
> we can call sgl_free_order() instead of sgl_free_n_order().

Yes true, I've only realized that in a later patch. Can rebase to move 
that change earlier in.

Regards,

Tvrtko

Re: [PATCH 4/6] lib/scatterlist: Unexport some trivial wrappers

[Tvrtko Ursulin]

On 07/03/18 16:19, Bart Van Assche wrote:
> On Wed, 2018-03-07 at 12:47 +0000, Tvrtko Ursulin wrote:
>> Save some kernel size by moving trivial wrappers to header as static
>> inline instead of exporting symbols for them.
> 
> Something that you may be unaware of is that the introduction of the sgl
> helper functions is only a first step. The next step will be to introduce
> a caching allocator for sg-lists. So for small sg-lists inlining won't
> help performance. But moving these definitions from a .c file into a .h
> file will (slightly) slow down kernel compilation. So I'd prefer that you
> drop this patch.

Question is how will the future work influence these trivial wrappers?

I wasn't suggesting I removed them for performance reasons, but just 
because they are really trivial and so there is no need right now to 
have them as exported symbols.

And actually in one of the earlier work I did in lib/scatterlist.c 
Andrew Morton complained a bit to the prevalence of these trivial 
wrappers. So I even had plans to remove some of the existing ones but 
never got round to it.

Regards,

Tvrtko

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

[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 2/6] lib/scatterlist: Skip requesting zeroed allocations in sgl_alloc_order

[Tvrtko Ursulin]

On 07/03/18 15:35, Andy Shevchenko wrote:
> On Wed, Mar 7, 2018 at 2:47 PM, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
> 
>> +       sgl = kmalloc_array(nent, sizeof(struct scatterlist), (gfp & ~GFP_DMA));
> 
> The parens now become redundant.

True thanks! I am also not sure that re-using the same gfp_t for 
metadata is backing store is the right approach. At least I think 
__GFP_HIGHMEM needs also to be cleared for kmalloc.

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

[Bart Van Assche]

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

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

[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 0/6] lib/scatterlist: Small SGL tidy

[Bart Van Assche]

On Wed, 2018-03-07 at 12:47 +0000, Tvrtko Ursulin wrote:
> I spotted a few small issues in the recent added SGL code so am sending some
> patches to tidy this.

Can you send the fixes as a separate series and keep the rework / behavior changes
for later such that Jens can queue the fixes for kernel v4.16?

Thanks,

Bart.

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

[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 0/6] lib/scatterlist: Small SGL tidy

[Tvrtko Ursulin]

On 07/03/18 18:38, Bart Van Assche wrote:
> On Wed, 2018-03-07 at 12:47 +0000, Tvrtko Ursulin wrote:
>> I spotted a few small issues in the recent added SGL code so am sending some
>> patches to tidy this.
> 
> Can you send the fixes as a separate series and keep the rework / behavior changes
> for later such that Jens can queue the fixes for kernel v4.16?

Yes of course. But which ones you consider rework and behaviour changes? 
To me all look like fixes / cleanups. (Last patch in the series needs a 
fix/respin btw)

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

[Bart Van Assche]

T24gVGh1LCAyMDE4LTAzLTA4IGF0IDA3OjU5ICswMDAwLCBUdnJ0a28gVXJzdWxpbiB3cm90ZToN
Cj4gSG93ZXZlciB0aGVyZSBpcyBhIGRpZmZlcmVudCBidWcgaW4gbXkgcGF0Y2ggcmVsYXRpbmcg
dG8gdGhlIGxhc3QgZW50cnkgDQo+IHdoaWNoIGNhbiBoYXZlIHNob3J0ZXIgbGVuZ3RoIGZyb20g
dGhlIHJlc3QuIFNvIGdldF9vcmRlciBvbiB0aGUgbGFzdCANCj4gZW50cnkgaXMgaW5jb3JyZWN0
IC0gSSBoYXZlIHRvIHN0b3JlIHRoZSBkZWR1Y2VkIG9yZGVyIGFuZCBjYXJyeSBpdCBvdmVyLg0K
DQpXaWxsIHRoYXQgd29yayBpZiB0aGVyZSBpcyBvbmx5IG9uZSBlbnRyeSBpbiB0aGUgbGlzdCBh
bmQgaWYgaXQgaXMgYSBzaG9ydA0KZW50cnk/DQoNCkJhcnQuDQoNCg0KDQo

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

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

Re: [PATCH 1/6] lib/scatterlist: Tidy types and fix overflow checking in sgl_alloc_order

[Tvrtko Ursulin]

On 07/03/18 17:06, Tvrtko Ursulin wrote:
> On 07/03/18 16:10, Bart Van Assche wrote:
>> On Wed, 2018-03-07 at 12:47 +0000, Tvrtko Ursulin wrote:
>>> sgl_alloc_order explicitly takes a 64-bit length (unsigned long long) 
>>> but
>>> then rejects it in overflow checking if greater than 4GiB allocation was
>>> requested. This is a consequence of using unsigned int for the right 
>>> hand
>>> side condition which then natuarally overflows when shifted left, 
>>> earlier
>>> than nent otherwise would.
>>>
>>> Fix is to promote the right hand side of the conditional to unsigned 
>>> long.
>>
>> Agreed.
>>
>>> It is also not useful to allow for 64-bit lenght on 32-bit platforms so
>>> I have changed this type to a natural unsigned long. Like this it 
>>> changes
>>> size naturally depending on the architecture.
>>
>> I do not agree. Although uncommon, it is possible that e.g. a SCSI 
>> initiator
>> sends a transfer of more than 4 GB to a target system and that that 
>> transfer
>> must not be split. Since this code is used by the SCSI target, I think 
>> that's
>> an example of an application where it is useful to allow allocations 
>> of more
>> than 4 GB at once on a 32-bit system.
> 
> If it can work on 32-bit (it can DMA from highmem or what?) and 
> allocation can realistically succeed then  I'm happy to defer to storage 
> experts on this one.

Furthermore on this specific point, the only caller of sgl_alloc_order 
in the tree passes in u32 for length.

So even if there will be some realistic use case for >4GiB allocations 
on 32-bit systems (where unsigned long is 32-bit, to be more precise) I 
don't see a problem with my patch right now.

First five patches from the series are all IMO fixes and cleanup of 
unused parts of the API.

Regards,

Tvrtko

>>
>>> 2.
>>>
>>> elem_len should not be explicitly sized u32 but unsigned int, to match
>>> the underlying struct scatterlist nents type. Same for the nent_p output
>>> parameter type.
>>
>> Are you sure it is useful to support allocations with an order that 
>> exceeds
>> (31 - PAGE_SHIFT)? Since memory gets fragmented easily in the Linux 
>> kernel I
>> think that it's unlikely that such allocations will succeed.
> 
> Not sure what you are getting at here.
> 
> There are not explicit width types anywhere in the SGL API apart this 
> u32 elem_lem.
> 
> So I changed it to unsigned int not to confuse. It gets passed in to 
> sg_set_page which takes unsigned int. So no reason for it to be u32.
> 
>>
>>> I renamed this to chunk_len and consolidated its use throughout the
>>> function.
>>
>> Please undo this change such that the diff remains as short as possible.
> 
> Name change only? Yeah can do that. Even though chunk as a term is 
> somewhat established elsewhere in lib/scatterlist.c.
> 
>>> -void sgl_free_n_order(struct scatterlist *sgl, int nents, int order)
>>> +void sgl_free_n_order(struct scatterlist *sgl, unsigned int nents,
>>> +              unsigned int order)
>>>   {
>>>       struct scatterlist *sg;
>>>       struct page *page;
>>> -    int i;
>>> +    unsigned int i;
>>>       for_each_sg(sgl, sg, nents, i) {
>>>           if (!sg)
>>> @@ -583,9 +587,9 @@ EXPORT_SYMBOL(sgl_free_n_order);
>>>    * @sgl: Scatterlist with one or more elements
>>>    * @order: Second argument for __free_pages()
>>>    */
>>> -void sgl_free_order(struct scatterlist *sgl, int order)
>>> +void sgl_free_order(struct scatterlist *sgl, unsigned int order)
>>>   {
>>> -    sgl_free_n_order(sgl, INT_MAX, order);
>>> +    sgl_free_n_order(sgl, UINT_MAX, order);
>>>   }
>>>   EXPORT_SYMBOL(sgl_free_order);
>>
>> Do you have an application that calls these functions to allocate more 
>> than
>> INT_MAX * PAGE_SIZE bytes at once? If not, please leave these changes 
>> out.
> 
> There is no reason to used signed int here and it is even inconsistent 
> with itself because sgl_alloc_order returns you nents in an unsigned 
> int. And sg_init_table takes unsigned int for nents. So really I see no 
> reason to have signed types for nents on sgl_free side of the API.
> 
> Regards,
> 
> Tvrtko