mm, oom: distinguish blockable mode for mmu notifiers

From: Michal Hocko <[email protected]>

There are several blockable mmu notifiers which might sleep in mmu_notifier_invalidate_range_start and that is a problem for the oom_reaper because it needs to guarantee a forward progress so it cannot depend on any sleepable locks.

Currently we simply back off and mark an oom victim with blockable mmu notifiers as done after a short sleep. That can result in selecting a new oom victim prematurely because the previous one still hasn't torn its memory down yet.

We can do much better though. Even if mmu notifiers use sleepable locks there is no reason to automatically assume those locks are held. Moreover majority of notifiers only care about a portion of the address space and there is absolutely zero reason to fail when we are unmapping an unrelated range. Many notifiers do really block and wait for HW which is harder to handle and we have to bail out though.

This patch handles the low hanging fruid. __mmu_notifier_invalidate_range_start gets a blockable flag and callbacks are not allowed to sleep if the flag is set to false. This is achieved by using trylock instead of the sleepable lock for most callbacks and continue as long as we do not block down the call chain.

I think we can improve that even further because there is a common pattern to do a range lookup first and then do something about that. The first part can be done without a sleeping lock in most cases AFAICS.

The oom_reaper end then simply retries if there is at least one notifier which couldn't make any progress in !blockable mode. A retry loop is already implemented to wait for the mmap_sem and this is basically the same thing.

...

mm/mmap.c +0 -0

 		set_bit(MMF_OOM_SKIP, &mm->flags);
 
...

Quoted 19 lines · On Mon, 16 Jul 2018 13:50:58 +0200 Michal Hocko <[email protected]> wrote:

From: Michal Hocko <[email protected]>

There are several blockable mmu notifiers which might sleep in mmu_notifier_invalidate_range_start and that is a problem for the oom_reaper because it needs to guarantee a forward progress so it cannot depend on any sleepable locks.

Currently we simply back off and mark an oom victim with blockable mmu notifiers as done after a short sleep. That can result in selecting a new oom victim prematurely because the previous one still hasn't torn its memory down yet.

We can do much better though. Even if mmu notifiers use sleepable locks there is no reason to automatically assume those locks are held. Moreover majority of notifiers only care about a portion of the address space and there is absolutely zero reason to fail when we are unmapping an unrelated range. Many notifiers do really block and wait for HW which is harder to handle and we have to bail out though.

Quoted 5 lines

This patch handles the low hanging fruid. __mmu_notifier_invalidate_range_start gets a blockable flag and callbacks are not allowed to sleep if the flag is set to false. This is achieved by using trylock instead of the sleepable lock for most callbacks and continue as long as we do not block down the call chain.

I assume device driver developers are wondering "what does this mean for me". As I understand it, the only time they will see blockable==false is when their driver is being called in response to an out-of-memory condition, yes? So it is a very rare thing.

Any suggestions regarding how the driver developers can test this code path? I don't think we presently have a way to fake an oom-killing event? Perhaps we should add such a thing, given the problems we're having with that feature.

Quoted 19 lines · On Mon, 16 Jul 2018 13:50:58 +0200 Michal Hocko <[email protected]> wrote:

From: Michal Hocko <[email protected]>

There are several blockable mmu notifiers which might sleep in mmu_notifier_invalidate_range_start and that is a problem for the oom_reaper because it needs to guarantee a forward progress so it cannot depend on any sleepable locks.

Currently we simply back off and mark an oom victim with blockable mmu notifiers as done after a short sleep. That can result in selecting a new oom victim prematurely because the previous one still hasn't torn its memory down yet.

We can do much better though. Even if mmu notifiers use sleepable locks there is no reason to automatically assume those locks are held. Moreover majority of notifiers only care about a portion of the address space and there is absolutely zero reason to fail when we are unmapping an unrelated range. Many notifiers do really block and wait for HW which is harder to handle and we have to bail out though.

Quoted 5 lines

This patch handles the low hanging fruid. __mmu_notifier_invalidate_range_start gets a blockable flag and callbacks are not allowed to sleep if the flag is set to false. This is achieved by using trylock instead of the sleepable lock for most callbacks and continue as long as we do not block down the call chain.

I assume device driver developers are wondering "what does this mean for me". As I understand it, the only time they will see blockable==false is when their driver is being called in response to an out-of-memory condition, yes? So it is a very rare thing.

Any suggestions regarding how the driver developers can test this code path? I don't think we presently have a way to fake an oom-killing event? Perhaps we should add such a thing, given the problems we're having with that feature.

Quoted 19 lines · On Mon, 16 Jul 2018 13:50:58 +0200 Michal Hocko <[email protected]> wrote:

From: Michal Hocko <[email protected]>

There are several blockable mmu notifiers which might sleep in mmu_notifier_invalidate_range_start and that is a problem for the oom_reaper because it needs to guarantee a forward progress so it cannot depend on any sleepable locks.

Currently we simply back off and mark an oom victim with blockable mmu notifiers as done after a short sleep. That can result in selecting a new oom victim prematurely because the previous one still hasn't torn its memory down yet.

We can do much better though. Even if mmu notifiers use sleepable locks there is no reason to automatically assume those locks are held. Moreover majority of notifiers only care about a portion of the address space and there is absolutely zero reason to fail when we are unmapping an unrelated range. Many notifiers do really block and wait for HW which is harder to handle and we have to bail out though.

Quoted 5 lines

This patch handles the low hanging fruid. __mmu_notifier_invalidate_range_start gets a blockable flag and callbacks are not allowed to sleep if the flag is set to false. This is achieved by using trylock instead of the sleepable lock for most callbacks and continue as long as we do not block down the call chain.

I assume device driver developers are wondering "what does this mean for me". As I understand it, the only time they will see blockable==false is when their driver is being called in response to an out-of-memory condition, yes? So it is a very rare thing.

Any suggestions regarding how the driver developers can test this code path? I don't think we presently have a way to fake an oom-killing event? Perhaps we should add such a thing, given the problems we're having with that feature.

Quoted 11 lines

I think we can improve that even further because there is a common pattern to do a range lookup first and then do something about that. The first part can be done without a sleeping lock in most cases AFAICS.

The oom_reaper end then simply retries if there is at least one notifier which couldn't make any progress in !blockable mode. A retry loop is already implemented to wait for the mmap_sem and this is basically the same thing.

...

include/linux/mmu_notifier.h static inline void mmu_notifier_invalidate_range_start(struct mm_struct *mm, · line 298

Quoted 9 lines

include/linux/mmu_notifier.h +8 -0

+static inline int mmu_notifier_invalidate_range_start_nonblock(struct mm_struct *mm,
+				  unsigned long start, unsigned long end)
+{
+	int ret = 0;
+	if (mm_has_notifiers(mm))
+		ret = __mmu_notifier_invalidate_range_start(mm, start, end, false);
+
+	return ret;
 }

nit,

{
	if (mm_has_notifiers(mm))
		return __mmu_notifier_invalidate_range_start(mm, start, end, false);
	return 0;
}

would suffice.

mm/mmap.c void exit_mmap(struct mm_struct *mm) · line 3078

Quoted 12 lines

...

mm/mmap.c +1 -1

@@ -3074,7 +3074,7 @@ void exit_mmap(struct mm_struct *mm)
 		 * reliably test it.
 		 */
 		mutex_lock(&oom_lock);
-		__oom_reap_task_mm(mm);
+		(void)__oom_reap_task_mm(mm);
 		mutex_unlock(&oom_lock);

What does this do?

Quoted 4 lines

Any suggestions regarding how the driver developers can test this code path? I don't think we presently have a way to fake an oom-killing event? Perhaps we should add such a thing, given the problems we're having with that feature.

The simplest way is to wrap an userspace code which uses these notifiers into a memcg and set the hard limit to hit the oom. This can be done e.g. after the test faults in all the mmu notifier managed memory and set the hard limit to something really small. Then we are looking for a proper process tear down.

Quoted 10 lines · On Tue, 17 Jul 2018 10:12:01 +0200 Michal Hocko <[email protected]> wrote:

Quoted 4 lines

Any suggestions regarding how the driver developers can test this code path? I don't think we presently have a way to fake an oom-killing event? Perhaps we should add such a thing, given the problems we're having with that feature.

The simplest way is to wrap an userspace code which uses these notifiers into a memcg and set the hard limit to hit the oom. This can be done e.g. after the test faults in all the mmu notifier managed memory and set the hard limit to something really small. Then we are looking for a proper process tear down.

Chances are, some of the intended audience don't know how to do this and will either have to hunt down a lot of documentation or will just not test it. But we want them to test it, so a little worked step-by-step example would help things along please.

Quoted 10 lines · On Tue, 17 Jul 2018 10:12:01 +0200 Michal Hocko <[email protected]> wrote:

Quoted 4 lines

Any suggestions regarding how the driver developers can test this code path? I don't think we presently have a way to fake an oom-killing event? Perhaps we should add such a thing, given the problems we're having with that feature.

The simplest way is to wrap an userspace code which uses these notifiers into a memcg and set the hard limit to hit the oom. This can be done e.g. after the test faults in all the mmu notifier managed memory and set the hard limit to something really small. Then we are looking for a proper process tear down.

Chances are, some of the intended audience don't know how to do this and will either have to hunt down a lot of documentation or will just not test it. But we want them to test it, so a little worked step-by-step example would help things along please.

Quoted 4 lines

Any suggestions regarding how the driver developers can test this code path? I don't think we presently have a way to fake an oom-killing event? Perhaps we should add such a thing, given the problems we're having with that feature.

The simplest way is to wrap an userspace code which uses these notifiers into a memcg and set the hard limit to hit the oom. This can be done e.g. after the test faults in all the mmu notifier managed memory and set the hard limit to something really small. Then we are looking for a proper process tear down.

Quoted 19 lines · On Mon, 16 Jul 2018 13:50:58 +0200 Michal Hocko <[email protected]> wrote:

From: Michal Hocko <[email protected]>

There are several blockable mmu notifiers which might sleep in mmu_notifier_invalidate_range_start and that is a problem for the oom_reaper because it needs to guarantee a forward progress so it cannot depend on any sleepable locks.

Currently we simply back off and mark an oom victim with blockable mmu notifiers as done after a short sleep. That can result in selecting a new oom victim prematurely because the previous one still hasn't torn its memory down yet.

We can do much better though. Even if mmu notifiers use sleepable locks there is no reason to automatically assume those locks are held. Moreover majority of notifiers only care about a portion of the address space and there is absolutely zero reason to fail when we are unmapping an unrelated range. Many notifiers do really block and wait for HW which is harder to handle and we have to bail out though.

Quoted 5 lines

This patch handles the low hanging fruid. __mmu_notifier_invalidate_range_start gets a blockable flag and callbacks are not allowed to sleep if the flag is set to false. This is achieved by using trylock instead of the sleepable lock for most callbacks and continue as long as we do not block down the call chain.

I assume device driver developers are wondering "what does this mean for me". As I understand it, the only time they will see blockable==false is when their driver is being called in response to an out-of-memory condition, yes? So it is a very rare thing.

Any suggestions regarding how the driver developers can test this code path? I don't think we presently have a way to fake an oom-killing event? Perhaps we should add such a thing, given the problems we're having with that feature.

Quoted 11 lines

I think we can improve that even further because there is a common pattern to do a range lookup first and then do something about that. The first part can be done without a sleeping lock in most cases AFAICS.

The oom_reaper end then simply retries if there is at least one notifier which couldn't make any progress in !blockable mode. A retry loop is already implemented to wait for the mmap_sem and this is basically the same thing.

...

include/linux/mmu_notifier.h static inline void mmu_notifier_invalidate_range_start(struct mm_struct *mm, · line 298

Quoted 9 lines

include/linux/mmu_notifier.h +8 -0

+static inline int mmu_notifier_invalidate_range_start_nonblock(struct mm_struct *mm,
+				  unsigned long start, unsigned long end)
+{
+	int ret = 0;
+	if (mm_has_notifiers(mm))
+		ret = __mmu_notifier_invalidate_range_start(mm, start, end, false);
+
+	return ret;
 }

nit,

{
	if (mm_has_notifiers(mm))
		return __mmu_notifier_invalidate_range_start(mm, start, end, false);
	return 0;
}

would suffice.

mm/mmap.c void exit_mmap(struct mm_struct *mm) · line 3078

Quoted 12 lines

...

mm/mmap.c +1 -1

@@ -3074,7 +3074,7 @@ void exit_mmap(struct mm_struct *mm)
 		 * reliably test it.
 		 */
 		mutex_lock(&oom_lock);
-		__oom_reap_task_mm(mm);
+		(void)__oom_reap_task_mm(mm);
 		mutex_unlock(&oom_lock);

What does this do?

From: Michal Hocko <[email protected]>

There are several blockable mmu notifiers which might sleep in mmu_notifier_invalidate_range_start and that is a problem for the oom_reaper because it needs to guarantee a forward progress so it cannot depend on any sleepable locks.

Currently we simply back off and mark an oom victim with blockable mmu notifiers as done after a short sleep. That can result in selecting a new oom victim prematurely because the previous one still hasn't torn its memory down yet.

We can do much better though. Even if mmu notifiers use sleepable locks there is no reason to automatically assume those locks are held. Moreover majority of notifiers only care about a portion of the address space and there is absolutely zero reason to fail when we are unmapping an unrelated range. Many notifiers do really block and wait for HW which is harder to handle and we have to bail out though.

This patch handles the low hanging fruid. __mmu_notifier_invalidate_range_start gets a blockable flag and callbacks are not allowed to sleep if the flag is set to false. This is achieved by using trylock instead of the sleepable lock for most callbacks and continue as long as we do not block down the call chain.

I think we can improve that even further because there is a common pattern to do a range lookup first and then do something about that. The first part can be done without a sleeping lock in most cases AFAICS.

The oom_reaper end then simply retries if there is at least one notifier which couldn't make any progress in !blockable mode. A retry loop is already implemented to wait for the mmap_sem and this is basically the same thing.

...

From: Michal Hocko <[email protected]>

There are several blockable mmu notifiers which might sleep in mmu_notifier_invalidate_range_start and that is a problem for the oom_reaper because it needs to guarantee a forward progress so it cannot depend on any sleepable locks.

...

Please take a look?

On Fri 20-07-18 17:09:02, Andrew Morton wrote:
[...]

Quoted 1 line

Please take a look?

Are you OK to have these in a separate patch?

Quoted 5 lines · Michal Hocko

On Fri 20-07-18 17:09:02, Andrew Morton wrote:
[...]

Quoted 1 line

Please take a look?

Are you OK to have these in a separate patch?

Btw. I will rebase this patch once oom stuff in linux-next settles. At least oom_lock removal from oom_reaper will conflict.

Quoted 5 lines · Michal Hocko

On Fri 20-07-18 17:09:02, Andrew Morton wrote:
[...]

Quoted 1 line

Please take a look?

Are you OK to have these in a separate patch?

Btw. I will rebase this patch once oom stuff in linux-next settles. At least oom_lock removal from oom_reaper will conflict.

On Fri 20-07-18 17:09:02, Andrew Morton wrote:
[...]

Quoted 1 line

Please take a look?

Are you OK to have these in a separate patch?

Please take a look?

- Undocumented return value.

- comment "failed to reap part..." is misleading - sounds like it's referring to something which happened in the past, is in fact referring to something which might happen in the future.

- fails to call trace_finish_task_reaping() in one case

- code duplication.

- Increases mmap_sem hold time a little by moving trace_finish_task_reaping() inside the locked region. So sue me ;)

- Sharing the finish: path means that the trace event won't distinguish between the two sources of finishing.

Please take a look?

On Fri 20-07-18 17:09:02, Andrew Morton wrote:
[...]

Quoted 10 lines

- Undocumented return value.

- comment "failed to reap part..." is misleading - sounds like it's referring to something which happened in the past, is in fact referring to something which might happen in the future.

- fails to call trace_finish_task_reaping() in one case

- code duplication.

Quoted 7 lines

- Increases mmap_sem hold time a little by moving trace_finish_task_reaping() inside the locked region. So sue me ;)

- Sharing the finish: path means that the trace event won't distinguish between the two sources of finishing.

Please take a look?

oom_reap_task_mm should return false when __oom_reap_task_mm return false. This is what my patch did but it seems this changed by
http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-oom-remove-oom_lock-from-oom_reaper.patch
so that one should be fixed.

mm/oom_kill.c +0 -0

index 104ef4a01a55..88657e018714 100644

On top of that the proposed cleanup looks as follows:

Quoted 2 lines

On top of that the proposed cleanup looks as follows:

Looks good to me. Seems a bit strange that we omit the pr_info() output if the mm was partially reaped - people would still want to know
this? Not very important though.

Quoted 2 lines

On top of that the proposed cleanup looks as follows:

Looks good to me. Seems a bit strange that we omit the pr_info() output if the mm was partially reaped - people would still want to know
this? Not very important though.

On Fri 20-07-18 17:09:02, Andrew Morton wrote:
[...]

Quoted 10 lines

- Undocumented return value.

- comment "failed to reap part..." is misleading - sounds like it's referring to something which happened in the past, is in fact referring to something which might happen in the future.

- fails to call trace_finish_task_reaping() in one case

- code duplication.

Quoted 7 lines

- Increases mmap_sem hold time a little by moving trace_finish_task_reaping() inside the locked region. So sue me ;)

- Sharing the finish: path means that the trace event won't distinguish between the two sources of finishing.

Please take a look?

oom_reap_task_mm should return false when __oom_reap_task_mm return false. This is what my patch did but it seems this changed by
http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-oom-remove-oom_lock-from-oom_reaper.patch
so that one should be fixed.

mm/oom_kill.c +0 -0

index 104ef4a01a55..88657e018714 100644

On top of that the proposed cleanup looks as follows:

oom_reap_task_mm should return false when __oom_reap_task_mm return false. This is what my patch did but it seems this changed by
http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-oom-remove-oom_lock-from-oom_reaper.patch
so that one should be fixed.

mm/oom_kill.c +1 -1

index 104ef4a01a55..88657e018714 100644
@@ -565,7 +565,7 @@ static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 	/* failed to reap part of the address space. Try again later */
 	if (!__oom_reap_task_mm(mm)) {
 		up_read(&mm->mmap_sem);
-		return true;
+		return false;
 	}
 
 	pr_info("oom_reaper: reaped process %d (%s), now anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB\n",

On top of that the proposed cleanup looks as follows:

mm/oom_kill.c +13 -6

index 88657e018714..4e185a282b3d 100644
@@ -541,8 +541,16 @@ bool __oom_reap_task_mm(struct mm_struct *mm)
 	return ret;
 }
 
+/*
+ * Reaps the address space of the give task.
+ *
+ * Returns true on success and false if none or part of the address space
+ * has been reclaimed and the caller should retry later.
+ */
 static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 {
+	bool ret = true;
+
 	if (!down_read_trylock(&mm->mmap_sem)) {
 		trace_skip_task_reaping(tsk->pid);
 		return false;

@@ -555,28 +563,28 @@ static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 	 * down_write();up_write() cycle in exit_mmap().
 	 */
 	if (test_bit(MMF_OOM_SKIP, &mm->flags)) {
-		up_read(&mm->mmap_sem);
 		trace_skip_task_reaping(tsk->pid);
-		return true;
+		goto out_unlock;
 	}
 
 	trace_start_task_reaping(tsk->pid);
 
 	/* failed to reap part of the address space. Try again later */
-	if (!__oom_reap_task_mm(mm)) {
-		up_read(&mm->mmap_sem);
-		return false;
-	}
+	ret = __oom_reap_task_mm(mm);
+	if (!ret)
+		goto out_finish;
 
 	pr_info("oom_reaper: reaped process %d (%s), now anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB\n",
 			task_pid_nr(tsk), tsk->comm,
 			K(get_mm_counter(mm, MM_ANONPAGES)),
 			K(get_mm_counter(mm, MM_FILEPAGES)),
 			K(get_mm_counter(mm, MM_SHMEMPAGES)));
+out_finish:

mm/oom_kill.c: clean up oom_reap_task_mm() fix

indicate reaping has been partially skipped so we can expect future skips or another start before finish.

Signed-off-by David Rientjes <[email protected]>

---
 mm/oom_kill.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

mm/oom_kill.c +4 -2

@@ -569,10 +569,12 @@ static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 
 	trace_start_task_reaping(tsk->pid);
 
-	/* failed to reap part of the address space. Try again later */
 	ret = __oom_reap_task_mm(mm);
-	if (!ret)
+	if (!ret) {
+		/* Failed to reap part of the address space. Try again later */
+		trace_skip_task_reaping(tsk->pid);
 		goto out_finish;
+	}
 
 	pr_info("oom_reaper: reaped process %d (%s), now anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB\n",
 			task_pid_nr(tsk), tsk->comm,

mm/oom_kill.c: clean up oom_reap_task_mm() fix

indicate reaping has been partially skipped so we can expect future skips or another start before finish.

Signed-off-by David Rientjes <[email protected]>

---
 mm/oom_kill.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

mm/oom_kill.c +4 -2

@@ -569,10 +569,12 @@ static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 
 	trace_start_task_reaping(tsk->pid);
 
-	/* failed to reap part of the address space. Try again later */
 	ret = __oom_reap_task_mm(mm);
-	if (!ret)
+	if (!ret) {
+		/* Failed to reap part of the address space. Try again later */
+		trace_skip_task_reaping(tsk->pid);
 		goto out_finish;
+	}
 
 	pr_info("oom_reaper: reaped process %d (%s), now anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB\n",
 			task_pid_nr(tsk), tsk->comm,

oom_reap_task_mm should return false when __oom_reap_task_mm return false. This is what my patch did but it seems this changed by
http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-oom-remove-oom_lock-from-oom_reaper.patch
so that one should be fixed.

mm/oom_kill.c +1 -1

index 104ef4a01a55..88657e018714 100644
@@ -565,7 +565,7 @@ static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 	/* failed to reap part of the address space. Try again later */
 	if (!__oom_reap_task_mm(mm)) {
 		up_read(&mm->mmap_sem);
-		return true;
+		return false;
 	}
 
 	pr_info("oom_reaper: reaped process %d (%s), now anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB\n",

On top of that the proposed cleanup looks as follows:

mm/oom_kill.c +13 -6

index 88657e018714..4e185a282b3d 100644
@@ -541,8 +541,16 @@ bool __oom_reap_task_mm(struct mm_struct *mm)
 	return ret;
 }
 
+/*
+ * Reaps the address space of the give task.
+ *
+ * Returns true on success and false if none or part of the address space
+ * has been reclaimed and the caller should retry later.
+ */
 static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 {
+	bool ret = true;
+
 	if (!down_read_trylock(&mm->mmap_sem)) {
 		trace_skip_task_reaping(tsk->pid);
 		return false;

@@ -555,28 +563,28 @@ static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 	 * down_write();up_write() cycle in exit_mmap().
 	 */
 	if (test_bit(MMF_OOM_SKIP, &mm->flags)) {
-		up_read(&mm->mmap_sem);
 		trace_skip_task_reaping(tsk->pid);
-		return true;
+		goto out_unlock;
 	}
 
 	trace_start_task_reaping(tsk->pid);
 
 	/* failed to reap part of the address space. Try again later */
-	if (!__oom_reap_task_mm(mm)) {
-		up_read(&mm->mmap_sem);
-		return false;
-	}
+	ret = __oom_reap_task_mm(mm);
+	if (!ret)
+		goto out_finish;
 
 	pr_info("oom_reaper: reaped process %d (%s), now anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB\n",
 			task_pid_nr(tsk), tsk->comm,
 			K(get_mm_counter(mm, MM_ANONPAGES)),
 			K(get_mm_counter(mm, MM_FILEPAGES)),
 			K(get_mm_counter(mm, MM_SHMEMPAGES)));
+out_finish:

- Undocumented return value.

- comment "failed to reap part..." is misleading - sounds like it's referring to something which happened in the past, is in fact referring to something which might happen in the future.

- fails to call trace_finish_task_reaping() in one case

- code duplication.

- Increases mmap_sem hold time a little by moving trace_finish_task_reaping() inside the locked region. So sue me ;)

- Sharing the finish: path means that the trace event won't distinguish between the two sources of finishing.

Please take a look?

From: Michal Hocko <[email protected]>

There are several blockable mmu notifiers which might sleep in mmu_notifier_invalidate_range_start and that is a problem for the oom_reaper because it needs to guarantee a forward progress so it cannot depend on any sleepable locks.

...

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +0 -1

@@ -178,12 +178,18 @@ void amdgpu_mn_unlock(struct amdgpu_mn *mn)
  *
  * @amn: our notifier
  */
-static void amdgpu_mn_read_lock(struct amdgpu_mn *amn)

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +2 -0

        mutex_unlock(&amn->read_lock);
+
+       return 0;
 }
 
 /**

mm/hmm.c +3 -2

@@ -177,16 +177,19 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
        up_write(&hmm->mirrors_sem);
 }
 
-static void hmm_invalidate_range_start(struct mmu_notifier *mn,
+static int hmm_invalidate_range_start(struct mmu_notifier *mn,
                                       struct mm_struct *mm,
                                       unsigned long start,
-                                      unsigned long end)
+                                      unsigned long end,
+                                      bool blockable)
 {

Two more worries for this patch.

Quoted 12 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +3 -2

@@ -178,12 +178,18 @@ void amdgpu_mn_unlock(struct amdgpu_mn *mn)
  *
  * @amn: our notifier
  */
-static void amdgpu_mn_read_lock(struct amdgpu_mn *amn)
+static int amdgpu_mn_read_lock(struct amdgpu_mn *amn, bool blockable)
 {
-       mutex_lock(&amn->read_lock);
+       if (blockable)
+               mutex_lock(&amn->read_lock);

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c void amdgpu_mn_unlock(struct amdgpu_mn *mn) · line 189

Quoted 5 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +3 -0

+       else if (!mutex_trylock(&amn->read_lock))
+               return -EAGAIN;
+
        if (atomic_inc_return(&amn->recursion) == 1)
                down_read_non_owner(&amn->lock);

Why don't we need to use trylock here if blockable == false ? Want comment why it is safe to use blocking lock here.

Quoted 15 lines

Two more worries for this patch.

Quoted 11 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +2 -2

@@ -178,12 +178,18 @@ void amdgpu_mn_unlock(struct amdgpu_mn *mn)
   *
   * @amn: our notifier
   */
-static void amdgpu_mn_read_lock(struct amdgpu_mn *amn)
+static int amdgpu_mn_read_lock(struct amdgpu_mn *amn, bool blockable)
  {
-       mutex_lock(&amn->read_lock);
+       if (blockable)

Quoted 8 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c void amdgpu_mn_unlock(struct amdgpu_mn *mn) · line 189

Quoted 6 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +4 -0

+               mutex_lock(&amn->read_lock);
+       else if (!mutex_trylock(&amn->read_lock))
+               return -EAGAIN;
+
         if (atomic_inc_return(&amn->recursion) == 1)
                 down_read_non_owner(&amn->lock);

Why don't we need to use trylock here if blockable == false ? Want comment why it is safe to use blocking lock here.

Hmm, I am pretty sure I have checked the code but it was quite confusing so I might have missed something. Double checking now, it seems that this read_lock is not used anywhere else and it is not _the_ lock we are interested about. It is the amn->lock (amdgpu_mn_lock) which matters as it is taken in exclusive mode for expensive operations.

Is that correct Christian? If this is correct then we need to update the locking here. I am struggling to grasp the ref counting part. Why cannot all readers simply take the lock rather than rely on somebody else to take it? 1ed3d2567c800 didn't really help me to understand the locking scheme here so any help would be appreciated.

I am wondering why we cannot do

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +5 -8

index e55508b39496..93034178673d 100644
@@ -180,14 +180,11 @@ 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;
-
-	if (atomic_inc_return(&amn->recursion) == 1)
-		down_read_non_owner(&amn->lock);
-	mutex_unlock(&amn->read_lock);
+	if (!down_read_trylock(&amn->lock)) {
+		if (!blockable)
+			return -EAGAIN;
+		down_read(amn->lock);
+	}
 
  	return 0;
  }

Am 24.08.2018 um 13:32 schrieb Michal Hocko:

Quoted 24 lines · Tetsuo Handa

Quoted 15 lines

Two more worries for this patch.

Quoted 11 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +2 -2

@@ -178,12 +178,18 @@ void amdgpu_mn_unlock(struct amdgpu_mn *mn)
   *
   * @amn: our notifier
   */
-static void amdgpu_mn_read_lock(struct amdgpu_mn *amn)
+static int amdgpu_mn_read_lock(struct amdgpu_mn *amn, bool blockable)
  {
-       mutex_lock(&amn->read_lock);
+       if (blockable)

Quoted 8 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c void amdgpu_mn_unlock(struct amdgpu_mn *mn) · line 189

Quoted 6 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +4 -0

+               mutex_lock(&amn->read_lock);
+       else if (!mutex_trylock(&amn->read_lock))
+               return -EAGAIN;
+
         if (atomic_inc_return(&amn->recursion) == 1)
                 down_read_non_owner(&amn->lock);

Why don't we need to use trylock here if blockable == false ? Want comment why it is safe to use blocking lock here.

Quoted 5 lines

Hmm, I am pretty sure I have checked the code but it was quite confusing so I might have missed something. Double checking now, it seems that this read_lock is not used anywhere else and it is not _the_ lock we are interested about. It is the amn->lock (amdgpu_mn_lock) which matters as it is taken in exclusive mode for expensive operations.

The write side of the lock is only taken in the command submission IOCTL.

So you actually don't need to change anything here (even the proposed changes are overkill) since we can't tear down the struct_mm while an IOCTL is still using.

Quoted 5 lines

Is that correct Christian? If this is correct then we need to update the locking here. I am struggling to grasp the ref counting part. Why cannot all readers simply take the lock rather than rely on somebody else to take it? 1ed3d2567c800 didn't really help me to understand the locking scheme here so any help would be appreciated.

That won't work like this there might be multiple invalidate_range_start()/invalidate_range_end() pairs open at the same time. E.g. the lock might be taken recursively and that is illegal for a rw_semaphore.

Quoted 32 lines

Am 24.08.2018 um 13:32 schrieb Michal Hocko:

Quoted 24 lines · Tetsuo Handa

Quoted 15 lines

Two more worries for this patch.

Quoted 11 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +2 -2

@@ -178,12 +178,18 @@ void amdgpu_mn_unlock(struct amdgpu_mn *mn)
    *
    * @amn: our notifier
    */
-static void amdgpu_mn_read_lock(struct amdgpu_mn *amn)
+static int amdgpu_mn_read_lock(struct amdgpu_mn *amn, bool blockable)
   {
-       mutex_lock(&amn->read_lock);
+       if (blockable)

Quoted 8 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c void amdgpu_mn_unlock(struct amdgpu_mn *mn) · line 189

Quoted 6 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +4 -0

+               mutex_lock(&amn->read_lock);
+       else if (!mutex_trylock(&amn->read_lock))
+               return -EAGAIN;
+
          if (atomic_inc_return(&amn->recursion) == 1)
                  down_read_non_owner(&amn->lock);

Why don't we need to use trylock here if blockable == false ? Want comment why it is safe to use blocking lock here.

Quoted 5 lines

Hmm, I am pretty sure I have checked the code but it was quite confusing so I might have missed something. Double checking now, it seems that this read_lock is not used anywhere else and it is not _the_ lock we are interested about. It is the amn->lock (amdgpu_mn_lock) which matters as it is taken in exclusive mode for expensive operations.

The write side of the lock is only taken in the command submission IOCTL.

Quoted 3 lines

So you actually don't need to change anything here (even the proposed changes are overkill) since we can't tear down the struct_mm while an IOCTL is still using.

I am not so sure. We are not in the mm destruction phase yet. This is mostly about the oom context which might fire right during the IOCTL. If any of the path which is holding the write lock blocks for unbound amount of time or even worse allocates a memory then we are screwed. So we need to back of when blockable = false.

Quoted 6 lines

Quoted 6 lines

Is that correct Christian? If this is correct then we need to update the locking here. I am struggling to grasp the ref counting part. Why cannot all readers simply take the lock rather than rely on somebody else to take it? 1ed3d2567c800 didn't really help me to understand the locking scheme here so any help would be appreciated.

Quoted 4 lines

That won't work like this there might be multiple invalidate_range_start()/invalidate_range_end() pairs open at the same time. E.g. the lock might be taken recursively and that is illegal for a rw_semaphore.

I am not sure I follow. Are you saying that one invalidate_range might trigger another one from the same path?

Am 24.08.2018 um 13:52 schrieb Michal Hocko:

Quoted 1 line · Christian König

Quoted 6 lines

Quoted 4 lines

That won't work like this there might be multiple invalidate_range_start()/invalidate_range_end() pairs open at the same time. E.g. the lock might be taken recursively and that is illegal for a rw_semaphore.

I am not sure I follow. Are you saying that one invalidate_range might trigger another one from the same path?

No, but what can happen is:

invalidate_range_start(A,B);
invalidate_range_start(C,D);
...
invalidate_range_end(C,D);
invalidate_range_end(A,B);

Grabbing the read lock twice would be illegal in this case.

Quoted 2 lines

Am 24.08.2018 um 13:52 schrieb Michal Hocko:

Quoted 1 line · Christian König

[...]

Quoted 14 lines

Quoted 6 lines

Quoted 4 lines

That won't work like this there might be multiple invalidate_range_start()/invalidate_range_end() pairs open at the same time. E.g. the lock might be taken recursively and that is illegal for a rw_semaphore.

I am not sure I follow. Are you saying that one invalidate_range might trigger another one from the same path?

No, but what can happen is:

invalidate_range_start(A,B);
invalidate_range_start(C,D);
...
invalidate_range_end(C,D);
invalidate_range_end(A,B);

Quoted 1 line

Grabbing the read lock twice would be illegal in this case.

I am sorry but I still do not follow. What is the context the two are called from?

Can you give me an example. I simply do not see it in the code, mostly because I am not familiar with it.

Am 24.08.2018 um 14:03 schrieb Michal Hocko:

Quoted 18 lines · Christian König

Quoted 2 lines

Am 24.08.2018 um 13:52 schrieb Michal Hocko:

Quoted 1 line · Christian König

[...]

Quoted 14 lines

Quoted 6 lines

Quoted 4 lines

That won't work like this there might be multiple invalidate_range_start()/invalidate_range_end() pairs open at the same time. E.g. the lock might be taken recursively and that is illegal for a rw_semaphore.

I am not sure I follow. Are you saying that one invalidate_range might trigger another one from the same path?

No, but what can happen is:

invalidate_range_start(A,B);
invalidate_range_start(C,D);
...
invalidate_range_end(C,D);
invalidate_range_end(A,B);

Quoted 3 lines

Quoted 1 line

Grabbing the read lock twice would be illegal in this case.

I am sorry but I still do not follow. What is the context the two are called from?

I don't have the slightest idea.

Quoted 2 lines

Can you give me an example. I simply do not see it in the code, mostly because I am not familiar with it.

I'm neither.

We stumbled over that by pure observation and after discussing the problem with Jerome came up with this solution.

No idea where exactly that case comes from, but I can confirm that it indeed happens.

Quoted 26 lines

Am 24.08.2018 um 14:03 schrieb Michal Hocko:

Quoted 18 lines · Christian König

Quoted 2 lines

Am 24.08.2018 um 13:52 schrieb Michal Hocko:

Quoted 1 line · Christian König

[...]

Quoted 14 lines

Quoted 6 lines

Quoted 4 lines

That won't work like this there might be multiple invalidate_range_start()/invalidate_range_end() pairs open at the same time. E.g. the lock might be taken recursively and that is illegal for a rw_semaphore.

I am not sure I follow. Are you saying that one invalidate_range might trigger another one from the same path?

No, but what can happen is:

invalidate_range_start(A,B);
invalidate_range_start(C,D);
...
invalidate_range_end(C,D);
invalidate_range_end(A,B);

Quoted 3 lines

Quoted 1 line

Grabbing the read lock twice would be illegal in this case.

I am sorry but I still do not follow. What is the context the two are called from?

I don't have the slightest idea.

Quoted 2 lines

Can you give me an example. I simply do not see it in the code, mostly because I am not familiar with it.

Quoted 7 lines

I'm neither.

We stumbled over that by pure observation and after discussing the problem with Jerome came up with this solution.

No idea where exactly that case comes from, but I can confirm that it indeed happens.

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.

Am 24.08.2018 um 14:33 schrieb Michal Hocko:

Quoted 9 lines

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.

Quoted 1 line

Am 24.08.2018 um 14:33 schrieb Michal Hocko:

[...]

Quoted 12 lines

Quoted 9 lines

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.

Quoted 2 lines

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.

Am 24.08.2018 um 15:01 schrieb Michal Hocko:

Quoted 15 lines · Christian König

Quoted 1 line

Am 24.08.2018 um 14:33 schrieb Michal Hocko:

[...]

Quoted 12 lines

Quoted 9 lines

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.

Quoted 6 lines

Quoted 2 lines

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....

Quoted 22 lines

Am 24.08.2018 um 15:01 schrieb Michal Hocko:

Quoted 21 lines · Christian König

Quoted 1 line

Am 24.08.2018 um 14:33 schrieb Michal Hocko:

[...]

Quoted 9 lines

Quoted 9 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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?

Am 24.08.2018 um 15:24 schrieb Michal Hocko:

Quoted 26 lines · Christian König

Quoted 25 lines

Am 24.08.2018 um 15:01 schrieb Michal Hocko:

Quoted 15 lines · Christian König

Quoted 1 line

Am 24.08.2018 um 14:33 schrieb Michal Hocko:

[...]

Quoted 12 lines

Quoted 9 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 3 lines

Quoted 2 lines

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.

Quoted 29 lines

Am 24.08.2018 um 15:24 schrieb Michal Hocko:

Quoted 28 lines · Christian König

Quoted 27 lines

Am 24.08.2018 um 15:01 schrieb Michal Hocko:

Quoted 15 lines · Christian König

Quoted 1 line

Am 24.08.2018 um 14:33 schrieb Michal Hocko:

[...]

Quoted 12 lines

Quoted 9 lines

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.

Quoted 6 lines

Quoted 2 lines

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....

Quoted 5 lines

Quoted 1 line

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.

So does this looks good to
you?

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +2 -5

index e55508b39496..48fa152231be 100644
@@ -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

Am 24.08.2018 um 15:40 schrieb Michal Hocko:

Quoted 30 lines · Christian König

Quoted 29 lines

Am 24.08.2018 um 15:24 schrieb Michal Hocko:

Quoted 28 lines · Christian König

Quoted 27 lines

Am 24.08.2018 um 15:01 schrieb Michal Hocko:

Quoted 15 lines · Christian König

Quoted 1 line

Am 24.08.2018 um 14:33 schrieb Michal Hocko:

[...]

Quoted 12 lines

Quoted 9 lines

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.

Quoted 6 lines

Quoted 2 lines

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....

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

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.

Quoted 1 line

So does this looks good to you?

Yeah, that looks perfect to me. Reviewed-by: Christian König <[email protected]>

(quoted diff) +2 -5

@@ -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

Quoted 18 lines

(quoted diff) +9 -5

@@ -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);

Review void amdgpu_mn_unlock(struct amdgpu_mn *mn) · line 194

Quoted 4 lines

(quoted diff) +0 -0

  	if (atomic_inc_return(&amn->recursion) == 1)
  		down_read_non_owner(&amn->lock);
  	mutex_unlock(&amn->read_lock);

I'm not following. Why don't we need to do like below (given that nobody except amdgpu_mn_read_lock() holds ->read_lock) because e.g. drm_sched_fence_create() from drm_sched_job_init() from amdgpu_cs_submit() is doing GFP_KERNEL memory allocation with ->lock held for write?

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +3 -13

index e55508b..e1cb344 100644
@@ -64,8 +64,6 @@
   * @node: hash table node to find structure by adev and mn
   * @lock: rw semaphore protecting the notifier nodes
   * @objects: interval tree containing amdgpu_mn_nodes
- * @read_lock: mutex for recursive locking of @lock
- * @recursion: depth of recursion
   *
   * Data for each amdgpu device and process address space.
   */

@@ -85,8 +83,6 @@ struct amdgpu_mn {
  	/* objects protected by lock */
  	struct rw_semaphore	lock;
  	struct rb_root_cached	objects;
-	struct mutex		read_lock;
-	atomic_t		recursion;
  };
 
  /**

@@ -181,14 +177,9 @@ 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))
+		down_read(&amn->lock);
+	else if (!down_read_trylock(&amn->lock))
  		return -EAGAIN;
-
-	if (atomic_inc_return(&amn->recursion) == 1)
-		down_read_non_owner(&amn->lock);
-	mutex_unlock(&amn->read_lock);
-
  	return 0;
  }
 

@@ -199,8 +190,7 @@ static int amdgpu_mn_read_lock(struct amdgpu_mn *amn, bool blockable)
   */
  static void amdgpu_mn_read_unlock(struct amdgpu_mn *amn)
  {
-	if (atomic_dec_return(&amn->recursion) == 0)
-		up_read_non_owner(&amn->lock);
+	up_read(&amn->lock);
  }
 
  /**

Am 26.08.2018 um 10:40 schrieb Tetsuo Handa:

Quoted 4 lines

I'm not following. Why don't we need to do like below (given that nobody except amdgpu_mn_read_lock() holds ->read_lock) because e.g. drm_sched_fence_create() from drm_sched_job_init() from amdgpu_cs_submit() is doing GFP_KERNEL memory allocation with ->lock held for write?

That's a bug which needs to be fixed separately.

Allocating memory with GFP_KERNEL while holding a lock which is also taken in the reclaim code path is illegal not matter what you do.

Patches to fix this are already on the appropriate mailing list and will be pushed upstream today.

Regards,
Christian.

Am 26.08.2018 um 10:40 schrieb Tetsuo Handa:

Quoted 4 lines

I'm not following. Why don't we need to do like below (given that nobody except amdgpu_mn_read_lock() holds ->read_lock) because e.g. drm_sched_fence_create() from drm_sched_job_init() from amdgpu_cs_submit() is doing GFP_KERNEL memory allocation with ->lock held for write?

That's a bug which needs to be fixed separately.

Allocating memory with GFP_KERNEL while holding a lock which is also taken in the reclaim code path is illegal not matter what you do.

Patches to fix this are already on the appropriate mailing list and will be pushed upstream today.

Regards,
Christian.

Quoted 18 lines

(quoted diff) +9 -5

@@ -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);

Review void amdgpu_mn_unlock(struct amdgpu_mn *mn) · line 194

Quoted 4 lines

(quoted diff) +0 -0

  	if (atomic_inc_return(&amn->recursion) == 1)
  		down_read_non_owner(&amn->lock);
  	mutex_unlock(&amn->read_lock);

I'm not following. Why don't we need to do like below (given that nobody except amdgpu_mn_read_lock() holds ->read_lock) because e.g. drm_sched_fence_create() from drm_sched_job_init() from amdgpu_cs_submit() is doing GFP_KERNEL memory allocation with ->lock held for write?

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +3 -13

index e55508b..e1cb344 100644
@@ -64,8 +64,6 @@
   * @node: hash table node to find structure by adev and mn
   * @lock: rw semaphore protecting the notifier nodes
   * @objects: interval tree containing amdgpu_mn_nodes
- * @read_lock: mutex for recursive locking of @lock
- * @recursion: depth of recursion
   *
   * Data for each amdgpu device and process address space.
   */

@@ -85,8 +83,6 @@ struct amdgpu_mn {
  	/* objects protected by lock */
  	struct rw_semaphore	lock;
  	struct rb_root_cached	objects;
-	struct mutex		read_lock;
-	atomic_t		recursion;
  };
 
  /**

@@ -181,14 +177,9 @@ 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))
+		down_read(&amn->lock);
+	else if (!down_read_trylock(&amn->lock))
  		return -EAGAIN;
-
-	if (atomic_inc_return(&amn->recursion) == 1)
-		down_read_non_owner(&amn->lock);
-	mutex_unlock(&amn->read_lock);
-
  	return 0;
  }
 

@@ -199,8 +190,7 @@ static int amdgpu_mn_read_lock(struct amdgpu_mn *amn, bool blockable)
   */
  static void amdgpu_mn_read_unlock(struct amdgpu_mn *amn)
  {
-	if (atomic_dec_return(&amn->recursion) == 0)
-		up_read_non_owner(&amn->lock);
+	up_read(&amn->lock);
  }
 
  /**

(quoted diff) +2 -5

@@ -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

Am 24.08.2018 um 15:40 schrieb Michal Hocko:

Quoted 30 lines · Christian König

Quoted 29 lines

Am 24.08.2018 um 15:24 schrieb Michal Hocko:

Quoted 28 lines · Christian König

Quoted 27 lines

Am 24.08.2018 um 15:01 schrieb Michal Hocko:

Quoted 15 lines · Christian König

Quoted 1 line

Am 24.08.2018 um 14:33 schrieb Michal Hocko:

[...]

Quoted 12 lines

Quoted 9 lines

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.

Quoted 6 lines

Quoted 2 lines

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....

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

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.

Quoted 1 line

So does this looks good to you?

Yeah, that looks perfect to me. Reviewed-by: Christian König <[email protected]>

Quoted 29 lines

Am 24.08.2018 um 15:24 schrieb Michal Hocko:

Quoted 28 lines · Christian König

Quoted 27 lines

Am 24.08.2018 um 15:01 schrieb Michal Hocko:

Quoted 15 lines · Christian König

Quoted 1 line

Am 24.08.2018 um 14:33 schrieb Michal Hocko:

[...]

Quoted 12 lines

Quoted 9 lines

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.

Quoted 6 lines

Quoted 2 lines

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....

Quoted 5 lines

Quoted 1 line

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.

So does this looks good to
you?

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +2 -5

index e55508b39496..48fa152231be 100644
@@ -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

Am 24.08.2018 um 15:24 schrieb Michal Hocko:

Quoted 26 lines · Christian König

Quoted 25 lines

Am 24.08.2018 um 15:01 schrieb Michal Hocko:

Quoted 15 lines · Christian König

Quoted 1 line

Am 24.08.2018 um 14:33 schrieb Michal Hocko:

[...]

Quoted 12 lines

Quoted 9 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 3 lines

Quoted 2 lines

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.

Quoted 22 lines

Am 24.08.2018 um 15:01 schrieb Michal Hocko:

Quoted 21 lines · Christian König

Quoted 1 line

Am 24.08.2018 um 14:33 schrieb Michal Hocko:

[...]

Quoted 9 lines

Quoted 9 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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?

Am 24.08.2018 um 15:01 schrieb Michal Hocko:

Quoted 15 lines · Christian König

Quoted 1 line

Am 24.08.2018 um 14:33 schrieb Michal Hocko:

[...]

Quoted 12 lines

Quoted 9 lines

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.

Quoted 6 lines

Quoted 2 lines

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....

Quoted 1 line

Am 24.08.2018 um 14:33 schrieb Michal Hocko:

[...]

Quoted 12 lines

Quoted 9 lines

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.

Quoted 2 lines

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.

Am 24.08.2018 um 14:33 schrieb Michal Hocko:

Quoted 9 lines

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.

Quoted 26 lines

Am 24.08.2018 um 14:03 schrieb Michal Hocko:

Quoted 18 lines · Christian König

Quoted 2 lines

Am 24.08.2018 um 13:52 schrieb Michal Hocko:

Quoted 1 line · Christian König

[...]

Quoted 14 lines

Quoted 6 lines

Quoted 4 lines

That won't work like this there might be multiple invalidate_range_start()/invalidate_range_end() pairs open at the same time. E.g. the lock might be taken recursively and that is illegal for a rw_semaphore.

I am not sure I follow. Are you saying that one invalidate_range might trigger another one from the same path?

No, but what can happen is:

invalidate_range_start(A,B);
invalidate_range_start(C,D);
...
invalidate_range_end(C,D);
invalidate_range_end(A,B);

Quoted 3 lines

Quoted 1 line

Grabbing the read lock twice would be illegal in this case.

I am sorry but I still do not follow. What is the context the two are called from?

I don't have the slightest idea.

Quoted 2 lines

Can you give me an example. I simply do not see it in the code, mostly because I am not familiar with it.

Quoted 7 lines

I'm neither.

We stumbled over that by pure observation and after discussing the problem with Jerome came up with this solution.

No idea where exactly that case comes from, but I can confirm that it indeed happens.

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.

Quoted 28 lines · Christian König

Am 24.08.2018 um 14:03 schrieb Michal Hocko:

Quoted 18 lines · Christian König

Quoted 2 lines

Am 24.08.2018 um 13:52 schrieb Michal Hocko:

Quoted 1 line · Christian König

[...]

Quoted 14 lines

Quoted 6 lines

Quoted 4 lines

That won't work like this there might be multiple invalidate_range_start()/invalidate_range_end() pairs open at the same time. E.g. the lock might be taken recursively and that is illegal for a rw_semaphore.

I am not sure I follow. Are you saying that one invalidate_range might trigger another one from the same path?

No, but what can happen is:

invalidate_range_start(A,B);
invalidate_range_start(C,D);
...
invalidate_range_end(C,D);
invalidate_range_end(A,B);

Quoted 3 lines

Quoted 1 line

Grabbing the read lock twice would be illegal in this case.

I am sorry but I still do not follow. What is the context the two are called from?

I don't have the slightest idea.

Quoted 2 lines

Can you give me an example. I simply do not see it in the code, mostly because I am not familiar with it.

Quoted 7 lines

I'm neither.

We stumbled over that by pure observation and after discussing the problem with Jerome came up with this solution.

No idea where exactly that case comes from, but I can confirm that it indeed happens.