Hi everybody,
we have documented here https://www.kernel.org/doc/html/latest/driver-api/dma-buf.html#dma-fence-cro... that dma_fence objects must signal in a reasonable amount of time, but at the same time note that drivers might have a different idea of what reasonable means.
Recently I realized that this is actually not a good idea. Background is that the wall clock timeout means that for example the OOM killer might actually wait for this timeout to be able to terminate a process and reclaim the memory used. And this is just an example of how general kernel features might depend on that.
Some drivers and fence implementations used 10 seconds and that raised complains by end users. So at least amdgpu recently switched to 2 second which triggered an internal discussion about it.
This patch set here now adds a define to the dma_fence header which gives 2 seconds as reasonable amount of time. SW-sync is modified to always taint the kernel (since it doesn't has a timeout), VGEM is switched over to the new define and the scheduler gets a warning and taints the kernel if a driver uses a timeout longer than that.
I have not much intention of actually committing the patches (maybe except the SW-sync one), but question is if 2 seconds are reasonable?
Regards, Christian.
Add a define implementations can use as reasonable maximum signaling timeout. Document that if this timeout is exceeded by config options implementations should taint the kernel.
Tainting the kernel is important for bug reports to detect that end users might be using a problematic configuration.
Signed-off-by: Christian König christian.koenig@amd.com --- include/linux/dma-fence.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index 64639e104110..b31dfa501c84 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -28,6 +28,20 @@ struct dma_fence_ops; struct dma_fence_cb; struct seq_file;
+/** + * define DMA_FENCE_MAX_REASONABLE_TIMEOUT - max reasonable signaling timeout + * + * The dma_fence object has a deep inter dependency with core memory + * management, for a detailed explanation see section DMA Fences under + * Documentation/driver-api/dma-buf.rst. + * + * Because of this all dma_fence implementations must guarantee that each fence + * completes in a finite time. This define here now gives a reasonable value for + * the timeout to use. It is possible to use a longer timeout in an + * implementation but that should taint the kernel. + */ +#define DMA_FENCE_MAX_REASONABLE_TIMEOUT (2*HZ) + /** * struct dma_fence - software synchronization primitive * @refcount: refcount for this fence
The SW-sync functionality should only be used for testing and debugging since it is inherently unsave.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/dma-buf/sw_sync.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index 3c20f1d31cf5..6f09d13be6b6 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -8,6 +8,7 @@ #include <linux/file.h> #include <linux/fs.h> #include <linux/uaccess.h> +#include <linux/panic.h> #include <linux/slab.h> #include <linux/sync_file.h>
@@ -349,6 +350,9 @@ static long sw_sync_ioctl_create_fence(struct sync_timeline *obj, struct sync_file *sync_file; struct sw_sync_create_fence_data data;
+ /* SW sync fence are inherently unsafe and can deadlock the kernel */ + add_taint(TAINT_SOFTLOCKUP, LOCKDEP_STILL_OK); + if (fd < 0) return fd;
Instead of 10 seconds just use the reasonable maximum timeout defined by the dma_fence framework.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/vgem/vgem_fence.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vgem/vgem_fence.c b/drivers/gpu/drm/vgem/vgem_fence.c index 07db319c3d7f..1ca14b83479d 100644 --- a/drivers/gpu/drm/vgem/vgem_fence.c +++ b/drivers/gpu/drm/vgem/vgem_fence.c @@ -27,8 +27,6 @@
#include "vgem_drv.h"
-#define VGEM_FENCE_TIMEOUT (10*HZ) - struct vgem_fence { struct dma_fence base; struct spinlock lock; @@ -81,8 +79,11 @@ static struct dma_fence *vgem_fence_create(struct vgem_file *vfile,
timer_setup(&fence->timer, vgem_fence_timeout, TIMER_IRQSAFE);
- /* We force the fence to expire within 10s to prevent driver hangs */ - mod_timer(&fence->timer, jiffies + VGEM_FENCE_TIMEOUT); + /* + * Force the fence to expire within a reasonable timeout to prevent + * hangs inside the memory management. + */ + mod_timer(&fence->timer, jiffies + DMA_FENCE_MAX_REASONABLE_TIMEOUT);
return &fence->base; }
Exceeding the recommended maximum timeout should be noted in logs and crash dumps.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/scheduler/sched_main.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 1d4f1b822e7b..88e24e140def 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -1318,12 +1318,22 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, const struct drm_sched_init_ sched->ops = args->ops; sched->credit_limit = args->credit_limit; sched->name = args->name; - sched->timeout = args->timeout; sched->hang_limit = args->hang_limit; sched->timeout_wq = args->timeout_wq ? args->timeout_wq : system_percpu_wq; sched->score = args->score ? args->score : &sched->_score; sched->dev = args->dev;
+ sched->timeout = args->timeout; + if (sched->timeout > DMA_FENCE_MAX_REASONABLE_TIMEOUT) { + dev_warn(sched->dev, "Timeout %ld exceeds the maximum recommended one!\n", + sched->timeout); + /* + * Make sure that exceeding the recommendation is noted in + * logs and crash dumps. + */ + add_taint(TAINT_SOFTLOCKUP, LOCKDEP_STILL_OK); + } + if (args->num_rqs > DRM_SCHED_PRIORITY_COUNT) { /* This is a gross violation--tell drivers what the problem is. */
linaro-mm-sig@lists.linaro.org
-
Christian König