Re: [PATCH v3 4/7] memcg: remove memcg from the reclaim iterators

[Michal Hocko <mhocko@suse.cz>]

On Mon 11-02-13 17:39:43, Johannes Weiner wrote:
> On Mon, Feb 11, 2013 at 10:27:56PM +0100, Michal Hocko wrote:
> > On Mon 11-02-13 14:58:24, Johannes Weiner wrote:
> > > On Mon, Feb 11, 2013 at 08:29:29PM +0100, Michal Hocko wrote:
> > > > On Mon 11-02-13 12:56:19, Johannes Weiner wrote:
> > > > > On Mon, Feb 11, 2013 at 04:16:49PM +0100, Michal Hocko wrote:
> > > > > > Maybe we could keep the counter per memcg but that would mean that we
> > > > > > would need to go up the hierarchy as well. We wouldn't have to go over
> > > > > > node-zone-priority cleanup so it would be much more lightweight.
> > > > > > 
> > > > > > I am not sure this is necessarily better than explicit cleanup because
> > > > > > it brings yet another kind of generation number to the game but I guess
> > > > > > I can live with it if people really thing the relaxed way is much
> > > > > > better.
> > > > > > What do you think about the patch below (untested yet)?
> > > > > 
> > > > > Better, but I think you can get rid of both locks:
> > > > 
> > > > What is the other lock you have in mind.
> > > 
> > > The iter lock itself.  I mean, multiple reclaimers can still race but
> > > there won't be any corruption (if you make iter->dead_count a long,
> > > setting it happens atomically, we nly need the memcg->dead_count to be
> > > an atomic because of the inc) and the worst that could happen is that
> > > a reclaim starts at the wrong point in hierarchy, right?
> > 
> > The lack of synchronization basically means that 2 parallel reclaimers
> > can reclaim every group exactly once (ideally) or up to each group
> > twice in the worst case.
> > So the exclusion was quite comfortable.
> 
> It's quite unlikely, though.  Don't forget that they actually reclaim
> in between, I just can't see them line up perfectly and race to the
> iterator at the same time repeatedly.  It's more likely to happen at
> the higher priority levels where less reclaim happens, and then it's
> not a big deal anyway.  With lower priority levels, when the glitches
> would be more problematic, they also become even less likely.

Fair enough, I will drop that patch in the next version.
 
> > > But as you said in the changelog that introduced the lock, it's never
> > > actually been a practical problem.
> > 
> > That is true but those bugs would be subtle though so I wouldn't be
> > opposed to prevent from them before we get burnt. But if you think that
> > we should keep the previous semantic I can drop that patch.
> 
> I just think that the problem is unlikely and not that big of a deal.
> 
> > > You just need to put the wmb back in place, so that we never see the
> > > dead_count give the green light while the cached position is stale, or
> > > we'll tryget random memory.
> > > 
> > > > > mem_cgroup_iter:
> > > > > rcu_read_lock()
> > > > > if atomic_read(&root->dead_count) == iter->dead_count:
> > > > >   smp_rmb()
> > > > >   if tryget(iter->position):
> > > > >     position = iter->position
> > > > > memcg = find_next(postion)
> > > > > css_put(position)
> > > > > iter->position = memcg
> > > > > smp_wmb() /* Write position cache BEFORE marking it uptodate */
> > > > > iter->dead_count = atomic_read(&root->dead_count)
> > > > > rcu_read_unlock()
> > > > 
> > > > Updated patch bellow:
> > > 
> > > Cool, thanks.  I hope you don't find it too ugly anymore :-)
> > 
> > It's getting trick and you know how people love when you have to play
> > and rely on atomics with memory barriers...
> 
> My bumper sticker reads "I don't believe in mutual exclusion" (the
> kernel hacker's version of smile for the red light camera).

Ohh, those easy riders.
 
> I mean, you were the one complaining about the lock...
> 
> > > That way, if the dead count gives the go-ahead, you KNOW that the
> > > position cache is valid, because it has been updated first.
> > 
> > OK, you are right. We can live without css_tryget because dead_count is
> > either OK which means that css would be alive at least this rcu period
> > (and RCU walk would be safe as well) or it is incremented which means
> > that we have started css_offline already and then css is dead already.
> > So css_tryget can be dropped.
> 
> Not quite :)
> 
> The dead_count check is for completed destructions,

Not quite :P. dead_count is incremented in css_offline callback which is
called before the cgroup core releases its last reference and unlinks
the group from the siblinks. css_tryget would already fail at this stage
because CSS_DEACT_BIAS is in place at that time but this doesn't break
RCU walk. So I think we are safe even without css_get.

Or am I missing something?
[...]
-- 
Michal Hocko
SUSE Labs