On Wed 21-02-18 14:37:56, Shakeel Butt wrote:
[...]
> +#ifdef CONFIG_MEMCG
> +static inline struct mem_cgroup *memalloc_memcg_save(struct mem_cgroup 
> *memcg)
> +{
> +     struct mem_cgroup *old_memcg = current->target_memcg;
> +     current->target_memcg = memcg;
> +     return old_memcg;
> +}

So you are relying that the caller will handle the reference counting
properly? I do not think this is a good idea. Also do we need some kind
of debugging facility to detect unbalanced save/restore scopes?

[...]
> @@ -2260,7 +2269,10 @@ struct kmem_cache *memcg_kmem_get_cache(struct 
> kmem_cache *cachep)
>       if (current->memcg_kmem_skip_account)
>               return cachep;
>  
> -     memcg = get_mem_cgroup_from_mm(current->mm);
> +     if (current->target_memcg)
> +             memcg = get_mem_cgroup(current->target_memcg);
> +     if (!memcg)
> +             memcg = get_mem_cgroup_from_mm(current->mm);
>       kmemcg_id = READ_ONCE(memcg->kmemcg_id);
>       if (kmemcg_id < 0)
>               goto out;

You are also adding one branch for _each_ charge path even though the
usecase is rather limited.

I will have to think about this approach more. It is clearly less code
than your previous attempt but I cannot say I would be really impressed.
-- 
Michal Hocko
SUSE Labs

Reply via email to