diff mbox

[v2,1/3] mm: remove GFP_THISNODE

Message ID 54F48980.3090008@suse.cz
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Vlastimil Babka March 2, 2015, 4:02 p.m. UTC 
On 03/02/2015 04:46 PM, Christoph Lameter wrote:
> On Mon, 2 Mar 2015, Vlastimil Babka wrote:
> 
>> So it would be IMHO better for longer-term maintainability to have the
>> relevant __GFP_THISNODE callers pass also __GFP_NO_KSWAPD to denote these
>> opportunistic allocation attempts, instead of having this subtle semantic
> 
> You are thinking about an opportunistic allocation attempt in SLAB?
> 
> AFAICT SLAB allocations should trigger reclaim.
> 

Well, let me quote your commit 952f3b51beb5:

--------
commit 952f3b51beb592f3f1de15adcdef802fc086ea91
Author: Christoph Lameter <clameter@sgi.com>
Date:   Wed Dec 6 20:33:26 2006 -0800

    [PATCH] GFP_THISNODE must not trigger global reclaim
    
    The intent of GFP_THISNODE is to make sure that an allocation occurs on a
    particular node.  If this is not possible then NULL needs to be returned so
    that the caller can choose what to do next on its own (the slab allocator
    depends on that).
    
    However, GFP_THISNODE currently triggers reclaim before returning a failure
    (GFP_THISNODE means GFP_NORETRY is set).  If we have over allocated a node
    then we will currently do some reclaim before returning NULL.  The caller
    may want memory from other nodes before reclaim should be triggered.  (If
    the caller wants reclaim then he can directly use __GFP_THISNODE instead).
    
    There is no flag to avoid reclaim in the page allocator and adding yet
    another GFP_xx flag would be difficult given that we are out of available
    flags.
    
    So just compare and see if all bits for GFP_THISNODE (__GFP_THISNODE,
    __GFP_NORETRY and __GFP_NOWARN) are set.  If so then we return NULL before
    waking up kswapd.
    
    Signed-off-by: Christoph Lameter <clameter@sgi.com>
    Signed-off-by: Andrew Morton <akpm@osdl.org>
    Signed-off-by: Linus Torvalds <torvalds@osdl.org>

So it seems to me that *at least some* allocations in slab are supposed
to work like this? Of course it's possible that since 2006, more
allocation sites in slab started passing GFP_THISNODE without realizing
this semantics. In that case, such sites should be fixed. (I think David
already mentioned some in this thread?)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Christoph Lameter (Ampere) March 2, 2015, 4:08 p.m. UTC | #1 
On Mon, 2 Mar 2015, Vlastimil Babka wrote:

> > You are thinking about an opportunistic allocation attempt in SLAB?
> >
> > AFAICT SLAB allocations should trigger reclaim.
> >
>
> Well, let me quote your commit 952f3b51beb5:

This was about global reclaim. Local reclaim is good and that can be
done via zone_reclaim.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vlastimil Babka March 2, 2015, 4:23 p.m. UTC | #2 
On 03/02/2015 05:08 PM, Christoph Lameter wrote:
> On Mon, 2 Mar 2015, Vlastimil Babka wrote:
>
>>> You are thinking about an opportunistic allocation attempt in SLAB?
>>>
>>> AFAICT SLAB allocations should trigger reclaim.
>>>
>>
>> Well, let me quote your commit 952f3b51beb5:
>
> This was about global reclaim. Local reclaim is good and that can be
> done via zone_reclaim.

Right, so the patch is a functional change for zone_reclaim_mode == 1, 
where !__GFP_WAIT will prevent it.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Rientjes March 2, 2015, 8:40 p.m. UTC | #3 
On Mon, 2 Mar 2015, Vlastimil Babka wrote:

> > > > You are thinking about an opportunistic allocation attempt in SLAB?
> > > > 
> > > > AFAICT SLAB allocations should trigger reclaim.
> > > > 
> > > 
> > > Well, let me quote your commit 952f3b51beb5:
> > 
> > This was about global reclaim. Local reclaim is good and that can be
> > done via zone_reclaim.
> 
> Right, so the patch is a functional change for zone_reclaim_mode == 1, where
> !__GFP_WAIT will prevent it.
> 

My patch is not a functional change, get_page_from_freelist() handles 
zone_reclaim_mode == 1 properly in the page allocator fastpath.  This 
patch only touches the slowpath.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 5d123b3..dc8753b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1151,6 +1151,17 @@  restart:
        if (page)
                goto got_pg;
 
+       /*
+        * GFP_THISNODE (meaning __GFP_THISNODE, __GFP_NORETRY and
+        * __GFP_NOWARN set) should not cause reclaim since the subsystem
+        * (f.e. slab) using GFP_THISNODE may choose to trigger reclaim
+        * using a larger set of nodes after it has established that the
+        * allowed per node queues are empty and that nodes are
+        * over allocated.
+        */
+       if (NUMA_BUILD && (gfp_mask & GFP_THISNODE) == GFP_THISNODE)
+               goto nopage;
+
        for (z = zonelist->zones; *z; z++)
                wakeup_kswapd(*z, order);
--------