On Thu 30-05-13 14:48:52, Tejun Heo wrote: > Hello, > > Sorry about the delay. Have been and still am traveling. > > On Fri, May 24, 2013 at 09:54:20AM +0200, Michal Hocko wrote: > > > > 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? > > Yeah, you're right. We need them. It's still a bummer that mark_dead > has the appearance of proper encapsulation while not really taking > care of synchronization. No objection to put barrier there. You are right it is more natural. > I think it'd make more sense for mark_dead to have the barrier (which > BTW should probably be smp_wmb() instead of wmb()) Yes, smp_wmb sounds like a better fit. > inside it or for the function to be just open-coded. More on this > topic later. > > > > 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? > > Yeap. > > > > 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. > > Yes, the barrier is necessary but I still think it's unnecessarily > elaborate. Among the locking constructs, the barriesr are the worst - > they don't enforce any structures, people often misunderstand / make > mistakes about them, bugs from misusages are extremely difficult to > trigger and reproduce especially on x86. It's a horrible construct > and should only be used if no other options can meet the performance > requirements required for the path. I am all for simplifying the code. I guess it deserves a separate patch though and it is a bit unrelated to the scope of the series. -- 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>