| Message ID | 20240216131446.101961-4-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TTM unlockable restartable LRU list iteration | expand |
Hi Thomas, kernel test robot noticed the following build warnings: [auto build test WARNING on drm-misc/drm-misc-next] [also build test WARNING on drm-tip/drm-tip next-20240216] [cannot apply to drm-intel/for-linux-next drm-intel/for-linux-next-fixes linus/master v6.8-rc4] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Thomas-Hellstr-m/drm-ttm-Allow-TTM-LRU-list-nodes-of-different-types/20240216-211801 base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next patch link: https://lore.kernel.org/r/20240216131446.101961-4-thomas.hellstrom%40linux.intel.com patch subject: [PATCH 3/4] drm/ttm: Consider hitch moves within bulk sublist moves config: arm-defconfig (https://download.01.org/0day-ci/archive/20240217/202402170714.cflOUDkj-lkp@intel.com/config) compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240217/202402170714.cflOUDkj-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/202402170714.cflOUDkj-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/gpu/drm/ttm/ttm_resource.c:545:20: warning: incompatible pointer to integer conversion assigning to 'u64' (aka 'unsigned long long') from 'u64 *' (aka 'unsigned long long *'); remove & [-Wint-conversion] cursor->bulk_age = &bulk->age; ^ ~~~~~~~~~~ >> drivers/gpu/drm/ttm/ttm_resource.c:551:23: warning: comparison between pointer and integer ('u64' (aka 'unsigned long long') and 'u64 *' (aka 'unsigned long long *')) [-Wpointer-integer-compare] if (cursor->bulk_age == &bulk->age) ~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~ 2 warnings generated. vim +545 drivers/gpu/drm/ttm/ttm_resource.c 517 518 /* Adjust to a bulk sublist being bumped while traversing it.*/ 519 static bool 520 ttm_resource_cursor_check_bulk(struct ttm_resource_cursor *cursor, 521 struct ttm_lru_item *next_lru) 522 { 523 struct ttm_resource *next = ttm_lru_item_to_res(next_lru); 524 struct ttm_lru_bulk_move *bulk = NULL; 525 struct ttm_buffer_object *bo = next->bo; 526 527 lockdep_assert_held(&cursor->man->bdev->lru_lock); 528 if (bo && bo->resource == next) 529 bulk = bo->bulk_move; 530 531 if (!bulk) { 532 ttm_resource_cursor_clear_bulk(cursor); 533 return false; 534 } 535 536 /* 537 * We encountered a bulk sublist. Record its age and 538 * set a hitch after the sublist. 539 */ 540 if (cursor->bulk != bulk) { 541 struct ttm_lru_bulk_move_pos *pos = 542 ttm_lru_bulk_move_pos(bulk, next); 543 544 cursor->bulk = bulk; > 545 cursor->bulk_age = &bulk->age; 546 list_move(&cursor->bulk_hitch.link, &pos->last->lru.link); 547 return false; 548 } 549 550 /* Continue iterating down the bulk sublist */ > 551 if (cursor->bulk_age == &bulk->age) 552 return false; 553 554 /* 555 * The bulk sublist in which we had a hitch has moved and the 556 * hitch moved with it. Restart iteration from a previously 557 * set hitch after the bulk_move, and remove that backup 558 * hitch. 559 */ 560 list_move(&cursor->hitch.link, &cursor->bulk_hitch.link); 561 ttm_resource_cursor_clear_bulk(cursor); 562 563 return true; 564 } 565
Hi Thomas, kernel test robot noticed the following build warnings: [auto build test WARNING on drm-misc/drm-misc-next] [also build test WARNING on drm-tip/drm-tip next-20240216] [cannot apply to drm-intel/for-linux-next drm-intel/for-linux-next-fixes linus/master v6.8-rc4] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Thomas-Hellstr-m/drm-ttm-Allow-TTM-LRU-list-nodes-of-different-types/20240216-211801 base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next patch link: https://lore.kernel.org/r/20240216131446.101961-4-thomas.hellstrom%40linux.intel.com patch subject: [PATCH 3/4] drm/ttm: Consider hitch moves within bulk sublist moves config: x86_64-randconfig-121-20240217 (https://download.01.org/0day-ci/archive/20240218/202402180043.xVwOJQs2-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240218/202402180043.xVwOJQs2-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/202402180043.xVwOJQs2-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/gpu/drm/ttm/ttm_resource.c:545:34: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned long long [usertype] bulk_age @@ got unsigned long long * @@ drivers/gpu/drm/ttm/ttm_resource.c:545:34: sparse: expected unsigned long long [usertype] bulk_age drivers/gpu/drm/ttm/ttm_resource.c:545:34: sparse: got unsigned long long * >> drivers/gpu/drm/ttm/ttm_resource.c:551:30: sparse: sparse: incompatible types for operation (==): drivers/gpu/drm/ttm/ttm_resource.c:551:30: sparse: unsigned long long [usertype] bulk_age drivers/gpu/drm/ttm/ttm_resource.c:551:30: sparse: unsigned long long * drivers/gpu/drm/ttm/ttm_resource.c: note: in included file (through include/linux/fwnode.h, include/linux/logic_pio.h, include/asm-generic/io.h, ...): include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true vim +545 drivers/gpu/drm/ttm/ttm_resource.c 517 518 /* Adjust to a bulk sublist being bumped while traversing it.*/ 519 static bool 520 ttm_resource_cursor_check_bulk(struct ttm_resource_cursor *cursor, 521 struct ttm_lru_item *next_lru) 522 { 523 struct ttm_resource *next = ttm_lru_item_to_res(next_lru); 524 struct ttm_lru_bulk_move *bulk = NULL; 525 struct ttm_buffer_object *bo = next->bo; 526 527 lockdep_assert_held(&cursor->man->bdev->lru_lock); 528 if (bo && bo->resource == next) 529 bulk = bo->bulk_move; 530 531 if (!bulk) { 532 ttm_resource_cursor_clear_bulk(cursor); 533 return false; 534 } 535 536 /* 537 * We encountered a bulk sublist. Record its age and 538 * set a hitch after the sublist. 539 */ 540 if (cursor->bulk != bulk) { 541 struct ttm_lru_bulk_move_pos *pos = 542 ttm_lru_bulk_move_pos(bulk, next); 543 544 cursor->bulk = bulk; > 545 cursor->bulk_age = &bulk->age; 546 list_move(&cursor->bulk_hitch.link, &pos->last->lru.link); 547 return false; 548 } 549 550 /* Continue iterating down the bulk sublist */ > 551 if (cursor->bulk_age == &bulk->age) 552 return false; 553 554 /* 555 * The bulk sublist in which we had a hitch has moved and the 556 * hitch moved with it. Restart iteration from a previously 557 * set hitch after the bulk_move, and remove that backup 558 * hitch. 559 */ 560 list_move(&cursor->hitch.link, &cursor->bulk_hitch.link); 561 ttm_resource_cursor_clear_bulk(cursor); 562 563 return true; 564 } 565
diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c index 911364e0a5fd..3139c27c9262 100644 --- a/drivers/gpu/drm/ttm/ttm_resource.c +++ b/drivers/gpu/drm/ttm/ttm_resource.c @@ -32,6 +32,14 @@ #include <drm/drm_util.h> +/* Detach the cursor's bulk hitch from the LRU list */ +static void +ttm_resource_cursor_clear_bulk(struct ttm_resource_cursor *cursor) +{ + cursor->bulk = NULL; + list_del_init(&cursor->bulk_hitch.link); +} + /** * ttm_resource_cursor_fini_locked() - Finalize the LRU list cursor usage * @cursor: The struct ttm_resource_cursor to finalize. @@ -44,6 +52,7 @@ void ttm_resource_cursor_fini_locked(struct ttm_resource_cursor *cursor) { lockdep_assert_held(&cursor->man->bdev->lru_lock); list_del_init(&cursor->hitch.link); + ttm_resource_cursor_clear_bulk(cursor); } /** @@ -104,6 +113,7 @@ void ttm_lru_bulk_move_tail(struct ttm_lru_bulk_move *bulk) &pos->last->lru.link); } } + bulk->age++; } EXPORT_SYMBOL(ttm_lru_bulk_move_tail); @@ -505,6 +515,54 @@ void ttm_resource_manager_debug(struct ttm_resource_manager *man, } EXPORT_SYMBOL(ttm_resource_manager_debug); +/* Adjust to a bulk sublist being bumped while traversing it.*/ +static bool +ttm_resource_cursor_check_bulk(struct ttm_resource_cursor *cursor, + struct ttm_lru_item *next_lru) +{ + struct ttm_resource *next = ttm_lru_item_to_res(next_lru); + struct ttm_lru_bulk_move *bulk = NULL; + struct ttm_buffer_object *bo = next->bo; + + lockdep_assert_held(&cursor->man->bdev->lru_lock); + if (bo && bo->resource == next) + bulk = bo->bulk_move; + + if (!bulk) { + ttm_resource_cursor_clear_bulk(cursor); + return false; + } + + /* + * We encountered a bulk sublist. Record its age and + * set a hitch after the sublist. + */ + if (cursor->bulk != bulk) { + struct ttm_lru_bulk_move_pos *pos = + ttm_lru_bulk_move_pos(bulk, next); + + cursor->bulk = bulk; + cursor->bulk_age = &bulk->age; + list_move(&cursor->bulk_hitch.link, &pos->last->lru.link); + return false; + } + + /* Continue iterating down the bulk sublist */ + if (cursor->bulk_age == &bulk->age) + return false; + + /* + * The bulk sublist in which we had a hitch has moved and the + * hitch moved with it. Restart iteration from a previously + * set hitch after the bulk_move, and remove that backup + * hitch. + */ + list_move(&cursor->hitch.link, &cursor->bulk_hitch.link); + ttm_resource_cursor_clear_bulk(cursor); + + return true; +} + /** * ttm_resource_manager_next() - Continue iterating over the resource manager * resources @@ -524,6 +582,8 @@ ttm_resource_manager_next(struct ttm_resource_cursor *cursor) lru = &cursor->hitch; list_for_each_entry_continue(lru, &man->lru[cursor->priority], link) { if (ttm_lru_item_is_res(lru)) { + if (ttm_resource_cursor_check_bulk(cursor, lru)) + continue; list_move(&cursor->hitch.link, &lru->link); return ttm_lru_item_to_res(lru); } @@ -533,9 +593,10 @@ ttm_resource_manager_next(struct ttm_resource_cursor *cursor) break; list_move(&cursor->hitch.link, &man->lru[cursor->priority]); + ttm_resource_cursor_clear_bulk(cursor); } while (true); - list_del_init(&cursor->hitch.link); + ttm_resource_cursor_fini_locked(cursor); return NULL; } @@ -560,6 +621,7 @@ ttm_resource_manager_first(struct ttm_resource_manager *man, cursor->priority = 0; cursor->man = man; ttm_lru_item_init(&cursor->hitch, TTM_LRU_HITCH); + ttm_lru_item_init(&cursor->bulk_hitch, TTM_LRU_HITCH); list_move(&cursor->hitch.link, &man->lru[cursor->priority]); return ttm_resource_manager_next(cursor); diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h index 7fdb9d32371b..432a93d0f789 100644 --- a/include/drm/ttm/ttm_resource.h +++ b/include/drm/ttm/ttm_resource.h @@ -269,25 +269,6 @@ ttm_lru_item_to_res(struct ttm_lru_item *item) return container_of(item, struct ttm_resource, lru); } -/** - * struct ttm_resource_cursor - * @man: The resource manager currently being iterated over - * @hitch: A hitch list node inserted before the next resource - * to iterate over. - * @priority: the current priority - * - * Cursor to iterate over the resources in a manager. - */ -struct ttm_resource_cursor { - struct ttm_resource_manager *man; - struct ttm_lru_item hitch; - unsigned int priority; -}; - -void ttm_resource_cursor_fini_locked(struct ttm_resource_cursor *cursor); - -void ttm_resource_cursor_fini(struct ttm_resource_cursor *cursor); - /** * struct ttm_lru_bulk_move_pos * @@ -303,16 +284,45 @@ struct ttm_lru_bulk_move_pos { /** * struct ttm_lru_bulk_move - * * @pos: first/last lru entry for resources in the each domain/priority + * @age: The number of times the bulk sublists were bumped, (moved to + * the LRU list tail). Protected by the lru_lock. * * Container for the current bulk move state. Should be used with * ttm_lru_bulk_move_init() and ttm_bo_set_bulk_move(). */ struct ttm_lru_bulk_move { struct ttm_lru_bulk_move_pos pos[TTM_NUM_MEM_TYPES][TTM_MAX_BO_PRIORITY]; + u64 age; }; +/** + * struct ttm_resource_cursor + * @man: The resource manager currently being iterated over + * @hitch: A hitch list node inserted before the next resource + * to iterate over. + * @bulk_hitch: A hitch list node inserted before the next + * resource to iterate over if the bulk sublist @hitch was + * inserted in is bumped. + * @bulk_age: The age of @bulk when @bulk_hitch was inserted. + * Used to detect whether @bulk was bumped since last iteration. + * @priority: the current priority + * + * Cursor to iterate over the resources in a manager. + */ +struct ttm_resource_cursor { + struct ttm_resource_manager *man; + struct ttm_lru_item hitch; + struct ttm_lru_item bulk_hitch; + struct ttm_lru_bulk_move *bulk; + u64 bulk_age; + unsigned int priority; +}; + +void ttm_resource_cursor_fini_locked(struct ttm_resource_cursor *cursor); + +void ttm_resource_cursor_fini(struct ttm_resource_cursor *cursor); + /** * struct ttm_kmap_iter_iomap - Specialization for a struct io_mapping + * struct sg_table backed struct ttm_resource.
To work around the problem with hitches moving when bulk move sublists are bumped, keep a second hitch when traversing a bulk move sublist, which is attached to the list *after* the bulk move sublist. If we detect a sublist bump, we use that second hitch as the continuation point of list traversal. Sublist bumps are detected by checking the sublist age which is increased by 1 each time it was bumped. The age is then compared to that of the last iteration returning an item within the sublist. Cc: Christian König <christian.koenig@amd.com> Cc: <dri-devel@lists.freedesktop.org> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> --- drivers/gpu/drm/ttm/ttm_resource.c | 64 +++++++++++++++++++++++++++++- include/drm/ttm/ttm_resource.h | 50 +++++++++++++---------- 2 files changed, 93 insertions(+), 21 deletions(-)