When converting to folios the cleanup path of shmem_get_pages() was missed. When a DMA remap fails and the max segment size is greater than PAGE_SIZE it will attempt to retry the remap with a PAGE_SIZEd segment size. The cleanup code isn't properly using the folio apis and as a result isn't handling compound pages correctly.
v1 -> v2: (Ville) Fixed locations where we were not clearing mapping unevictable.
Cc: stable@vger.kernel.org Cc: Ville Syrjala ville.syrjala@linux.intel.com Cc: Vidya Srinivas vidya.srinivas@intel.com Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13487 Link: https://lore.kernel.org/lkml/20250116135636.410164-1-bgeffon@google.com/ Fixes: 0b62af28f249 ("i915: convert shmem_sg_free_table() to use a folio_batch") Signed-off-by: Brian Geffon bgeffon@google.com Suggested-by: Tomasz Figa tfiga@google.com --- drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 +-- drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 23 +++++++++------------- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 7 ++++--- 3 files changed, 14 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 3dc61cbd2e11..0f122a12d4a5 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -843,8 +843,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st, size_t size, struct intel_memory_region *mr, struct address_space *mapping, unsigned int max_segment); -void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping, - bool dirty, bool backup); +void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup); void __shmem_writeback(size_t size, struct address_space *mapping);
#ifdef CONFIG_MMU_NOTIFIER diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c index fe69f2c8527d..b320d9dfd6d3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c @@ -29,16 +29,13 @@ static void check_release_folio_batch(struct folio_batch *fbatch) cond_resched(); }
-void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping, - bool dirty, bool backup) +void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup) { struct sgt_iter sgt_iter; struct folio_batch fbatch; struct folio *last = NULL; struct page *page;
- mapping_clear_unevictable(mapping); - folio_batch_init(&fbatch); for_each_sgt_page(page, sgt_iter, st) { struct folio *folio = page_folio(page); @@ -180,10 +177,10 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st, return 0; err_sg: sg_mark_end(sg); + mapping_clear_unevictable(mapping); if (sg != st->sgl) { - shmem_sg_free_table(st, mapping, false, false); + shmem_sg_free_table(st, false, false); } else { - mapping_clear_unevictable(mapping); sg_free_table(st); }
@@ -209,8 +206,6 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) struct address_space *mapping = obj->base.filp->f_mapping; unsigned int max_segment = i915_sg_segment_size(i915->drm.dev); struct sg_table *st; - struct sgt_iter sgt_iter; - struct page *page; int ret;
/* @@ -239,9 +234,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) * for PAGE_SIZE chunks instead may be helpful. */ if (max_segment > PAGE_SIZE) { - for_each_sgt_page(page, sgt_iter, st) - put_page(page); - sg_free_table(st); + /* Leave the mapping unevictable while we retry */ + shmem_sg_free_table(st, false, false); kfree(st);
max_segment = PAGE_SIZE; @@ -265,7 +259,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) return 0;
err_pages: - shmem_sg_free_table(st, mapping, false, false); + mapping_clear_unevictable(mapping); + shmem_sg_free_table(st, false, false); /* * shmemfs first checks if there is enough memory to allocate the page * and reports ENOSPC should there be insufficient, along with the usual @@ -402,8 +397,8 @@ void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_ if (i915_gem_object_needs_bit17_swizzle(obj)) i915_gem_object_save_bit_17_swizzle(obj, pages);
- shmem_sg_free_table(pages, file_inode(obj->base.filp)->i_mapping, - obj->mm.dirty, obj->mm.madv == I915_MADV_WILLNEED); + mapping_clear_unevictable(file_inode(obj->base.filp)->i_mapping); + shmem_sg_free_table(pages, obj->mm.dirty, obj->mm.madv == I915_MADV_WILLNEED); kfree(pages); obj->mm.dirty = false; } diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index 10d8673641f7..37f51a04b838 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -232,7 +232,8 @@ static int i915_ttm_tt_shmem_populate(struct ttm_device *bdev, return 0;
err_free_st: - shmem_sg_free_table(st, filp->f_mapping, false, false); + mapping_clear_unevictable(filp->f_mapping); + shmem_sg_free_table(st, false, false);
return err; } @@ -243,8 +244,8 @@ static void i915_ttm_tt_shmem_unpopulate(struct ttm_tt *ttm) bool backup = ttm->page_flags & TTM_TT_FLAG_SWAPPED; struct sg_table *st = &i915_tt->cached_rsgt.table;
- shmem_sg_free_table(st, file_inode(i915_tt->filp)->i_mapping, - backup, backup); + mapping_clear_unevictable(file_inode(i915_tt->filp)->i_mapping); + shmem_sg_free_table(st, backup, backup); }
static void i915_ttm_tt_release(struct kref *ref)
Hello Brian, Many thanks for the fix. I am adding my tested-by. Tested-by: Vidya Srinivas vidya.srinivas@intel.com
-----Original Message----- From: Brian Geffon bgeffon@google.com Sent: 16 January 2025 21:24 To: intel-gfx@lists.freedesktop.org Cc: Wilson, Chris P chris.p.wilson@intel.com; Saarinen, Jani jani.saarinen@intel.com; Mistat, Tomasz tomasz.mistat@intel.com; Srinivas, Vidya vidya.srinivas@intel.com; ville.syrjala@linux.intel.com; jani.nikula@linux.intel.com; linux-kernel@vger.kernel.org; dri- devel@lists.freedesktop.org; Joonas Lahtinen joonas.lahtinen@linux.intel.com; Brian Geffon bgeffon@google.com; stable@vger.kernel.org; Tomasz Figa tfiga@google.com Subject: [PATCH v2] drm/i915: Fix page cleanup on DMA remap failure
When converting to folios the cleanup path of shmem_get_pages() was missed. When a DMA remap fails and the max segment size is greater than PAGE_SIZE it will attempt to retry the remap with a PAGE_SIZEd segment size. The cleanup code isn't properly using the folio apis and as a result isn't handling compound pages correctly.
v1 -> v2: (Ville) Fixed locations where we were not clearing mapping unevictable.
Cc: stable@vger.kernel.org Cc: Ville Syrjala ville.syrjala@linux.intel.com Cc: Vidya Srinivas vidya.srinivas@intel.com Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13487 Link: https://lore.kernel.org/lkml/20250116135636.410164-1- bgeffon@google.com/ Fixes: 0b62af28f249 ("i915: convert shmem_sg_free_table() to use a folio_batch") Signed-off-by: Brian Geffon bgeffon@google.com Suggested-by: Tomasz Figa tfiga@google.com
drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 +-- drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 23 +++++++++------------- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 7 ++++--- 3 files changed, 14 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 3dc61cbd2e11..0f122a12d4a5 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -843,8 +843,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st, size_t size, struct intel_memory_region *mr, struct address_space *mapping, unsigned int max_segment); -void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping,
bool dirty, bool backup);+void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup); void __shmem_writeback(size_t size, struct address_space *mapping);
#ifdef CONFIG_MMU_NOTIFIER diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c index fe69f2c8527d..b320d9dfd6d3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c @@ -29,16 +29,13 @@ static void check_release_folio_batch(struct folio_batch *fbatch) cond_resched(); }
-void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping,
bool dirty, bool backup)+void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup) { struct sgt_iter sgt_iter; struct folio_batch fbatch; struct folio *last = NULL; struct page *page;
- mapping_clear_unevictable(mapping);
- folio_batch_init(&fbatch); for_each_sgt_page(page, sgt_iter, st) { struct folio *folio = page_folio(page); @@ -180,10 +177,10
@@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st, return 0; err_sg: sg_mark_end(sg);
- mapping_clear_unevictable(mapping); if (sg != st->sgl) {
shmem_sg_free_table(st, mapping, false, false);
shmem_sg_free_table(st, false, false);
mapping_clear_unevictable(mapping);@@ -209,8 +206,6 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) struct address_space *mapping = obj->base.filp->f_mapping; unsigned int max_segment = i915_sg_segment_size(i915->drm.dev); struct sg_table *st;
struct sgt_iter sgt_iter;
struct page *page; int ret;
/*
@@ -239,9 +234,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) * for PAGE_SIZE chunks instead may be helpful. */ if (max_segment > PAGE_SIZE) {
for_each_sgt_page(page, sgt_iter, st)put_page(page);sg_free_table(st);
/* Leave the mapping unevictable while we retry */shmem_sg_free_table(st, false, false); kfree(st); max_segment = PAGE_SIZE;@@ -265,7 +259,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) return 0;
err_pages:
- shmem_sg_free_table(st, mapping, false, false);
- mapping_clear_unevictable(mapping);
- shmem_sg_free_table(st, false, false); /*
- shmemfs first checks if there is enough memory to allocate the
page * and reports ENOSPC should there be insufficient, along with the usual @@ -402,8 +397,8 @@ void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_ if (i915_gem_object_needs_bit17_swizzle(obj)) i915_gem_object_save_bit_17_swizzle(obj, pages);
- shmem_sg_free_table(pages, file_inode(obj->base.filp)->i_mapping,
obj->mm.dirty, obj->mm.madv ==I915_MADV_WILLNEED);
- mapping_clear_unevictable(file_inode(obj->base.filp)->i_mapping);
- shmem_sg_free_table(pages, obj->mm.dirty, obj->mm.madv ==
+I915_MADV_WILLNEED); kfree(pages); obj->mm.dirty = false; } diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index 10d8673641f7..37f51a04b838 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -232,7 +232,8 @@ static int i915_ttm_tt_shmem_populate(struct ttm_device *bdev, return 0;
err_free_st:
- shmem_sg_free_table(st, filp->f_mapping, false, false);
mapping_clear_unevictable(filp->f_mapping);
shmem_sg_free_table(st, false, false);
return err;
} @@ -243,8 +244,8 @@ static void i915_ttm_tt_shmem_unpopulate(struct ttm_tt *ttm) bool backup = ttm->page_flags & TTM_TT_FLAG_SWAPPED; struct sg_table *st = &i915_tt->cached_rsgt.table;
- shmem_sg_free_table(st, file_inode(i915_tt->filp)->i_mapping,
backup, backup);
- mapping_clear_unevictable(file_inode(i915_tt->filp)->i_mapping);
- shmem_sg_free_table(st, backup, backup);
}
static void i915_ttm_tt_release(struct kref *ref)
2.48.0.rc2.279.g1de40edade-goog
On Wed, Jan 22, 2025 at 10:07 PM Srinivas, Vidya vidya.srinivas@intel.com wrote:
Hello Brian, Many thanks for the fix. I am adding my tested-by. Tested-by: Vidya Srinivas vidya.srinivas@intel.com
Thanks for testing Vidya.
Can we get a maintainer to take a look?
-----Original Message----- From: Brian Geffon bgeffon@google.com Sent: 16 January 2025 21:24 To: intel-gfx@lists.freedesktop.org Cc: Wilson, Chris P chris.p.wilson@intel.com; Saarinen, Jani jani.saarinen@intel.com; Mistat, Tomasz tomasz.mistat@intel.com; Srinivas, Vidya vidya.srinivas@intel.com; ville.syrjala@linux.intel.com; jani.nikula@linux.intel.com; linux-kernel@vger.kernel.org; dri- devel@lists.freedesktop.org; Joonas Lahtinen joonas.lahtinen@linux.intel.com; Brian Geffon bgeffon@google.com; stable@vger.kernel.org; Tomasz Figa tfiga@google.com Subject: [PATCH v2] drm/i915: Fix page cleanup on DMA remap failure
When converting to folios the cleanup path of shmem_get_pages() was missed. When a DMA remap fails and the max segment size is greater than PAGE_SIZE it will attempt to retry the remap with a PAGE_SIZEd segment size. The cleanup code isn't properly using the folio apis and as a result isn't handling compound pages correctly.
v1 -> v2: (Ville) Fixed locations where we were not clearing mapping unevictable.
Cc: stable@vger.kernel.org Cc: Ville Syrjala ville.syrjala@linux.intel.com Cc: Vidya Srinivas vidya.srinivas@intel.com Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13487 Link: https://lore.kernel.org/lkml/20250116135636.410164-1- bgeffon@google.com/ Fixes: 0b62af28f249 ("i915: convert shmem_sg_free_table() to use a folio_batch") Signed-off-by: Brian Geffon bgeffon@google.com Suggested-by: Tomasz Figa tfiga@google.com
drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 +-- drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 23 +++++++++------------- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 7 ++++--- 3 files changed, 14 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 3dc61cbd2e11..0f122a12d4a5 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -843,8 +843,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st, size_t size, struct intel_memory_region *mr, struct address_space *mapping, unsigned int max_segment); -void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping,
bool dirty, bool backup);+void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup); void __shmem_writeback(size_t size, struct address_space *mapping);
#ifdef CONFIG_MMU_NOTIFIER diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c index fe69f2c8527d..b320d9dfd6d3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c @@ -29,16 +29,13 @@ static void check_release_folio_batch(struct folio_batch *fbatch) cond_resched(); }
-void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping,
bool dirty, bool backup)+void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup) { struct sgt_iter sgt_iter; struct folio_batch fbatch; struct folio *last = NULL; struct page *page;
mapping_clear_unevictable(mapping);folio_batch_init(&fbatch); for_each_sgt_page(page, sgt_iter, st) { struct folio *folio = page_folio(page); @@ -180,10 +177,10@@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st, return 0; err_sg: sg_mark_end(sg);
mapping_clear_unevictable(mapping); if (sg != st->sgl) {
shmem_sg_free_table(st, mapping, false, false);
shmem_sg_free_table(st, false, false); } else {
mapping_clear_unevictable(mapping); sg_free_table(st); }@@ -209,8 +206,6 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) struct address_space *mapping = obj->base.filp->f_mapping; unsigned int max_segment = i915_sg_segment_size(i915->drm.dev); struct sg_table *st;
struct sgt_iter sgt_iter;struct page *page; int ret; /*@@ -239,9 +234,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) * for PAGE_SIZE chunks instead may be helpful. */ if (max_segment > PAGE_SIZE) {
for_each_sgt_page(page, sgt_iter, st)put_page(page);sg_free_table(st);
/* Leave the mapping unevictable while we retry */shmem_sg_free_table(st, false, false); kfree(st); max_segment = PAGE_SIZE;@@ -265,7 +259,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) return 0;
err_pages:
shmem_sg_free_table(st, mapping, false, false);
mapping_clear_unevictable(mapping);shmem_sg_free_table(st, false, false); /* * shmemfs first checks if there is enough memory to allocate thepage * and reports ENOSPC should there be insufficient, along with the usual @@ -402,8 +397,8 @@ void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_ if (i915_gem_object_needs_bit17_swizzle(obj)) i915_gem_object_save_bit_17_swizzle(obj, pages);
shmem_sg_free_table(pages, file_inode(obj->base.filp)->i_mapping,obj->mm.dirty, obj->mm.madv ==I915_MADV_WILLNEED);
mapping_clear_unevictable(file_inode(obj->base.filp)->i_mapping);shmem_sg_free_table(pages, obj->mm.dirty, obj->mm.madv ==+I915_MADV_WILLNEED); kfree(pages); obj->mm.dirty = false; } diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index 10d8673641f7..37f51a04b838 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -232,7 +232,8 @@ static int i915_ttm_tt_shmem_populate(struct ttm_device *bdev, return 0;
err_free_st:
shmem_sg_free_table(st, filp->f_mapping, false, false);
mapping_clear_unevictable(filp->f_mapping);shmem_sg_free_table(st, false, false); return err;} @@ -243,8 +244,8 @@ static void i915_ttm_tt_shmem_unpopulate(struct ttm_tt *ttm) bool backup = ttm->page_flags & TTM_TT_FLAG_SWAPPED; struct sg_table *st = &i915_tt->cached_rsgt.table;
shmem_sg_free_table(st, file_inode(i915_tt->filp)->i_mapping,backup, backup);
mapping_clear_unevictable(file_inode(i915_tt->filp)->i_mapping);shmem_sg_free_table(st, backup, backup);}
static void i915_ttm_tt_release(struct kref *ref)
2.48.0.rc2.279.g1de40edade-goog
Thanks Brian
-----Original Message----- From: Brian Geffon bgeffon@google.com Sent: 24 January 2025 20:50 To: Srinivas, Vidya vidya.srinivas@intel.com Cc: intel-gfx@lists.freedesktop.org; Wilson, Chris P chris.p.wilson@intel.com; Saarinen, Jani jani.saarinen@intel.com; Mistat, Tomasz tomasz.mistat@intel.com; ville.syrjala@linux.intel.com; jani.nikula@linux.intel.com; linux-kernel@vger.kernel.org; dri- devel@lists.freedesktop.org; Joonas Lahtinen joonas.lahtinen@linux.intel.com; stable@vger.kernel.org; Tomasz Figa tfiga@google.com Subject: Re: [PATCH v2] drm/i915: Fix page cleanup on DMA remap failure
On Wed, Jan 22, 2025 at 10:07 PM Srinivas, Vidya vidya.srinivas@intel.com wrote:
Hello Brian, Many thanks for the fix. I am adding my tested-by. Tested-by: Vidya Srinivas vidya.srinivas@intel.com
Thanks for testing Vidya.
Can we get a maintainer to take a look?
Hello Brian, Thank you so much. Tomasz has left a comment on gitlab, I think he will help on review.
Regards Vidya
-----Original Message----- From: Brian Geffon bgeffon@google.com Sent: 16 January 2025 21:24 To: intel-gfx@lists.freedesktop.org Cc: Wilson, Chris P chris.p.wilson@intel.com; Saarinen, Jani jani.saarinen@intel.com; Mistat, Tomasz tomasz.mistat@intel.com; Srinivas, Vidya vidya.srinivas@intel.com; ville.syrjala@linux.intel.com; jani.nikula@linux.intel.com; linux-kernel@vger.kernel.org; dri- devel@lists.freedesktop.org; Joonas Lahtinen joonas.lahtinen@linux.intel.com; Brian Geffon bgeffon@google.com; stable@vger.kernel.org; Tomasz Figa tfiga@google.com Subject: [PATCH v2] drm/i915: Fix page cleanup on DMA remap failure
When converting to folios the cleanup path of shmem_get_pages() was missed. When a DMA remap fails and the max segment size is greater than PAGE_SIZE it will attempt to retry the remap with a PAGE_SIZEd
segment size.
The cleanup code isn't properly using the folio apis and as a result isn't handling compound pages correctly.
v1 -> v2: (Ville) Fixed locations where we were not clearing mapping unevictable.
Cc: stable@vger.kernel.org Cc: Ville Syrjala ville.syrjala@linux.intel.com Cc: Vidya Srinivas vidya.srinivas@intel.com Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13487 Link: https://lore.kernel.org/lkml/20250116135636.410164-1- bgeffon@google.com/ Fixes: 0b62af28f249 ("i915: convert shmem_sg_free_table() to use a folio_batch") Signed-off-by: Brian Geffon bgeffon@google.com Suggested-by: Tomasz Figa tfiga@google.com
drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 +-- drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 23 +++++++++------------
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 7 ++++--- 3 files changed, 14 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 3dc61cbd2e11..0f122a12d4a5 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -843,8 +843,7 @@ int shmem_sg_alloc_table(struct
drm_i915_private
*i915, struct sg_table *st, size_t size, struct intel_memory_region *mr, struct address_space *mapping, unsigned int max_segment); -void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping,
bool dirty, bool backup);+void shmem_sg_free_table(struct sg_table *st, bool dirty, bool +backup); void __shmem_writeback(size_t size, struct address_space *mapping);
#ifdef CONFIG_MMU_NOTIFIER diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c index fe69f2c8527d..b320d9dfd6d3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c @@ -29,16 +29,13 @@ static void check_release_folio_batch(struct folio_batch *fbatch) cond_resched(); }
-void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping,
bool dirty, bool backup)+void shmem_sg_free_table(struct sg_table *st, bool dirty, bool +backup) { struct sgt_iter sgt_iter; struct folio_batch fbatch; struct folio *last = NULL; struct page *page;
mapping_clear_unevictable(mapping);folio_batch_init(&fbatch); for_each_sgt_page(page, sgt_iter, st) { struct folio *folio = page_folio(page); @@ -180,10+177,10 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st, return 0; err_sg: sg_mark_end(sg);
mapping_clear_unevictable(mapping); if (sg != st->sgl) {
shmem_sg_free_table(st, mapping, false, false);
shmem_sg_free_table(st, false, false); } else {
mapping_clear_unevictable(mapping); sg_free_table(st); }@@ -209,8 +206,6 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) struct address_space *mapping = obj->base.filp->f_mapping; unsigned int max_segment = i915_sg_segment_size(i915->drm.dev); struct sg_table *st;
struct sgt_iter sgt_iter;struct page *page; int ret; /*@@ -239,9 +234,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) * for PAGE_SIZE chunks instead may be helpful. */ if (max_segment > PAGE_SIZE) {
for_each_sgt_page(page, sgt_iter, st)put_page(page);sg_free_table(st);
/* Leave the mapping unevictable while we retry */shmem_sg_free_table(st, false, false); kfree(st); max_segment = PAGE_SIZE; @@ -265,7 +259,8 @@static int shmem_get_pages(struct drm_i915_gem_object *obj) return 0;
err_pages:
shmem_sg_free_table(st, mapping, false, false);
mapping_clear_unevictable(mapping);shmem_sg_free_table(st, false, false); /* * shmemfs first checks if there is enough memory to allocatethe page * and reports ENOSPC should there be insufficient, along with the usual @@ -402,8 +397,8 @@ void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_ if (i915_gem_object_needs_bit17_swizzle(obj)) i915_gem_object_save_bit_17_swizzle(obj, pages);
shmem_sg_free_table(pages, file_inode(obj->base.filp)->i_mapping,obj->mm.dirty, obj->mm.madv ==I915_MADV_WILLNEED);
mapping_clear_unevictable(file_inode(obj->base.filp)->i_mapping);shmem_sg_free_table(pages, obj->mm.dirty, obj->mm.madv ==+I915_MADV_WILLNEED); kfree(pages); obj->mm.dirty = false; } diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index 10d8673641f7..37f51a04b838 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -232,7 +232,8 @@ static int i915_ttm_tt_shmem_populate(struct ttm_device *bdev, return 0;
err_free_st:
shmem_sg_free_table(st, filp->f_mapping, false, false);
mapping_clear_unevictable(filp->f_mapping);shmem_sg_free_table(st, false, false); return err;} @@ -243,8 +244,8 @@ static void i915_ttm_tt_shmem_unpopulate(struct ttm_tt *ttm) bool backup = ttm->page_flags & TTM_TT_FLAG_SWAPPED; struct sg_table *st = &i915_tt->cached_rsgt.table;
shmem_sg_free_table(st, file_inode(i915_tt->filp)->i_mapping,backup, backup);
mapping_clear_unevictable(file_inode(i915_tt->filp)->i_mapping);shmem_sg_free_table(st, backup, backup);}
static void i915_ttm_tt_release(struct kref *ref)
2.48.0.rc2.279.g1de40edade-goog
Thanks Brian
On Thu, Jan 16, 2025 at 10:53:40AM -0500, Brian Geffon wrote:
When converting to folios the cleanup path of shmem_get_pages() was missed. When a DMA remap fails and the max segment size is greater than PAGE_SIZE it will attempt to retry the remap with a PAGE_SIZEd segment size. The cleanup code isn't properly using the folio apis and as a result isn't handling compound pages correctly.
v1 -> v2: (Ville) Fixed locations where we were not clearing mapping unevictable.
Cc: stable@vger.kernel.org Cc: Ville Syrjala ville.syrjala@linux.intel.com Cc: Vidya Srinivas vidya.srinivas@intel.com Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13487 Link: https://lore.kernel.org/lkml/20250116135636.410164-1-bgeffon@google.com/ Fixes: 0b62af28f249 ("i915: convert shmem_sg_free_table() to use a folio_batch") Signed-off-by: Brian Geffon bgeffon@google.com Suggested-by: Tomasz Figa tfiga@google.com
drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 +-- drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 23 +++++++++------------- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 7 ++++--- 3 files changed, 14 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 3dc61cbd2e11..0f122a12d4a5 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -843,8 +843,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st, size_t size, struct intel_memory_region *mr, struct address_space *mapping, unsigned int max_segment); -void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping,
bool dirty, bool backup);+void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup); void __shmem_writeback(size_t size, struct address_space *mapping); #ifdef CONFIG_MMU_NOTIFIER diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c index fe69f2c8527d..b320d9dfd6d3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c @@ -29,16 +29,13 @@ static void check_release_folio_batch(struct folio_batch *fbatch) cond_resched(); } -void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping,
bool dirty, bool backup)+void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup)
This still makes the alloc vs. free completely asymmetric. This is not what we want because it just makes it very easy to make it mistake in the caller.
I think the correct fix is to simply call the current shmem_sg_free_table() from the now broken failure path. mapping_{set,clear}_unevictable() just seems to be some bit operation so AFAICS the slight ping-pong should be inconsequential.
{ struct sgt_iter sgt_iter; struct folio_batch fbatch; struct folio *last = NULL; struct page *page;
- mapping_clear_unevictable(mapping);
- folio_batch_init(&fbatch); for_each_sgt_page(page, sgt_iter, st) { struct folio *folio = page_folio(page);
@@ -180,10 +177,10 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st, return 0; err_sg: sg_mark_end(sg);
- mapping_clear_unevictable(mapping); if (sg != st->sgl) {
shmem_sg_free_table(st, mapping, false, false);
shmem_sg_free_table(st, false, false);
mapping_clear_unevictable(mapping);@@ -209,8 +206,6 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) struct address_space *mapping = obj->base.filp->f_mapping; unsigned int max_segment = i915_sg_segment_size(i915->drm.dev); struct sg_table *st;
- struct sgt_iter sgt_iter;
- struct page *page; int ret;
/* @@ -239,9 +234,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) * for PAGE_SIZE chunks instead may be helpful. */ if (max_segment > PAGE_SIZE) {
for_each_sgt_page(page, sgt_iter, st)put_page(page);sg_free_table(st);
/* Leave the mapping unevictable while we retry */shmem_sg_free_table(st, false, false); kfree(st);max_segment = PAGE_SIZE; @@ -265,7 +259,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) return 0; err_pages:
- shmem_sg_free_table(st, mapping, false, false);
- mapping_clear_unevictable(mapping);
- shmem_sg_free_table(st, false, false); /*
- shmemfs first checks if there is enough memory to allocate the page
- and reports ENOSPC should there be insufficient, along with the usual
@@ -402,8 +397,8 @@ void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_ if (i915_gem_object_needs_bit17_swizzle(obj)) i915_gem_object_save_bit_17_swizzle(obj, pages);
- shmem_sg_free_table(pages, file_inode(obj->base.filp)->i_mapping,
obj->mm.dirty, obj->mm.madv == I915_MADV_WILLNEED);
- mapping_clear_unevictable(file_inode(obj->base.filp)->i_mapping);
- shmem_sg_free_table(pages, obj->mm.dirty, obj->mm.madv == I915_MADV_WILLNEED); kfree(pages); obj->mm.dirty = false;
} diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index 10d8673641f7..37f51a04b838 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -232,7 +232,8 @@ static int i915_ttm_tt_shmem_populate(struct ttm_device *bdev, return 0; err_free_st:
- shmem_sg_free_table(st, filp->f_mapping, false, false);
- mapping_clear_unevictable(filp->f_mapping);
- shmem_sg_free_table(st, false, false);
return err; } @@ -243,8 +244,8 @@ static void i915_ttm_tt_shmem_unpopulate(struct ttm_tt *ttm) bool backup = ttm->page_flags & TTM_TT_FLAG_SWAPPED; struct sg_table *st = &i915_tt->cached_rsgt.table;
- shmem_sg_free_table(st, file_inode(i915_tt->filp)->i_mapping,
backup, backup);
- mapping_clear_unevictable(file_inode(i915_tt->filp)->i_mapping);
- shmem_sg_free_table(st, backup, backup);
} static void i915_ttm_tt_release(struct kref *ref) -- 2.48.0.rc2.279.g1de40edade-goog
On Mon, Jan 27, 2025 at 1:47 PM Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Thu, Jan 16, 2025 at 10:53:40AM -0500, Brian Geffon wrote:
When converting to folios the cleanup path of shmem_get_pages() was missed. When a DMA remap fails and the max segment size is greater than PAGE_SIZE it will attempt to retry the remap with a PAGE_SIZEd segment size. The cleanup code isn't properly using the folio apis and as a result isn't handling compound pages correctly.
v1 -> v2: (Ville) Fixed locations where we were not clearing mapping unevictable.
Cc: stable@vger.kernel.org Cc: Ville Syrjala ville.syrjala@linux.intel.com Cc: Vidya Srinivas vidya.srinivas@intel.com Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13487 Link: https://lore.kernel.org/lkml/20250116135636.410164-1-bgeffon@google.com/ Fixes: 0b62af28f249 ("i915: convert shmem_sg_free_table() to use a folio_batch") Signed-off-by: Brian Geffon bgeffon@google.com Suggested-by: Tomasz Figa tfiga@google.com
drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 +-- drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 23 +++++++++------------- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 7 ++++--- 3 files changed, 14 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 3dc61cbd2e11..0f122a12d4a5 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -843,8 +843,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st, size_t size, struct intel_memory_region *mr, struct address_space *mapping, unsigned int max_segment); -void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping,
bool dirty, bool backup);+void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup); void __shmem_writeback(size_t size, struct address_space *mapping);
#ifdef CONFIG_MMU_NOTIFIER diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c index fe69f2c8527d..b320d9dfd6d3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c @@ -29,16 +29,13 @@ static void check_release_folio_batch(struct folio_batch *fbatch) cond_resched(); }
-void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping,
bool dirty, bool backup)+void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup)
This still makes the alloc vs. free completely asymmetric. This is not what we want because it just makes it very easy to make it mistake in the caller.
I think the correct fix is to simply call the current shmem_sg_free_table() from the now broken failure path. mapping_{set,clear}_unevictable() just seems to be some bit operation so AFAICS the slight ping-pong should be inconsequential.
Ok, I guess that's true, it'll be set unevictable again in the call to shmem_sg_alloc_table() after it jumps back to rebuild_st. I'll send a v3 which should then become a 1 line change.
{ struct sgt_iter sgt_iter; struct folio_batch fbatch; struct folio *last = NULL; struct page *page;
mapping_clear_unevictable(mapping);folio_batch_init(&fbatch); for_each_sgt_page(page, sgt_iter, st) { struct folio *folio = page_folio(page);@@ -180,10 +177,10 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st, return 0; err_sg: sg_mark_end(sg);
mapping_clear_unevictable(mapping); if (sg != st->sgl) {
shmem_sg_free_table(st, mapping, false, false);
shmem_sg_free_table(st, false, false); } else {
mapping_clear_unevictable(mapping); sg_free_table(st); }@@ -209,8 +206,6 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) struct address_space *mapping = obj->base.filp->f_mapping; unsigned int max_segment = i915_sg_segment_size(i915->drm.dev); struct sg_table *st;
struct sgt_iter sgt_iter;struct page *page; int ret; /*@@ -239,9 +234,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) * for PAGE_SIZE chunks instead may be helpful. */ if (max_segment > PAGE_SIZE) {
for_each_sgt_page(page, sgt_iter, st)put_page(page);sg_free_table(st);
/* Leave the mapping unevictable while we retry */shmem_sg_free_table(st, false, false); kfree(st); max_segment = PAGE_SIZE;@@ -265,7 +259,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) return 0;
err_pages:
shmem_sg_free_table(st, mapping, false, false);
mapping_clear_unevictable(mapping);shmem_sg_free_table(st, false, false); /* * shmemfs first checks if there is enough memory to allocate the page * and reports ENOSPC should there be insufficient, along with the usual@@ -402,8 +397,8 @@ void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_ if (i915_gem_object_needs_bit17_swizzle(obj)) i915_gem_object_save_bit_17_swizzle(obj, pages);
shmem_sg_free_table(pages, file_inode(obj->base.filp)->i_mapping,obj->mm.dirty, obj->mm.madv == I915_MADV_WILLNEED);
mapping_clear_unevictable(file_inode(obj->base.filp)->i_mapping);shmem_sg_free_table(pages, obj->mm.dirty, obj->mm.madv == I915_MADV_WILLNEED); kfree(pages); obj->mm.dirty = false;} diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index 10d8673641f7..37f51a04b838 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -232,7 +232,8 @@ static int i915_ttm_tt_shmem_populate(struct ttm_device *bdev, return 0;
err_free_st:
shmem_sg_free_table(st, filp->f_mapping, false, false);
mapping_clear_unevictable(filp->f_mapping);shmem_sg_free_table(st, false, false); return err;} @@ -243,8 +244,8 @@ static void i915_ttm_tt_shmem_unpopulate(struct ttm_tt *ttm) bool backup = ttm->page_flags & TTM_TT_FLAG_SWAPPED; struct sg_table *st = &i915_tt->cached_rsgt.table;
shmem_sg_free_table(st, file_inode(i915_tt->filp)->i_mapping,backup, backup);
mapping_clear_unevictable(file_inode(i915_tt->filp)->i_mapping);shmem_sg_free_table(st, backup, backup);}
static void i915_ttm_tt_release(struct kref *ref)
2.48.0.rc2.279.g1de40edade-goog
-- Ville Syrjälä Intel
Thanks, Brian
linux-stable-mirror@lists.linaro.org
-
Brian Geffon -
Srinivas, Vidya -
Ville Syrjälä