[PATCH V2] staging: android: lowmemorykiller: imporve lmk to avoid deadlock issue

[PATCH V2] staging: android: lowmemorykiller: imporve lmk to avoid deadlock issue

[Wang, Biao]

Consider the following case:
Task A trigger lmk with a lock held, while task B try to
get this lock, but unfortunately B is the very culprit task lmk select to
kill. Then B will never be killed, and A will forever select B to kill.
Such dead lock will trigger softlock up issue.

This patch try to pick the next task to break this loop.

Signed-off-by: Wang Biao <biao.wang@intel.com>
Reviewed-by: Zhang Di <di.zhang@intel.com>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Joe Perches <joe@perches.com>
---
 drivers/staging/android/lowmemorykiller.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/android/lowmemorykiller.c b/drivers/staging/android/lowmemorykiller.c
index feafa17..23d9832 100644
--- a/drivers/staging/android/lowmemorykiller.c
+++ b/drivers/staging/android/lowmemorykiller.c
@@ -127,9 +127,10 @@ static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)
 		if (!p)
 			continue;
 
-		if (test_tsk_thread_flag(p, TIF_MEMDIE) &&
-		    time_before_eq(jiffies, lowmem_deathpending_timeout)) {
+		if (test_tsk_thread_flag(p, TIF_MEMDIE)) {
 			task_unlock(p);
+			if (time_after(jiffies, lowmem_deathpending_timeout)) 
+				continue;
 			rcu_read_unlock();
 			return 0;
 		}
-- 
1.7.9.5

Re: [PATCH V2] staging: android: lowmemorykiller: imporve lmk to avoid deadlock issue

[Dan Carpenter]

On Mon, Aug 03, 2015 at 05:53:22AM +0000, Wang, Biao wrote:
> Consider the following case:
> Task A trigger lmk with a lock held, while task B try to
> get this lock, but unfortunately B is the very culprit task lmk select to
> kill. Then B will never be killed, and A will forever select B to kill.
> Such dead lock will trigger softlock up issue.
> 
> This patch try to pick the next task to break this loop.
> 
> Signed-off-by: Wang Biao <biao.wang@intel.com>
> Reviewed-by: Zhang Di <di.zhang@intel.com>
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

I don't really feel comfortable saying I reviewed this code.  I just
commented on a few process issues.  I don't know the subsystem well
enough to give it a seal of approval.

> Reviewed-by: Joe Perches <joe@perches.com>

regards,
dan carpenter

Re: [PATCH V2] staging: android: lowmemorykiller: imporve lmk to avoid deadlock issue

[Dan Carpenter]

On Mon, Aug 03, 2015 at 09:15:56AM +0300, Dan Carpenter wrote:
> > Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> I don't really feel comfortable saying I reviewed this code.  I just
> commented on a few process issues.  I don't know the subsystem well
> enough to give it a seal of approval.
> 

Biao was asking off list, how to fix this.  I guess just leave it.  I
more care about going forward that we don't start doing this all the
time.

I recently added a reviewed-by tag for someone.  He told me off list
that the patch "looks OK".  He was the subsystem expert and we were
patching his code so it was his responsibility to review the fix.  If
the patch breaks everyone's system then he's absolutely the guy that I
want people to blame.  :)

regards,
dan carpenter

Re: [PATCH V2] staging: android: lowmemorykiller: imporve lmk to avoid deadlock issue

[Joe Perches]

On Mon, 2015-08-03 at 09:15 +0300, Dan Carpenter wrote:
> On Mon, Aug 03, 2015 at 05:53:22AM +0000, Wang, Biao wrote:
> > Consider the following case:
> > Task A trigger lmk with a lock held, while task B try to
> > get this lock, but unfortunately B is the very culprit task lmk 
> > select to
> > kill. Then B will never be killed, and A will forever select B to 
> > kill.
> > Such dead lock will trigger softlock up issue.
> > 
> > This patch try to pick the next task to break this loop.
> > 
> > Signed-off-by: Wang Biao <biao.wang@intel.com>
> > Reviewed-by: Zhang Di <di.zhang@intel.com>
> > Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> I don't really feel comfortable saying I reviewed this code.  I just
> commented on a few process issues.  I don't know the subsystem well
> enough to give it a seal of approval.
> 
> > Reviewed-by: Joe Perches <joe@perches.com>

Please don't say I reviewed this either.

I may have commented on it, but I certainly
did't add a "Reviewed-by" signature.

Please don't add signatures unless people
specifically state so.

Re: [PATCH V2] staging: android: lowmemorykiller: imporve lmk to avoid deadlock issue

[Dave Hansen]

On 08/02/2015 10:53 PM, Wang, Biao wrote:
> Consider the following case:
> Task A trigger lmk with a lock held, while task B try to
> get this lock, but unfortunately B is the very culprit task lmk select to
> kill. Then B will never be killed, and A will forever select B to kill.
> Such dead lock will trigger softlock up issue.

It would be interesting to have some actual data about where this helps.
 For instance, which locks does this happen on?  What kind of
allocation?  Also, we apparently _do_ mark a lowmemorykiller victim as
an oom victim and let them use memory reserves.  Why does that not allow
the allocation to complete at least long enough to get the kill signal
to the victim?