Re: [RFC -mm] memcg: prevent from OOM with too many dirty pages

[Fengguang Wu]

[restore CC list]

On Thu, May 31, 2012 at 08:25:09PM +0200, Michal Hocko wrote:
> On Fri 01-06-12 00:01:29, Fengguang Wu wrote:
> > On Thu, May 31, 2012 at 05:49:00PM +0200, Michal Hocko wrote:
> > > On Thu 31-05-12 23:42:48, Fengguang Wu wrote:
> > > > On Thu, May 31, 2012 at 05:32:49PM +0200, Michal Hocko wrote:
> > > > > JFYI: You might have missed https://lkml.org/lkml/2012/5/31/122 because
> > > > > intel mail server returned with "we do not like shell scripts as a
> > > > > attachment".
> > > > 
> > > > Yeah I did miss it.. A quick question: does the PageReclaim patch
> > > > tested include the (priority < 3) test, or it's the originally posted
> > > > patch?
> > > 
> > > It's the original patch.
> > 
> > OK, that's fine. I suspect even adding (priority < 3), it can help
> > only some situations. In the others, the effect will be that writeback
> > pages get accumulated in LRU until it starts to throttle page reclaims
> > from both reads/writes. There will have to be some throttling somewhere.
> 
> Yes, I agree. But it could help at least sporadic "hey this is a PageReclaim"
> issues. I just didn't like to push priority into shrink_page_list
> without. The justification is quite hard.

Yeah priority is just a rule of thumb "there may be lots of
dirty/writeback pages or other pressure if priority goes low". 
And it's already been used this way in shrink_page_list().

Considering that it's also targeting for -stable merge, we do need
a very strict condition to safeguard no regressions on other cases.
This is also true for the wait_iff_congested() scheme.

> > Subject: mm: pass __GFP_WRITE to memcg charge and reclaim routines
> > 
> > __GFP_WRITE will be tested in vmscan to find out the write tasks.
> > 
> > For good interactive performance, we try to focus dirty reclaim waits on
> > them and avoid blocking unrelated tasks.
> > 
> > Signed-off-by: Fengguang Wu <fengguang.wu@xxxxxxxxx>
> 
> I will have a look at this one tomorrow with a clean head.

OK. The usage in my mind is

        if (PageWriteback(page) && PageReclaim(page))
+               if ((sc->gfp_mask & __GFP_WAIT) || (priority < 3))
                        do some dirty throttling

But note that it only detects writes to new pages (ie. simple dd).
Overwrites to already cached clean pages cannot be detected this way..

Thanks,
Fengguang

> > ---
> >  include/linux/gfp.h |    2 +-
> >  mm/filemap.c        |   17 ++++++++++-------
> >  2 files changed, 11 insertions(+), 8 deletions(-)
> > 
> > --- linux.orig/include/linux/gfp.h	2012-03-02 14:06:47.501765703 +0800
> > +++ linux/include/linux/gfp.h	2012-03-02 14:07:39.921766949 +0800
> > @@ -129,7 +129,7 @@ struct vm_area_struct;
> >  /* Control page allocator reclaim behavior */
> >  #define GFP_RECLAIM_MASK (__GFP_WAIT|__GFP_HIGH|__GFP_IO|__GFP_FS|\
> >  			__GFP_NOWARN|__GFP_REPEAT|__GFP_NOFAIL|\
> > -			__GFP_NORETRY|__GFP_NOMEMALLOC)
> > +			__GFP_NORETRY|__GFP_NOMEMALLOC|__GFP_WRITE)
> >  
> >  /* Control slab gfp mask during early boot */
> >  #define GFP_BOOT_MASK (__GFP_BITS_MASK & ~(__GFP_WAIT|__GFP_IO|__GFP_FS))
> > --- linux.orig/mm/filemap.c	2012-03-02 14:07:21.000000000 +0800
> > +++ linux/mm/filemap.c	2012-03-02 14:07:53.709767277 +0800
> > @@ -2339,23 +2339,26 @@ struct page *grab_cache_page_write_begin
> >  	int status;
> >  	gfp_t gfp_mask;
> >  	struct page *page;
> > -	gfp_t gfp_notmask = 0;
> > +	gfp_t lru_gfp_mask = GFP_KERNEL;
> >  
> >  	gfp_mask = mapping_gfp_mask(mapping);
> > -	if (mapping_cap_account_dirty(mapping))
> > +	if (mapping_cap_account_dirty(mapping)) {
> >  		gfp_mask |= __GFP_WRITE;
> > -	if (flags & AOP_FLAG_NOFS)
> > -		gfp_notmask = __GFP_FS;
> > +		lru_gfp_mask |= __GFP_WRITE;
> > +	}
> > +	if (flags & AOP_FLAG_NOFS) {
> > +		gfp_mask &= ~__GFP_FS;
> > +		lru_gfp_mask &= ~__GFP_FS;
> > +	}
> >  repeat:
> >  	page = find_lock_page(mapping, index);
> >  	if (page)
> >  		goto found;
> >  
> > -	page = __page_cache_alloc(gfp_mask & ~gfp_notmask);
> > +	page = __page_cache_alloc(gfp_mask);
> >  	if (!page)
> >  		return NULL;
> > -	status = add_to_page_cache_lru(page, mapping, index,
> > -						GFP_KERNEL & ~gfp_notmask);
> > +	status = add_to_page_cache_lru(page, mapping, index, lru_gfp_mask);
> >  	if (unlikely(status)) {
> >  		page_cache_release(page);
> >  		if (status == -EEXIST)
> 
> 
> -- 
> Michal Hocko
> SUSE Labs
> SUSE LINUX s.r.o.
> Lihovarska 1060/12
> 190 00 Praha 9    
> Czech Republic
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/