Messages in this thread | | | Date | Mon, 19 Jan 2009 17:28:48 -0800 | Subject | Re: [PATCH 1/3] cgroup: convert open-coded mutex_lock(&cgroup_mutex) calls into cgroup_lock() calls | From | Paul Menage <> |
| |
On Sun, Jan 18, 2009 at 5:41 PM, Ingo Molnar <mingo@elte.hu> wrote: > > * Paul Menage <menage@google.com> wrote: > >> On Sun, Jan 18, 2009 at 1:10 AM, Ingo Molnar <mingo@elte.hu> 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
| |