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

[Dan Magenheimer <dan.magenheimer@oracle.com>]

> From: KAMEZAWA Hiroyuki [mailto:kamezawa.hiroyu@jp.fujitsu.com]
> Subject: Re: Subject: [PATCH V7 2/4] mm: frontswap: core code
> 
> 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.

> > 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)

Since the exact values of these counters is not required
by any code (just information for userland), I think I will
just add a comment.

> > > 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--

Hmmm... OK, you've convinced me.  If this counter should be one and
a race leaves it as zero, I think data corruption could result on
a swapoff or partial swapoff.  And after thinking about it, I
think I also need to check for locking on frontswap_set/clear
as I don't think these bitfield modifiers are atomic.

Thanks for pointing this out.  Good catch!  I will need to
play with this and test it so probably will not submit V8 until
next week as today is a vacation day for me.

Dan