[Devel] Re: [PATCH -mmotm 3/3] memcg: dirty pages instrumentation

[Andrea Righi]

On Tue, Mar 02, 2010 at 06:59:32PM -0500, Vivek Goyal wrote:
> On Tue, Mar 02, 2010 at 11:22:48PM +0100, Andrea Righi wrote:
> > On Tue, Mar 02, 2010 at 10:05:29AM -0500, Vivek Goyal wrote:
> > > On Mon, Mar 01, 2010 at 11:18:31PM +0100, Andrea Righi wrote:
> > > > On Mon, Mar 01, 2010 at 05:02:08PM -0500, Vivek Goyal wrote:
> > > > > > @@ -686,10 +699,14 @@ void throttle_vm_writeout(gfp_t gfp_mask)
> > > > > >                   */
> > > > > >                  dirty_thresh += dirty_thresh / 10;      /* wheeee... */
> > > > > >  
> > > > > > -                if (global_page_state(NR_UNSTABLE_NFS) +
> > > > > > -			global_page_state(NR_WRITEBACK) <= dirty_thresh)
> > > > > > -                        	break;
> > > > > > -                congestion_wait(BLK_RW_ASYNC, HZ/10);
> > > > > > +
> > > > > > +		dirty = mem_cgroup_page_stat(MEMCG_NR_DIRTY_WRITEBACK_PAGES);
> > > > > > +		if (dirty < 0)
> > > > > > +			dirty = global_page_state(NR_UNSTABLE_NFS) +
> > > > > > +				global_page_state(NR_WRITEBACK);
> > > > > 
> > > > > dirty is unsigned long. As mentioned last time, above will never be true?
> > > > > In general these patches look ok to me. I will do some testing with these.
> > > > 
> > > > Re-introduced the same bug. My bad. :(
> > > > 
> > > > The value returned from mem_cgroup_page_stat() can be negative, i.e.
> > > > when memory cgroup is disabled. We could simply use a long for dirty,
> > > > the unit is in # of pages so s64 should be enough. Or cast dirty to long
> > > > only for the check (see below).
> > > > 
> > > > Thanks!
> > > > -Andrea
> > > > 
> > > > Signed-off-by: Andrea Righi <arighi at develer.com>
> > > > ---
> > > >  mm/page-writeback.c |    2 +-
> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > > > index d83f41c..dbee976 100644
> > > > --- a/mm/page-writeback.c
> > > > +++ b/mm/page-writeback.c
> > > > @@ -701,7 +701,7 @@ void throttle_vm_writeout(gfp_t gfp_mask)
> > > >  
> > > >  
> > > >  		dirty = mem_cgroup_page_stat(MEMCG_NR_DIRTY_WRITEBACK_PAGES);
> > > > -		if (dirty < 0)
> > > > +		if ((long)dirty < 0)
> > > 
> > > This will also be problematic as on 32bit systems, your uppper limit of
> > > dirty memory will be 2G?
> > > 
> > > I guess, I will prefer one of the two.
> > > 
> > > - return the error code from function and pass a pointer to store stats
> > >   in as function argument.
> > > 
> > > - Or Peter's suggestion of checking mem_cgroup_has_dirty_limit() and if
> > >   per cgroup dirty control is enabled, then use per cgroup stats. In that
> > >   case you don't have to return negative values.
> > > 
> > >   Only tricky part will be careful accouting so that none of the stats go
> > >   negative in corner cases of migration etc.
> > 
> > What do you think about Peter's suggestion + the locking stuff? (see the
> > previous email). Otherwise, I'll choose the other solution, passing a
> > pointer and always return the error code is not bad.
> > 
> 
> Ok, so you are worried about that by the we finish mem_cgroup_has_dirty_limit()
> call, task might change cgroup and later we might call
> mem_cgroup_get_page_stat() on a different cgroup altogether which might or
> might not have dirty limits specified?

Correct.

> 
> But in what cases you don't want to use memory cgroup specified limit? I
> thought cgroup disabled what the only case where we need to use global
> limits. Otherwise a memory cgroup will have either dirty_bytes specified
> or by default inherit global dirty_ratio which is a valid number. If
> that's the case then you don't have to take rcu_lock() outside
> get_page_stat()?
> 
> IOW, apart from cgroup being disabled, what are the other cases where you
> expect to not use cgroup's page stat and use global stats?

At boot, when mem_cgroup_from_task() may return NULL. But this is not
related to the RCU acquisition.

Anyway, probably the RCU protection is not so critical for this
particular case, and we can simply get rid of it. In this way we can
easily implement the interface proposed by Peter.

-Andrea
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers