Re: [PATCH rdma-next v2 1/2] lib/scatterlist: Fix wrong update of orig_nents
From: Jason Gunthorpe <jgg@nvidia.com>
Date: 2021-07-22 13:39:07
Also in:
dri-devel, intel-gfx, lkml
Subsystem:
drm driver for vmware virtual gpu, drm drivers, drm drivers and misc gpu patches, intel drm i915 driver (meteor lake, dg2 and older excluding poulsbo, moorestown and derivative), library code, the rest · Maintainers:
Zack Rusin, David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, Andrew Morton, Linus Torvalds
On Thu, Jul 22, 2021 at 02:07:51PM +0100, Christoph Hellwig wrote:
On Thu, Jul 22, 2021 at 10:00:40AM -0300, Jason Gunthorpe wrote:quoted
this is better: struct sg_append_table state; sg_append_init(&state, sgt, gfp_mask); while (..) ret = sg_append_pages(&state, pages, n_pages, ..) if (ret) sg_append_abort(&state); // Frees the sgt and puts it to NULL sg_append_complete(&state) Which allows sg_alloc_table_from_pages() to be written as struct sg_append_table state; sg_append_init(&state, sgt, gfp_mask); ret = sg_append_pages(&state,pages, n_pages, offset, size, UINT_MAX) if (ret) { sg_append_abort(&state); return ret; } sg_append_complete(&state); return 0; And then the API can manage all of this in some sane and understandable way.That would be a lot easier to use for sure. Not sure how invasive the changes would be, though.
Looks pretty good, Maor can you try it?
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 1739d10a2c556e..6c8e761ab389e8 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c@@ -806,27 +806,27 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = { struct sg_table *drm_prime_pages_to_sg(struct drm_device *dev, struct page **pages, unsigned int nr_pages) { - struct sg_table *sg; - struct scatterlist *sge; + struct sg_table *sgt; size_t max_segment = 0; + int ret; - sg = kmalloc(sizeof(struct sg_table), GFP_KERNEL); - if (!sg) + sgt = kmalloc(sizeof(struct sg_table), GFP_KERNEL); + if (!sgt) return ERR_PTR(-ENOMEM); if (dev) max_segment = dma_max_mapping_size(dev->dev); if (max_segment == 0) max_segment = UINT_MAX; - sge = __sg_alloc_table_from_pages(sg, pages, nr_pages, 0, - nr_pages << PAGE_SHIFT, - max_segment, - NULL, 0, GFP_KERNEL, NULL); - if (IS_ERR(sge)) { - kfree(sg); - sg = ERR_CAST(sge); + + ret = sg_alloc_table_from_pages_segment(sgt, pages, nr_pages, 0, + nr_pages << PAGE_SHIFT, + max_segment, GFP_KERNEL); + if (ret) { + kfree(sgt); + return ERR_PTR(ret); } - return sg; + return sgt; } EXPORT_SYMBOL(drm_prime_pages_to_sg);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index c341d3e3386ccb..a2c5e1b30f425f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c@@ -131,6 +131,7 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj) struct drm_i915_private *i915 = to_i915(obj->base.dev); const unsigned long num_pages = obj->base.size >> PAGE_SHIFT; unsigned int max_segment = i915_sg_segment_size(); + struct sg_append_table state; struct sg_table *st; unsigned int sg_page_sizes; struct scatterlist *sg;
@@ -153,13 +154,11 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj) spin_unlock(&i915->mm.notifier_lock); alloc_table: - sg = __sg_alloc_table_from_pages(st, pvec, num_pages, 0, - num_pages << PAGE_SHIFT, max_segment, - NULL, 0, GFP_KERNEL, NULL); - if (IS_ERR(sg)) { - ret = PTR_ERR(sg); + ret = sg_alloc_table_from_pages_segment(st, pvec, num_pages, 0, + num_pages << PAGE_SHIFT, + max_segment, GFP_KERNEL); + if (ret) goto err; - } ret = i915_gem_gtt_prepare_pages(obj, st); if (ret) {
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
index 2ad889272b0a15..56214bcc6c5280 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c@@ -386,15 +386,12 @@ static int vmw_ttm_map_dma(struct vmw_ttm_tt *vmw_tt) if (unlikely(ret != 0)) return ret; - sg = __sg_alloc_table_from_pages(&vmw_tt->sgt, vsgt->pages, - vsgt->num_pages, 0, - (unsigned long) vsgt->num_pages << PAGE_SHIFT, - dma_get_max_seg_size(dev_priv->drm.dev), - NULL, 0, GFP_KERNEL, NULL); - if (IS_ERR(sg)) { - ret = PTR_ERR(sg); + ret = sg_alloc_table_from_pages_segment( + vmw_tt->sgt, vsgt->pages, vsgt->num_pages, 0, + (unsigned long)vsgt->num_pages << PAGE_SHIFT, + dma_get_max_seg_size(dev_priv->drm.dev), GFP_KERNEL); + if (ret) goto out_sg_alloc_fail; - } if (vsgt->num_pages > vmw_tt->sgt.orig_nents) { uint64_t over_alloc =
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 5cc41ae962ec1d..53de1ec77326be 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c@@ -556,6 +556,25 @@ struct scatterlist *__sg_alloc_table_from_pages(struct sg_table *sgt, } EXPORT_SYMBOL(__sg_alloc_table_from_pages); +int sg_alloc_table_from_pages_segment(struct sg_table *sgt, struct page **pages, + unsigned int n_pages, unsigned int offset, + unsigned long size, + unsigned int max_segment, gfp_t gfp_mask) +{ + struct sg_append_table state; + int ret; + + sg_append_init(&state, sgt, max_segment, gfp_mask); + ret = sg_append_pages(&state, pages, n_pages, offset, size); + if (ret) { + sg_append_abort(&state); + return ret; + } + sg_append_complete(&state); + return 0; +} +EXPORT_SYMBOL(sg_alloc_table_from_pages_segment); + /** * sg_alloc_table_from_pages - Allocate and initialize an sg table from * an array of pages
@@ -580,8 +599,8 @@ 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 PTR_ERR_OR_ZERO(__sg_alloc_table_from_pages(sgt, pages, n_pages, - offset, size, UINT_MAX, NULL, 0, gfp_mask, NULL)); + return sg_alloc_table_from_pages_segment(sgt, pages, n_pages, offset, + size, UINT_MAX, gfp_mask); } EXPORT_SYMBOL(sg_alloc_table_from_pages);