| Message ID | 20200207151720.2812125-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mm: Break long searches in fragmented address spaces | expand |
Hi Chris, On Fri, Feb 07, 2020 at 03:17:20PM +0000, Chris Wilson wrote: > We try hard to select a suitable hole in the drm_mm first time. But if > that is unsuccessful, we then have to look at neighbouring nodes, and > this requires traversing the rbtree. Walking the rbtree can be slow > (much slower than a linear list for deep trees), and if the drm_mm has > been purposefully fragmented our search can be trapped for a long, long > time. For non-preemptible kernels, we need to break up long CPU bound > sections by manually checking for cond_resched(); similarly we should checking for "fatal signals" you mean? > also bail out if we have been told to terminate. (In an ideal world, we > would break for any signal, but we need to trade off having to perform > the search again after ERESTARTSYS, which again may form a trap of > making no forward progress.) > > Reported-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> nice! Reviewed-by: Andi Shyti <andi.shyti@intel.com> Thanks, Andi > --- > drivers/gpu/drm/drm_mm.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c > index 2a6e34663146..47d5de9ca0a8 100644 > --- a/drivers/gpu/drm/drm_mm.c > +++ b/drivers/gpu/drm/drm_mm.c > @@ -45,6 +45,7 @@ > #include <linux/export.h> > #include <linux/interval_tree_generic.h> > #include <linux/seq_file.h> > +#include <linux/sched/signal.h> > #include <linux/slab.h> > #include <linux/stacktrace.h> > > @@ -366,6 +367,11 @@ next_hole(struct drm_mm *mm, > struct drm_mm_node *node, > enum drm_mm_insert_mode mode) > { > + /* Searching is slow; check if we ran out of time/patience */ > + cond_resched(); > + if (fatal_signal_pending(current)) > + return NULL; > + > switch (mode) { > default: > case DRM_MM_INSERT_BEST: > @@ -557,7 +563,7 @@ int drm_mm_insert_node_in_range(struct drm_mm * const mm, > return 0; > } > > - return -ENOSPC; > + return signal_pending(current) ? -ERESTARTSYS : -ENOSPC; > } > EXPORT_SYMBOL(drm_mm_insert_node_in_range); > > -- > 2.25.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Andi Shyti (2020-02-11 22:56:44) > Hi Chris, > > On Fri, Feb 07, 2020 at 03:17:20PM +0000, Chris Wilson wrote: > > We try hard to select a suitable hole in the drm_mm first time. But if > > that is unsuccessful, we then have to look at neighbouring nodes, and > > this requires traversing the rbtree. Walking the rbtree can be slow > > (much slower than a linear list for deep trees), and if the drm_mm has > > been purposefully fragmented our search can be trapped for a long, long > > time. For non-preemptible kernels, we need to break up long CPU bound > > sections by manually checking for cond_resched(); similarly we should > > checking for "fatal signals" you mean? We need to call schedule() ourselves for non-voluntary CONFIG_PREEMPT kernels, and we need to escape from the kernel back to userspace to handle a pending signal in this process. Other applications, sysadmins tend to notice and complain about driver hogging a CPU not letting anything else run; typically the user notices that their application doesn't close on ^C. (That is schedule() delays of a few ms will be noticed, but it takes a couple of seconds before a user will become aware of a severe problem with a stuck signal. :) Now since responding to a signal itself may be expensive (we have to restart the syscall and re-process all the state from before this point), we make the trade-off here of only responding to a fatal-signal (process exit), which should allow for faster system recovery under pathological conditions. -Chris
diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c index 2a6e34663146..47d5de9ca0a8 100644 --- a/drivers/gpu/drm/drm_mm.c +++ b/drivers/gpu/drm/drm_mm.c @@ -45,6 +45,7 @@ #include <linux/export.h> #include <linux/interval_tree_generic.h> #include <linux/seq_file.h> +#include <linux/sched/signal.h> #include <linux/slab.h> #include <linux/stacktrace.h> @@ -366,6 +367,11 @@ next_hole(struct drm_mm *mm, struct drm_mm_node *node, enum drm_mm_insert_mode mode) { + /* Searching is slow; check if we ran out of time/patience */ + cond_resched(); + if (fatal_signal_pending(current)) + return NULL; + switch (mode) { default: case DRM_MM_INSERT_BEST: @@ -557,7 +563,7 @@ int drm_mm_insert_node_in_range(struct drm_mm * const mm, return 0; } - return -ENOSPC; + return signal_pending(current) ? -ERESTARTSYS : -ENOSPC; } EXPORT_SYMBOL(drm_mm_insert_node_in_range);
We try hard to select a suitable hole in the drm_mm first time. But if that is unsuccessful, we then have to look at neighbouring nodes, and this requires traversing the rbtree. Walking the rbtree can be slow (much slower than a linear list for deep trees), and if the drm_mm has been purposefully fragmented our search can be trapped for a long, long time. For non-preemptible kernels, we need to break up long CPU bound sections by manually checking for cond_resched(); similarly we should also bail out if we have been told to terminate. (In an ideal world, we would break for any signal, but we need to trade off having to perform the search again after ERESTARTSYS, which again may form a trap of making no forward progress.) Reported-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/drm_mm.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)