Re: [PATCH v3 4/6] memcg: replace cgroup_lock with memcg specific memcg_lock

[Michal Hocko <mhocko@suse.cz>]

On Mon 21-01-13 19:12:00, Glauber Costa wrote:
> On 01/21/2013 06:49 PM, Michal Hocko wrote:
> > On Mon 21-01-13 15:13:31, Glauber Costa wrote:
> >> After the preparation work done in earlier patches, the cgroup_lock can
> >> be trivially replaced with a memcg-specific lock. This is an automatic
> >> translation in every site the values involved were queried.
> >>
> >> The sites were values are written, however, used to be naturally called
> >> under cgroup_lock. This is the case for instance of the css_online
> >> callback. For those, we now need to explicitly add the memcg_lock.
> >>
> >> Also, now that the memcg_mutex is available, there is no need to abuse
> >> the set_limit mutex in kmemcg value setting. The memcg_mutex will do a
> >> better job, and we now resort to it.
> > 
> > You will hate me for this because I should have said that in the
> > previous round already (but I will use "I shown a mercy on you and
> > that blinded me" for my defense).
> > I am not so sure it will do a better job (it is only kmem that uses both
> > locks). I thought that memcg_mutex is just a first step and that we move
> > to a more finer grained locking later (a too general documentation of
> > the lock even asks for it).  So I would keep the limit mutex and figure
> > whether memcg_mutex could be split up even further.
> > 
> > Other than that the patch looks good to me
> > 
> By now I have more than enough reasons to hate you, so this one won't
> add much. Even then, don't worry. Beer resets it all.
> 
> That said, I disagree with you.
> 
> As you noted yourself, kmem needs both locks:
> 1) cgroup_lock, because we need to prevent creation of sub-groups.
> 2) set_limit lock, because we need one - any one - memcg global lock be
> held while we are manipulating the kmem-specific data structures, and we
> would like to spread cgroup_lock all around for that.
> 
> I now regret not having created the memcg_mutex for that: I'd be now
> just extending it to other users, instead of trying a replacement.
> 
> So first of all, if the limit mutex is kept, we would *still* need to
> hold the memcg mutex to avoid children appearing. If we *ever* switch to
> a finer-grained lock(*), we will have to hold that lock anyway. So why
> hold set_limit_mutex??

Yeah but memcg is not just kmem, is it? See mem_cgroup_resize_limit for
example. Why should it be linearized with, say, a new group creation.
Same thing with memsw. Besides that you know what those two locks are
intended for. memcg_mutex to prevent from races with a new group
creation and the limit lock for races with what-ever limit setting.
This sounds much more specific than
"
The memcg mutex needs to be held for any globally visible cgroup change.
"

> (*) None of the operations protected by this mutex are fast paths...
[...]
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>