On Tue, 27 May 2014, Andrew Morton wrote:
> On Tue, 27 May 2014 14:36:04 -0700 (PDT) Hugh Dickins <hughd@xxxxxxxxxx> wrote:
>
> > mem_cgroup_within_guarantee() oopses in _raw_spin_lock_irqsave() when
> > booted with cgroup_disable=memory. Fix that in the obvious inelegant
> > way for now - though I hope we are moving towards a world in which
> > almost all of the mem_cgroup_disabled() tests will vanish, with a
> > root_mem_cgroup which can handle the basics even when disabled.
> >
> > I bet there's a neater way of doing this, rearranging the loop (and we
> > shall want to avoid spinlocking on root_mem_cgroup when we reach that
> > new world), but that's the kind of thing I'd get wrong in a hurry!
> >
> > ...
> >
> > @@ -2793,6 +2793,9 @@ static struct mem_cgroup *mem_cgroup_loo
> > bool mem_cgroup_within_guarantee(struct mem_cgroup *memcg,
> > struct mem_cgroup *root)
> > {
> > + if (mem_cgroup_disabled())
> > + return false;
> > +
> > do {
> > if (!res_counter_low_limit_excess(&memcg->res))
> > return true;
>
> This seems to be an awfully late and deep place at which to be noticing
> mem_cgroup_disabled(). Should mem_cgroup_within_guarantee() even be called
> in this state?
I think it's a natural consequence of our preferring to use a single
path for memcg and non-memcg, outside of memcontrol.c itself. So in
vmscan.c there are loops iterating through a subtree of memcgs, which
in the non-memcg case can only ever encounter root_mem_cgroup (or NULL).
In doing so, it's not surprising that __shrink_zone() should want to
check mem_cgroup_within_guarantee(). Now, __shrink_zone() does have an
honor_memcg_guarantee arg passed in, and I did consider initializing
that according to !mem_cgroup_disabled(): which would be not so late
and not so deep. But then noticed mem_cgroup_all_within_guarantee(),
which is called without condition on honor_guarantee, so backed away:
we could very easily change that, I suppose, but...
I'm sure there is a better way of dealing with this than sprinkling
mem_cgroup_disabled() tests all over, and IIUC Hannes is moving us
towards that by making root_mem_cgroup more of a first-class citizen
(following on from earlier per-cpu-ification of memcg's most expensive
fields).
My attitude is that for now we just chuck in a !mem_cgroup_disabled()
wherever it stops a crash, as before; but in future aim to give the
cgroup_disabled=memory root_mem_cgroup all it needs to handle this
seamlessly. Ideally just a !mem_cgroup_disabled() test at the point
of memcg creation, and everything else fall out naturally (but maybe
some more lookup_page_cgroup() NULL tests). In practice we may identify
other places, where it's useful to add a special test to avoid expense;
though usually that would be expense worth avoiding at the root, even
when !mem_cgroup_disabled().
And probably a static dummy root_mem_cgroup even when !CONFIG_MEMCG.
(Not that I'm expecting to do any of this work myself!)
Hugh
--
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>