Sorry, I have missed this. CCing would help. Anyway putting myself to CC now :P On Wed 22-05-13 16:36:27, Li Zefan wrote: > On 2013/5/18 2:08, Tejun Heo wrote: > > Hey, > > > > On Fri, May 17, 2013 at 03:04:06PM +0800, Li Zefan wrote: > >> + /* > >> + * Releases a reference taken in kmem_cgroup_css_offline in case > >> + * this last uncharge is racing with the offlining code or it is > >> + * outliving the memcg existence. > >> + * > >> + * The memory barrier imposed by test&clear is paired with the > >> + * explicit one in kmem_cgroup_css_offline. > > > > Paired with the wmb to achieve what? https://lkml.org/lkml/2013/4/4/190 " ! > + css_get(&memcg->css); ! I think that you need a write memory barrier here because css_get ! nor memcg_kmem_mark_dead implies it. memcg_uncharge_kmem uses ! memcg_kmem_test_and_clear_dead which imply a full memory barrier but it ! should see the elevated reference count. No? ! ! > + /* ! > + * We need to call css_get() first, because memcg_uncharge_kmem() ! > + * will call css_put() if it sees the memcg is dead. ! > + */ ! > memcg_kmem_mark_dead(memcg); " Does it make sense to you Tejun? > > > >> + */ > >> if (memcg_kmem_test_and_clear_dead(memcg)) > >> - mem_cgroup_put(memcg); > >> + css_put(&memcg->css); > > > > The other side is wmb, so there gotta be something which wants to read > > which were written before wmb here but the only thing after the > > barrier is css_put() which doesn't need such thing, so I'm lost on > > what the barrier pair is achieving here. > > > > In general, please be *very* explicit about what's going on whenever > > something is depending on barrier pairs. It'll make it easier for > > both the author and reviewers to actually understand what's going on > > and why it's necessary. > > > > ... > >> @@ -5858,23 +5856,39 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss) > >> return mem_cgroup_sockets_init(memcg, ss); > >> } > >> > >> -static void kmem_cgroup_destroy(struct mem_cgroup *memcg) > >> +static void kmem_cgroup_css_offline(struct mem_cgroup *memcg) > >> { > >> - mem_cgroup_sockets_destroy(memcg); > >> + if (!memcg_kmem_is_active(memcg)) > >> + return; > >> > >> + /* > >> + * kmem charges can outlive the cgroup. In the case of slab > >> + * pages, for instance, a page contain objects from various > >> + * processes. As we prevent from taking a reference for every > >> + * such allocation we have to be careful when doing uncharge > >> + * (see memcg_uncharge_kmem) and here during offlining. > >> + * > >> + * The idea is that that only the _last_ uncharge which sees > >> + * the dead memcg will drop the last reference. An additional > >> + * reference is taken here before the group is marked dead > >> + * which is then paired with css_put during uncharge resp. here. > >> + * > >> + * Although this might sound strange as this path is called when > >> + * the reference has already dropped down to 0 and shouldn't be > >> + * incremented anymore (css_tryget would fail) we do not have > > > > Hmmm? offline is called on cgroup destruction regardless of css > > refcnt. The above comment seems a bit misleading. > > > > The comment is wrong. I'll fix it. Ohh, right. "Althouth this might sound strange as this path is called from css_offline when the reference might have dropped down to 0 and shouldn't ..." Sounds better? > >> + * other options because of the kmem allocations lifetime. > >> + */ > >> + css_get(&memcg->css); > >> + > >> + /* see comment in memcg_uncharge_kmem() */ > >> + wmb(); > >> memcg_kmem_mark_dead(memcg); > > > > Is the wmb() trying to prevent reordering between css_get() and > > memcg_kmem_mark_dead()? If so, it isn't necessary - the compiler > > isn't allowed to reorder two atomic ops (they're all asm volatiles) > > and the visibility order is guaranteed by the nature of the two > > operations going on here - both perform modify-and-test on one end of > > the operations. As I have copied my comment from the earlier thread above. css_get does atomic_add which doesn't imply any barrier AFAIK and memcg_kmem_mark_dead uses a simple set_bit which doesn't imply it either. What am I missing? > > > > Yeah, I think you're right. > > > It could be argued that having memory barriers is better for > > completeness of mark/test interface but then those barriers should > > really moved into memcg_kmem_mark_dead() and its clearing counterpart. > > > > While it's all clever and dandy, my recommendation would be just using > > a lock for synchronization. It isn't a hot path. Why be clever? > > > > I don't quite like adding a lock not to protect data but just ensure code > orders. Agreed. > Michal, what's your preference? I want to be sure that everyone is happy > so the next version will hopefully be the last version. I still do not see why the barrier is not needed and the lock seems too big hammer. -- 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>