I think it doesn't matter if workitem schedule out. Even we did not schedule out, the workqueue itself will schedule out later. So it did not break anything with this patch I think. ________________________________ From: Pan, Xinhui <xinhui....@amd.com> Sent: Thursday, April 9, 2020 10:07:09 PM To: Lucas Stach <l.st...@pengutronix.de>; amd-...@lists.freedesktop.org <amd-...@lists.freedesktop.org>; Koenig, Christian <christian.koe...@amd.com> Cc: dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org> Subject: Re: [PATCH] drm/ttm: Schedule out if possibe in bo delayed delete worker
Why we break out the loops when there are pending bos to be released? And I just checked the process_one_work. Right after the work item callback is called, the workqueue itself will call cond_resched. So I think ________________________________ From: Koenig, Christian <christian.koe...@amd.com> Sent: Thursday, April 9, 2020 9:38:24 PM To: Lucas Stach <l.st...@pengutronix.de>; Pan, Xinhui <xinhui....@amd.com>; amd-...@lists.freedesktop.org <amd-...@lists.freedesktop.org> Cc: dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org> Subject: Re: [PATCH] drm/ttm: Schedule out if possibe in bo delayed delete worker Am 09.04.20 um 15:25 schrieb Lucas Stach: > Am Donnerstag, den 09.04.2020, 14:35 +0200 schrieb Christian König: >> Am 09.04.20 um 03:31 schrieb xinhui pan: >>> The delayed delete list is per device which might be very huge. And in >>> a heavy workload test, the list might always not be empty. That will >>> trigger any RCU stall warnings or softlockups in non-preemptible kernels >>> Lets do schedule out if possible in that case. >> Mhm, I'm not sure if that is actually allowed. This is called from a >> work item and those are not really supposed to be scheduled away. > Huh? Workitems can schedule out just fine, otherwise they would be > horribly broken when it comes to sleeping locks. Let me refine the sentence: Work items are not really supposed to be scheduled purposely. E.g. you shouldn't call schedule() or cond_resched() like in the case here. Getting scheduled away because we wait for a lock is of course perfectly fine. > The workqueue code > even has measures to keep the workqueues at the expected concurrency > level by starting other workitems when one of them goes to sleep. Yeah, and exactly that's what I would say we should avoid here :) In other words work items can be scheduled away, but they should not if not really necessary (e.g. waiting for a lock). Otherwise as you said new threads for work item processing are started up and I don't think we want that. Just returning from the work item and waiting for the next cycle is most likely the better option. Regards, Christian. > > Regards, > Lucas > >> Maybe rather change the while into while (!list_empty(&bdev->ddestroy) >> && !should_reschedule(0)). >> >> Christian. >> >>> Signed-off-by: xinhui pan <xinhui....@amd.com> >>> --- >>> drivers/gpu/drm/ttm/ttm_bo.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c >>> index 9e07c3f75156..b8d853cab33b 100644 >>> --- a/drivers/gpu/drm/ttm/ttm_bo.c >>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c >>> @@ -541,6 +541,7 @@ static bool ttm_bo_delayed_delete(struct ttm_bo_device >>> *bdev, bool remove_all) >>> } >>> >>> ttm_bo_put(bo); >>> + cond_resched(); >>> spin_lock(&glob->lru_lock); >>> } >>> list_splice_tail(&removed, &bdev->ddestroy); >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7Cchristian.koenig%40amd.com%7C0a47486676a74702f05408d7dc89839c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637220355504145868&sdata=wbRkYBPI6mYuZjKBtQN3AGLDOwqJlWY3XUtwwSiUQHg%3D&reserved=0
_______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
