linux-kernel - Re: [patch] mm: memcontrol: do not iterate uninitialized memcgs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 26 Sep 2014 15:39:25 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Johannes Weiner <hannes@...xchg.org>
Cc:	Tejun Heo <tj@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Hugh Dickins <hughd@...gle.com>, Michal Hocko <mhocko@...e.cz>,
	linux-mm@...ck.org, cgroups@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [patch] mm: memcontrol: do not iterate uninitialized memcgs

On Thu, Sep 25, 2014 at 09:43:42AM -0400, Johannes Weiner wrote:
> > > +		if (next_css == &root->css ||
> > > +		    css_tryget_online(next_css)) {
> > > +			struct mem_cgroup *memcg;
> > > +
> > > +			memcg = mem_cgroup_from_css(next_css);
> > > +			if (memcg->initialized) {
> > > +				/*
> > > +				 * Make sure the caller's accesses to
> > > +				 * the memcg members are issued after
> > > +				 * we see this flag set.
> > 
> > I usually prefer if the comment points to the exact location that the
> > matching memory barriers live.  Sometimes it's difficult to locate the
> > partner barrier even w/ the functional explanation.

That is indeed good practise! :-)

> > > +				 */
> > > +				smp_rmb();
> > > +				return memcg;
> > 
> > In an unlikely event this rmb becomes an issue, a self-pointing
> > pointer which is set/read using smp_store_release() and
> > smp_load_acquire() respectively can do with plain barrier() on the
> > reader side on archs which don't need data dependency barrier
> > (basically everything except alpha).  Not sure whether that'd be more
> > or less readable than this tho.

> So as far as I understand memory-barriers.txt we do not even need a
> data dependency here to use store_release and load_acquire:
> 
> mem_cgroup_css_online():
> <initialize memcg>
> smp_store_release(&memcg->initialized, 1);
> 
> mem_cgroup_iter():
> <look up maybe-initialized memcg>
> if (smp_load_acquire(&memcg->initialized))
>   return memcg;
> 
> So while I doubt that the smp_rmb() will become a problem in this
> path, it would be neat to annotate the state flag around which we
> synchronize like this, rather than have an anonymous barrier.
> 
> Peter, would you know if this is correct, or whether these primitives
> actually do require a data dependency?

I'm fairly sure you do not. load_acquire() has the same barrier in on
Alpha that read_barrier_depends() does, and that's the only arch that
matters.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/