Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2

[Vladimir Davydov <vdavydov@virtuozzo.com>]

On Thu, Dec 10, 2015 at 11:00:27AM -0500, Johannes Weiner wrote:
> On Thu, Dec 10, 2015 at 02:39:14PM +0300, Vladimir Davydov wrote:
...
> > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > index c6a5ed2f2744..993c9a26b637 100644
> > --- a/include/linux/memcontrol.h
> > +++ b/include/linux/memcontrol.h
> > @@ -169,6 +169,7 @@ struct mem_cgroup {
> >  
> >  	/* Accounted resources */
> >  	struct page_counter memory;
> > +	struct page_counter swap;
> >  	struct page_counter memsw;
> >  	struct page_counter kmem;
> 
> We should probably separate this to differentiate the new counters
> from the old ones. Only memory and swap are actual resources, the
> memsw and kmem counters are counting consumer-oriented.

Yeah, but we'd better do it in a separate patch.

> 
> > diff --git a/include/linux/swap.h b/include/linux/swap.h
> > index 457181844b6e..f4b3ccdcba91 100644
> > --- a/include/linux/swap.h
> > +++ b/include/linux/swap.h
> > @@ -368,11 +368,16 @@ static inline int mem_cgroup_swappiness(struct mem_cgroup *mem)
> >  #endif
> >  #ifdef CONFIG_MEMCG_SWAP
> >  extern void mem_cgroup_swapout(struct page *page, swp_entry_t entry);
> > +extern int mem_cgroup_charge_swap(struct page *page, swp_entry_t entry);
> 
> Should this be mem_cgroup_try_swap() to keep in line with the page
> counter terminology? So it's clear this is not forcing a charge.

Hmm, I thought we only use try_charge name in memcontrol.c if there is
commit stage, e.g. we have memcg_kmem_charge, not memcg_kmem_try_charge.
This conflicts with page_counter semantics though, so we might want to
rename it.

> 
> > @@ -1248,12 +1248,15 @@ static unsigned long mem_cgroup_get_limit(struct mem_cgroup *memcg)
> >  {
> >  	unsigned long limit;
> >  
> > -	limit = memcg->memory.limit;
> > +	limit = READ_ONCE(memcg->memory.limit);
> >  	if (mem_cgroup_swappiness(memcg)) {
> >  		unsigned long memsw_limit;
> > +		unsigned long swap_limit;
> >  
> > -		memsw_limit = memcg->memsw.limit;
> > -		limit = min(limit + total_swap_pages, memsw_limit);
> > +		memsw_limit = READ_ONCE(memcg->memsw.limit);
> > +		swap_limit = min(READ_ONCE(memcg->swap.limit),
> > +				 (unsigned long)total_swap_pages);
> > +		limit = min(limit + swap_limit, memsw_limit);
> >  	}
> >  	return limit;
> 
> This is taking a racy snapshot, so we don't rely on 100% accuracy. Can
> we do without the READ_ONCE()?

Well, I suppose we can, but passing a volatile value to min macro looks
a bit scary to me. What if swap_limit is changed from a finite value
less than total_swap_pages to inf while we are there? We might use an
infinite memory size in OOM which would screw up OOM scores AFAIU.
Unlikely, but still.

> 
> > @@ -5754,26 +5760,66 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
> >  	memcg_check_events(memcg, page);
> >  }
> >  
> > +/*
> > + * mem_cgroup_charge_swap - charge a swap entry
> > + * @page: page being added to swap
> > + * @entry: swap entry to charge
> > + *
> > + * Try to charge @entry to the memcg that @page belongs to.
> > + *
> > + * Returns 0 on success, -ENOMEM on failure.
> > + */
> > +int mem_cgroup_charge_swap(struct page *page, swp_entry_t entry)
> > +{
> > +	struct mem_cgroup *memcg;
> > +	struct page_counter *counter;
> > +	unsigned short oldid;
> > +
> > +	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) || !do_swap_account)
> > +		return 0;
> > +
> > +	memcg = page->mem_cgroup;
> > +
> > +	/* Readahead page, never charged */
> > +	if (!memcg)
> > +		return 0;
> > +
> > +	if (!mem_cgroup_is_root(memcg) &&
> > +	    !page_counter_try_charge(&memcg->swap, 1, &counter))
> > +		return -ENOMEM;
> > +
> > +	oldid = swap_cgroup_record(entry, mem_cgroup_id(memcg));
> > +	VM_BUG_ON_PAGE(oldid, page);
> > +	mem_cgroup_swap_statistics(memcg, true);
> > +
> > +	css_get(&memcg->css);
> 
> I think we don't have to duplicate the swap record code. Both cgroup1
> and cgroup2 could run this function to handle the swapout record and
> statistics, and then mem_cgroup_swapout() would simply uncharge memsw.

Well, may be. I'm afraid this might make mem_cgroup_charge_swap look a
bit messy due to necessity to check cgroup_subsys_on_dfl in the middle
of it, but I'll give it a try.

> 
> > @@ -5828,6 +5931,8 @@ static int __init mem_cgroup_swap_init(void)
> >  {
> >  	if (!mem_cgroup_disabled() && really_do_swap_account) {
> >  		do_swap_account = 1;
> > +		WARN_ON(cgroup_add_dfl_cftypes(&memory_cgrp_subsys,
> > +					       swap_files));
> >  		WARN_ON(cgroup_add_legacy_cftypes(&memory_cgrp_subsys,
> >  						  memsw_cgroup_files));
> 
> I guess we could also support cgroup.memory=noswap.
> 

Yeah, that would be cleaner. I wonder if we could drop swapaccount boot
param then so as not to clutter the API.

Thanks for the review!