[PATCH 07/12] cgroup: unify cgroup_write_X64() and cgroup_write_string()

Michal Hocko mhocko at suse.cz
Mon Dec 2 09:54:01 UTC 2013


On Fri 29-11-13 15:05:25, Tejun Heo wrote:
> Hello, Michal.
> 
> On Thu, Nov 28, 2013 at 12:18:18PM +0100, Michal Hocko wrote:
> > > Unify the two into cgroup_file_write() which always allocates dynamic
> > > buffer for simplicity
> > 
> > It's true that this is simpler but the allocation might cause some
> > issues with memcg. Although it is not common that userspace oom handler
> > is a part of the target memcg there are users (e.g. Google) who do that.
> > 
> > Why is that a problem? Consider that a memcg is under OOM, handler gets
> > notified and tries to solve the situation by enlarging the hard limit.
> > This worked before this patch because cgroup_write_X64 used an on stack
> > buffer but now it would use kmalloc which might be accounted and trip
> > over the same OOM situation.
> > 
> > This is not limited to the OOM handling. The group might be close to OOM
> > and the last thing user expects is to trigger OOM when he tries to
> > enlarge the limit.
> > 
> > Is the additional simplicity worth breaking those usecases?
> 
> Whoa, so we support oom handler inside the memcg that it handles?
> Does that work reliably?  Changing the above detail in this patch
> isn't difficult (and we'll later need to update kernfs too) but
> supporting such setup properly would be a *lot* of commitment and I'm
> very doubtful we'd be able to achieve that by just carefully avoiding
> memory allocation in the operations that usreland oom handler uses -
> that set is destined to expand over time, extremely fragile and will
> be hellish to maintain.
> 
> So, I'm not at all excited about commiting to this guarantee.  This
> one is an easy one but it looks like the first step onto dizzying
> slippery slope.
> 
> Am I misunderstanding something here?  Are you and Johannes firm on
> supporting this?

Well, I am not happy about the use case as well and I will always
discourage people from doing this. I was merely pointing out that the
patch will break those even though it seems trivial to not do so. That
being said I would rather see no allocation in that path but if that
doesn't seem viable to you then I will not loose any sleep over it.
-- 
Michal Hocko
SUSE Labs


More information about the Containers mailing list