[PATCH] cgroup: Fixes the un-paired cgroup lock problem

[PATCH] cgroup: Fixes the un-paired cgroup lock problem

[Liu Aleaxander]

From: Liu Aleaxander <Aleaxander@gmail.com>
Date: Wed, 4 Nov 2009 09:27:06 +0800
Subject: [PATCH] Fixes the un-paired cgroup lock problem

In cgroup_lock_live_group, it locks the cgroup by mutex_lock, while in the
cgroup_tasks_write, it unlock it by cgroup_unlock. Even though they are
equal, but I do think we should make it pair.

BTW, should we replace others with cgroup_lock and cgroup_unlock?
Since we already have a wrapper one and it's meaningful.

Signed-off-by: Liu Aleaxander <Aleaxander@gmail.com>
---
 kernel/cgroup.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 0249f4b..ee2274e 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1660,9 +1660,9 @@ static int cgroup_tasks_write(struct cgroup
*cgrp, struct cftype *cft, u64 pid)
  */
 bool cgroup_lock_live_group(struct cgroup *cgrp)
 {
-	mutex_lock(&cgroup_mutex);
+	cgroup_lock();
 	if (cgroup_is_removed(cgrp)) {
-		mutex_unlock(&cgroup_mutex);
+		cgroup_unlock();
 		return false;
 	}
 	return true;
-- 
1.6.2.5

-- 
regards
Liu Aleaxander

Re: [PATCH] cgroup: Fixes the un-paired cgroup lock problem

[Li Zefan]

Liu Aleaxander wrote:
> From: Liu Aleaxander <Aleaxander@gmail.com>
> Date: Wed, 4 Nov 2009 09:27:06 +0800
> Subject: [PATCH] Fixes the un-paired cgroup lock problem
> 
> In cgroup_lock_live_group, it locks the cgroup by mutex_lock, while in the
> cgroup_tasks_write, it unlock it by cgroup_unlock. Even though they are
> equal, but I do think we should make it pair.
> 
> BTW, should we replace others with cgroup_lock and cgroup_unlock?
> Since we already have a wrapper one and it's meaningful.
> 

Before I read the email body, I thought there is a bug where
there is a lock without unlock or vise versa.

I agree the case here can be called "unpaired", but I'm not
convinced this patch is needed. The code is not buggy or
confusing. So the patch neither fixes a bug nor make the code
more readable.

Re: [PATCH] cgroup: Fixes the un-paired cgroup lock problem

[Bill Davidsen]

Li Zefan wrote:
> Liu Aleaxander wrote:
>> From: Liu Aleaxander <Aleaxander@gmail.com>
>> Date: Wed, 4 Nov 2009 09:27:06 +0800
>> Subject: [PATCH] Fixes the un-paired cgroup lock problem
>>
>> In cgroup_lock_live_group, it locks the cgroup by mutex_lock, while in the
>> cgroup_tasks_write, it unlock it by cgroup_unlock. Even though they are
>> equal, but I do think we should make it pair.
>>
>> BTW, should we replace others with cgroup_lock and cgroup_unlock?
>> Since we already have a wrapper one and it's meaningful.
>>
> 
> Before I read the email body, I thought there is a bug where
> there is a lock without unlock or vise versa.
> 
> I agree the case here can be called "unpaired", but I'm not
> convinced this patch is needed. The code is not buggy or
> confusing. So the patch neither fixes a bug nor make the code
> more readable.
> 
I would say it fixes a bug, the one that would be introduced when the two 
methods are no longer compatible and essentially two names for the same thing. 
And while you may know the code so well that you knew without looking that this 
was (currently) okay, there will be lots of eyes on this code over the years, I 
think most people would find use of cgroup_lock to lock the cgroup a LOT more 
readable.

While you can't go back in time to murder your grandfather, it creates no 
paradox to fix a bug before someone writes it.

-- 
Bill Davidsen <davidsen@tmr.com>
   "We have more to fear from the bungling of the incompetent than from
the machinations of the wicked."  - from Slashdot

Re: [PATCH] cgroup: Fixes the un-paired cgroup lock problem

[Li Zefan]

Bill Davidsen wrote:
> Li Zefan wrote:
>> Liu Aleaxander wrote:
>>> From: Liu Aleaxander <Aleaxander@gmail.com>
>>> Date: Wed, 4 Nov 2009 09:27:06 +0800
>>> Subject: [PATCH] Fixes the un-paired cgroup lock problem
>>>
>>> In cgroup_lock_live_group, it locks the cgroup by mutex_lock, while
>>> in the
>>> cgroup_tasks_write, it unlock it by cgroup_unlock. Even though they are
>>> equal, but I do think we should make it pair.
>>>
>>> BTW, should we replace others with cgroup_lock and cgroup_unlock?
>>> Since we already have a wrapper one and it's meaningful.
>>>
>>
>> Before I read the email body, I thought there is a bug where
>> there is a lock without unlock or vise versa.
>>
>> I agree the case here can be called "unpaired", but I'm not
>> convinced this patch is needed. The code is not buggy or
>> confusing. So the patch neither fixes a bug nor make the code
>> more readable.
>>
> I would say it fixes a bug, the one that would be introduced when the
> two methods are no longer compatible and essentially two names for the
> same thing. And while you may know the code so well that you knew
> without looking that this was (currently) okay, there will be lots of
> eyes on this code over the years, I think most people would find use of
> cgroup_lock to lock the cgroup a LOT more readable.
> 
> While you can't go back in time to murder your grandfather, it creates
> no paradox to fix a bug before someone writes it.
> 

cgroup_lock() is not necessarily more readable than mutex_lock(&cgroup_mutex),
at least the former doesn't tell you the lock is a spin_lock or a mutex.

In fact, Ingo showed his distaste towards cgroup_lock():
	http://lkml.org/lkml/2009/1/18/39

And I won't worry about the issue you mentioned above. If It does
happen, the one, who makes the 2 mehtods no long compatible, will
definitely find out all the places where cgroup_mutex is used and
make proper change.

Re: [PATCH] cgroup: Fixes the un-paired cgroup lock problem

[Matt Helsley]

On Thu, Nov 05, 2009 at 07:14:11PM -0500, Bill Davidsen wrote:
> Li Zefan wrote:
> > Bill Davidsen wrote:
> >   
> >> Li Zefan wrote:
> >>     
> >>> Liu Aleaxander wrote:
> >>>       
> >>>> From: Liu Aleaxander <Aleaxander@gmail.com>
> >>>> Date: Wed, 4 Nov 2009 09:27:06 +0800
> >>>> Subject: [PATCH] Fixes the un-paired cgroup lock problem
> >>>>
> >>>> In cgroup_lock_live_group, it locks the cgroup by mutex_lock, while
> >>>> in the
> >>>> cgroup_tasks_write, it unlock it by cgroup_unlock. Even though they are
> >>>> equal, but I do think we should make it pair.
> >>>>
> >>>> BTW, should we replace others with cgroup_lock and cgroup_unlock?
> >>>> Since we already have a wrapper one and it's meaningful.
> >>>>
> >>>>         
> >>> Before I read the email body, I thought there is a bug where
> >>> there is a lock without unlock or vise versa.
> >>>
> >>> I agree the case here can be called "unpaired", but I'm not
> >>> convinced this patch is needed. The code is not buggy or
> >>> confusing. So the patch neither fixes a bug nor make the code
> >>> more readable.
> >>>
> >>>       
> >> I would say it fixes a bug, the one that would be introduced when the
> >> two methods are no longer compatible and essentially two names for the
> >> same thing. And while you may know the code so well that you knew
> >> without looking that this was (currently) okay, there will be lots of
> >> eyes on this code over the years, I think most people would find use of
> >> cgroup_lock to lock the cgroup a LOT more readable.
> >>
> >> While you can't go back in time to murder your grandfather, it creates
> >> no paradox to fix a bug before someone writes it.
> >>
> >>     
> >
> > cgroup_lock() is not necessarily more readable than mutex_lock(&cgroup_mutex),
> > at least the former doesn't tell you the lock is a spin_lock or a mutex.
> >
> >   
> That's the point, cgroup_lock() is an abstraction, you want to lock the 
> cgroup, you call the macro, the macro handles the details, and if 
> thinking (or the most common cache configurations) change, the code 
> still works.

Except it doesn't really "lock the cgroup" as you've been saying -- else
it would take the cgroup to lock as a parameter. Instead it locks
"all cgroups". Clearly there's room for misunderstanding even with
cgroup_lock().

	-Matt Helsley

Re: [PATCH] cgroup: Fixes the un-paired cgroup lock problem

[Bill Davidsen]

Matt Helsley wrote:
> On Thu, Nov 05, 2009 at 07:14:11PM -0500, Bill Davidsen wrote:
>   
>> Li Zefan wrote:
>>     
>>> Bill Davidsen wrote:
>>>   
>>>       
>>>> Li Zefan wrote:
>>>>     
>>>>         
>>>>> Liu Aleaxander wrote:
>>>>>       
>>>>>           
>>>>>> From: Liu Aleaxander <Aleaxander@gmail.com>
>>>>>> Date: Wed, 4 Nov 2009 09:27:06 +0800
>>>>>> Subject: [PATCH] Fixes the un-paired cgroup lock problem
>>>>>>
>>>>>> In cgroup_lock_live_group, it locks the cgroup by mutex_lock, while
>>>>>> in the
>>>>>> cgroup_tasks_write, it unlock it by cgroup_unlock. Even though they are
>>>>>> equal, but I do think we should make it pair.
>>>>>>
>>>>>> BTW, should we replace others with cgroup_lock and cgroup_unlock?
>>>>>> Since we already have a wrapper one and it's meaningful.
>>>>>>
>>>>>>         
>>>>>>             
>>>>> Before I read the email body, I thought there is a bug where
>>>>> there is a lock without unlock or vise versa.
>>>>>
>>>>> I agree the case here can be called "unpaired", but I'm not
>>>>> convinced this patch is needed. The code is not buggy or
>>>>> confusing. So the patch neither fixes a bug nor make the code
>>>>> more readable.
>>>>>
>>>>>       
>>>>>           
>>>> I would say it fixes a bug, the one that would be introduced when the
>>>> two methods are no longer compatible and essentially two names for the
>>>> same thing. And while you may know the code so well that you knew
>>>> without looking that this was (currently) okay, there will be lots of
>>>> eyes on this code over the years, I think most people would find use of
>>>> cgroup_lock to lock the cgroup a LOT more readable.
>>>>
>>>> While you can't go back in time to murder your grandfather, it creates
>>>> no paradox to fix a bug before someone writes it.
>>>>
>>>>     
>>>>         
>>> cgroup_lock() is not necessarily more readable than mutex_lock(&cgroup_mutex),
>>> at least the former doesn't tell you the lock is a spin_lock or a mutex.
>>>
>>>   
>>>       
>> That's the point, cgroup_lock() is an abstraction, you want to lock the 
>> cgroup, you call the macro, the macro handles the details, and if 
>> thinking (or the most common cache configurations) change, the code 
>> still works.
>>     
>
> Except it doesn't really "lock the cgroup" as you've been saying -- else
> it would take the cgroup to lock as a parameter. Instead it locks
> "all cgroups". Clearly there's room for misunderstanding even with
> cgroup_lock().
>   

Now that seems to be a good argument for better naming of the locks, 
something like lock_all_cgroups or whatever.

It does seem to support my point of using a macro named after "waht 
operation you are doing" rather than "how you are doing it today."

Thanks for the clarification.

-- 
Bill Davidsen <davidsen@tmr.com>
  Unintended results are the well-earned reward for incompetence.

Re: [PATCH] cgroup: Fixes the un-paired cgroup lock problem

[Bill Davidsen]

Li Zefan wrote:
> Bill Davidsen wrote:
>   
>> Li Zefan wrote:
>>     
>>> Liu Aleaxander wrote:
>>>       
>>>> From: Liu Aleaxander <Aleaxander@gmail.com>
>>>> Date: Wed, 4 Nov 2009 09:27:06 +0800
>>>> Subject: [PATCH] Fixes the un-paired cgroup lock problem
>>>>
>>>> In cgroup_lock_live_group, it locks the cgroup by mutex_lock, while
>>>> in the
>>>> cgroup_tasks_write, it unlock it by cgroup_unlock. Even though they are
>>>> equal, but I do think we should make it pair.
>>>>
>>>> BTW, should we replace others with cgroup_lock and cgroup_unlock?
>>>> Since we already have a wrapper one and it's meaningful.
>>>>
>>>>         
>>> Before I read the email body, I thought there is a bug where
>>> there is a lock without unlock or vise versa.
>>>
>>> I agree the case here can be called "unpaired", but I'm not
>>> convinced this patch is needed. The code is not buggy or
>>> confusing. So the patch neither fixes a bug nor make the code
>>> more readable.
>>>
>>>       
>> I would say it fixes a bug, the one that would be introduced when the
>> two methods are no longer compatible and essentially two names for the
>> same thing. And while you may know the code so well that you knew
>> without looking that this was (currently) okay, there will be lots of
>> eyes on this code over the years, I think most people would find use of
>> cgroup_lock to lock the cgroup a LOT more readable.
>>
>> While you can't go back in time to murder your grandfather, it creates
>> no paradox to fix a bug before someone writes it.
>>
>>     
>
> cgroup_lock() is not necessarily more readable than mutex_lock(&cgroup_mutex),
> at least the former doesn't tell you the lock is a spin_lock or a mutex.
>
>   
That's the point, cgroup_lock() is an abstraction, you want to lock the 
cgroup, you call the macro, the macro handles the details, and if 
thinking (or the most common cache configurations) change, the code 
still works.

> In fact, Ingo showed his distaste towards cgroup_lock():
> 	http://lkml.org/lkml/2009/1/18/39
>
> And I won't worry about the issue you mentioned above. If It does
> happen, the one, who makes the 2 mehtods no long compatible, will
> definitely find out all the places where cgroup_mutex is used and
> make proper change.
>
>   


-- 
Bill Davidsen <davidsen@tmr.com>
  Unintended results are the well-earned reward for incompetence.