In cgroup_get_sb, the lock sequence is:
mutex_lock(&inode->i_mutex);
mutex_lock(&cgroup->mutex);
so the last unlock sequence should be:
mutex_unlock(&cgroup->mutex);
mutex_unlock(&inode->i_mutex);
Signed-off-by: Xiaotian Feng <[email protected]>
---
kernel/cgroup.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 3737a68..11ef162 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1140,8 +1140,8 @@ static int cgroup_get_sb(struct file_system_type *fs_type,
BUG_ON(root->number_of_cgroups != 1);
cgroup_populate_dir(root_cgrp);
- mutex_unlock(&inode->i_mutex);
mutex_unlock(&cgroup_mutex);
+ mutex_unlock(&inode->i_mutex);
}
simple_set_mnt(mnt, sb);
--
1.6.2.5
* Xiaotian Feng <[email protected]> [2009-07-21 18:25:26]:
> In cgroup_get_sb, the lock sequence is:
> mutex_lock(&inode->i_mutex);
> mutex_lock(&cgroup->mutex);
> so the last unlock sequence should be:
> mutex_unlock(&cgroup->mutex);
> mutex_unlock(&inode->i_mutex);
>
> Signed-off-by: Xiaotian Feng <[email protected]>
> ---
> kernel/cgroup.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 3737a68..11ef162 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1140,8 +1140,8 @@ static int cgroup_get_sb(struct file_system_type *fs_type,
> BUG_ON(root->number_of_cgroups != 1);
>
> cgroup_populate_dir(root_cgrp);
> - mutex_unlock(&inode->i_mutex);
> mutex_unlock(&cgroup_mutex);
> + mutex_unlock(&inode->i_mutex);
> }
>
Seems reasonable to me. You might also want to mention that elsewhere
the sequence is unlock cgroup_mutex followed by inode->i_mutex.
Acked-by: Balbir Singh <[email protected]>
--
Balbir
On 07/21/2009 07:10 PM, Balbir Singh wrote:
> * Xiaotian Feng<[email protected]> [2009-07-21 18:25:26]:
>
>> In cgroup_get_sb, the lock sequence is:
>> mutex_lock(&inode->i_mutex);
>> mutex_lock(&cgroup->mutex);
>> so the last unlock sequence should be:
>> mutex_unlock(&cgroup->mutex);
>> mutex_unlock(&inode->i_mutex);
>>
>> Signed-off-by: Xiaotian Feng<[email protected]>
>> ---
>> kernel/cgroup.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>> index 3737a68..11ef162 100644
>> --- a/kernel/cgroup.c
>> +++ b/kernel/cgroup.c
>> @@ -1140,8 +1140,8 @@ static int cgroup_get_sb(struct file_system_type *fs_type,
>> BUG_ON(root->number_of_cgroups != 1);
>>
>> cgroup_populate_dir(root_cgrp);
>> - mutex_unlock(&inode->i_mutex);
>> mutex_unlock(&cgroup_mutex);
>> + mutex_unlock(&inode->i_mutex);
>> }
>>
>
> Seems reasonable to me. You might also want to mention that elsewhere
> the sequence is unlock cgroup_mutex followed by inode->i_mutex.
Yep, thank you very much:-)
>
> Acked-by: Balbir Singh<[email protected]>
>
>
* Zefan Li <[email protected]> [2009-07-21 19:38:03]:
> 2009/7/21, Balbir Singh <[email protected]>:
> >
> > * Xiaotian Feng <[email protected]> [2009-07-21 18:25:26]:
> >
> > > In cgroup_get_sb, the lock sequence is:
> > > mutex_lock(&inode->i_mutex);
> > > mutex_lock(&cgroup->mutex);
> > > so the last unlock sequence should be:
> > > mutex_unlock(&cgroup->mutex);
> > > mutex_unlock(&inode->i_mutex);
> > >
> > > Signed-off-by: Xiaotian Feng <[email protected]>
> > > ---
> > > kernel/cgroup.c | 2 +-
> > > 1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> > > index 3737a68..11ef162 100644
> > > --- a/kernel/cgroup.c
> > > +++ b/kernel/cgroup.c
> > > @@ -1140,8 +1140,8 @@ static int cgroup_get_sb(struct file_system_type
> > *fs_type,
> > > BUG_ON(root->number_of_cgroups != 1);
> > >
> > > cgroup_populate_dir(root_cgrp);
> > > - mutex_unlock(&inode->i_mutex);
> > > mutex_unlock(&cgroup_mutex);
> > > + mutex_unlock(&inode->i_mutex);
> > > }
> > >
> >
> > Seems reasonable to me. You might also want to mention that elsewhere
> > the sequence is unlock cgroup_mutex followed by inode->i_mutex.
> >
> > Acked-by: Balbir Singh [email protected]
>
>
> No, the unlock order is irrelevant. It's the lock order that matters. So
> this patch
> fixes nothing.
>
> Xiaotian, you didn't run into deadlock, did you?
>
Li, Consider the following
lock(A)
lock(B)
unlock(A)
unlock(B)
Tomorrow if a unsuspecting programmer does this
lock(A)
lock(B)
unlock(A)
code block
unlock(B)
What protects code block? lock B? Is that the intention?
--
Balbir
On Tue, Jul 21, 2009 at 5:01 AM, Balbir Singh<[email protected]> wrote:
>
> lock(A)
> lock(B)
> unlock(A)
> unlock(B)
>
> Tomorrow if a unsuspecting programmer does this
>
> lock(A)
> lock(B)
> unlock(A)
>
> code block
>
> unlock(B)
>
>
> What protects code block? lock B? Is that the intention?
>
An "unsuspecting programmer" shouldn't be adding code to
multi-threaded routines without thoroughly understanding the locking.
I guess there's no harm in this patch, but as Li says, it doesn't
really change anything.
Paul
* [email protected] <[email protected]> [2009-07-21 08:34:51]:
> On Tue, Jul 21, 2009 at 5:01 AM, Balbir Singh<[email protected]> wrote:
> >
> > lock(A)
> > lock(B)
> > unlock(A)
> > unlock(B)
> >
> > Tomorrow if a unsuspecting programmer does this
> >
> > lock(A)
> > lock(B)
> > unlock(A)
> >
> > code block
> >
> > unlock(B)
> >
> >
> > What protects code block? lock B? Is that the intention?
> >
>
> An "unsuspecting programmer" shouldn't be adding code to
> multi-threaded routines without thoroughly understanding the locking.
>
Agreed, but why leave behind places for people to do so. There is the
consistency factor as well, see below.
> I guess there's no harm in this patch, but as Li says, it doesn't
> really change anything.
>
Well all the other places do it right in the same routine.
--
Balbir
On Tue, Jul 21, 2009 at 3:25 AM, Xiaotian Feng<[email protected]> wrote:
> In cgroup_get_sb, the lock sequence is:
> ? ? ? ?mutex_lock(&inode->i_mutex);
> ? ? ? ?mutex_lock(&cgroup->mutex);
> so the last unlock sequence should be:
Make this "so for consistency the last ..." ?
Maybe make the patch title "Make unlock sequence in cgroup_get_sb
consistent" so someone looking through the change logs for fixes to
backport doesn't wrongly thing that this fixes any bug"?
> ? ? ? ?mutex_unlock(&cgroup->mutex);
> ? ? ? ?mutex_unlock(&inode->i_mutex);
>
> Signed-off-by: Xiaotian Feng <[email protected]>
Acked-by: Paul Menage <[email protected]>
Paul
>>> Seems reasonable to me. You might also want to mention that elsewhere
>>> the sequence is unlock cgroup_mutex followed by inode->i_mutex.
>>>
>>> Acked-by: Balbir Singh [email protected]
>>
>> No, the unlock order is irrelevant. It's the lock order that matters. So
>> this patch
>> fixes nothing.
>>
>> Xiaotian, you didn't run into deadlock, did you?
>>
>
>
> Li, Consider the following
>
>
> lock(A)
> lock(B)
> unlock(A)
> unlock(B)
>
> Tomorrow if a unsuspecting programmer does this
>
> lock(A)
> lock(B)
> unlock(A)
>
> code block
>
> unlock(B)
>
>
> What protects code block? lock B? Is that the intention?
>
I won't worry about that. If unlock order is a concern,
we should have taught lockdep to detect it.
Here's a reply from Linus on this issue:
http://lkml.org/lkml/2008/10/8/150
On 07/22/2009 08:53 AM, Li Zefan wrote:
>>>> Seems reasonable to me. You might also want to mention that elsewhere
>>>> the sequence is unlock cgroup_mutex followed by inode->i_mutex.
>>>>
>>>> Acked-by: Balbir Singh [email protected]
>>>>
>>> No, the unlock order is irrelevant. It's the lock order that matters. So
>>> this patch
>>> fixes nothing.
>>>
>>> Xiaotian, you didn't run into deadlock, did you?
>>>
>>>
>> Li, Consider the following
>>
>>
>> lock(A)
>> lock(B)
>> unlock(A)
>> unlock(B)
>>
>> Tomorrow if a unsuspecting programmer does this
>>
>> lock(A)
>> lock(B)
>> unlock(A)
>>
>> code block
>>
>> unlock(B)
>>
>>
>> What protects code block? lock B? Is that the intention?
>>
>>
>
> I won't worry about that. If unlock order is a concern,
> we should have taught lockdep to detect it.
>
> Here's a reply from Linus on this issue:
>
> http://lkml.org/lkml/2008/10/8/150
>
OK, this patch is trivial. Just for consistency with previous unlock
sequence:-)
On 07/22/2009 12:03 AM, Paul Menage wrote:
> On Tue, Jul 21, 2009 at 3:25 AM, Xiaotian Feng<[email protected]> wrote:
>> In cgroup_get_sb, the lock sequence is:
>> mutex_lock(&inode->i_mutex);
>> mutex_lock(&cgroup->mutex);
>> so the last unlock sequence should be:
>
> Make this "so for consistency the last ..." ?
>
> Maybe make the patch title "Make unlock sequence in cgroup_get_sb
> consistent" so someone looking through the change logs for fixes to
> backport doesn't wrongly thing that this fixes any bug"?
>
Yep, this is a trivial patch. Modified following your suggestion, thank
you.
>> mutex_unlock(&cgroup->mutex);
>> mutex_unlock(&inode->i_mutex);
>>
>> Signed-off-by: Xiaotian Feng<[email protected]>
>
> Acked-by: Paul Menage<[email protected]>
>
> Paul
Danny Feng wrote:
> On 07/22/2009 12:03 AM, Paul Menage wrote:
>> On Tue, Jul 21, 2009 at 3:25 AM, Xiaotian Feng<[email protected]> wrote:
>>> In cgroup_get_sb, the lock sequence is:
>>> mutex_lock(&inode->i_mutex);
>>> mutex_lock(&cgroup->mutex);
>>> so the last unlock sequence should be:
>>
>> Make this "so for consistency the last ..." ?
>>
>> Maybe make the patch title "Make unlock sequence in cgroup_get_sb
>> consistent" so someone looking through the change logs for fixes to
>> backport doesn't wrongly thing that this fixes any bug"?
>>
>
> Yep, this is a trivial patch. Modified following your suggestion, thank
> you.
>
As far as it's not declared as a fix, I has no objection to this
patch.
Please always inline the patch in the mail body. And when resending
the patch, add the acked-by you collected in it:
Acked-by: Balbir ...
Acked-by: Paul ...
Signed-off-by: Xiaotian ...
You may resend the patch to Andrew Morton, who picks up cgroup
patches, otherwise the patch may be overlooked.
>>> mutex_unlock(&cgroup->mutex);
>>> mutex_unlock(&inode->i_mutex);
>>>
>>> Signed-off-by: Xiaotian Feng<[email protected]>
>>
>> Acked-by: Paul Menage<[email protected]>
>>
>> Paul
>
On 07/22/2009 10:18 AM, Li Zefan wrote:
> Danny Feng wrote:
>> On 07/22/2009 12:03 AM, Paul Menage wrote:
>>> On Tue, Jul 21, 2009 at 3:25 AM, Xiaotian Feng<[email protected]> wrote:
>>>> In cgroup_get_sb, the lock sequence is:
>>>> mutex_lock(&inode->i_mutex);
>>>> mutex_lock(&cgroup->mutex);
>>>> so the last unlock sequence should be:
>>> Make this "so for consistency the last ..." ?
>>>
>>> Maybe make the patch title "Make unlock sequence in cgroup_get_sb
>>> consistent" so someone looking through the change logs for fixes to
>>> backport doesn't wrongly thing that this fixes any bug"?
>>>
>> Yep, this is a trivial patch. Modified following your suggestion, thank
>> you.
>>
>
> As far as it's not declared as a fix, I has no objection to this
> patch.
>
> Please always inline the patch in the mail body. And when resending
> the patch, add the acked-by you collected in it:
>
> Acked-by: Balbir ...
> Acked-by: Paul ...
> Signed-off-by: Xiaotian ...
>
> You may resend the patch to Andrew Morton, who picks up cgroup
> patches, otherwise the patch may be overlooked.
>
Got it, thank you very much.
>>>> mutex_unlock(&cgroup->mutex);
>>>> mutex_unlock(&inode->i_mutex);
>>>>
>>>> Signed-off-by: Xiaotian Feng<[email protected]>
>>> Acked-by: Paul Menage<[email protected]>
>>>
>>> Paul
>