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@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>