Re: [PATCH 09/16] mm: memcg/slab: charge individual slab objects instead of pages

[Roman Gushchin]

On Fri, Oct 25, 2019 at 03:41:18PM -0400, Johannes Weiner wrote:
> On Thu, Oct 17, 2019 at 05:28:13PM -0700, Roman Gushchin wrote:
> > +static inline struct kmem_cache *memcg_slab_pre_alloc_hook(struct kmem_cache *s,
> > +						struct mem_cgroup **memcgp,
> > +						size_t size, gfp_t flags)
> > +{
> > +	struct kmem_cache *cachep;
> > +
> > +	cachep = memcg_kmem_get_cache(s, memcgp);
> > +	if (is_root_cache(cachep))
> > +		return s;
> > +
> > +	if (__memcg_kmem_charge_subpage(*memcgp, size * s->size, flags)) {
> > +		mem_cgroup_put(*memcgp);
> > +		memcg_kmem_put_cache(cachep);
> > +		cachep = NULL;
> > +	}
> > +
> > +	return cachep;
> > +}
> > +
> >  static inline void memcg_slab_post_alloc_hook(struct kmem_cache *s,
> >  					      struct mem_cgroup *memcg,
> >  					      size_t size, void **p)
> >  {
> >  	struct mem_cgroup_ptr *memcg_ptr;
> > +	struct lruvec *lruvec;
> >  	struct page *page;
> >  	unsigned long off;
> >  	size_t i;
> > @@ -439,6 +393,11 @@ static inline void memcg_slab_post_alloc_hook(struct kmem_cache *s,
> >  			off = obj_to_index(s, page, p[i]);
> >  			mem_cgroup_ptr_get(memcg_ptr);
> >  			page->mem_cgroup_vec[off] = memcg_ptr;
> > +			lruvec = mem_cgroup_lruvec(page_pgdat(page), memcg);
> > +			mod_lruvec_memcg_state(lruvec, cache_vmstat_idx(s),
> > +					       s->size);
> > +		} else {
> > +			__memcg_kmem_uncharge_subpage(memcg, s->size);
> >  		}
> >  	}
> >  	mem_cgroup_ptr_put(memcg_ptr);
> 
> The memcg_ptr as a collection vessel for object references makes a lot
> of sense. But this code showcases that it should be a first-class
> memory tracking API that the allocator interacts with, rather than
> having to deal with a combination of memcg_ptr and memcg.
> 
> In the two hunks here, on one hand we charge bytes to the memcg
> object, and then handle all the refcounting through a different
> bucketing object. To support that in the first place, we have to
> overload the memcg API all the way down to try_charge() to support
> bytes and pages. This is difficult to follow throughout all layers.
> 
> What would be better is for this to be an abstraction layer for a
> subpage object tracker that sits on top of the memcg page tracker -
> not unlike the page allocator and the slab allocators themselves.
> 
> And then the slab allocator would only interact with the subpage
> object tracker, and the object tracker would deal with the memcg page
> tracker under the hood.

Hello, Johannes!

After some thoughts and coding I don't think it's a right direction to go.
Under direction I mean building the new subpage/obj_cgroup API on top
of the intact existing memcg API.
Let me illustrate my point on the code you provided (please, look below).

> 
> Something like the below, just a rough sketch to convey the idea:
> 
> On allocation, we look up an obj_cgroup from the current task, charge
> it with obj_cgroup_charge() and bump its refcount by the number of
> objects we allocated. obj_cgroup_charge() in turn uses the memcg layer
> to charge pages, and then doles them out as bytes using
> objcg->nr_stocked_bytes and the byte per-cpu stock.
> 
> On freeing, we call obj_cgroup_uncharge() and drop the references. In
> turn, this first refills the byte per-cpu stock and drains the
> overflow to the memcg layer (which in turn refills its own page stock
> and drains to the page_counter).
> 
> On cgroup deletion, we reassign the obj_cgroup to the parent, and all
> remaining live objects will free their pages to the updated parental
> obj_cgroup->memcg, as per usual.
> 
> ---
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index f36203cf75f8..efc3398aaf02 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -200,15 +200,15 @@ struct memcg_cgwb_frn {
>  };
>  
>  /*
> - * A pointer to a memory cgroup with a built-in reference counter.
> - * For a use as an intermediate object to simplify reparenting of
> - * objects charged to the cgroup. The memcg pointer can be switched
> - * to the parent cgroup without a need to modify all objects
> - * which hold the reference to the cgroup.
> + * Bucket for arbitrarily byte-sized objects charged to a memory
> + * cgroup. The bucket can be reparented in one piece when the cgroup
> + * is destroyed, without having to round up the individual references
> + * of all live memory objects in the wild.
>   */
> -struct mem_cgroup_ptr {
> -	struct percpu_ref refcnt;
> +struct obj_cgroup {
> +	struct percpu_ref count;
>  	struct mem_cgroup *memcg;
> +	unsigned long nr_stocked_bytes;
>  	union {
>  		struct list_head list;
>  		struct rcu_head rcu;
> @@ -230,7 +230,6 @@ struct mem_cgroup {
>  	/* Accounted resources */
>  	struct page_counter memory;
>  	struct page_counter swap;
> -	atomic_t nr_stocked_bytes;
>  
>  	/* Legacy consumer-oriented counters */
>  	struct page_counter memsw;
> @@ -327,8 +326,8 @@ struct mem_cgroup {
>          /* Index in the kmem_cache->memcg_params.memcg_caches array */
>  	int kmemcg_id;
>  	enum memcg_kmem_state kmem_state;
> -	struct mem_cgroup_ptr __rcu *kmem_memcg_ptr;
> -	struct list_head kmem_memcg_ptr_list;
> +	struct obj_cgroup __rcu *slab_objs;
> +	struct list_head slab_objs_list;
>  #endif
>  
>  	int last_scanned_node;
> @@ -474,21 +473,6 @@ struct mem_cgroup *mem_cgroup_from_css(struct cgroup_subsys_state *css){
>  	return css ? container_of(css, struct mem_cgroup, css) : NULL;
>  }
>  
> -static inline bool mem_cgroup_ptr_tryget(struct mem_cgroup_ptr *ptr)
> -{
> -	return percpu_ref_tryget(&ptr->refcnt);
> -}
> -
> -static inline void mem_cgroup_ptr_get(struct mem_cgroup_ptr *ptr)
> -{
> -	percpu_ref_get(&ptr->refcnt);
> -}
> -
> -static inline void mem_cgroup_ptr_put(struct mem_cgroup_ptr *ptr)
> -{
> -	percpu_ref_put(&ptr->refcnt);
> -}
> -
>  static inline void mem_cgroup_put(struct mem_cgroup *memcg)
>  {
>  	if (memcg)
> @@ -1444,9 +1428,9 @@ int __memcg_kmem_charge_memcg(struct page *page, gfp_t gfp, int order,
>  			      struct mem_cgroup *memcg);
>  void __memcg_kmem_uncharge_memcg(struct mem_cgroup *memcg,
>  				 unsigned int nr_pages);
> -int __memcg_kmem_charge_subpage(struct mem_cgroup *memcg, size_t size,
> +int obj_cgroup_charge(struct obj_cgroup *objcg, size_t size,
>  				gfp_t gfp);
> -void __memcg_kmem_uncharge_subpage(struct mem_cgroup *memcg, size_t size);
> +void obj_cgroup_uncharge(struct obj_cgroup *objcg, size_t size);
>  
>  extern struct static_key_false memcg_kmem_enabled_key;
>  extern struct workqueue_struct *memcg_kmem_cache_wq;
> @@ -1513,20 +1497,20 @@ static inline int memcg_cache_id(struct mem_cgroup *memcg)
>  	return memcg ? memcg->kmemcg_id : -1;
>  }
>  
> -static inline struct mem_cgroup_ptr *
> -mem_cgroup_get_kmem_ptr(struct mem_cgroup *memcg)
> +static inline struct obj_cgroup *
> +mem_cgroup_get_slab_objs(struct mem_cgroup *memcg)
>  {
> -	struct mem_cgroup_ptr *memcg_ptr;
> +	struct obj_cgroup *objcg;
>  
>  	rcu_read_lock();
>  	do {
> -		memcg_ptr = rcu_dereference(memcg->kmem_memcg_ptr);
> -		if (memcg_ptr && mem_cgroup_ptr_tryget(memcg_ptr))
> +		objcg = rcu_dereference(memcg->slab_objs);
> +		if (objcg && percpu_ref_tryget(&objcg->count))
>  			break;
>  	} while ((memcg = parent_mem_cgroup(memcg)));
>  	rcu_read_unlock();
>  
> -	return memcg_ptr;
> +	return objcg;
>  }
>  
>  #else
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 4d99ee5a9c53..7f663fd53c17 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -200,7 +200,7 @@ struct page {
>  #ifdef CONFIG_MEMCG
>  	union {
>  		struct mem_cgroup *mem_cgroup;
> -		struct mem_cgroup_ptr **mem_cgroup_vec;
> +		struct obj_cgroup **obj_cgroups;
>  	};
>  #endif
>  
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index b0d0c833150c..47110f4f0f4c 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -260,52 +260,51 @@ struct cgroup_subsys_state *vmpressure_to_css(struct vmpressure *vmpr)
>  #ifdef CONFIG_MEMCG_KMEM
>  extern spinlock_t css_set_lock;
>  
> -static void memcg_ptr_release(struct percpu_ref *ref)
> +static void obj_cgroup_release(struct percpu_ref *ref)
>  {
> -	struct mem_cgroup_ptr *ptr = container_of(ref, struct mem_cgroup_ptr,
> -						  refcnt);
> +	struct obj_cgroup *objcg;
>  	unsigned long flags;
>  
> +	objcg = container_of(ref, struct obj_cgroup, count);
> +
>  	spin_lock_irqsave(&css_set_lock, flags);
> -	list_del(&ptr->list);
> +	list_del(&objcg->list);
>  	spin_unlock_irqrestore(&css_set_lock, flags);
>  
> -	mem_cgroup_put(ptr->memcg);
> +	mem_cgroup_put(objcg->memcg);
>  	percpu_ref_exit(ref);
> -	kfree_rcu(ptr, rcu);
> +	kfree_rcu(objcg, rcu);
>  }

Btw, the part below is looking good, thanks for the idea!

>  
> -static int memcg_init_kmem_memcg_ptr(struct mem_cgroup *memcg)
> +static int obj_cgroup_alloc(struct mem_cgroup *memcg)
>  {
> -	struct mem_cgroup_ptr *kmem_memcg_ptr;
> +	struct obj_cgroup *objcg;
>  	int ret;
>  
> -	kmem_memcg_ptr = kmalloc(sizeof(struct mem_cgroup_ptr), GFP_KERNEL);
> -	if (!kmem_memcg_ptr)
> +	objcg = kmalloc(sizeof(struct obj_cgroup), GFP_KERNEL);
> +	if (!objcg)
>  		return -ENOMEM;
>  
> -	ret = percpu_ref_init(&kmem_memcg_ptr->refcnt, memcg_ptr_release,
> +	ret = percpu_ref_init(&objcg->count, obj_cgroup_release,
>  			      0, GFP_KERNEL);
>  	if (ret) {
> -		kfree(kmem_memcg_ptr);
> +		kfree(objcg);
>  		return ret;
>  	}
>  
> -	kmem_memcg_ptr->memcg = memcg;
> -	INIT_LIST_HEAD(&kmem_memcg_ptr->list);
> -	rcu_assign_pointer(memcg->kmem_memcg_ptr, kmem_memcg_ptr);
> -	list_add(&kmem_memcg_ptr->list, &memcg->kmem_memcg_ptr_list);
> +	INIT_LIST_HEAD(&objcg->list);
> +	objcg->memcg = memcg;
>  	return 0;
>  }
>  

< cut >

> @@ -3117,15 +3095,24 @@ void __memcg_kmem_uncharge(struct page *page, int order)
>  	css_put_many(&memcg->css, nr_pages);
>  }
>  
> -int __memcg_kmem_charge_subpage(struct mem_cgroup *memcg, size_t size,
> -				gfp_t gfp)
> +int obj_cgroup_charge(struct obj_cgroup *objcg, size_t size, gfp_t gfp)
>  {
> -	return try_charge(memcg, gfp, size, true);
> +	int ret;
> +
> +	if (consume_obj_stock(objcg, nr_bytes))
> +		return 0;
> +
> +	ret = try_charge(objcg->memcg, gfp, 1);
> +	if (ret)
> +		return ret;

So, the first problem is here: try_charge() will bump the memcg reference.
It works for generic pages, where we set the mem_cgroup pointer, so that
the reference corresponds to the actual physical page.
But in this case, the actual page is shared between multiple cgroups.
Memcg will be basically referenced once for each stock cache miss.
I struggle to understand what does it mean and how to balance it
with unreferencing on the free path.

The second problem is also here. If a task belonging to a different memcg
is scheduled on this cpu, most likely we will need to refill both stocks,
even if we need only a small temporarily allocation.

> +
> +	refill_obj_stock(objcg, PAGE_SIZE - size);

And the third problem is here. Percpu allocations (on which accounting I'm
working right now) can be larger than a page.

This is fairly small issue in comparison to the first one. But it illustrates
well the main point: we can't simple get a page from the existing API and
sublease it in parts. The problem is that we need to break the main principle
that a page belongs to a single memcg.

Please, let me know what do you think about it.

Thank you!

Roman