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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 2 Mar 2010 10:05:29 -0500
From:	Vivek Goyal <vgoyal@...hat.com>
To:	Andrea Righi <arighi@...eler.com>
Cc:	Balbir Singh <balbir@...ux.vnet.ibm.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Suleiman Souhlal <suleiman@...gle.com>,
	Greg Thelen <gthelen@...gle.com>,
	Daisuke Nishimura <nishimura@....nes.nec.co.jp>,
	"Kirill A. Shutemov" <kirill@...temov.name>,
	Andrew Morton <akpm@...ux-foundation.org>,
	containers@...ts.linux-foundation.org,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH -mmotm 3/3] memcg: dirty pages instrumentation

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@...eler.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.

Thanks
Vivek

>  			dirty = global_page_state(NR_UNSTABLE_NFS) +
>  				global_page_state(NR_WRITEBACK);
>  		if (dirty <= dirty_thresh)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/