Skip to contentSkip to conversation

mm: mempolicy: Check return code of check_range

PATCH In review Kulikov Vasiliy 4 participants 9 messages Sep 5, 2010 – Sep 7, 2010

Subsystems (3)
Memory management Kernel janitors MM NUMA placement
lore
Share & tools
lore

Kulikov Vasiliy

author
#0 lore raw

PATCH 13/14 mm: mempolicy: Check return code of check_range

From: Vasiliy Kulikov <[email protected]>

Function check_range may return ERR_PTR(...). Check for it.

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

---
Compile tested.

 mm/mempolicy.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
mm/mempolicy.c +4 -1
index f969da5..b73f02c 100644
@@ -924,12 +924,15 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest,
 	nodemask_t nmask;
 	LIST_HEAD(pagelist);
 	int err = 0;
+	struct vm_area_struct *vma;
 
 	nodes_clear(nmask);
 	node_set(source, nmask);
 
-	check_range(mm, mm->mmap->vm_start, mm->task_size, &nmask,
+	vma = check_range(mm, mm->mmap->vm_start, mm->task_size, &nmask,
 			flags | MPOL_MF_DISCONTIG_OK, &pagelist);
+	if (IS_ERR(vma))
+		return PTR_ERR(vma);
 
 	if (!list_empty(&pagelist))
 		err = migrate_pages(&pagelist, new_node_page, dest, 0);
Signature · 1 line
1.7.0.4
#3 lore raw
In reply to Kulikov Vasiliy
Quoted 3 lines

From: Vasiliy Kulikov <[email protected]>

Function check_range may return ERR_PTR(...). Check for it.

When happen this issue?

afaik, check_range return error when following condition.
1) mm->mmap->vm_start argument is incorrect
2) don't have neigher MPOL_MF_STATS, MPOL_MF_MOVE and MPOL_MF_MOVE_ALL

I think both case is not happen in real. Am I overlooking anything?

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

---
Compile tested.

 mm/mempolicy.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
mm/mempolicy.c +4 -1
index f969da5..b73f02c 100644
@@ -924,12 +924,15 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest,
 	nodemask_t nmask;
 	LIST_HEAD(pagelist);
 	int err = 0;
+	struct vm_area_struct *vma;
 
 	nodes_clear(nmask);
 	node_set(source, nmask);
 
-	check_range(mm, mm->mmap->vm_start, mm->task_size, &nmask,
+	vma = check_range(mm, mm->mmap->vm_start, mm->task_size, &nmask,
 			flags | MPOL_MF_DISCONTIG_OK, &pagelist);
+	if (IS_ERR(vma))
+		return PTR_ERR(vma);
 
 	if (!list_empty(&pagelist))
 		err = migrate_pages(&pagelist, new_node_page, dest, 0);
Signature · 1 line
1.7.0.4
#4 lore raw
Quoted 4 lines · KOSAKI Motohiro
Quoted 3 lines

From: Vasiliy Kulikov <[email protected]>

Function check_range may return ERR_PTR(...). Check for it.

Quoted 8 lines

When happen this issue?

afaik, check_range return error when following condition.
1) mm->mmap->vm_start argument is incorrect
2) don't have neigher MPOL_MF_STATS, MPOL_MF_MOVE and MPOL_MF_MOVE_ALL

I think both case is not happen in real. Am I overlooking anything?

There's no reason not to check the return value of a function when the implementation of either could change at any time. migrate_to_node() is certainly not in any fastpath where we can't sacrifice a branch for more robust code.

author
#5 lore raw
Quoted 12 lines · David Rientjes
Quoted 10 lines · KOSAKI Motohiro
Quoted 3 lines

From: Vasiliy Kulikov <[email protected]>

Function check_range may return ERR_PTR(...). Check for it.

When happen this issue?

afaik, check_range return error when following condition.
1) mm->mmap->vm_start argument is incorrect
2) don't have neigher MPOL_MF_STATS, MPOL_MF_MOVE and MPOL_MF_MOVE_ALL

Quoted 7 lines
Quoted 2 lines

I think both case is not happen in real. Am I overlooking anything?

There's no reason not to check the return value of a function when the implementation of either could change at any time. migrate_to_node() is certainly not in any fastpath where we can't sacrifice a branch for more robust code.

Agreed, if you know that the caller must check input data and must not check return code, it's better to make this function return void.

Signature · 1 line
Vasiliy
#6 lore raw
In reply to David Rientjes
Quoted 12 lines · KOSAKI Motohiro
Quoted 11 lines
Quoted 4 lines

From: Vasiliy Kulikov <[email protected]>

Function check_range may return ERR_PTR(...). Check for it.

When happen this issue?

afaik, check_range return error when following condition.
1) mm->mmap->vm_start argument is incorrect
2) don't have neigher MPOL_MF_STATS, MPOL_MF_MOVE and MPOL_MF_MOVE_ALL

Quoted 7 lines
Quoted 2 lines

I think both case is not happen in real. Am I overlooking anything?

There's no reason not to check the return value of a function when the implementation of either could change at any time. migrate_to_node() is certainly not in any fastpath where we can't sacrifice a branch for more robust code.

I was not against this change. I was asking patch effectness.

#7 lore raw
In reply to KOSAKI Motohiro
Quoted 1 line · KOSAKI Motohiro

I think both case is not happen in real. Am I overlooking anything?

Its good to check the return code regardless. There is a lot of tinkering going on with that code.

Reviewed-by
#8 lore raw
Quoted 6 lines · KOSAKI Motohiro
Quoted 2 lines

I think both case is not happen in real. Am I overlooking anything?

Its good to check the return code regardless. There is a lot of tinkering going on with that code.

OK. so, I'm convinced this is not -stable material.

Reviewed-by KOSAKI Motohiro <[email protected]>