Re: [Intel-gfx] [PATCH 1/2] drm/i915: Use mutex_lock_killable() from inside the shrinker
On 10/01/2019 11:15, Chris Wilson wrote: Quoting Tvrtko Ursulin (2019-01-10 10:54:33) On 10/01/2019 10:47, Chris Wilson wrote: Quoting Tvrtko Ursulin (2019-01-10 10:24:09) On 09/01/2019 16:42, Chris Wilson wrote: If the current process is being killed (it was interrupted with SIGKILL or equivalent), it will not make any progress in page allocation and we can abort performing the shrinking on its behalf. So we can use mutex_lock_killable() instead (although this path should only be reachable from kswapd currently). kswapd is hopefully not killable so it is not reachable via that route. But should be via other i915_gem_shrink_all callers. Is it starting to look like we need to pass some flags to say normal/interruptible/killable (kswapd/debugfs/?)? killable is justifiable for all callers, I think, even if SIGKILL may never be delivered. interruptible? Do we want to conceptually fail a As long as using mutex_lock_killable doesn't make something killable which otherwise wouldn't be. I have to say I don't know how the details of that work. I don't think so... (Famous last words.) My understanding was that signals, not even SIGKILL, would be delivered to kthreads. mutex_lock_killable doesn't install a signal handler, it just allows the scheduler to wake up the process should a high priority signal be delivered. Okay having snooped around I indeed don't see that it would make kswapd killable. kmalloc due to a signal, as that's likely to end up with ENOMEM and not EINTR. (Pretty sure that's not common practice but there's a bit of shrink-unless-killable around.) So I don't think we need to make normal aka uninterruptible a special case, and returning before shrinking on any signal seems unexpected. debugfs was the only reason I considered interruptible. There I think makes sense to allow bail up. I hate stuck shell sessions at least so anything which can be done to avoid them is tempting. I see. killable is an improvement for debugfs, not much of one but still infinitely better ;) Truer words haven't been spoken. :) Having said that, debugfs does try to be interruptible and we could pass a flag, but okay, I can send another patch you'll dislike later. Reviewed-by: Tvrtko Ursulin Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Use mutex_lock_killable() from inside the shrinker
Quoting Tvrtko Ursulin (2019-01-10 10:54:33) > > On 10/01/2019 10:47, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2019-01-10 10:24:09) > >> > >> On 09/01/2019 16:42, Chris Wilson wrote: > >>> If the current process is being killed (it was interrupted with SIGKILL > >>> or equivalent), it will not make any progress in page allocation and we > >>> can abort performing the shrinking on its behalf. So we can use > >>> mutex_lock_killable() instead (although this path should only be > >>> reachable from kswapd currently). > >> > >> kswapd is hopefully not killable so it is not reachable via that route. > >> But should be via other i915_gem_shrink_all callers. Is it starting to > >> look like we need to pass some flags to say > >> normal/interruptible/killable (kswapd/debugfs/?)? > > > > killable is justifiable for all callers, I think, even if SIGKILL may > > never be delivered. interruptible? Do we want to conceptually fail a > > As long as using mutex_lock_killable doesn't make something killable > which otherwise wouldn't be. I have to say I don't know how the details > of that work. I don't think so... (Famous last words.) My understanding was that signals, not even SIGKILL, would be delivered to kthreads. mutex_lock_killable doesn't install a signal handler, it just allows the scheduler to wake up the process should a high priority signal be delivered. > > kmalloc due to a signal, as that's likely to end up with ENOMEM and not > > EINTR. (Pretty sure that's not common practice but there's a bit of > > shrink-unless-killable around.) So I don't think we need to make > > normal aka uninterruptible a special case, and returning before > > shrinking on any signal seems unexpected. > > debugfs was the only reason I considered interruptible. There I think > makes sense to allow bail up. I hate stuck shell sessions at least so > anything which can be done to avoid them is tempting. I see. killable is an improvement for debugfs, not much of one but still infinitely better ;) -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Use mutex_lock_killable() from inside the shrinker
On 10/01/2019 10:47, Chris Wilson wrote: Quoting Tvrtko Ursulin (2019-01-10 10:24:09) On 09/01/2019 16:42, Chris Wilson wrote: If the current process is being killed (it was interrupted with SIGKILL or equivalent), it will not make any progress in page allocation and we can abort performing the shrinking on its behalf. So we can use mutex_lock_killable() instead (although this path should only be reachable from kswapd currently). kswapd is hopefully not killable so it is not reachable via that route. But should be via other i915_gem_shrink_all callers. Is it starting to look like we need to pass some flags to say normal/interruptible/killable (kswapd/debugfs/?)? killable is justifiable for all callers, I think, even if SIGKILL may never be delivered. interruptible? Do we want to conceptually fail a As long as using mutex_lock_killable doesn't make something killable which otherwise wouldn't be. I have to say I don't know how the details of that work. kmalloc due to a signal, as that's likely to end up with ENOMEM and not EINTR. (Pretty sure that's not common practice but there's a bit of shrink-unless-killable around.) So I don't think we need to make normal aka uninterruptible a special case, and returning before shrinking on any signal seems unexpected. debugfs was the only reason I considered interruptible. There I think makes sense to allow bail up. I hate stuck shell sessions at least so anything which can be done to avoid them is tempting. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Use mutex_lock_killable() from inside the shrinker
Quoting Tvrtko Ursulin (2019-01-10 10:24:09) > > On 09/01/2019 16:42, Chris Wilson wrote: > > If the current process is being killed (it was interrupted with SIGKILL > > or equivalent), it will not make any progress in page allocation and we > > can abort performing the shrinking on its behalf. So we can use > > mutex_lock_killable() instead (although this path should only be > > reachable from kswapd currently). > > kswapd is hopefully not killable so it is not reachable via that route. > But should be via other i915_gem_shrink_all callers. Is it starting to > look like we need to pass some flags to say > normal/interruptible/killable (kswapd/debugfs/?)? killable is justifiable for all callers, I think, even if SIGKILL may never be delivered. interruptible? Do we want to conceptually fail a kmalloc due to a signal, as that's likely to end up with ENOMEM and not EINTR. (Pretty sure that's not common practice but there's a bit of shrink-unless-killable around.) So I don't think we need to make normal aka uninterruptible a special case, and returning before shrinking on any signal seems unexpected. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Use mutex_lock_killable() from inside the shrinker
On 09/01/2019 16:42, Chris Wilson wrote:
If the current process is being killed (it was interrupted with SIGKILL
or equivalent), it will not make any progress in page allocation and we
can abort performing the shrinking on its behalf. So we can use
mutex_lock_killable() instead (although this path should only be
reachable from kswapd currently).
kswapd is hopefully not killable so it is not reachable via that route.
But should be via other i915_gem_shrink_all callers. Is it starting to
look like we need to pass some flags to say
normal/interruptible/killable (kswapd/debugfs/?)?
Regards,
Tvrtko
Signed-off-by: Chris Wilson
Cc: Tvrtko Ursulin
---
drivers/gpu/drm/i915/i915_gem_shrinker.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c
b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index 34b108f73f1d..8ad9519779cc 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -39,18 +39,18 @@ static bool shrinker_lock(struct drm_i915_private *i915,
unsigned int flags,
bool *unlock)
{
- switch (mutex_trylock_recursive(&i915->drm.struct_mutex)) {
+ struct mutex *m = &i915->drm.struct_mutex;
+
+ switch (mutex_trylock_recursive(m)) {
case MUTEX_TRYLOCK_RECURSIVE:
*unlock = false;
return true;
case MUTEX_TRYLOCK_FAILED:
*unlock = false;
- if (flags & I915_SHRINK_ACTIVE) {
- mutex_lock_nested(&i915->drm.struct_mutex,
- I915_MM_SHRINKER);
+ if (flags & I915_SHRINK_ACTIVE &&
+ mutex_lock_killable_nested(m, I915_MM_SHRINKER) == 0)
*unlock = true;
- }
return *unlock;
case MUTEX_TRYLOCK_SUCCESS:
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
