Re: [PATCH 4/4] cpuset,mm: use rwlock to protect task->mempolicy and mems_allowed

[Miao Xie <miaox@cn.fujitsu.com>]

on 2010-3-4 7:50, Andrew Morton wrote:
> On Wed, 03 Mar 2010 18:52:39 +0800
> Miao Xie <miaox@cn.fujitsu.com> wrote:
> 
>> if MAX_NUMNODES > BITS_PER_LONG, loading/storing task->mems_allowed or mems_allowed in
>> task->mempolicy are not atomic operations, and the kernel page allocator gets an empty
>> mems_allowed when updating task->mems_allowed or mems_allowed in task->mempolicy. So we
>> use a rwlock to protect them to fix this probelm.
> 
> Boy, that is one big ugly patch.  Is there no other way of doing this?

Let me consider!

> 
>>
>> ...
>>
>> --- a/include/linux/mempolicy.h
>> +++ b/include/linux/mempolicy.h
>> @@ -51,6 +51,7 @@ enum {
>>   */
>>  #define MPOL_F_SHARED  (1 << 0)	/* identify shared policies */
>>  #define MPOL_F_LOCAL   (1 << 1)	/* preferred local allocation */
>> +#define MPOL_F_TASK    (1 << 2)	/* identify tasks' policies */
> 
> What's this?  It wasn't mentioned in the changelog - I suspect it
> should have been?

I hope task->mempolicy has the same get/put operation just like shared mempolicy,
this new feature is used when the kernel memory allocater accesses
task->mempolicy.

I'll rewrite the changelog in the next version of the patch if I still
use this flag.

>> +int cpuset_mems_allowed_intersects(struct task_struct *tsk1,
>> +				   struct task_struct *tsk2)
>>  {
>> -	return nodes_intersects(tsk1->mems_allowed, tsk2->mems_allowed);
>> +	unsigned long flags1, flags2;
>> +	int retval;
>> +
>> +	read_mem_lock_irqsave(tsk1, flags1);
>> +	read_mem_lock_irqsave(tsk2, flags2);
>> +	retval = nodes_intersects(tsk1->mems_allowed, tsk2->mems_allowed);
>> +	read_mem_unlock_irqrestore(tsk2, flags2);
>> +	read_mem_unlock_irqrestore(tsk1, flags1);
> 
> I suspect this is deadlockable in sufficiently arcane circumstances:
> one task takes the locks in a,b order, another task takes them in b,a
> order and a third task gets in at the right time and does a
> write_lock().  Probably that's not possible for some reason, dunno.  The usual
> way of solving this is to always take the locks in
> sorted-by-ascending-virtual-address order.

Don't worry about this problem, because rwlock is read_preference lock.

But your advice is very good, I'll change it in the next version of the patch.

Thanks!

> 
> 
> 
> 
>