[Intel-gfx] [PATCH 1/2] drm/i915: Use mutex_lock_killable() from inside the shrinker

[Tvrtko Ursulin]

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 <tvrtko.ursulin at intel.com>

Regards,

Tvrtko