On Mon 21-01-13 19:34:27, Glauber Costa wrote: > On 01/21/2013 07:20 PM, Michal Hocko wrote: > > 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? > > No, it belongs to all of us. It is usually called collaboration, but in > the memcg context, we can say we are accomplices. > > > See mem_cgroup_resize_limit for > > example. Why should it be linearized with, say, a new group creation. > > Because it is simpler to use the same lock, and all those operations are > not exactly frequent. > > > Same thing with memsw. > > See, I'm not the only culprit! FWIW, that one doesn't need other log as well. > > 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 > > Again: Can I keep holding the set_limit_mutex? Sure I can. But we still > need to hold both, because kmemcg is also forbidden for groups that > already have tasks. Holding both is not a big deal if they are necessary - granted the ordering is correct - which is as it doesn't change with the move from cgroup_mutex. But please do not add new dependencies (like regular limit setting with memcg creation). > And the reason why kmemcg holds the set_limit mutex > is just to protect from itself, then there is no *need* to hold any > extra lock (and we'll never be able to stop holding the creation lock, > whatever it is). So my main point here is not memcg_mutex vs > set_limit_mutex, but rather, memcg_mutex is needed anyway, and once it > is taken, the set_limit_mutex *can* be held, but doesn't need to. So you can update kmem specific usage of set_limit_mutex. -- 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>