On 9/17/16 23:46, Michal Hocko wrote:

On Sat 17-09-16 15:20:36, chengang@emindsoft.com.cn wrote:

quoted

Also change their related pure Boolean function numamigrate_isolate_page.

this is not true. Just look at the current usage

	migrated = migrate_misplaced_page(page, vma, target_nid);
	if (migrated) {
		page_nid = target_nid;
		flags |= TNF_MIGRATED;
	} else
		flags |= TNF_MIGRATE_FAIL;

and now take your change which changes -EAGAIN into false. See the
difference? Now I didn't even try to understand why
CONFIG_NUMA_BALANCING=n pretends a success but then in order to keep the
current semantic your patch should return true in that path. So NAK from
me until you either explain why this is OK or change it.

For me, it really need return false:

 - For real implementation, when do nothing, it will return false.

 - I assume that the input page already is in a node (although maybe my
   assumption incorrect), and migrate to the same node. When the real
   implementation fails (e.g. -EAGAIN 10 times), it still returns false.

 - Original dummy implementation always return -EAGAIN, And -EAGAIN in
   real implementation will trigger returning false, after 10 times.

 - After grep TNF_MIGRATE_FAIL and TNF_MIGRATED, we only use them in
   task_numa_fault in kernel/sched/fair.c for numa_pages_migrated and
   numa_faults_locality, I guess they are only used for statistics.

So for me the dummy implementation need return false instead of -EAGAIN.
 

But to be honest I am not keen of this int -> bool changes much.
Especially if they are bringing a risk of subtle behavior change like
this patch. And without a good changelog explaining why this makes
sense.

If our original implementation already used bool, our this issue (return
-EAGAIN) would be avoided (compiler would help us to find this issue).


Thanks.
-- 
Chen Gang (e??a??)

Managing Natural Environments is the Duty of Human Beings.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>