Re: [PATCH 1/3] cgroup: convert open-coded mutex_lock(&cgroup_mutex) calls into cgroup_lock() calls

[Paul Menage]

On Sun, Jan 18, 2009 at 5:41 PM, Ingo Molnar <mingo@xxxxxxx> wrote:
>
> * Paul Menage <menage@xxxxxxxxxx> wrote:
>
>> On Sun, Jan 18, 2009 at 1:10 AM, Ingo Molnar <mingo@xxxxxxx> wrote:
>> > this just changes over a clean mutex call to a wrapped lock/unlock
>> > sequence that has higher overhead in the common case.
>> >
>> > We should do the exact opposite, we should change this opaque API:
>> >
>> >  void cgroup_lock(void)
>> >  {
>> >         mutex_lock(&cgroup_mutex);
>> >  }
>> >
>> > To something more explicit (and more maintainable) like:
>>
>> I disagree - cgroup_mutex is a very coarse lock that can be held for
>> pretty long periods of time by the cgroups framework, and should never
>> be part of any fastpath code. So the overhead of a function call should
>> be irrelevant.
>>
>> The change that you're proposing would send the message that
>> cgroup_mutex_lock(&cgroup_mutex) is appropriate to use in a
>> performance-sensitive function, when in fact we want to discourage such
>> code from taking this lock and instead use more appropriately
>> fine-grained locks.
>
> Uhm, how does that 'discourage' its use in fastpath code?

I agree, the existing code doesn't exactly discourage its use in
fastpath code, but we should be doing so. (The recent addition of
hierarchy_mutex is a step in that direction, although it still has
some issues to be cleaned up). But it seems to me that exposing the
lock is an invitation for people to use it even more than they do
currently. There's certainly no performance argument for exposing it,

>
> It just hides the real lock

Yes, I'd rather hide the real lock in this case.

Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/