on 2009-1-21 18:41 Paul Menage wrote:
> On Wed, Jan 21, 2009 at 12:06 AM, Miao Xie <miaox@cn.fujitsu.com> wrote:
>> The task still allocated the page caches on old node after modifying its
>> cpuset's mems when 'memory_spread_page' was set, it is caused by the old
>> mem_allowed_list of the task, the current kernel doesn't updates it unless some
>> function invokes cpuset_update_task_memory_state(), it is too late sometimes.
> 
> Can you give a more concrete example of how the current code can break?

I'm sorry for replying late.

A test program which reproduces the problem on current kernel is attached.
This program just repeats reading the file "DATAFILE" after receiving SIGUSR1
and exits after receiving SIGINT.
	Test Task
	  sigsuspend();
	  while(!end) {
		read DATAFILE;
		sigsuspend();
	  }

Steps to reproduce:
# mkdir /dev/cpuset
# mount -t cpuset xxx /dev/cpuset
# mkdir /dev/cpuset/0
# echo 0 > /dev/cpuset/0/cpus
# echo 1-2 > /dev/cpuset/0/mems
# echo 1 > /dev/cpuset/0/memory_spread_page
# dd if=/dev/zero of=DATAFILE bs=4096 count=20480	<- create a 80M file
# ./mem-hog &
# tst_pid=$!
# cat /proc/$tst_pid/status
...
Mems_allowed:   00000000,00000007
Mems_allowed_list:      0-2
...
# echo $tst_pid > /dev/cpuset/0/tasks
# echo 3 > /proc/sys/vm/drop_caches
# find /sys/devices/system/node/ -name "meminfo" -exec cat {} + | grep "FilePages"
Node 0 FilePages:         15488 kB
Node 1 FilePages:             0 kB
Node 2 FilePages:             0 kB
# cat /proc/$tst_pid/status
...
Mems_allowed:   00000000,00000007		<- old memory list
Mems_allowed_list:      0-2
...
# kill -s sigusr1 $tst_pid

	...wait a moment...

# find /sys/devices/system/node/ -name "meminfo" -exec cat {} + | grep "FilePages"
Node 0 FilePages:         97548 kB
Node 1 FilePages:             0 kB
Node 2 FilePages:             0 kB
# cat /proc/$tst_pid/status
...
Mems_allowed:   00000000,00000007               <- old memory list
Mems_allowed_list:      0-2
...

>> We must update the mem_allowed_list of the tasks in time.
> 
> This is a fairly fundamental change to the way that mems_allowed is
> handled - it dates back to fairly early in cpusets' history.

I found Mr. Paul Jackson had discussed it with somebody:

  http://lkml.org/lkml/2004/8/11/211

According to what Mr. Paul Jackson said, my patch has a serious bug.
Maybe we fix this bug just by choosing a good callsite for
cpuset_update_task_memory_state().

Thanks!
Miao

>> - * The task_struct fields mems_allowed and mems_generation may only
>> - * be accessed in the context of that task, so require no locks.
>> + * The task_struct fields mems_allowed may only be accessed in the context
>> + * of that task, so require no locks.
> 
> This comment is no longer true, since mems_allowed is now being
> updated by other processes.
> 
> What's to stop a task reading current->mems_allowed and racing with an
> update from update_tasks_nodemask() ? Or else, why can that not lead
> to badness?
> 
> Paul
> 
> 
>