Skip to contentSkip to conversation

mm: oom_kill: use IS_ERR() instead of strict checking

PATCH In review Kulikov Vasiliy 3 participants 4 messages Sep 5, 2010 – Sep 6, 2010

Subsystems (3)
Memory management Kernel janitors Memory management - OOM killer
lore
Share & tools
lore

Kulikov Vasiliy

author
#0 lore raw

PATCH 14/14 mm: oom_kill: use IS_ERR() instead of strict checking

From: Vasiliy Kulikov <[email protected]>

Use IS_ERR() instead of strict checking.

Signed-off-by Vasiliy Kulikov <[email protected]>

---
Compile tested.

 mm/oom_kill.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
mm/oom_kill.c +2 -2
index fc81cb2..2ee3350 100644
@@ -514,7 +514,7 @@ void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
 	read_lock(&tasklist_lock);
 retry:
 	p = select_bad_process(&points, limit, mem, NULL);
-	if (!p || PTR_ERR(p) = -1UL)
+	if (IS_ERR_OR_NULL(p))
 		goto out;
 
 	if (oom_kill_process(p, gfp_mask, 0, points, limit, mem, NULL,

@@ -691,7 +691,7 @@ retry:
 	p = select_bad_process(&points, totalpages, NULL,
 			constraint = CONSTRAINT_MEMORY_POLICY ? nodemask :
 								 NULL);
-	if (PTR_ERR(p) = -1UL)
+	if (IS_ERR(p))
 		goto out;
 
 	/* Found nothing?!?! Either we hang forever, or we panic. */
Signature · 1 line
1.7.0.4
#2 lore raw
In reply to Kulikov Vasiliy
Quoted 3 lines

From: Vasiliy Kulikov <[email protected]>

Use IS_ERR() instead of strict checking.

Umm...

I don't like this. IS_ERR() imply an argument is error code. but in this case, we don't use error code. -1 mean oom special purpose meaning value.

So, if we take this direction, It would be better to use EAGAIN or something instead -1.

Quoted 33 lines
Signed-off-by Vasiliy Kulikov <[email protected]>

---
Compile tested.

 mm/oom_kill.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
mm/oom_kill.c +2 -2
index fc81cb2..2ee3350 100644
@@ -514,7 +514,7 @@ void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
 	read_lock(&tasklist_lock);
 retry:
 	p = select_bad_process(&points, limit, mem, NULL);
-	if (!p || PTR_ERR(p) = -1UL)
+	if (IS_ERR_OR_NULL(p))
 		goto out;
 
 	if (oom_kill_process(p, gfp_mask, 0, points, limit, mem, NULL,

@@ -691,7 +691,7 @@ retry:
 	p = select_bad_process(&points, totalpages, NULL,
 			constraint = CONSTRAINT_MEMORY_POLICY ? nodemask :
 								 NULL);
-	if (PTR_ERR(p) = -1UL)
+	if (IS_ERR(p))
 		goto out;
 
 	/* Found nothing?!?! Either we hang forever, or we panic. */
Signature · 1 line
1.7.0.4
reviewer
#3 lore raw
Quoted 10 lines · KOSAKI Motohiro
Quoted 3 lines

From: Vasiliy Kulikov <[email protected]>

Use IS_ERR() instead of strict checking.

Umm...

I don't like this. IS_ERR() imply an argument is error code. but in this case, we don't use error code. -1 mean oom special purpose meaning value.

You could make the same argument by saying the current use of PTR_ERR() implies an error code. We've simply hijacked -1UL for simplicity in this case and because select_bad_process() can only return one other value besides a pointer to a process or NULL.

Quoted 3 lines

So, if we take this direction, It would be better to use EAGAIN or something instead -1.

I agree it would probably better to return ERR_PTR(-EAGAIN) instead of using -1UL.