[PATCH 4/5] drm/i915: Use __sg_alloc_table_from_pages for allocating object backing store

[Tvrtko Ursulin]

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

With the current way of allocating backing store which over-estimates
the number of sg entries required we typically waste around 1-6 MiB of
memory on unused sg entries at runtime.

We can instead have the intermediate step of storing our pages in an
array and use __sg_alloc_table_from_pages which will create us the
most compact list possible.

Signed-off-by: Tvrtko Ursulin <tvrtko....@intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 72 ++++++++++++++++++++---------------------
1 file changed, 35 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8ed8e24025ac..4bf675568a37 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2208,9 +2208,9 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
static unsigned int swiotlb_max_size(void)
{
#if IS_ENABLED(CONFIG_SWIOTLB)
- return rounddown(swiotlb_nr_tbl() << IO_TLB_SHIFT, PAGE_SIZE);
+ return swiotlb_nr_tbl() << IO_TLB_SHIFT;
#else
- return 0;
+ return UINT_MAX;
#endif
}

@@ -2221,11 +2221,8 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
int page_count, i;
struct address_space *mapping;
struct sg_table *st;
- struct scatterlist *sg;
- struct sgt_iter sgt_iter;
- struct page *page;
- unsigned long last_pfn = 0; /* suppress gcc warning */
- unsigned int max_segment;
+ struct page *page, **pages;
+ unsigned int max_segment = swiotlb_max_size();
int ret;
gfp_t gfp;

@@ -2236,18 +2233,16 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
BUG_ON(obj->base.read_domains & I915_GEM_GPU_DOMAINS);
BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAINS);

- max_segment = swiotlb_max_size();
- if (!max_segment)
- max_segment = rounddown(UINT_MAX, PAGE_SIZE);
-
- st = kmalloc(sizeof(*st), GFP_KERNEL);
- if (st == NULL)
- return -ENOMEM;
-
page_count = obj->base.size / PAGE_SIZE;
- if (sg_alloc_table(st, page_count, GFP_KERNEL)) {
- kfree(st);
+ pages = drm_malloc_gfp(page_count, sizeof(struct page *),
+ GFP_TEMPORARY | __GFP_ZERO);
+ if (!pages)
return -ENOMEM;
+
+ st = kmalloc(sizeof(*st), GFP_KERNEL);
+ if (st == NULL) {
+ ret = -ENOMEM;
+ goto err_st;
}

/* Get the list of pages out of our struct file. They'll be pinned
@@ -2258,8 +2253,6 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
mapping = obj->base.filp->f_mapping;
gfp = mapping_gfp_constraint(mapping, ~(__GFP_IO | __GFP_RECLAIM));
gfp |= __GFP_NORETRY | __GFP_NOWARN;
- sg = st->sgl;
- st->nents = 0;
for (i = 0; i < page_count; i++) {
page = shmem_read_mapping_page_gfp(mapping, i, gfp);
if (IS_ERR(page)) {
@@ -2281,29 +2274,28 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
goto err_pages;
}
}
- if (!i ||
- sg->length >= max_segment ||
- page_to_pfn(page) != last_pfn + 1) {
- if (i)
- sg = sg_next(sg);
- st->nents++;
- sg_set_page(sg, page, PAGE_SIZE, 0);
- } else {
- sg->length += PAGE_SIZE;
- }
- last_pfn = page_to_pfn(page);
+
+ pages[i] = page;

/* Check that the i965g/gm workaround works. */
- WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
+ WARN_ON((gfp & __GFP_DMA32) &&
+ (page_to_pfn(page) >= 0x00100000UL));
}
- if (sg) /* loop terminated early; short sg table */
- sg_mark_end(sg);
+
+ ret = __sg_alloc_table_from_pages(st, pages, page_count, 0,
+ obj->base.size, GFP_KERNEL,
+ max_segment);
+ if (ret)
+ goto err_pages;
+
obj->pages = st;

ret = i915_gem_gtt_prepare_object(obj);
if (ret)
goto err_pages;

+ drm_free_large(pages);
+
if (i915_gem_object_needs_bit17_swizzle(obj))
i915_gem_object_do_bit_17_swizzle(obj);

@@ -2314,10 +2306,13 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
return 0;

err_pages:
- sg_mark_end(sg);
- for_each_sgt_page(page, sgt_iter, st)
- put_page(page);
- sg_free_table(st);
+ for (i = 0; i < page_count; i++) {
+ if (pages[i])
+ put_page(pages[i]);
+ else
+ break;
+ }
+
kfree(st);

/* shmemfs first checks if there is enough memory to allocate the page
@@ -2331,6 +2326,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
if (ret == -ENOSPC)
ret = -ENOMEM;

+err_st:
+ drm_free_large(pages);
+
return ret;
}

--
2.7.4

[Tvrtko Ursulin]

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

We can decrease the i915 kernel memory usage by doing more sg list
coallescing and avoiding the pessimistic list allocation.

At the moment we got two places in our code, the main shmemfs backed
object allocator, and the userptr object allocator, which both can
allocate sg list size pessimistically, and in the latter case also do
not exploit entry coallescing when it is possible.

This results in between one to six megabytes of memory wasted on unused
sg list entries under some common workloads:

* Logging into KDE there is 1-2 MiB of unused sg entries.
* Running the T-Rex benchamrk aroun 3 Mib.
* Similarly for Manhattan 5-6 MiB.

To remove this wastage this series starts with some cleanups in the
sg_alloc_table_from_pages implementation and then adds and exports a new
__sg_alloc_table_from_pages function.

This then gets used by the i915 driver to achieve the described savings.

Tvrtko Ursulin (5):
lib/scatterlist: Fix offset type in sg_alloc_table_from_pages
lib/scatterlist: Avoid potential scatterlist entry overflow
lib/scatterlist: Introduce and export __sg_alloc_table_from_pages
drm/i915: Use __sg_alloc_table_from_pages for allocating object
backing store
drm/i915: Use __sg_alloc_table_from_pages for userptr allocations

drivers/gpu/drm/i915/i915_drv.h | 9 +++
drivers/gpu/drm/i915/i915_gem.c | 77 +++++++++++--------------
drivers/gpu/drm/i915/i915_gem_userptr.c | 29 +++-------
drivers/media/v4l2-core/videobuf2-dma-contig.c | 4 +-
drivers/rapidio/devices/rio_mport_cdev.c | 4 +-
include/linux/scatterlist.h | 11 ++--
lib/scatterlist.c | 78 ++++++++++++++++++++------
7 files changed, 120 insertions(+), 92 deletions(-)

--
2.7.4

[Tvrtko Ursulin]

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

Since the scatterlist length field is an unsigned int, make
sure that sg_alloc_table_from_pages does not overflow it while
coallescing pages to a single entry.

It is I think only a theoretical possibility at the moment,
but the ability to limit the coallesced size will have
another use in following patches.

Signed-off-by: Tvrtko Ursulin <tvrtko....@intel.com>
Cc: Masahiro Yamada <yamada....@socionext.com>
Cc: linux-...@vger.kernel.org
---
lib/scatterlist.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index e05e7fc98892..d928fa04aee3 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -394,7 +394,8 @@ int sg_alloc_table_from_pages(struct sg_table *sgt,
unsigned int offset, unsigned long size,
gfp_t gfp_mask)
{
- unsigned int chunks;
+ const unsigned int max_segment = ~0;
+ unsigned int seg_len, chunks;
unsigned int i;
unsigned int cur_page;
int ret;
@@ -402,9 +403,16 @@ int sg_alloc_table_from_pages(struct sg_table *sgt,

/* compute number of contiguous chunks */
chunks = 1;
- for (i = 1; i < n_pages; ++i)
- if (page_to_pfn(pages[i]) != page_to_pfn(pages[i - 1]) + 1)
+ seg_len = PAGE_SIZE;
+ for (i = 1; i < n_pages; ++i) {
+ if (seg_len >= max_segment ||
+ page_to_pfn(pages[i]) != page_to_pfn(pages[i - 1]) + 1) {
++chunks;
+ seg_len = PAGE_SIZE;
+ } else {
+ seg_len += PAGE_SIZE;
+ }
+ }

ret = sg_alloc_table(sgt, chunks, gfp_mask);
if (unlikely(ret))
@@ -413,17 +421,22 @@ int sg_alloc_table_from_pages(struct sg_table *sgt,
/* merging chunks and putting them into the scatterlist */
cur_page = 0;
for_each_sg(sgt->sgl, s, sgt->orig_nents, i) {
- unsigned long chunk_size;
+ unsigned int chunk_size;
unsigned int j;

/* look for the end of the current chunk */
+ seg_len = PAGE_SIZE;
for (j = cur_page + 1; j < n_pages; ++j)
- if (page_to_pfn(pages[j]) !=
+ if (seg_len >= max_segment ||
+ page_to_pfn(pages[j]) !=
page_to_pfn(pages[j - 1]) + 1)
break;
+ else
+ seg_len += PAGE_SIZE;

chunk_size = ((j - cur_page) << PAGE_SHIFT) - offset;
- sg_set_page(s, pages[cur_page], min(size, chunk_size), offset);
+ sg_set_page(s, pages[cur_page],
+ min_t(unsigned long, size, chunk_size), offset);
size -= chunk_size;
offset = 0;
cur_page = j;
--
2.7.4

[Tvrtko Ursulin]

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

Drivers like i915 benefit from being able to control the maxium
size of the sg coallesced segment while building the scatter-
gather list.

Introduce and export the __sg_alloc_table_from_pages function
which will allow it that control.

Signed-off-by: Tvrtko Ursulin <tvrtko....@intel.com>
Cc: Masahiro Yamada <yamada....@socionext.com>
Cc: linux-...@vger.kernel.org
---

include/linux/scatterlist.h | 11 +++++----
lib/scatterlist.c | 55 ++++++++++++++++++++++++++++++++++-----------
2 files changed, 49 insertions(+), 17 deletions(-)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index c981bee1a3ae..29591dbb20fd 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -261,10 +261,13 @@ void sg_free_table(struct sg_table *);
int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int,
struct scatterlist *, gfp_t, sg_alloc_fn *);
int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);
-int sg_alloc_table_from_pages(struct sg_table *sgt,
- struct page **pages, unsigned int n_pages,
- unsigned int offset, unsigned long size,
- gfp_t gfp_mask);
+int __sg_alloc_table_from_pages(struct sg_table *sgt, struct page **pages,
+ unsigned int n_pages, unsigned int offset,
+ unsigned long size, gfp_t gfp_mask,
+ unsigned int max_segment);
+int sg_alloc_table_from_pages(struct sg_table *sgt, struct page **pages,
+ unsigned int n_pages, unsigned int offset,
+ unsigned long size, gfp_t gfp_mask);

size_t sg_copy_buffer(struct scatterlist *sgl, unsigned int nents, void *buf,
size_t buflen, off_t skip, bool to_buffer);
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index d928fa04aee3..0378c5fd7caa 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -370,14 +370,15 @@ int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask)
EXPORT_SYMBOL(sg_alloc_table);

/**
- * sg_alloc_table_from_pages - Allocate and initialize an sg table from
- * an array of pages
- * @sgt: The sg table header to use
- * @pages: Pointer to an array of page pointers
- * @n_pages: Number of pages in the pages array
- * @offset: Offset from start of the first page to the start of a buffer
- * @size: Number of valid bytes in the buffer (after offset)
- * @gfp_mask: GFP allocation mask
+ * __sg_alloc_table_from_pages - Allocate and initialize an sg table from
+ * an array of pages
+ * @sgt: The sg table header to use
+ * @pages: Pointer to an array of page pointers
+ * @n_pages: Number of pages in the pages array
+ * @offset: Offset from start of the first page to the start of a buffer
+ * @size: Number of valid bytes in the buffer (after offset)
+ * @gfp_mask: GFP allocation mask
+ * @max_segment: Maximum size of a single scatterlist node in bytes
*
* Description:
* Allocate and initialize an sg table from a list of pages. Contiguous
@@ -389,12 +390,11 @@ EXPORT_SYMBOL(sg_alloc_table);
* Returns:
* 0 on success, negative error on failure
*/
-int sg_alloc_table_from_pages(struct sg_table *sgt,
- struct page **pages, unsigned int n_pages,
- unsigned int offset, unsigned long size,
- gfp_t gfp_mask)
+int __sg_alloc_table_from_pages(struct sg_table *sgt, struct page **pages,
+ unsigned int n_pages, unsigned int offset,
+ unsigned long size, gfp_t gfp_mask,
+ unsigned int max_segment)
{
- const unsigned int max_segment = ~0;

unsigned int seg_len, chunks;
unsigned int i;
unsigned int cur_page;

@@ -444,6 +444,35 @@ int sg_alloc_table_from_pages(struct sg_table *sgt,

return 0;
}
+EXPORT_SYMBOL(__sg_alloc_table_from_pages);
+
+/**
+ * sg_alloc_table_from_pages - Allocate and initialize an sg table from
+ * an array of pages
+ * @sgt: The sg table header to use
+ * @pages: Pointer to an array of page pointers
+ * @n_pages: Number of pages in the pages array
+ * @offset: Offset from start of the first page to the start of a buffer
+ * @size: Number of valid bytes in the buffer (after offset)
+ * @gfp_mask: GFP allocation mask
+ *
+ * Description:
+ * Allocate and initialize an sg table from a list of pages. Contiguous
+ * ranges of the pages are squashed into a single scatterlist node. A user
+ * may provide an offset at a start and a size of valid data in a buffer
+ * specified by the page array. The returned sg table is released by
+ * sg_free_table.
+ *
+ * Returns:
+ * 0 on success, negative error on failure
+ */
+int sg_alloc_table_from_pages(struct sg_table *sgt, struct page **pages,
+ unsigned int n_pages, unsigned int offset,
+ unsigned long size, gfp_t gfp_mask)
+{
+ return __sg_alloc_table_from_pages(sgt, pages, n_pages, offset,
+ size, gfp_mask, ~0);
+}
EXPORT_SYMBOL(sg_alloc_table_from_pages);

void __sg_page_iter_start(struct sg_page_iter *piter,
--
2.7.4

[Tvrtko Ursulin]

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

Scatterlist entries have an unsigned int for the offset so
correct the sg_alloc_table_from_pages function accordingly.

Since these are offsets withing a page, unsigned int is
wide enough.

Also converts callers which were using unsigned long locally
with the lower_32_bits annotation to make it explicitly
clear what is happening.

Signed-off-by: Tvrtko Ursulin <tvrtko....@intel.com>
Cc: Masahiro Yamada <yamada....@socionext.com>

Cc: Pawel Osciak <pa...@osciak.com>
Cc: Marek Szyprowski <m.szyp...@samsung.com>
Cc: Kyungmin Park <kyungm...@samsung.com>
Cc: Tomasz Stanislawski <t.stan...@samsung.com>
Cc: Matt Porter <mpo...@kernel.crashing.org>
Cc: Alexandre Bounine <alexandr...@idt.com>
Cc: linux...@vger.kernel.org
Cc: linux-...@vger.kernel.org
---
drivers/media/v4l2-core/videobuf2-dma-contig.c | 4 ++--
drivers/rapidio/devices/rio_mport_cdev.c | 4 ++--
include/linux/scatterlist.h | 2 +-
lib/scatterlist.c | 2 +-
4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index fb6a177be461..a3aac7533241 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -478,7 +478,7 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
{
struct vb2_dc_buf *buf;
struct frame_vector *vec;
- unsigned long offset;
+ unsigned int offset;
int n_pages, i;
int ret = 0;
struct sg_table *sgt;
@@ -506,7 +506,7 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
buf->dev = dev;
buf->dma_dir = dma_dir;

- offset = vaddr & ~PAGE_MASK;
+ offset = lower_32_bits(vaddr & ~PAGE_MASK);
vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE);
if (IS_ERR(vec)) {
ret = PTR_ERR(vec);
diff --git a/drivers/rapidio/devices/rio_mport_cdev.c b/drivers/rapidio/devices/rio_mport_cdev.c
index 436dfe871d32..f545cf20561f 100644
--- a/drivers/rapidio/devices/rio_mport_cdev.c
+++ b/drivers/rapidio/devices/rio_mport_cdev.c
@@ -876,10 +876,10 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode,
* offset within the internal buffer specified by handle parameter.
*/
if (xfer->loc_addr) {
- unsigned long offset;
+ unsigned int offset;
long pinned;

- offset = (unsigned long)(uintptr_t)xfer->loc_addr & ~PAGE_MASK;
+ offset = lower_32_bits(xfer->loc_addr & ~PAGE_MASK);
nr_pages = PAGE_ALIGN(xfer->length + offset) >> PAGE_SHIFT;

page_list = kmalloc_array(nr_pages,
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index cb3c8fe6acd7..c981bee1a3ae 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -263,7 +263,7 @@ int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int,

int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);

int sg_alloc_table_from_pages(struct sg_table *sgt,

struct page **pages, unsigned int n_pages,

- unsigned long offset, unsigned long size,
+ unsigned int offset, unsigned long size,

gfp_t gfp_mask);

size_t sg_copy_buffer(struct scatterlist *sgl, unsigned int nents, void *buf,

diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 004fc70fc56a..e05e7fc98892 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -391,7 +391,7 @@ EXPORT_SYMBOL(sg_alloc_table);
*/
int sg_alloc_table_from_pages(struct sg_table *sgt,

struct page **pages, unsigned int n_pages,

- unsigned long offset, unsigned long size,
+ unsigned int offset, unsigned long size,
gfp_t gfp_mask)
{
unsigned int chunks;
--
2.7.4

[Chris Wilson]

On Fri, Oct 21, 2016 at 03:11:22PM +0100, Tvrtko Ursulin wrote:
> @@ -2236,18 +2233,16 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> BUG_ON(obj->base.read_domains & I915_GEM_GPU_DOMAINS);
> BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAINS);
>
> - max_segment = swiotlb_max_size();
> - if (!max_segment)
> - max_segment = rounddown(UINT_MAX, PAGE_SIZE);
> -
> - st = kmalloc(sizeof(*st), GFP_KERNEL);
> - if (st == NULL)
> - return -ENOMEM;
> -
> page_count = obj->base.size / PAGE_SIZE;
> - if (sg_alloc_table(st, page_count, GFP_KERNEL)) {
> - kfree(st);
> + pages = drm_malloc_gfp(page_count, sizeof(struct page *),
> + GFP_TEMPORARY | __GFP_ZERO);
> + if (!pages)
> return -ENOMEM;

Full circle! The whole reason this exists was to avoid that vmalloc. I
don't really want it back...
-Chris

--
Chris Wilson, Intel Open Source Technology Centre

[Tvrtko Ursulin]

Yes, it is not ideal.

However all objects under 4 MiB should fall under the kmalloc fast path
(8 KiB of struct page pointers, which should always be available), and
possibly bigger ones as well if there is room.

It only fallbacks to vmalloc for objects larger than 4 MiB, when it also
fails to get the page pointer array from the SLAB (GFP_TEMPORARY).

So perhaps SLAB would most of the time have some nice chunks for us to
pretty much limit vmalloc apart for the huge objects? And then, is
creation time for those so performance critical?

I came up with this because I started to dislike my previous
sg_trim_table approach as too ugly. It had an advantage of simplicity
after fixing the theoretical chunk overflow in sg_alloc_table_from_pages.

If we choose none of the two, only third option I can think of is to
allocate the sg table as we add entries to it. I don't think it would be
hard to do that.

Regards,

Tvrtko

[Marek Szyprowski]

Hi Tvrtko,

On 2016-10-21 16:11, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko....@intel.com>
>
> Scatterlist entries have an unsigned int for the offset so
> correct the sg_alloc_table_from_pages function accordingly.
>
> Since these are offsets withing a page, unsigned int is
> wide enough.
>
> Also converts callers which were using unsigned long locally
> with the lower_32_bits annotation to make it explicitly
> clear what is happening.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko....@intel.com>
> Cc: Masahiro Yamada <yamada....@socionext.com>
> Cc: Pawel Osciak <pa...@osciak.com>
> Cc: Marek Szyprowski <m.szyp...@samsung.com>
> Cc: Kyungmin Park <kyungm...@samsung.com>
> Cc: Tomasz Stanislawski <t.stan...@samsung.com>
> Cc: Matt Porter <mpo...@kernel.crashing.org>
> Cc: Alexandre Bounine <alexandr...@idt.com>
> Cc: linux...@vger.kernel.org
> Cc: linux-...@vger.kernel.org

Acked-by: Marek Szyprowski <m.szyp...@samsung.com>

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland