Re: [PATCH 05/25] Break up the allocator entry point into fast and slow paths

[KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>]

> > >  
> > > -	/* This allocation should allow future memory freeing. */
> > > -
> > > -rebalance:
> > > -	if (((p->flags & PF_MEMALLOC) || unlikely(test_thread_flag(TIF_MEMDIE)))
> > > -			&& !in_interrupt()) {
> > > -		if (!(gfp_mask & __GFP_NOMEMALLOC)) {
> > > -nofail_alloc:
> > > -			/* go through the zonelist yet again, ignoring mins */
> > > -			page = get_page_from_freelist(gfp_mask, nodemask, order,
> > > -				zonelist, high_zoneidx, ALLOC_NO_WATERMARKS);
> > > -			if (page)
> > > -				goto got_pg;
> > > -			if (gfp_mask & __GFP_NOFAIL) {
> > > -				congestion_wait(WRITE, HZ/50);
> > > -				goto nofail_alloc;
> > > -			}
> > > -		}
> > > -		goto nopage;
> > > -	}
> > > +	/* Allocate without watermarks if the context allows */
> > > +	if (is_allocation_high_priority(p, gfp_mask))
> > > +		page = __alloc_pages_high_priority(gfp_mask, order,
> > > +			zonelist, high_zoneidx, nodemask);
> > > +	if (page)
> > > +		goto got_pg;
> > >  
> > >  	/* Atomic allocations - we can't balance anything */
> > >  	if (!wait)
> > >  		goto nopage;
> > >  
> > 
> > old code is below.
> > if PF_MEMALLOC and !in_interrupt() and __GFP_NOMEMALLOC case,
> > old code jump to nopage, your one call reclaim.
> > 
> > I think, if the task have PF_MEMALLOC, it shouldn't call reclaim.
> > if not, endless reclaim recursion happend.
> > 
> > --------------------------------------------------------------------
> > rebalance:
> >         if (((p->flags & PF_MEMALLOC) || unlikely(test_thread_flag(TIF_MEMDIE)))
> >                         && !in_interrupt()) {
> >                 if (!(gfp_mask & __GFP_NOMEMALLOC)) {
> > nofail_alloc:
> >                         /* go through the zonelist yet again, ignoring mins */
> >                         page = get_page_from_freelist(gfp_mask, nodemask, order,
> >                                 zonelist, high_zoneidx, ALLOC_NO_WATERMARKS);
> >                         if (page)
> >                                 goto got_pg;
> >                         if (gfp_mask & __GFP_NOFAIL) {
> >                                 congestion_wait(WRITE, HZ/50);
> >                                 goto nofail_alloc;
> >                         }
> >                 }
> >                 goto nopage;
> >         }
> > --------------------------------------------------------------------
> > 
> 
> I altered the modified version to look like
> 
> static inline int
> is_allocation_high_priority(struct task_struct *p, gfp_t gfp_mask)
> {
>         if (((p->flags & PF_MEMALLOC) || unlikely(test_thread_flag(TIF_MEMDIE)))
>                         && !in_interrupt())
>                 return 1;
>         return 0;
> }
> 
> Note the check to __GFP_NOMEMALLOC is no longer there.
> 
> ....
> 
>         /* Allocate without watermarks if the context allows */
>         if (is_allocation_high_priority(p, gfp_mask)) {
> 		/* Do not dip into emergency reserves if specified */
>                 if (!(gfp_mask & __GFP_NOMEMALLOC)) {
>                         page = __alloc_pages_high_priority(gfp_mask, order,
>                                 zonelist, high_zoneidx, nodemask);
>                         if (page)
>                                 goto got_pg;
>                 }
> 
> 		/* Ensure no recursion into the allocator */
>                 goto nopage;
>         }
> 
> 
> Is that better?

nice.



> > >  	/* Atomic allocations - we can't balance anything */
> > >  	if (!wait)
> > >  		goto nopage;
> > >  
> > > -	cond_resched();
> > > -
> > > -	/* We now go into synchronous reclaim */
> > > -	cpuset_memory_pressure_bump();
> > > -
> > > -	p->flags |= PF_MEMALLOC;
> > > -
> > > -	lockdep_set_current_reclaim_state(gfp_mask);
> > > -	reclaim_state.reclaimed_slab = 0;
> > > -	p->reclaim_state = &reclaim_state;
> > > -
> > > -	did_some_progress = try_to_free_pages(zonelist, order,
> > > -						gfp_mask, nodemask);
> > > -
> > > -	p->reclaim_state = NULL;
> > > -	lockdep_clear_current_reclaim_state();
> > > -	p->flags &= ~PF_MEMALLOC;
> > > -
> > > -	cond_resched();
> > > +	/* Try direct reclaim and then allocating */
> > > +	page = __alloc_pages_direct_reclaim(gfp_mask, order,
> > > +					zonelist, high_zoneidx,
> > > +					nodemask,
> > > +					alloc_flags, &did_some_progress);
> > > +	if (page)
> > > +		goto got_pg;
> > >  
> > > -	if (order != 0)
> > > -		drain_all_pages();
> > > +	/*
> > > +	 * If we failed to make any progress reclaiming, then we are
> > > +	 * running out of options and have to consider going OOM
> > > +	 */
> > > +	if (!did_some_progress) {
> > > +		if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
> > > +			page = __alloc_pages_may_oom(gfp_mask, order,
> > > +					zonelist, high_zoneidx,
> > > +					nodemask);
> > > +			if (page)
> > > +				goto got_pg;
> > 
> > the old code here.
> > 
> > ------------------------------------------------------------------------
> >         } else if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
> >                 if (!try_set_zone_oom(zonelist, gfp_mask)) {
> >                         schedule_timeout_uninterruptible(1);
> >                         goto restart;
> >                 }
> > 
> >                 /*
> >                  * Go through the zonelist yet one more time, keep
> >                  * very high watermark here, this is only to catch
> >                  * a parallel oom killing, we must fail if we're still
> >                  * under heavy pressure.
> >                  */
> >                 page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, nodemask,
> >                         order, zonelist, high_zoneidx,
> >                         ALLOC_WMARK_HIGH|ALLOC_CPUSET);
> >                 if (page) {
> >                         clear_zonelist_oom(zonelist, gfp_mask);
> >                         goto got_pg;
> >                 }
> > 
> >                 /* The OOM killer will not help higher order allocs so fail */
> >                 if (order > PAGE_ALLOC_COSTLY_ORDER) {
> >                         clear_zonelist_oom(zonelist, gfp_mask);
> >                         goto nopage;
> >                 }
> > 
> >                 out_of_memory(zonelist, gfp_mask, order);
> >                 clear_zonelist_oom(zonelist, gfp_mask);
> >                 goto restart;
> >         }
> > ------------------------------------------------------------------------
> > 
> > if get_page_from_freelist() return NULL and order > PAGE_ALLOC_COSTLY_ORDER,
> > old code jump to nopage, your one jump to restart.
> > 
> 
> Good spot. The new section now looks like
> 
>                         page = __alloc_pages_may_oom(gfp_mask, order,
>                                         zonelist, high_zoneidx,
>                                         nodemask);
>                         if (page)
>                                 goto got_pg;
> 
>                         /*
>                          * The OOM killer does not trigger for high-order allocations
>                          * but if no progress is being made, there are no other
>                          * options and retrying is unlikely to help
>                          */
>                         if (order > PAGE_ALLOC_COSTLY_ORDER)
>                                 goto nopage;
> 
> Better?

very good.




> > > -	/*
> > > -	 * Don't let big-order allocations loop unless the caller explicitly
> > > -	 * requests that.  Wait for some write requests to complete then retry.
> > > -	 *
> > > -	 * In this implementation, order <= PAGE_ALLOC_COSTLY_ORDER
> > > -	 * means __GFP_NOFAIL, but that may not be true in other
> > > -	 * implementations.
> > > -	 *
> > > -	 * For order > PAGE_ALLOC_COSTLY_ORDER, if __GFP_REPEAT is
> > > -	 * specified, then we retry until we no longer reclaim any pages
> > > -	 * (above), or we've reclaimed an order of pages at least as
> > > -	 * large as the allocation's order. In both cases, if the
> > > -	 * allocation still fails, we stop retrying.
> > > -	 */
> > > +	/* Check if we should retry the allocation */
> > >  	pages_reclaimed += did_some_progress;
> > > -	do_retry = 0;
> > > -	if (!(gfp_mask & __GFP_NORETRY)) {
> > > -		if (order <= PAGE_ALLOC_COSTLY_ORDER) {
> > > -			do_retry = 1;
> > > -		} else {
> > > -			if (gfp_mask & __GFP_REPEAT &&
> > > -				pages_reclaimed < (1 << order))
> > > -					do_retry = 1;
> > > -		}
> > > -		if (gfp_mask & __GFP_NOFAIL)
> > > -			do_retry = 1;
> > > -	}
> > > -	if (do_retry) {
> > > +	if (should_alloc_retry(gfp_mask, order, pages_reclaimed)) {
> > > +		/* Wait for some write requests to complete then retry */
> > >  		congestion_wait(WRITE, HZ/50);
> > > -		goto rebalance;
> > > +		goto restart;
> > 
> > this change rebalance to restart.
> > 
> 
> True, it's makes more sense to me sensible to goto restart at that point
> after waiting on IO to complete but it's a functional change and doesn't
> belong in this patch. I've fixed it up.
> 
> Very well spotted.

excellent.

thanks.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>