Re: Subject: [PATCH V7 2/4] mm: frontswap: core code

[KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>]

On Thu, 25 Aug 2011 10:37:05 -0700 (PDT)
Dan Magenheimer <dan.magenheimer@oracle.com> wrote:

> > From: KAMEZAWA Hiroyuki [mailto:kamezawa.hiroyu@jp.fujitsu.com]
> > Subject: Re: Subject: [PATCH V7 2/4] mm: frontswap: core code

> > BTW, Do I have a chance to implement frontswap accounting per cgroup
> > (under memcg) ? Or Do I need to enable/disale switch for frontswap per memcg ?
> > Do you think it is worth to do ?
> 
> I'm not very familiar with cgroups or memcg but I think it may be possible
> to implement transcendent memory with cgroup as the "guest" and the default
> cgroup as the "host" to allow for more memory elasticity for cgroups.
> (See http://lwn.net/Articles/454795/ for a good overview of all of
> transcendent memory.)
> 
Ok, I'll see it.

I just wonder following case.

Assume 2 memcgs.
	memcg X: memory limit = 300M.
	memcg Y: memory limit = 300M.

This limitation is done for performance isolation.
When using frontswap, X and Y can cause resource confliction in frontswap and
performance of X and Y cannot be predictable.


> > > +/*
> > > + * This global enablement flag reduces overhead on systems where frontswap_ops
> > > + * has not been registered, so is preferred to the slower alternative: a
> > > + * function call that checks a non-global.
> > > + */
> > > +int frontswap_enabled;
> > > +EXPORT_SYMBOL(frontswap_enabled);
> > > +
> > > +/* useful stats available in /sys/kernel/mm/frontswap */
> > > +static unsigned long frontswap_gets;
> > > +static unsigned long frontswap_succ_puts;
> > > +static unsigned long frontswap_failed_puts;
> > > +static unsigned long frontswap_flushes;
> > > +
> > 
> > What lock guard these ? swap_lock ?
> 
> These are informational statistics so do not need to be protected
> by a lock or an atomic-type.  If an increment is lost due to a cpu
> race, it is not a problem.
> 

Hmm...Personally, I don't like incorrect counters. Could you add comments ?
Or How anout using percpu_counter ? (see lib/percpu_counter.c)


> > > +/* Called when a swap device is swapon'd */
> > > +void __frontswap_init(unsigned type)
> > > +{
> > > +	struct swap_info_struct *sis = swap_info[type];
> > > +
> > > +	BUG_ON(sis == NULL);
> > > +	if (sis->frontswap_map == NULL)
> > > +		return;
> > > +	if (frontswap_enabled)
> > > +		(*frontswap_ops.init)(type);
> > > +}
> > > +EXPORT_SYMBOL(__frontswap_init);
> > > +
> > > +/*
> > > + * "Put" data from a page to frontswap and associate it with the page's
> > > + * swaptype and offset.  Page must be locked and in the swap cache.
> > > + * If frontswap already contains a page with matching swaptype and
> > > + * offset, the frontswap implmentation may either overwrite the data
> > > + * and return success or flush the page from frontswap and return failure
> > > + */
> > 
> > What lock should be held to guard global variables ? swap_lock ?
> 
> Which global variables do you mean and in what routines?  I think the
> page lock is required for put/get (as documented in the comments)
> but not the swap_lock.
> 

My concern was race in counters. Even you allow race in frontswap_succ_puts++,

Don't you need some lock for
	sis->frontswap_pages++
	sis->frontswap_pages--
?

Thanks,
-Kame