Re: [PATCH] mm: memcontrol: fix cgroup creation failure after many small jobs

[Tejun Heo <tj@kernel.org>]

Hello,

Looks generally good to me.  Some comments below.

On Wed, Jun 15, 2016 at 11:42:44PM -0400, Johannes Weiner wrote:
> @@ -6205,6 +6205,24 @@ struct cgroup *cgroup_get_from_path(const char *path)
>  }
>  EXPORT_SYMBOL_GPL(cgroup_get_from_path);
>  
> +/**
> + * css_id_free - relinquish an existing CSS's ID
> + * @css: the CSS
> + *
> + * This releases the @css's ID and allows it to be recycled while the
> + * CSS continues to exist. This is useful for controllers with state
> + * that extends past a cgroup's lifetime but doesn't need precious ID
> + * address space.
> + *
> + * This invalidates @css->id, and css_from_id() might return NULL or a
> + * new css if the ID has been recycled in the meantime.
> + */
> +void css_id_free(struct cgroup_subsys_state *css)
> +{
> +	cgroup_idr_remove(&css->ss->css_idr, css->id);
> +	css->id = 0;
> +}

I don't quite get why we're trying to free css->id earlier when memcg
is gonna be using its private id anyway.  From cgroup core side, the
id space isn't restricted.

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 75e74408cc8f..1d8a6dffdc25 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
...
> +static void mem_cgroup_id_put(struct mem_cgroup *memcg)
> +{
> +	if (atomic_dec_and_test(&memcg->id.ref)) {
> +		idr_remove(&mem_cgroup_idr, memcg->id.id);

Maybe this should do "memcg->id.id = 0"?

> +		css_id_free(&memcg->css);
> +		css_put(&memcg->css);
> +	}
> +}
> +
> +/**
> + * mem_cgroup_from_id - look up a memcg from a memcg id
> + * @id: the memcg id to look up
> + *
> + * Caller must hold rcu_read_lock().
> + */
> +struct mem_cgroup *mem_cgroup_from_id(unsigned short id)
> +{
> +	WARN_ON_ONCE(!rcu_read_lock_held());
> +	return id > 0 ? idr_find(&mem_cgroup_idr, id) : NULL;
> +}

css_from_id() has it too but I don't think id > 0 test is necessary.
We prolly should take it out of css_from_id() too.

It might be useful to add comment explaining why memcg needs private
ids.

Thanks.

-- 
tejun