| Message ID | alpine.DEB.2.10.1502271415510.7225@chino.kir.corp.google.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 27 Feb 2015, David Rientjes wrote: > +/* > + * Construct gfp mask to allocate from a specific node but do not invoke reclaim > + * or warn about failures. > + */ We should be triggering reclaim from slab allocations. Why would we not do this? Otherwise we will be going uselessly off node for slab allocations. > +static inline gfp_t gfp_exact_node(gfp_t flags) > +{ > + return (flags | __GFP_THISNODE | __GFP_NOWARN) & ~__GFP_WAIT; > +} > #endif Reclaim needs to be triggered. In particular zone reclaim was made to be triggered from slab allocations to create more room if needed. -- 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
On Fri, 27 Feb 2015, Christoph Lameter wrote: > > +/* > > + * Construct gfp mask to allocate from a specific node but do not invoke reclaim > > + * or warn about failures. > > + */ > > We should be triggering reclaim from slab allocations. Why would we not do > this? > > Otherwise we will be going uselessly off node for slab allocations. > > > +static inline gfp_t gfp_exact_node(gfp_t flags) > > +{ > > + return (flags | __GFP_THISNODE | __GFP_NOWARN) & ~__GFP_WAIT; > > +} > > #endif > > Reclaim needs to be triggered. In particular zone reclaim was made to be > triggered from slab allocations to create more room if needed. > This illustrates the precise need for a patch like this that removes GFP_THISNODE entirely: notice there's no functional change with this patch. GFP_THISNODE is __GFP_THISNODE | __GFP_NOWARN | __GFP_NORETRY. The calls to ____cache_alloc_node() and cache_grow() modified by this patch in mm/slab.c that pass GFP_THISNODE get caught in the page allocator slowpath by this: if (IS_ENABLED(CONFIG_NUMA) && (gfp_mask & GFP_THISNODE) == GFP_THISNODE) goto nopage; with today's kernel. In fact, there is no way for the slab allocator to currently allocate exactly on one node, allow reclaim, and avoid looping forever while suppressing the page allocation failure warning. The reason is because of how GFP_THISNODE is defined above. With this patch, it would be possible to modify gfp_exact_node() so that instead of doing return (flags | __GFP_THISNODE | __GFP_NOWARN) & ~__GFP_WAIT; which has no functional change from today, it could be return flags | __GFP_THISNODE | __GFP_NOWARN | __GFP_NORETRY; so that we _can_ do reclaim for that node and avoid looping forever when the allocation fails. These three flags are the exact same bits set in today's GFP_THISNODE and it is, I agree, what the slab allocator really wants to do in cache_grow(). But the conditional above is what short-circuits such an allocation and needs to be removed, which is what this patch does. -- 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
On 02/27/2015 11:16 PM, David Rientjes wrote: > NOTE: this is not about __GFP_THISNODE, this is only about GFP_THISNODE. > > GFP_THISNODE is a secret combination of gfp bits that have different > behavior than expected. It is a combination of __GFP_THISNODE, > __GFP_NORETRY, and __GFP_NOWARN and is special-cased in the page allocator > slowpath to fail without trying reclaim even though it may be used in > combination with __GFP_WAIT. > > An example of the problem this creates: commit e97ca8e5b864 ("mm: fix > GFP_THISNODE callers and clarify") fixed up many users of GFP_THISNODE > that really just wanted __GFP_THISNODE. The problem doesn't end there, > however, because even it was a no-op for alloc_misplaced_dst_page(), > which also sets __GFP_NORETRY and __GFP_NOWARN, and > migrate_misplaced_transhuge_page(), where __GFP_NORETRY and __GFP_NOWAIT > is set in GFP_TRANSHUGE. Converting GFP_THISNODE to __GFP_THISNODE is > a no-op in these cases since the page allocator special-cases > __GFP_THISNODE && __GFP_NORETRY && __GFP_NOWARN. > > It's time to just remove GFP_THISNODE entirely. We leave __GFP_THISNODE > to restrict an allocation to a local node, but remove GFP_THISNODE and > its obscurity. Instead, we require that a caller clear __GFP_WAIT if it > wants to avoid reclaim. > > This allows the aforementioned functions to actually reclaim as they > should. It also enables any future callers that want to do > __GFP_THISNODE but also __GFP_NORETRY && __GFP_NOWARN to reclaim. The > rule is simple: if you don't want to reclaim, then don't set __GFP_WAIT. > > Aside: ovs_flow_stats_update() really wants to avoid reclaim as well, so > it is unchanged. > > Signed-off-by: David Rientjes <rientjes@google.com> Acked-by: Vlastimil Babka <vbabka@suse.cz> So you've convinced me that this is a non-functional change for slab and a prerequisity for patch 2/3 which is the main point of this series for 4.0. That said, the new 'goto nopage' condition is still triggered by a combination of flag states, and the less we have those, the better for us IMHO. Looking at commit 952f3b51be which introduced this particular check using GFP_THISNODE, it seemed like it was a workaround to avoid triggering reclaim, without needing a new gfp flag. Nowadays, we have such flag called __GFP_NO_KSWAPD and as I explained in my reply to v1 (where I missed the new condition), passing the flag would already prevent wake_all_kswapds() and treating the allocation as atomic in gfp_to_alloc_flags(). So the whole difference would be another get_page_from_freelist() attempt (possibly less constrained than the fast path one) and then bail out on !wait. 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 difference attached to __GFP_THISNODE && !__GFP_WAIT. It would be probably too risky for 4.0. On the other hand, I don't think even this series is really urgent. It's true that patch 2/3 fixes two commits, including a 4.0 one, but those commits are already not regressions without the fix. But maybe current -rcX is low enough to proceed with this series and catch any regressions in time, allowing the larger cleanups later. -- 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
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. -- 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 --git a/include/linux/gfp.h b/include/linux/gfp.h --- a/include/linux/gfp.h +++ b/include/linux/gfp.h @@ -117,16 +117,6 @@ struct vm_area_struct; __GFP_NOMEMALLOC | __GFP_NORETRY | __GFP_NOWARN | \ __GFP_NO_KSWAPD) -/* - * GFP_THISNODE does not perform any reclaim, you most likely want to - * use __GFP_THISNODE to allocate from a given node without fallback! - */ -#ifdef CONFIG_NUMA -#define GFP_THISNODE (__GFP_THISNODE | __GFP_NOWARN | __GFP_NORETRY) -#else -#define GFP_THISNODE ((__force gfp_t)0) -#endif - /* This mask makes up all the page movable related flags */ #define GFP_MOVABLE_MASK (__GFP_RECLAIMABLE|__GFP_MOVABLE) diff --git a/mm/page_alloc.c b/mm/page_alloc.c --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -2355,13 +2355,7 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order, /* The OOM killer does not compensate for light reclaim */ if (!(gfp_mask & __GFP_FS)) goto out; - /* - * GFP_THISNODE contains __GFP_NORETRY and we never hit this. - * Sanity check for bare calls of __GFP_THISNODE, not real OOM. - * The caller should handle page allocation failure by itself if - * it specifies __GFP_THISNODE. - * Note: Hugepage uses it but will hit PAGE_ALLOC_COSTLY_ORDER. - */ + /* The OOM killer may not free memory on a specific node */ if (gfp_mask & __GFP_THISNODE) goto out; } @@ -2615,15 +2609,11 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order, } /* - * 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 this allocation cannot block and it is for a specific node, then + * fail early. There's no need to wakeup kswapd or retry for a + * speculative node-specific allocation. */ - if (IS_ENABLED(CONFIG_NUMA) && - (gfp_mask & GFP_THISNODE) == GFP_THISNODE) + if (IS_ENABLED(CONFIG_NUMA) && (gfp_mask & __GFP_THISNODE) && !wait) goto nopage; retry: @@ -2816,7 +2806,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order, /* * Check the zones suitable for the gfp_mask contain at least one * valid zone. It's possible to have an empty zonelist as a result - * of GFP_THISNODE and a memoryless node + * of __GFP_THISNODE and a memoryless node */ if (unlikely(!zonelist->_zonerefs->zone)) return NULL; diff --git a/mm/slab.c b/mm/slab.c --- a/mm/slab.c +++ b/mm/slab.c @@ -857,6 +857,11 @@ static inline void *____cache_alloc_node(struct kmem_cache *cachep, return NULL; } +static inline gfp_t gfp_exact_node(gfp_t flags) +{ + return flags; +} + #else /* CONFIG_NUMA */ static void *____cache_alloc_node(struct kmem_cache *, gfp_t, int); @@ -1023,6 +1028,15 @@ static inline int cache_free_alien(struct kmem_cache *cachep, void *objp) return __cache_free_alien(cachep, objp, node, page_node); } + +/* + * Construct gfp mask to allocate from a specific node but do not invoke reclaim + * or warn about failures. + */ +static inline gfp_t gfp_exact_node(gfp_t flags) +{ + return (flags | __GFP_THISNODE | __GFP_NOWARN) & ~__GFP_WAIT; +} #endif /* @@ -2825,7 +2839,7 @@ alloc_done: if (unlikely(!ac->avail)) { int x; force_grow: - x = cache_grow(cachep, flags | GFP_THISNODE, node, NULL); + x = cache_grow(cachep, gfp_exact_node(flags), node, NULL); /* cache_grow can reenable interrupts, then ac could change. */ ac = cpu_cache_get(cachep); @@ -3019,7 +3033,7 @@ retry: get_node(cache, nid) && get_node(cache, nid)->free_objects) { obj = ____cache_alloc_node(cache, - flags | GFP_THISNODE, nid); + gfp_exact_node(flags), nid); if (obj) break; } @@ -3047,7 +3061,7 @@ retry: nid = page_to_nid(page); if (cache_grow(cache, flags, nid, page)) { obj = ____cache_alloc_node(cache, - flags | GFP_THISNODE, nid); + gfp_exact_node(flags), nid); if (!obj) /* * Another processor may allocate the @@ -3118,7 +3132,7 @@ retry: must_grow: spin_unlock(&n->list_lock); - x = cache_grow(cachep, flags | GFP_THISNODE, nodeid, NULL); + x = cache_grow(cachep, gfp_exact_node(flags), nodeid, NULL); if (x) goto retry; diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c --- a/net/openvswitch/flow.c +++ b/net/openvswitch/flow.c @@ -100,7 +100,9 @@ void ovs_flow_stats_update(struct sw_flow *flow, __be16 tcp_flags, new_stats = kmem_cache_alloc_node(flow_stats_cache, - GFP_THISNODE | + GFP_NOWAIT | + __GFP_THISNODE | + __GFP_NOWARN | __GFP_NOMEMALLOC, node); if (likely(new_stats)) {
NOTE: this is not about __GFP_THISNODE, this is only about GFP_THISNODE. GFP_THISNODE is a secret combination of gfp bits that have different behavior than expected. It is a combination of __GFP_THISNODE, __GFP_NORETRY, and __GFP_NOWARN and is special-cased in the page allocator slowpath to fail without trying reclaim even though it may be used in combination with __GFP_WAIT. An example of the problem this creates: commit e97ca8e5b864 ("mm: fix GFP_THISNODE callers and clarify") fixed up many users of GFP_THISNODE that really just wanted __GFP_THISNODE. The problem doesn't end there, however, because even it was a no-op for alloc_misplaced_dst_page(), which also sets __GFP_NORETRY and __GFP_NOWARN, and migrate_misplaced_transhuge_page(), where __GFP_NORETRY and __GFP_NOWAIT is set in GFP_TRANSHUGE. Converting GFP_THISNODE to __GFP_THISNODE is a no-op in these cases since the page allocator special-cases __GFP_THISNODE && __GFP_NORETRY && __GFP_NOWARN. It's time to just remove GFP_THISNODE entirely. We leave __GFP_THISNODE to restrict an allocation to a local node, but remove GFP_THISNODE and its obscurity. Instead, we require that a caller clear __GFP_WAIT if it wants to avoid reclaim. This allows the aforementioned functions to actually reclaim as they should. It also enables any future callers that want to do __GFP_THISNODE but also __GFP_NORETRY && __GFP_NOWARN to reclaim. The rule is simple: if you don't want to reclaim, then don't set __GFP_WAIT. Aside: ovs_flow_stats_update() really wants to avoid reclaim as well, so it is unchanged. Signed-off-by: David Rientjes <rientjes@google.com> --- v2: fix typos in commit message per Vlastimil include/linux/gfp.h | 10 ---------- mm/page_alloc.c | 22 ++++++---------------- mm/slab.c | 22 ++++++++++++++++++---- net/openvswitch/flow.c | 4 +++- 4 files changed, 27 insertions(+), 31 deletions(-) -- 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