| Message ID | 20230623200113.62051-1-Yunxiang.Li@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] dma-buf: allow nested dma_resv_reserve_fences | expand |
Hi Yunxiang, kernel test robot noticed the following build errors: url: https://github.com/intel-lab-lkp/linux/commits/UPDATE-20230624-040209/Yunxiang-Li/drm-amdgpu-fix-missing-fence-reserve-in-amdgpu_vm_sdma_commit/20230622-002915 base: the 2th patch of https://lore.kernel.org/r/20230621162652.10875-3-Yunxiang.Li%40amd.com patch link: https://lore.kernel.org/r/20230623200113.62051-1-Yunxiang.Li%40amd.com patch subject: [PATCH v2] dma-buf: allow nested dma_resv_reserve_fences config: m68k-randconfig-r002-20230622 (https://download.01.org/0day-ci/archive/20230624/202306240538.Mz3kjtT7-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 12.3.0 reproduce: (https://download.01.org/0day-ci/archive/20230624/202306240538.Mz3kjtT7-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202306240538.Mz3kjtT7-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/dma-buf/dma-resv.c: In function 'dma_resv_add_fence': >> drivers/dma-buf/dma-resv.c:326: error: unterminated #else 326 | #ifdef CONFIG_DEBUG_MUTEXES | drivers/dma-buf/dma-resv.c:327:9: note: '-Wmisleading-indentation' is disabled from this point onwards, since column-tracking was disabled due to the size of the code/headers 327 | else | ^~~~ drivers/dma-buf/dma-resv.c:327:9: note: adding '-flarge-source-files' will allow for more column-tracking support, at the expense of compilation time and memory >> drivers/dma-buf/dma-resv.c:328:17: error: expected declaration or statement at end of input 328 | WARN_ON(1); // missing fence slot allocation | ^~~~~~~ vim +326 drivers/dma-buf/dma-resv.c 274 275 /** 276 * dma_resv_add_fence - Add a fence to the dma_resv obj 277 * @obj: the reservation object 278 * @fence: the fence to add 279 * @usage: how the fence is used, see enum dma_resv_usage 280 * 281 * Add a fence to a slot, @obj must be locked with dma_resv_lock(), and 282 * dma_resv_reserve_fences() has been called. 283 * 284 * See also &dma_resv.fence for a discussion of the semantics. 285 */ 286 void dma_resv_add_fence(struct dma_resv *obj, struct dma_fence *fence, 287 enum dma_resv_usage usage) 288 { 289 struct dma_resv_list *fobj; 290 struct dma_fence *old; 291 unsigned int i, count; 292 293 dma_fence_get(fence); 294 295 dma_resv_assert_held(obj); 296 297 /* Drivers should not add containers here, instead add each fence 298 * individually. 299 */ 300 WARN_ON(dma_fence_is_container(fence)); 301 302 retry: 303 fobj = dma_resv_fences_list(obj); 304 count = fobj->num_fences; 305 306 for (i = 0; i < count; ++i) { 307 enum dma_resv_usage old_usage; 308 309 dma_resv_list_entry(fobj, i, obj, &old, &old_usage); 310 if ((old->context == fence->context && old_usage >= usage && 311 dma_fence_is_later(fence, old)) || 312 dma_fence_is_signaled(old)) { 313 dma_resv_list_set(fobj, i, fence, usage); 314 dma_fence_put(old); 315 return; 316 } 317 } 318 319 if (WARN_ON(fobj->num_fences == fobj->max_fences)) { 320 // try our best to avoid memory corruption 321 dma_resv_reserve_fences(obj, 1); 322 goto retry; 323 } 324 if (fobj->reserved_fences) 325 fobj->reserved_fences -= 1; > 326 #ifdef CONFIG_DEBUG_MUTEXES 327 else > 328 WARN_ON(1); // missing fence slot allocation 329 #else 330 count++; 331 332 dma_resv_list_set(fobj, i, fence, usage); 333 /* pointer update must be visible before we extend the num_fences */ 334 smp_store_mb(fobj->num_fences, count); 335 } 336 EXPORT_SYMBOL(dma_resv_add_fence); 337
Hi Yunxiang, kernel test robot noticed the following build errors: url: https://github.com/intel-lab-lkp/linux/commits/UPDATE-20230624-040209/Yunxiang-Li/drm-amdgpu-fix-missing-fence-reserve-in-amdgpu_vm_sdma_commit/20230622-002915 base: the 2th patch of https://lore.kernel.org/r/20230621162652.10875-3-Yunxiang.Li%40amd.com patch link: https://lore.kernel.org/r/20230623200113.62051-1-Yunxiang.Li%40amd.com patch subject: [PATCH v2] dma-buf: allow nested dma_resv_reserve_fences config: s390-randconfig-r016-20230621 (https://download.01.org/0day-ci/archive/20230624/202306240524.WBCHWUlJ-lkp@intel.com/config) compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a) reproduce: (https://download.01.org/0day-ci/archive/20230624/202306240524.WBCHWUlJ-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202306240524.WBCHWUlJ-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/dma-buf/dma-resv.c:326:2: error: unterminated conditional directive #ifdef CONFIG_DEBUG_MUTEXES ^ >> drivers/dma-buf/dma-resv.c:817:7: error: expected '}' #endif ^ drivers/dma-buf/dma-resv.c:288:1: note: to match this '{' { ^ 2 errors generated. vim +326 drivers/dma-buf/dma-resv.c 274 275 /** 276 * dma_resv_add_fence - Add a fence to the dma_resv obj 277 * @obj: the reservation object 278 * @fence: the fence to add 279 * @usage: how the fence is used, see enum dma_resv_usage 280 * 281 * Add a fence to a slot, @obj must be locked with dma_resv_lock(), and 282 * dma_resv_reserve_fences() has been called. 283 * 284 * See also &dma_resv.fence for a discussion of the semantics. 285 */ 286 void dma_resv_add_fence(struct dma_resv *obj, struct dma_fence *fence, 287 enum dma_resv_usage usage) 288 { 289 struct dma_resv_list *fobj; 290 struct dma_fence *old; 291 unsigned int i, count; 292 293 dma_fence_get(fence); 294 295 dma_resv_assert_held(obj); 296 297 /* Drivers should not add containers here, instead add each fence 298 * individually. 299 */ 300 WARN_ON(dma_fence_is_container(fence)); 301 302 retry: 303 fobj = dma_resv_fences_list(obj); 304 count = fobj->num_fences; 305 306 for (i = 0; i < count; ++i) { 307 enum dma_resv_usage old_usage; 308 309 dma_resv_list_entry(fobj, i, obj, &old, &old_usage); 310 if ((old->context == fence->context && old_usage >= usage && 311 dma_fence_is_later(fence, old)) || 312 dma_fence_is_signaled(old)) { 313 dma_resv_list_set(fobj, i, fence, usage); 314 dma_fence_put(old); 315 return; 316 } 317 } 318 319 if (WARN_ON(fobj->num_fences == fobj->max_fences)) { 320 // try our best to avoid memory corruption 321 dma_resv_reserve_fences(obj, 1); 322 goto retry; 323 } 324 if (fobj->reserved_fences) 325 fobj->reserved_fences -= 1; > 326 #ifdef CONFIG_DEBUG_MUTEXES 327 else 328 WARN_ON(1); // missing fence slot allocation 329 #else 330 count++; 331 332 dma_resv_list_set(fobj, i, fence, usage); 333 /* pointer update must be visible before we extend the num_fences */ 334 smp_store_mb(fobj->num_fences, count); 335 } 336 EXPORT_SYMBOL(dma_resv_add_fence); 337
Hi Yunxiang,
kernel test robot noticed the following build errors:
url: https://github.com/intel-lab-lkp/linux/commits/UPDATE-20230624-040209/Yunxiang-Li/drm-amdgpu-fix-missing-fence-reserve-in-amdgpu_vm_sdma_commit/20230622-002915
base: the 2th patch of https://lore.kernel.org/r/20230621162652.10875-3-Yunxiang.Li%40amd.com
patch link: https://lore.kernel.org/r/20230623200113.62051-1-Yunxiang.Li%40amd.com
patch subject: [PATCH v2] dma-buf: allow nested dma_resv_reserve_fences
config: arc-randconfig-r006-20230622 (https://download.01.org/0day-ci/archive/20230624/202306240508.nAff52YL-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230624/202306240508.nAff52YL-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306240508.nAff52YL-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/dma-buf/dma-resv.c: In function 'dma_resv_add_fence':
drivers/dma-buf/dma-resv.c:326: error: unterminated #else
326 | #ifdef CONFIG_DEBUG_MUTEXES
|
drivers/dma-buf/dma-resv.c:327:9: note: '-Wmisleading-indentation' is disabled from this point onwards, since column-tracking was disabled due to the size of the code/headers
327 | else
| ^~~~
drivers/dma-buf/dma-resv.c:327:9: note: adding '-flarge-source-files' will allow for more column-tracking support, at the expense of compilation time and memory
In file included from include/linux/build_bug.h:5,
from include/linux/container_of.h:5,
from include/linux/list.h:5,
from include/linux/mutex.h:15,
from include/linux/ww_mutex.h:20,
from include/linux/dma-resv.h:42,
from drivers/dma-buf/dma-resv.c:36:
>> include/linux/compiler.h:24:39: error: expected declaration or statement at end of input
24 | static struct ftrace_likely_data \
| ^~~~~~~~~~~~~~~~~~
include/linux/compiler.h:47:26: note: in expansion of macro '__branch_check__'
47 | # define unlikely(x) (__branch_check__(x, 0, __builtin_constant_p(x)))
| ^~~~~~~~~~~~~~~~
include/asm-generic/bug.h:125:9: note: in expansion of macro 'unlikely'
125 | unlikely(__ret_warn_on); \
| ^~~~~~~~
drivers/dma-buf/dma-resv.c:328:17: note: in expansion of macro 'WARN_ON'
328 | WARN_ON(1); // missing fence slot allocation
| ^~~~~~~
vim +24 include/linux/compiler.h
1f0d69a9fc815d Steven Rostedt 2008-11-12 21
d45ae1f7041ac5 Steven Rostedt (VMware 2017-01-17 22) #define __branch_check__(x, expect, is_constant) ({ \
2026d35741f2c3 Mikulas Patocka 2018-05-30 23 long ______r; \
134e6a034cb004 Steven Rostedt (VMware 2017-01-19 @24) static struct ftrace_likely_data \
e04462fb82f8dd Miguel Ojeda 2018-09-03 25 __aligned(4) \
33def8498fdde1 Joe Perches 2020-10-21 26 __section("_ftrace_annotated_branch") \
1f0d69a9fc815d Steven Rostedt 2008-11-12 27 ______f = { \
134e6a034cb004 Steven Rostedt (VMware 2017-01-19 28) .data.func = __func__, \
134e6a034cb004 Steven Rostedt (VMware 2017-01-19 29) .data.file = __FILE__, \
134e6a034cb004 Steven Rostedt (VMware 2017-01-19 30) .data.line = __LINE__, \
1f0d69a9fc815d Steven Rostedt 2008-11-12 31 }; \
d45ae1f7041ac5 Steven Rostedt (VMware 2017-01-17 32) ______r = __builtin_expect(!!(x), expect); \
d45ae1f7041ac5 Steven Rostedt (VMware 2017-01-17 33) ftrace_likely_update(&______f, ______r, \
d45ae1f7041ac5 Steven Rostedt (VMware 2017-01-17 34) expect, is_constant); \
1f0d69a9fc815d Steven Rostedt 2008-11-12 35 ______r; \
1f0d69a9fc815d Steven Rostedt 2008-11-12 36 })
1f0d69a9fc815d Steven Rostedt 2008-11-12 37
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index b6f71eb00866..d9a1831ae7f9 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -62,7 +62,7 @@ EXPORT_SYMBOL(reservation_ww_class); struct dma_resv_list { struct rcu_head rcu; - u32 num_fences, max_fences; + u32 num_fences, reserved_fences, max_fences; struct dma_fence __rcu *table[]; }; @@ -107,6 +107,8 @@ static struct dma_resv_list *dma_resv_list_alloc(unsigned int max_fences) if (!list) return NULL; + list->num_fences = 0; + list->reserved_fences = 0; /* Given the resulting bucket size, recalculated max_fences. */ list->max_fences = (size - offsetof(typeof(*list), table)) / sizeof(*list->table); @@ -173,7 +175,7 @@ static inline struct dma_resv_list *dma_resv_fences_list(struct dma_resv *obj) * locked through dma_resv_lock(). * * Note that the preallocated slots need to be re-reserved if @obj is unlocked - * at any time before calling dma_resv_add_fence(). This is validated when + * at any time before calling dma_resv_add_fence(). This produces a warning when * CONFIG_DEBUG_MUTEXES is enabled. * * RETURNS @@ -182,22 +184,27 @@ static inline struct dma_resv_list *dma_resv_fences_list(struct dma_resv *obj) int dma_resv_reserve_fences(struct dma_resv *obj, unsigned int num_fences) { struct dma_resv_list *old, *new; - unsigned int i, j, k, max; + unsigned int i, j, k, new_max; dma_resv_assert_held(obj); old = dma_resv_fences_list(obj); if (old && old->max_fences) { - if ((old->num_fences + num_fences) <= old->max_fences) + num_fences += old->reserved_fences; + if ((old->num_fences + num_fences) <= old->max_fences) { + old->reserved_fences = num_fences; return 0; - max = max(old->num_fences + num_fences, old->max_fences * 2); + } + new_max = + max(old->num_fences + num_fences, old->max_fences * 2); } else { - max = max(4ul, roundup_pow_of_two(num_fences)); + new_max = max(num_fences, 4u); } - new = dma_resv_list_alloc(max); + new = dma_resv_list_alloc(new_max); if (!new) return -ENOMEM; + new_max = new->max_fences; /* * no need to bump fence refcounts, rcu_read access @@ -205,7 +212,7 @@ int dma_resv_reserve_fences(struct dma_resv *obj, unsigned int num_fences) * references from the old struct are carried over to * the new. */ - for (i = 0, j = 0, k = max; i < (old ? old->num_fences : 0); ++i) { + for (i = 0, j = 0, k = new_max; i < (old ? old->num_fences : 0); ++i) { enum dma_resv_usage usage; struct dma_fence *fence; @@ -216,6 +223,7 @@ int dma_resv_reserve_fences(struct dma_resv *obj, unsigned int num_fences) dma_resv_list_set(new, j++, fence, usage); } new->num_fences = j; + new->reserved_fences = num_fences; /* * We are not changing the effective set of fences here so can @@ -231,7 +239,7 @@ int dma_resv_reserve_fences(struct dma_resv *obj, unsigned int num_fences) return 0; /* Drop the references to the signaled fences */ - for (i = k; i < max; ++i) { + for (i = k; i < new_max; ++i) { struct dma_fence *fence; fence = rcu_dereference_protected(new->table[i], @@ -244,27 +252,25 @@ int dma_resv_reserve_fences(struct dma_resv *obj, unsigned int num_fences) } EXPORT_SYMBOL(dma_resv_reserve_fences); -#ifdef CONFIG_DEBUG_MUTEXES /** - * dma_resv_reset_max_fences - reset fences for debugging + * dma_resv_reset_reserved_fences - reset fence reservation * @obj: the dma_resv object to reset * - * Reset the number of pre-reserved fence slots to test that drivers do + * Reset the number of pre-reserved fence slots to make sure that drivers do * correct slot allocation using dma_resv_reserve_fences(). See also - * &dma_resv_list.max_fences. + * &dma_resv_list.reserved_fences. */ -void dma_resv_reset_max_fences(struct dma_resv *obj) +void dma_resv_reset_reserved_fences(struct dma_resv *obj) { struct dma_resv_list *fences = dma_resv_fences_list(obj); dma_resv_assert_held(obj); - /* Test fence slot reservation */ + /* reset fence slot reservation */ if (fences) - fences->max_fences = fences->num_fences; + fences->reserved_fences = 0; } -EXPORT_SYMBOL(dma_resv_reset_max_fences); -#endif +EXPORT_SYMBOL(dma_resv_reset_reserved_fences); /** * dma_resv_add_fence - Add a fence to the dma_resv obj @@ -293,6 +299,7 @@ void dma_resv_add_fence(struct dma_resv *obj, struct dma_fence *fence, */ WARN_ON(dma_fence_is_container(fence)); +retry: fobj = dma_resv_fences_list(obj); count = fobj->num_fences; @@ -309,7 +316,17 @@ void dma_resv_add_fence(struct dma_resv *obj, struct dma_fence *fence, } } - BUG_ON(fobj->num_fences >= fobj->max_fences); + if (WARN_ON(fobj->num_fences == fobj->max_fences)) { + // try our best to avoid memory corruption + dma_resv_reserve_fences(obj, 1); + goto retry; + } + if (fobj->reserved_fences) + fobj->reserved_fences -= 1; +#ifdef CONFIG_DEBUG_MUTEXES + else + WARN_ON(1); // missing fence slot allocation +#else count++; dma_resv_list_set(fobj, i, fence, usage); @@ -531,7 +548,6 @@ int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src) dma_resv_iter_end(&cursor); return -ENOMEM; } - list->num_fences = 0; } dma_fence_get(f); diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index 8d0e34dad446..9b2d76484ff4 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -311,11 +311,7 @@ static inline bool dma_resv_iter_is_restarted(struct dma_resv_iter *cursor) #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base) #define dma_resv_assert_held(obj) lockdep_assert_held(&(obj)->lock.base) -#ifdef CONFIG_DEBUG_MUTEXES -void dma_resv_reset_max_fences(struct dma_resv *obj); -#else -static inline void dma_resv_reset_max_fences(struct dma_resv *obj) {} -#endif +void dma_resv_reset_reserved_fences(struct dma_resv *obj); /** * dma_resv_lock - lock the reservation object @@ -460,7 +456,7 @@ static inline struct ww_acquire_ctx *dma_resv_locking_ctx(struct dma_resv *obj) */ static inline void dma_resv_unlock(struct dma_resv *obj) { - dma_resv_reset_max_fences(obj); + dma_resv_reset_reserved_fences(obj); ww_mutex_unlock(&obj->lock); }
Calling dma_resv_reserve_fences a second time would not reserve memory correctly, this is quite unintuitive and the current debug build option cannot catch this error reliably because of the slack in max_fences. Rework the function to allow multiple invocations, also make reserve check stricter. This fixes issue where ttm_bo_mem_space's reserve is ignored in various amdgpu ioctl paths, and was causing soft-lockup when VRAM is exhausted. v2: try to avoid memory corruption if possible. Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com> --- drivers/dma-buf/dma-resv.c | 56 ++++++++++++++++++++++++-------------- include/linux/dma-resv.h | 8 ++---- 2 files changed, 38 insertions(+), 26 deletions(-)