[Devel] [PATCH v2 3/7] memcg, slab: separate memcg vs root cache creation paths

[Michal Hocko]

On Thu 06-02-14 21:12:51, Vladimir Davydov wrote:
> On 02/06/2014 08:41 PM, Michal Hocko wrote:
[...]
> >> +int kmem_cache_create_memcg(struct mem_cgroup *memcg, struct kmem_cache *cachep)
> >>  {
> >> -	return kmem_cache_create_memcg(NULL, name, size, align, flags, ctor, NULL);
> >> +	struct kmem_cache *s;
> >> +	int err;
> >> +
> >> +	get_online_cpus();
> >> +	mutex_lock(&slab_mutex);
> >> +
> >> +	/*
> >> +	 * Since per-memcg caches are created asynchronously on first
> >> +	 * allocation (see memcg_kmem_get_cache()), several threads can try to
> >> +	 * create the same cache, but only one of them may succeed.
> >> +	 */
> >> +	err = -EEXIST;
> > Does it make any sense to report the error here? If we are racing then at
> > least on part wins and the work is done.
> 
> Yeah, you're perfectly right. It's better to return 0 here.

Why not void?

> > We should probably warn about errors which prevent from accounting but
> > I do not think there is much more we can do so returning an error code
> > from this function seems pointless. memcg_create_cache_work_func ignores
> > the return value anyway.
> 
> I do not think warnings are appropriate here, because it is not actually
> an error if we are short on memory and can't do proper memcg accounting
> due to this. Perhaps, we'd better add fail counters for memcg cache
> creations and/or accounting to the root cache instead of memcg's one.
> That would be useful for debugging. I'm not sure though.

warn on once per memcg would be probably sufficient but it would still
be great if an admin could see that a memcg is not accounted although it
is supposed to be. Scanning all the memcgs might be really impractical.
We do not fail allocations needed for those object in the real life now
but we shouldn't rely on that.

-- 
Michal Hocko
SUSE Labs