From: Tvrtko Ursulin <[email protected]>
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 <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Johannes Thumshirn <[email protected]>
Cc: Jens Axboe <[email protected]>
--
2.14.1
From: Tvrtko Ursulin <[email protected]>
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 <[email protected]>
Cc: Bart Van Assche <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Johannes Thumshirn <[email protected]>
Cc: Jens Axboe <[email protected]>
---
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
From: Tvrtko Ursulin <[email protected]>
sg_init_table will clear the allocated block so requesting zeroed memory
from the allocator is redundant.
Signed-off-by: Tvrtko Ursulin <[email protected]>
Cc: Bart Van Assche <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Johannes Thumshirn <[email protected]>
Cc: Jens Axboe <[email protected]>
---
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
From: Tvrtko Ursulin <[email protected]>
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 <[email protected]>
Cc: Bart Van Assche <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Johannes Thumshirn <[email protected]>
Cc: Jens Axboe <[email protected]>
---
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
From: Tvrtko Ursulin <[email protected]>
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 <[email protected]>
Cc: Bart Van Assche <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Johannes Thumshirn <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: "Nicholas A. Bellinger" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
From: Tvrtko Ursulin <[email protected]>
No code calls it so remove it.
Signed-off-by: Tvrtko Ursulin <[email protected]>
Cc: Bart Van Assche <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Johannes Thumshirn <[email protected]>
Cc: Jens Axboe <[email protected]>
---
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
From: Tvrtko Ursulin <[email protected]>
Save some kernel size by moving trivial wrappers to header as static
inline instead of exporting symbols for them.
Signed-off-by: Tvrtko Ursulin <[email protected]>
Cc: Bart Van Assche <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Johannes Thumshirn <[email protected]>
Cc: Jens Axboe <[email protected]>
---
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
On Wed, Mar 7, 2018 at 2:47 PM, Tvrtko Ursulin <[email protected]> wrote:
> + sgl = kmalloc_array(nent, sizeof(struct scatterlist), (gfp & ~GFP_DMA));
The parens now become redundant.
--
With Best Regards,
Andy Shevchenko
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.
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.
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.
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.
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
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
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
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
On 07/03/18 15:35, Andy Shevchenko wrote:
> On Wed, Mar 7, 2018 at 2:47 PM, Tvrtko Ursulin <[email protected]> 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
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.
On Wed, 2018-03-07 at 12:47 +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <[email protected]>
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
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.
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 <[email protected]>
>
> 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
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
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.
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
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