Am 24.08.2018 um 15:40 schrieb Michal Hocko:
On Fri 24-08-18 15:28:33, Christian König wrote:
Am 24.08.2018 um 15:24 schrieb Michal Hocko:
On Fri 24-08-18 15:10:08, Christian König wrote:
Am 24.08.2018 um 15:01 schrieb Michal Hocko:
On Fri 24-08-18 14:52:26, Christian König wrote:
Am 24.08.2018 um 14:33 schrieb Michal Hocko:
[...]
Thiking about it some more, I can imagine that a notifier callback which
performs an allocation might trigger a memory reclaim and that in turn
might trigger a notifier to be invoked and recurse. But notifier
shouldn't really allocate memory. They are called from deep MM code
paths and this would be extremely deadlock prone. Maybe Jerome can come
up some more realistic scenario. If not then I would propose to simplify
the locking here. We have lockdep to catch self deadlocks and it is
always better to handle a specific issue rather than having a code
without a clear indication how it can recurse.
Well I agree that we should probably fix that, but I have some concerns to
remove the existing workaround.
See we added that to get rid of a real problem in a customer environment and
I don't want to that to show up again.
It would really help to know more about that case and fix it properly
rather than workaround it like this. Anyway, let me think how to handle
the non-blocking notifier invocation then. I was not able to come up
with anything remotely sane yet.
With avoiding allocating memory in the write lock path I don't see an issue
any more with that.
All what the write lock path does now is adding items to a linked lists,
arrays etc....
Can we change it to non-sleepable lock then?
No, the write side doesn't sleep any more, but the read side does.
See amdgpu_mn_invalidate_node() and that is where you actually need to
handle the non-blocking flag correctly.
Ohh, right you are. We already handle that by bailing out before calling
amdgpu_mn_invalidate_node in !blockable mode.
Yeah, that is sufficient.
It could be improved because we have something like 90% chance that
amdgpu_mn_invalidate_node() actually doesn't need to do anything.
But I can take care of that when the patch set has landed.
So does this looks good to
you?
Yeah, that looks perfect to me. Reviewed-by: Christian König
<christian.koenig@xxxxxxx>
Thanks,
Christian.
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
index e55508b39496..48fa152231be 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
@@ -180,11 +180,15 @@ void amdgpu_mn_unlock(struct amdgpu_mn *mn)
*/
static int amdgpu_mn_read_lock(struct amdgpu_mn *amn, bool blockable)
{
- if (blockable)
- mutex_lock(&amn->read_lock);
- else if (!mutex_trylock(&amn->read_lock))
- return -EAGAIN;
-
+ /*
+ * We can take sleepable lock even on !blockable mode because
+ * read_lock is only ever take from this path and the notifier
+ * lock never really sleeps. In fact the only reason why the
+ * later is sleepable is because the notifier itself might sleep
+ * in amdgpu_mn_invalidate_node but blockable mode is handled
+ * before calling into that path.
+ */
+ mutex_lock(&amn->read_lock);
if (atomic_inc_return(&amn->recursion) == 1)
down_read_non_owner(&amn->lock);
mutex_unlock(&amn->read_lock);
