| 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
| ||
|
Date: Tue, 21 Apr 2009 19:44:00 +0900 (JST)
From: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
To: Mel Gorman <mel@....ul.ie>
Cc: kosaki.motohiro@...fujitsu.com,
Linux Memory Management List <linux-mm@...ck.org>,
Christoph Lameter <cl@...ux-foundation.org>,
Nick Piggin <npiggin@...e.de>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Lin Ming <ming.m.lin@...el.com>,
Zhang Yanmin <yanmin_zhang@...ux.intel.com>,
Peter Zijlstra <peterz@...radead.org>,
Pekka Enberg <penberg@...helsinki.fi>,
Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 05/25] Break up the allocator entry point into fast and slow paths
> > >
> > > - /* 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 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/
Powered by blists - more mailing lists