cpuset: mm: Reduce large amounts of memory barrier related damage v3
Subsystems (12)
Mel Gorman
reviewerPATCH 1/1 cpuset: mm: Reduce large amounts of memory barrier related damage v3
Changelog since V2
o Documentation (akpm)
o Do not retry hugetlb allocations in the event of an error (akpm)
o Properly initialise seqcount (akpm)
Changelog since V1
o Use seqcount with rmb instead of atomics (Peter, Christoph)
Commit [c0ff7453: cpuset,mm: fix no node to alloc memory when changing cpuset's mems] wins a super prize for the largest number of memory barriers entered into fast paths for one commit. [get|put]_mems_allowed is incredibly heavy with pairs of full memory barriers inserted into a number of hot paths. This was detected while investigating at large page allocator slowdown introduced some time after 2.6.32. The largest portion of this overhead was shown by oprofile to be at an mfence introduced by this commit into the page allocator hot path.
For extra style points, the commit introduced the use of yield() in an implementation of what looks like a spinning mutex.
This patch replaces the full memory barriers on both read and write sides with a sequence counter with just read barriers on the fast path side. This is much cheaper on some architectures, including x86. The main bulk of the patch is the retry logic if the nodemask changes in a manner that can cause a false failure.
While updating the nodemask, a check is made to see if a false failure is a risk. If it is, the sequence number gets bumped and parallel allocators will briefly stall while the nodemask update takes place.
In a page fault test microbenchmark, oprofile samples from __alloc_pages_nodemask went from 4.53% of all samples to 1.15%. The actual results were
3.3.0-rc3 3.3.0-rc3
rc3-vanilla nobarrier-v2r1
Clients 1 UserTime 0.07 ( 0.00%) 0.08 (-14.19%)
Clients 2 UserTime 0.07 ( 0.00%) 0.07 ( 2.72%)
Clients 4 UserTime 0.08 ( 0.00%) 0.07 ( 3.29%)
Clients 1 SysTime 0.70 ( 0.00%) 0.65 ( 6.65%)
Clients 2 SysTime 0.85 ( 0.00%) 0.82 ( 3.65%)
Clients 4 SysTime 1.41 ( 0.00%) 1.41 ( 0.32%)
Clients 1 WallTime 0.77 ( 0.00%) 0.74 ( 4.19%)
Clients 2 WallTime 0.47 ( 0.00%) 0.45 ( 3.73%)
Clients 4 WallTime 0.38 ( 0.00%) 0.37 ( 1.58%)
Clients 1 Flt/sec/cpu 497620.28 ( 0.00%) 520294.53 ( 4.56%)
Clients 2 Flt/sec/cpu 414639.05 ( 0.00%) 429882.01 ( 3.68%)
Clients 4 Flt/sec/cpu 257959.16 ( 0.00%) 258761.48 ( 0.31%)
Clients 1 Flt/sec 495161.39 ( 0.00%) 517292.87 ( 4.47%)
Clients 2 Flt/sec 820325.95 ( 0.00%) 850289.77 ( 3.65%)
Clients 4 Flt/sec 1020068.93 ( 0.00%) 1022674.06 ( 0.26%)
MMTests Statistics: duration
Sys Time Running Test (seconds) 135.68 132.17
User+Sys Time Running Test (seconds) 164.2 160.13
Total Elapsed Time (seconds) 123.46 120.87The overall improvement is small but the System CPU time is much improved and roughly in correlation to what oprofile reported (these performance figures are without profiling so skew is expected). The actual number of page faults is noticeably improved.
For benchmarks like kernel builds, the overall benefit is marginal but the system CPU time is slightly reduced.
To test the actual bug the commit fixed I opened two terminals. The first ran within a cpuset and continually ran a small program that faulted 100M of anonymous data. In a second window, the nodemask of the cpuset was continually randomised in a loop. Without the commit, the program would fail every so often (usually within 10 seconds) and obviously with the commit everything worked fine. With this patch applied, it also worked fine so the fix should be functionally equivalent.
--- include/linux/cpuset.h | 47 +++++++++++++++++++-------------------------- include/linux/init_task.h | 8 ++++++++ include/linux/sched.h | 2 +- kernel/cpuset.c | 43 ++++++++--------------------------------- kernel/fork.c | 1 + mm/filemap.c | 11 +++++++---- mm/hugetlb.c | 15 +++++++++++---- mm/mempolicy.c | 28 ++++++++++++++++++++------- mm/page_alloc.c | 33 +++++++++++++++++++++---------- mm/slab.c | 13 ++++++++----- mm/slub.c | 40 +++++++++++++++++++++++--------------- mm/vmscan.c | 2 -- 12 files changed, 133 insertions(+), 110 deletions(-)
include/linux/cpuset.h +20 -27
@@ -89,42 +89,33 @@ extern void rebuild_sched_domains(void);extern void cpuset_print_task_mems_allowed (struct task_struct *p);/* -* reading current mems_allowed and mempolicy in the fastpath must protected -* by get_mems_allowed() +* get_mems_allowed is required when making decisions involving mems_allowed +* such as during page allocation. mems_allowed can be updated in parallel +* and depending on the new value an operation can fail potentially causing +* process failure. A retry loop with get_mems_allowed and put_mems_allowed +* prevents these artificial failures. */ -static inline void get_mems_allowed (void ) +static inline unsigned int get_mems_allowed (void ) { - current->mems_allowed_change_disable++; - -/* -* ensure that reading mems_allowed and mempolicy happens after the -* update of ->mems_allowed_change_disable. -* -* the write-side task finds ->mems_allowed_change_disable is not 0, -* and knows the read-side task is reading mems_allowed or mempolicy, -* so it will clear old bits lazily. -*/ -smp_mb (); +return read_seqcount_begin (¤t->mems_allowed_seq); } -static inline void put_mems_allowed (void ) +/* +* If this returns false, the operation that took place after get_mems_allowed +* may have failed. It is up to the caller to retry the operation if +* appropriate. +*/ +static inline bool put_mems_allowed (unsigned int seq) { -/* -* ensure that reading mems_allowed and mempolicy before reducing -* mems_allowed_change_disable. -* -* the write-side task will know that the read-side task is still -* reading mems_allowed or mempolicy, don't clears old bits in the -* nodemask. -*/ -smp_mb (); - --ACCESS_ONCE (current->mems_allowed_change_disable); +return !read_seqcount_retry (¤t->mems_allowed_seq, seq); }static inline void set_mems_allowed (nodemask_t nodemask) {task_lock (current); +write_seqcount_begin (¤t->mems_allowed_seq); current->mems_allowed = nodemask; +write_seqcount_end (¤t->mems_allowed_seq);task_unlock (current); }
@@ -234,12 +225,14 @@ static inline void set_mems_allowed(nodemask_t nodemask) { } -static inline void get_mems_allowed (void ) +static inline unsigned int get_mems_allowed (void ) { +return 0 ; } -static inline void put_mems_allowed (void ) +static inline bool put_mems_allowed (unsigned int seq) { +return true; }#endif /* !CONFIG_CPUSETS */
include/linux/init_task.h +8 -0
@@ -29,6 +29,13 @@ extern struct fs_struct init_fs;#define INIT_GROUP_RWSEM (sig)#endif +#ifdef CONFIG_CPUSETS +#define INIT_CPUSET_SEQ \ + .mems_allowed_seq = SEQCNT_ZERO, +#else +#define INIT_CPUSET_SEQ +#endif +#define INIT_SIGNALS (sig) { \ .nr_threads = 1, \ .wait_chldexit = __WAIT_QUEUE_HEAD_INITIALIZER(sig.wait_chldexit),\
@@ -192,6 +199,7 @@ extern struct cred init_cred;INIT_FTRACE_GRAPH \ INIT_TRACE_RECURSION \INIT_TASK_RCU_PREEMPT (tsk) \ + INIT_CPUSET_SEQ \ }
include/linux/sched.h +1 -1
@@ -1498,7 +1498,7 @@ struct task_struct {#endif #ifdef CONFIG_CPUSETS nodemask_t mems_allowed;/* Protected by alloc_lock */ -int mems_allowed_change_disable; +seqcount_t mems_allowed_seq;/* Seqence no to catch updates */ int cpuset_mem_spread_rotor;int cpuset_slab_spread_rotor;#endif
kernel/cpuset.c +8 -35
60 lines not on this page · raw message
kernel/fork.c +1 -0
@@ -1195,6 +1195,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,#ifdef CONFIG_CPUSETS p->cpuset_mem_spread_rotor =NUMA_NO_NODE ; p->cpuset_slab_spread_rotor =NUMA_NO_NODE ; +seqcount_init (&p->mems_allowed_seq);#endif #ifdef CONFIG_TRACE_IRQFLAGS p->irq_events =0 ;
mm/filemap.c +7 -4
@@ -500,10 +500,13 @@ struct page *__page_cache_alloc(gfp_t gfp)struct page *page;if (cpuset_do_page_mem_spread ()) { -get_mems_allowed (); - n =cpuset_mem_spread_node (); - page =alloc_pages_exact_node (n, gfp,0 ); -put_mems_allowed (); +unsigned int cpuset_mems_cookie; +do { + cpuset_mems_cookie =get_mems_allowed (); + n =cpuset_mem_spread_node (); + page =alloc_pages_exact_node (n, gfp,0 ); + }while (!put_mems_allowed (cpuset_mems_cookie) && !page); +return page; }return alloc_pages (gfp,0 );
mm/hugetlb.c +11 -4
@@ -454,14 +454,16 @@ static struct page *dequeue_huge_page_vma(struct hstate *h,struct vm_area_struct *vma, unsigned long address, int avoid_reserve) { - struct page *page = NULL; + struct page *page;struct mempolicy *mpol;nodemask_t *nodemask;struct zonelist *zonelist;struct zone *zone;struct zoneref *z; +unsigned int cpuset_mems_cookie; -get_mems_allowed (); +retry_cpuset: + cpuset_mems_cookie =get_mems_allowed (); zonelist =huge_zonelist (vma, address, htlb_alloc_mask, &mpol, &nodemask);/*
@@ -488,10 +490,15 @@ static struct page *dequeue_huge_page_vma(struct hstate *h, } } } -err: +mpol_cond_put (mpol); -put_mems_allowed (); +if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) + goto retry_cpuset;return page; + +err: +mpol_cond_put (mpol); +return NULL ; }static void update_and_free_page (struct hstate *h,struct page *page)
mm/mempolicy.c +21 -7
@@ -1843,18 +1843,24 @@ struct page * alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma, unsigned long addr, int node) { - struct mempolicy *pol = get_vma_policy(current, vma, addr); + struct mempolicy *pol;struct zonelist *zl;struct page *page; +unsigned int cpuset_mems_cookie; + +retry_cpuset: + pol =get_vma_policy (current, vma, addr); + cpuset_mems_cookie =get_mems_allowed (); -get_mems_allowed ();if (unlikely (pol->mode ==MPOL_INTERLEAVE )) {unsigned nid; nid =interleave_nid (pol, vma, addr,PAGE_SHIFT + order);mpol_cond_put (pol); page =alloc_page_interleave (gfp, order, nid); -put_mems_allowed (); +if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) + goto retry_cpuset; +return page; } zl =policy_zonelist (gfp, pol, node);
@@ -1865,7 +1871,8 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma,struct page *page =__alloc_pages_nodemask (gfp, order, zl,policy_nodemask (gfp, pol));__mpol_put (pol); -put_mems_allowed (); +if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) + goto retry_cpuset;return page; }/*
@@ -1873,7 +1880,8 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma,*/ page =__alloc_pages_nodemask (gfp, order, zl,policy_nodemask (gfp, pol)); -put_mems_allowed (); +if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) + goto retry_cpuset;return page; }
@@ -1900,11 +1908,14 @@ struct page *alloc_pages_current(gfp_t gfp, unsigned order) {struct mempolicy *pol = current->mempolicy;struct page *page; +unsigned int cpuset_mems_cookie;if (!pol ||in_interrupt () || (gfp & __GFP_THISNODE)) pol = &default_policy; -get_mems_allowed (); +retry_cpuset: + cpuset_mems_cookie =get_mems_allowed (); +/* * No reference counting needed for current->mempolicy * nor system default_policy
@@ -1915,7 +1926,10 @@ struct page *alloc_pages_current(gfp_t gfp, unsigned order) page =__alloc_pages_nodemask (gfp, order,policy_zonelist (gfp, pol,numa_node_id ()),policy_nodemask (gfp, pol)); -put_mems_allowed (); + +if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) + goto retry_cpuset; +return page; }EXPORT_SYMBOL (alloc_pages_current);
mm/page_alloc.c +23 -10
@@ -2378,8 +2378,9 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order, {enum zone_type high_zoneidx =gfp_zone (gfp_mask);struct zone *preferred_zone; -struct page *page; +struct page *page =NULL ;int migratetype =allocflags_to_migratetype (gfp_mask); +unsigned int cpuset_mems_cookie; gfp_mask &= gfp_allowed_mask;
@@ -2398,15 +2399,15 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,if (unlikely (!zonelist->_zonerefs->zone))return NULL ; -get_mems_allowed (); +retry_cpuset: + cpuset_mems_cookie =get_mems_allowed (); +/* The preferred zone is used for statistics later */ first_zones_zonelist (zonelist, high_zoneidx, nodemask ? : &cpuset_current_mems_allowed, &preferred_zone); -if (!preferred_zone) { -put_mems_allowed (); -return NULL ; - } +if (!preferred_zone) + goto out;/* First allocation attempt */ page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, nodemask, order,
@@ -2416,9 +2417,19 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order, page =__alloc_pages_slowpath (gfp_mask, order, zonelist, high_zoneidx, nodemask, preferred_zone, migratetype); -put_mems_allowed ();trace_mm_page_alloc (page, order, gfp_mask, migratetype); + +out: +/* +* When updating a task's mems_allowed, it is possible to race with +* parallel threads in such a way that an allocation can fail while +* the mask is being updated. If a page allocation is about to fail, +* check if the cpuset changed during allocation and if so, retry. +*/ +if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) + goto retry_cpuset; +return page; }EXPORT_SYMBOL (__alloc_pages_nodemask);
@@ -2632,13 +2643,15 @@ void si_meminfo_node(struct sysinfo *val, int nid)bool skip_free_areas_node (unsigned int flags,int nid) {bool ret = false; +unsigned int cpuset_mems_cookie;if (!(flags &SHOW_MEM_FILTER_NODES )) goto out; -get_mems_allowed (); - ret = !node_isset (nid, cpuset_current_mems_allowed); -put_mems_allowed (); +do { + cpuset_mems_cookie =get_mems_allowed (); + ret = !node_isset (nid, cpuset_current_mems_allowed); + }while (!put_mems_allowed (cpuset_mems_cookie)); out:return ret; }
mm/slab.c +8 -5
@@ -3284,12 +3284,10 @@ static void *alternate_node_alloc(struct kmem_cache *cachep, gfp_t flags)if (in_interrupt () || (flags & __GFP_THISNODE))return NULL ; nid_alloc = nid_here =numa_mem_id (); -get_mems_allowed ();if (cpuset_do_slab_mem_spread () && (cachep->flags &SLAB_MEM_SPREAD )) nid_alloc =cpuset_slab_spread_node ();else if (current->mempolicy) nid_alloc =slab_node (current->mempolicy); -put_mems_allowed ();if (nid_alloc != nid_here)return ____cache_alloc_node (cachep, flags, nid_alloc);return NULL ;
@@ -3312,14 +3310,17 @@ static void *fallback_alloc(struct kmem_cache *cache, gfp_t flags)enum zone_type high_zoneidx =gfp_zone (flags);void *obj =NULL ;int nid; +unsigned int cpuset_mems_cookie;if (flags & __GFP_THISNODE)return NULL ; -get_mems_allowed (); - zonelist =node_zonelist (slab_node (current->mempolicy), flags); local_flags = flags & (GFP_CONSTRAINT_MASK |GFP_RECLAIM_MASK ); +retry_cpuset: + cpuset_mems_cookie =get_mems_allowed (); + zonelist =node_zonelist (slab_node (current->mempolicy), flags); + retry:/* * Look through allowed nodes for objects available
@@ -3372,7 +3373,9 @@ retry: } } } -put_mems_allowed (); + +if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !obj)) + goto retry_cpuset;return obj; }
mm/slub.c +25 -15
@@ -1581,6 +1581,7 @@ static struct page *get_any_partial(struct kmem_cache *s, gfp_t flags,struct zone *zone;enum zone_type high_zoneidx =gfp_zone (flags);void *object; +unsigned int cpuset_mems_cookie;/* * The defrag ratio allows a configuration of the tradeoffs between
@@ -1604,23 +1605,32 @@ static struct page *get_any_partial(struct kmem_cache *s, gfp_t flags, get_cycles() % 1024 > s->remote_node_defrag_ratio)return NULL ; -get_mems_allowed (); - zonelist =node_zonelist (slab_node (current->mempolicy), flags); -for_each_zone_zonelist (zone, z, zonelist, high_zoneidx) { -struct kmem_cache_node *n; - - n =get_node (s,zone_to_nid (zone)); - -if (n &&cpuset_zone_allowed_hardwall (zone, flags) && - n->nr_partial > s->min_partial) { - object =get_partial_node (s, n, c); -if (object) { -put_mems_allowed (); -return object; +do { + cpuset_mems_cookie =get_mems_allowed (); + zonelist =node_zonelist (slab_node (current->mempolicy), flags); +for_each_zone_zonelist (zone, z, zonelist, high_zoneidx) { +struct kmem_cache_node *n; + + n =get_node (s,zone_to_nid (zone)); + +if (n &&cpuset_zone_allowed_hardwall (zone, flags) && + n->nr_partial > s->min_partial) { + object =get_partial_node (s, n, c); +if (object) { +/* +* Return the object even if +* put_mems_allowed indicated that +* the cpuset mems_allowed was +* updated in parallel. It's a +* harmless race between the alloc +* and the cpuset update. +*/ +put_mems_allowed (cpuset_mems_cookie); +return object; + } } } - } -put_mems_allowed (); + }while (!put_mems_allowed (cpuset_mems_cookie));#endif return NULL ; }
mm/vmscan.c +0 -2
@@ -2337,7 +2337,6 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,unsigned long writeback_threshold;bool aborted_reclaim; -get_mems_allowed ();delayacct_freepages_start ();if (global_reclaim (sc))
@@ -2401,7 +2400,6 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist, out:delayacct_freepages_end (); -put_mems_allowed ();if (sc->nr_reclaimed)return sc->nr_reclaimed;
Signature · 9 lines
Mel Gorman SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 1 line · Mel Gorman
+ } while (!put_mems_allowed(cpuset_mems_cookie) && !page);
Sorry for only noticing this now, but wouldn't it be better to first check page and only then bother with the put_mems_allowed() thing? That avoids the smp_rmb() and seqcount conditional all together in the likely case the allocation actually succeeded.
---
Subject: mm: Optimize put_mems_allowed() usage
Avoid calling put_mems_allowed() in case the page allocation succeeded.
--- include/linux/cpuset.h | 2 +- mm/filemap.c | 2 +- mm/hugetlb.c | 2 +- mm/mempolicy.c | 6 +++--- mm/page_alloc.c | 2 +- mm/slab.c | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-)
include/linux/cpuset.h +1 -1
@@ -107,7 +107,7 @@ static inline unsigned int get_mems_allowed(void)*/ static inline bool put_mems_allowed (unsigned int seq) { -return !read_seqcount_retry (¤t->mems_allowed_seq, seq); +return likely (!read_seqcount_retry (¤t->mems_allowed_seq, seq)); }static inline void set_mems_allowed (nodemask_t nodemask)
mm/filemap.c +1 -1
@@ -504,7 +504,7 @@ struct page *__page_cache_alloc(gfp_t gfp) cpuset_mems_cookie =get_mems_allowed (); n =cpuset_mem_spread_node (); page =alloc_pages_exact_node (n, gfp,0 ); - }while (!put_mems_allowed (cpuset_mems_cookie) && !page); + }while (!page && !put_mems_allowed (cpuset_mems_cookie));return page; }
mm/hugetlb.c +1 -1
@@ -570,7 +570,7 @@ static struct page *dequeue_huge_page_vma(struct hstate *h, }mpol_cond_put (mpol); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page && !put_mems_allowed (cpuset_mems_cookie))) goto retry_cpuset;return page;
mm/mempolicy.c +3 -3
@@ -1865,7 +1865,7 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma, nid =interleave_nid (pol, vma, addr,PAGE_SHIFT + order);mpol_cond_put (pol); page =alloc_page_interleave (gfp, order, nid); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page && !put_mems_allowed (cpuset_mems_cookie))) goto retry_cpuset;return page;
@@ -1878,7 +1878,7 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma,struct page *page =__alloc_pages_nodemask (gfp, order, zl,policy_nodemask (gfp, pol));__mpol_put (pol); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page && !put_mems_allowed (cpuset_mems_cookie))) goto retry_cpuset;return page; }
@@ -1887,7 +1887,7 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma,*/ page =__alloc_pages_nodemask (gfp, order, zl,policy_nodemask (gfp, pol)); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page && !put_mems_allowed (cpuset_mems_cookie))) goto retry_cpuset;return page; }
mm/page_alloc.c +1 -1
@@ -2429,7 +2429,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,* the mask is being updated. If a page allocation is about to fail, * check if the cpuset changed during allocation and if so, retry. */ -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page && !put_mems_allowed (cpuset_mems_cookie))) goto retry_cpuset;return page;
mm/slab.c +1 -1
@@ -3374,7 +3374,7 @@ static void *fallback_alloc(struct kmem_cache *cache, gfp_t flags) } } -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !obj)) +if (unlikely (!obj && !put_mems_allowed (cpuset_mems_cookie))) goto retry_cpuset;return obj; }
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Peter Zijlstra
maintainerinclude/linux/cpuset.h static inline unsigned int get_mems_allowed(void) · line 111
Quoted 5 lines · Peter Zijlstra
static inline bool
put_mems_allowed(unsigned int seq){include/linux/cpuset.h +1
-1-return !read_seqcount_retry (¤t->mems_allowed_seq, seq); +return likely (!read_seqcount_retry (¤t->mems_allowed_seq, seq)); }
Ignore this hunk, read_seqcount_retry() already has a branch hint in. I'll send a new version if people thing the rest of the patch is worth it.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Mel Gorman
reviewerQuoted 15 lines · Peter Zijlstra
Quoted 1 line · Mel Gorman
+ } while (!put_mems_allowed(cpuset_mems_cookie) && !page);
Sorry for only noticing this now, but wouldn't it be better to first check page and only then bother with the
put_mems_allowed()thing? That avoids thesmp_rmb()and seqcount conditional all together in the likely case the allocation actually succeeded.<SNIP>
mm/filemap.c +0
-0
mm/filemap.c struct page *__page_cache_alloc(gfp_t gfp) · line 510
Quoted 8 lines
mm/filemap.c +1
-1@@ -504,7 +504,7 @@ struct page *__page_cache_alloc(gfp_t gfp) cpuset_mems_cookie =get_mems_allowed (); n =cpuset_mem_spread_node (); page =alloc_pages_exact_node (n, gfp,0 ); - }while (!put_mems_allowed (cpuset_mems_cookie) && !page); + }while (!page && !put_mems_allowed (cpuset_mems_cookie));return page; }
I think such a change would be better but should also rename the API. If developers see a get_foo type call, they will expect to see a put_foo call or assume it's a bug even though the implementation happens to be ok with that. Any suggestion on what a good new name would be?
How about read_mems_allowed_begin() and read_mems_allowed_retry()?
read_mems_allowed_begin would be a rename of get_mems_allowed(). In an error path, read_mems_allowed_retry() would documented to be *optionally* called when deciding whether to retry the operation or not. In this scheme, !put_mems_allowed would become read_mems_allowed_retry() which might be a bit easier to read overall.
Signature · 9 lines
Mel Gorman SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 30 lines · Mel Gorman
Quoted 15 lines · Peter Zijlstra
Quoted 1 line · Mel Gorman
+ } while (!put_mems_allowed(cpuset_mems_cookie) && !page);
Sorry for only noticing this now, but wouldn't it be better to first check page and only then bother with the
put_mems_allowed()thing? That avoids thesmp_rmb()and seqcount conditional all together in the likely case the allocation actually succeeded.<SNIP>
mm/filemap.c +0
-0mm/filemap.c struct page *__page_cache_alloc(gfp_t gfp) · line 510
Quoted 8 lines
mm/filemap.c +1
-1@@ -504,7 +504,7 @@ struct page *__page_cache_alloc(gfp_t gfp) cpuset_mems_cookie =get_mems_allowed (); n =cpuset_mem_spread_node (); page =alloc_pages_exact_node (n, gfp,0 ); - }while (!put_mems_allowed (cpuset_mems_cookie) && !page); + }while (!page && !put_mems_allowed (cpuset_mems_cookie));return page; }I think such a change would be better but should also rename the API. If developers see a
get_footype call, they will expect to see aput_foocall or assume it's a bug even though the implementation happens to be ok with that. Any suggestion on what a good new name would be?
Quoted 7 lines
How about
read_mems_allowed_begin()andread_mems_allowed_retry()?
read_mems_allowed_beginwould be a rename ofget_mems_allowed(). In an error path,read_mems_allowed_retry()would documented to be *optionally* called when deciding whether to retry the operation or not. In this scheme, !put_mems_allowed would becomeread_mems_allowed_retry()which might be a bit easier to read overall.
One:
git grep -l "\(get\|put\)_mems_allowed" | while read file; do sed -i -e 's/\<get_mems_allowed\>/read_mems_allowed_begin/g' -e 's/\<put_mems_allowed\>/read_mems_allowed_retry/g' $file; done
and a few edits later..
--- include/linux/cpuset.h | 18 +++++++++--------- kernel/cpuset.c | 2 +- mm/filemap.c | 4 ++-- mm/hugetlb.c | 4 ++-- mm/mempolicy.c | 14 +++++++------- mm/page_alloc.c | 8 ++++---- mm/slab.c | 4 ++-- mm/slub.c | 16 +++------------- 8 files changed, 30 insertions(+), 40 deletions(-)
include/linux/cpuset.h +9 -9
@@ -89,25 +89,25 @@ extern void rebuild_sched_domains(void);extern void cpuset_print_task_mems_allowed (struct task_struct *p);/* -* get_mems_allowed is required when making decisions involving mems_allowed +* read_mems_allowed_begin is required when making decisions involving mems_allowed * such as during page allocation. mems_allowed can be updated in parallel * and depending on the new value an operation can fail potentially causing -* process failure. A retry loop with get_mems_allowed and put_mems_allowed +* process failure. A retry loop with read_mems_allowed_begin and read_mems_allowed_retry * prevents these artificial failures. */ -static inline unsigned int get_mems_allowed (void ) +static inline unsigned int read_mems_allowed_begin (void ) {return read_seqcount_begin (¤t->mems_allowed_seq); }/* -* If this returns false, the operation that took place after get_mems_allowed +* If this returns false, the operation that took place after read_mems_allowed_begin * may have failed. It is up to the caller to retry the operation if * appropriate. */ -static inline bool put_mems_allowed (unsigned int seq) +static inline bool read_mems_allowed_retry (unsigned int seq) { -return !read_seqcount_retry (¤t->mems_allowed_seq, seq); +return read_seqcount_retry (¤t->mems_allowed_seq, seq); }static inline void set_mems_allowed (nodemask_t nodemask)
@@ -225,14 +225,14 @@ static inline void set_mems_allowed(nodemask_t nodemask) { } -static inline unsigned int get_mems_allowed (void ) +static inline unsigned int read_mems_allowed_begin (void ) {return 0 ; } -static inline bool put_mems_allowed (unsigned int seq) +static inline bool read_mems_allowed_retry (unsigned int seq) { -return true; +return false; }#endif /* !CONFIG_CPUSETS */
kernel/cpuset.c +1 -1
@@ -976,7 +976,7 @@ static void cpuset_change_task_nodemask(struct task_struct *tsk,task_lock (tsk);/* * Determine if a loop is necessary if another thread is doing -* get_mems_allowed(). If at least one node remains unchanged and +* read_mems_allowed_begin(). If at least one node remains unchanged and * tsk does not have a mempolicy, then an empty nodemask will not be * possible when mems_allowed is larger than a word. */
mm/filemap.c +2 -2
@@ -501,10 +501,10 @@ struct page *__page_cache_alloc(gfp_t gfp)if (cpuset_do_page_mem_spread ()) {unsigned int cpuset_mems_cookie;do { - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin (); n =cpuset_mem_spread_node (); page =alloc_pages_exact_node (n, gfp,0 ); - }while (!put_mems_allowed (cpuset_mems_cookie) && !page); + }while (!page &&read_mems_allowed_retry (cpuset_mems_cookie));return page; }
mm/hugetlb.c +2 -2
@@ -541,7 +541,7 @@ static struct page *dequeue_huge_page_vma(struct hstate *h,unsigned int cpuset_mems_cookie; retry_cpuset: - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin (); zonelist =huge_zonelist (vma, address, htlb_alloc_mask, &mpol, &nodemask);/*
@@ -570,7 +570,7 @@ static struct page *dequeue_huge_page_vma(struct hstate *h, }mpol_cond_put (mpol); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page;
mm/mempolicy.c +7 -7
@@ -1692,7 +1692,7 @@ int node_random(const nodemask_t *maskp)* If the effective policy is 'BIND, returns a pointer to the mempolicy's * @nodemask for filtering the zonelist. * -* Must be protected by get_mems_allowed() +* Must be protected by read_mems_allowed_begin() */ struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr, gfp_t gfp_flags, struct mempolicy **mpol,
@@ -1857,7 +1857,7 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma, retry_cpuset: pol =get_vma_policy (current, vma, addr); - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin ();if (unlikely (pol->mode ==MPOL_INTERLEAVE )) {unsigned nid;
@@ -1865,7 +1865,7 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma, nid =interleave_nid (pol, vma, addr,PAGE_SHIFT + order);mpol_cond_put (pol); page =alloc_page_interleave (gfp, order, nid); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page;
@@ -1878,7 +1878,7 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma,struct page *page =__alloc_pages_nodemask (gfp, order, zl,policy_nodemask (gfp, pol));__mpol_put (pol); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page; }
@@ -1887,7 +1887,7 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma,*/ page =__alloc_pages_nodemask (gfp, order, zl,policy_nodemask (gfp, pol)); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page; }
@@ -1921,7 +1921,7 @@ struct page *alloc_pages_current(gfp_t gfp, unsigned order) pol = &default_policy; retry_cpuset: - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin ();/* * No reference counting needed for current->mempolicy
@@ -1934,7 +1934,7 @@ struct page *alloc_pages_current(gfp_t gfp, unsigned order)policy_zonelist (gfp, pol,numa_node_id ()), policy_nodemask(gfp, pol)); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page;
mm/page_alloc.c +4 -4
@@ -2402,7 +2402,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,return NULL ; retry_cpuset: - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin ();/* The preferred zone is used for statistics later */ first_zones_zonelist(zonelist, high_zoneidx,
@@ -2429,7 +2429,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,* the mask is being updated. If a page allocation is about to fail, * check if the cpuset changed during allocation and if so, retry. */ -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page;
@@ -2651,9 +2651,9 @@ bool skip_free_areas_node(unsigned int flags, int nid) goto out;do { - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin (); ret = !node_isset (nid, cpuset_current_mems_allowed); - }while (!put_mems_allowed (cpuset_mems_cookie)); + }while (read_mems_allowed_retry (cpuset_mems_cookie)); out:return ret; }
mm/slab.c +2 -2
@@ -3318,7 +3318,7 @@ static void *fallback_alloc(struct kmem_cache *cache, gfp_t flags) local_flags = flags & (GFP_CONSTRAINT_MASK |GFP_RECLAIM_MASK ); retry_cpuset: - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin (); zonelist =node_zonelist (slab_node (current->mempolicy), flags); retry:
@@ -3374,7 +3374,7 @@ static void *fallback_alloc(struct kmem_cache *cache, gfp_t flags) } } -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !obj)) +if (unlikely (!obj &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return obj; }
mm/slub.c +3 -13
@@ -1606,7 +1606,7 @@ static struct page *get_any_partial(struct kmem_cache *s, gfp_t flags,return NULL ; do { - cpuset_mems_cookie = get_mems_allowed(); + cpuset_mems_cookie = read_mems_allowed_begin(); zonelist = node_zonelist(slab_node(current->mempolicy), flags); for_each_zone_zonelist(zone, z, zonelist, high_zoneidx) { struct kmem_cache_node *n;
@@ -1616,21 +1616,11 @@ static struct page *get_any_partial(struct kmem_cache *s, gfp_t flags,if (n &&cpuset_zone_allowed_hardwall (zone, flags) && n->nr_partial > s->min_partial) { object =get_partial_node (s, n, c); -if (object) { -/* -* Return the object even if -* put_mems_allowed indicated that -* the cpuset mems_allowed was -* updated in parallel. It's a -* harmless race between the alloc -* and the cpuset update. -*/ -put_mems_allowed (cpuset_mems_cookie); +if (object)return object; - } } } - }while (!put_mems_allowed (cpuset_mems_cookie)); + }while (read_mems_allowed_retry (cpuset_mems_cookie));#endif return NULL ; }
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 40 lines · Peter Zijlstra
Quoted 7 lines
<SNIP>
I think such a change would be better but should also rename the API. If developers see a
get_footype call, they will expect to see aput_foocall or assume it's a bug even though the implementation happens to be ok with that. Any suggestion on what a good new name would be?Quoted 7 lines
How about
read_mems_allowed_begin()andread_mems_allowed_retry()?
read_mems_allowed_beginwould be a rename ofget_mems_allowed(). In an error path,read_mems_allowed_retry()would documented to be *optionally* called when deciding whether to retry the operation or not. In this scheme, !put_mems_allowed would becomeread_mems_allowed_retry()which might be a bit easier to read overall.One:
git grep -l "\(get\|put\)_mems_allowed" | while read file; do sed -i -e 's/\<get_mems_allowed\>/read_mems_allowed_begin/g' -e 's/\<put_mems_allowed\>/read_mems_allowed_retry/g' $file; done
and a few edits later..
--- include/linux/cpuset.h | 18 +++++++++--------- kernel/cpuset.c | 2 +- mm/filemap.c | 4 ++-- mm/hugetlb.c | 4 ++-- mm/mempolicy.c | 14 +++++++------- mm/page_alloc.c | 8 ++++---- mm/slab.c | 4 ++-- mm/slub.c | 16 +++------------- 8 files changed, 30 insertions(+), 40 deletions(-)include/linux/cpuset.h +0
-0@@ -89,25 +89,25 @@ extern void rebuild_sched_domains(void);extern void cpuset_print_task_mems_allowed (struct task_struct *p);
include/linux/cpuset.h extern void rebuild_sched_domains(void); · line 97
Quoted 10 lines
include/linux/cpuset.h +2
-2/* -* get_mems_allowed is required when making decisions involving mems_allowed +* read_mems_allowed_begin is required when making decisions involving mems_allowed * such as during page allocation. mems_allowed can be updated in parallel * and depending on the new value an operation can fail potentially causing -* process failure. A retry loop with get_mems_allowed and put_mems_allowed +* process failure. A retry loop with read_mems_allowed_begin and read_mems_allowed_retry * prevents these artificial failures. */
Going over 80 columns there. This happens in other places in the patch but the alternative in those cases is less readable.
include/linux/cpuset.h extern void rebuild_sched_domains(void); · line 107
Quoted 12 lines
include/linux/cpuset.h +2
-2-static inline unsigned int get_mems_allowed (void ) +static inline unsigned int read_mems_allowed_begin (void ) {return read_seqcount_begin (¤t->mems_allowed_seq); }/* -* If this returns false, the operation that took place after get_mems_allowed +* If this returns false, the operation that took place after read_mems_allowed_begin * may have failed. It is up to the caller to retry the operation if * appropriate. */
80 cols again and it should be "returns true". Something like this?
/*
* If this returns true, the operation that took place after
* read_mems_allowed_begin may have failed artifically due to a paralle
* update of mems_allowed. It is up to the caller to retry the operation
* if appropriate.
*/
Other than that, the changes looked good and I agree that it is better overall.
Signature · 9 lines
Mel Gorman SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
PATCH 1/1 mm: Optimize put_mems_allowed() usage
Subject: mm: Optimize put_mems_allowed() usage
From: Peter Zijlstra <[email protected]>
Date: Mon Mar 26 14:13:05 CEST 2012
Since put_mems_allowed() is strictly optional, its a seqcount retry, we don't need to evaluate the function if the allocation was in fact successful, saving a smp_rmb some loads and comparisons on some relative fast-paths.
Since the naming, get/put_mems_allowed() does suggest a mandatory pairing, rename the interface, as suggested by Mel, to resemble the seqcount interface.
This gives us: read_mems_allowed_begin() and read_mems_allowed_retry(), where it is important to note that the return value of the latter call is inverted from its previous incarnation.
--- include/linux/cpuset.h | 27 ++++++++++++++------------- kernel/cpuset.c | 2 +- mm/filemap.c | 4 ++-- mm/hugetlb.c | 4 ++-- mm/mempolicy.c | 14 +++++++------- mm/page_alloc.c | 8 ++++---- mm/slab.c | 4 ++-- mm/slub.c | 16 +++------------- 8 files changed, 35 insertions(+), 44 deletions(-)
include/linux/cpuset.h +14 -13
@@ -89,25 +89,26 @@ extern void rebuild_sched_domains(void);extern void cpuset_print_task_mems_allowed (struct task_struct *p);/* -* get_mems_allowed is required when making decisions involving mems_allowed -* such as during page allocation. mems_allowed can be updated in parallel -* and depending on the new value an operation can fail potentially causing -* process failure. A retry loop with get_mems_allowed and put_mems_allowed -* prevents these artificial failures. +* read_mems_allowed_begin is required when making decisions involving +* mems_allowed such as during page allocation. mems_allowed can be updated in +* parallel and depending on the new value an operation can fail potentially +* causing process failure. A retry loop with read_mems_allowed_begin and +* read_mems_allowed_retry prevents these artificial failures. */ -static inline unsigned int get_mems_allowed (void ) +static inline unsigned int read_mems_allowed_begin (void ) {return read_seqcount_begin (¤t->mems_allowed_seq); }/* -* If this returns false, the operation that took place after get_mems_allowed -* may have failed. It is up to the caller to retry the operation if +* If this returns true, the operation that took place after +* read_mems_allowed_begin may have failed artificially due to a concurrent +* update of mems_allowed. It is up to the caller to retry the operation if * appropriate. */ -static inline bool put_mems_allowed (unsigned int seq) +static inline bool read_mems_allowed_retry (unsigned int seq) { -return !read_seqcount_retry (¤t->mems_allowed_seq, seq); +return read_seqcount_retry (¤t->mems_allowed_seq, seq); }static inline void set_mems_allowed (nodemask_t nodemask)
@@ -225,14 +226,14 @@ static inline void set_mems_allowed(node { } -static inline unsigned int get_mems_allowed (void ) +static inline unsigned int read_mems_allowed_begin (void ) {return 0 ; } -static inline bool put_mems_allowed (unsigned int seq) +static inline bool read_mems_allowed_retry (unsigned int seq) { -return true; +return false; }#endif /* !CONFIG_CPUSETS */
kernel/cpuset.c +1 -1
@@ -976,7 +976,7 @@ static void cpuset_change_task_nodemask(task_lock (tsk);/* * Determine if a loop is necessary if another thread is doing -* get_mems_allowed(). If at least one node remains unchanged and +* read_mems_allowed_begin(). If at least one node remains unchanged and * tsk does not have a mempolicy, then an empty nodemask will not be * possible when mems_allowed is larger than a word. */
mm/filemap.c +2 -2
@@ -501,10 +501,10 @@ struct page *__page_cache_alloc(gfp_t gfif (cpuset_do_page_mem_spread ()) {unsigned int cpuset_mems_cookie;do { - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin (); n =cpuset_mem_spread_node (); page =alloc_pages_exact_node (n, gfp,0 ); - }while (!put_mems_allowed (cpuset_mems_cookie) && !page); + }while (!page &&read_mems_allowed_retry (cpuset_mems_cookie));return page; }
mm/hugetlb.c +2 -2
@@ -541,7 +541,7 @@ static struct page *dequeue_huge_page_vmunsigned int cpuset_mems_cookie; retry_cpuset: - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin (); zonelist =huge_zonelist (vma, address, htlb_alloc_mask, &mpol, &nodemask);/*
@@ -570,7 +570,7 @@ static struct page *dequeue_huge_page_vm }mpol_cond_put (mpol); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page;
mm/mempolicy.c +7 -7
@@ -1692,7 +1692,7 @@ int node_random(const nodemask_t *maskp)* If the effective policy is 'BIND, returns a pointer to the mempolicy's * @nodemask for filtering the zonelist. * -* Must be protected by get_mems_allowed() +* Must be protected by read_mems_allowed_begin() */ struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr, gfp_t gfp_flags, struct mempolicy **mpol,
@@ -1857,7 +1857,7 @@ alloc_pages_vma(gfp_t gfp, int order, st retry_cpuset: pol =get_vma_policy (current, vma, addr); - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin ();if (unlikely (pol->mode ==MPOL_INTERLEAVE )) {unsigned nid;
@@ -1865,7 +1865,7 @@ alloc_pages_vma(gfp_t gfp, int order, st nid =interleave_nid (pol, vma, addr,PAGE_SHIFT + order);mpol_cond_put (pol); page =alloc_page_interleave (gfp, order, nid); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page;
@@ -1878,7 +1878,7 @@ alloc_pages_vma(gfp_t gfp, int order, ststruct page *page =__alloc_pages_nodemask (gfp, order, zl,policy_nodemask (gfp, pol));__mpol_put (pol); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page; }
@@ -1887,7 +1887,7 @@ alloc_pages_vma(gfp_t gfp, int order, st*/ page =__alloc_pages_nodemask (gfp, order, zl,policy_nodemask (gfp, pol)); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page; }
@@ -1921,7 +1921,7 @@ struct page *alloc_pages_current(gfp_t g pol = &default_policy; retry_cpuset: - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin ();/* * No reference counting needed for current->mempolicy
@@ -1934,7 +1934,7 @@ struct page *alloc_pages_current(gfp_t gpolicy_zonelist (gfp, pol,numa_node_id ()), policy_nodemask(gfp, pol)); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page;
mm/page_alloc.c +4 -4
@@ -2402,7 +2402,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, ureturn NULL ; retry_cpuset: - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin ();/* The preferred zone is used for statistics later */ first_zones_zonelist(zonelist, high_zoneidx,
@@ -2429,7 +2429,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, u* the mask is being updated. If a page allocation is about to fail, * check if the cpuset changed during allocation and if so, retry. */ -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page;
@@ -2651,9 +2651,9 @@ bool skip_free_areas_node(unsigned int f goto out;do { - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin (); ret = !node_isset (nid, cpuset_current_mems_allowed); - }while (!put_mems_allowed (cpuset_mems_cookie)); + }while (read_mems_allowed_retry (cpuset_mems_cookie)); out:return ret; }
mm/slab.c +2 -2
@@ -3318,7 +3318,7 @@ static void *fallback_alloc(struct kmem_ local_flags = flags & (GFP_CONSTRAINT_MASK |GFP_RECLAIM_MASK ); retry_cpuset: - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin (); zonelist =node_zonelist (slab_node (current->mempolicy), flags); retry:
@@ -3374,7 +3374,7 @@ static void *fallback_alloc(struct kmem_ } } -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !obj)) +if (unlikely (!obj &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return obj; }
mm/slub.c +3 -13
@@ -1606,7 +1606,7 @@ static struct page *get_any_partial(strureturn NULL ; do { - cpuset_mems_cookie = get_mems_allowed(); + cpuset_mems_cookie = read_mems_allowed_begin(); zonelist = node_zonelist(slab_node(current->mempolicy), flags); for_each_zone_zonelist(zone, z, zonelist, high_zoneidx) { struct kmem_cache_node *n;
@@ -1616,21 +1616,11 @@ static struct page *get_any_partial(struif (n &&cpuset_zone_allowed_hardwall (zone, flags) && n->nr_partial > s->min_partial) { object =get_partial_node (s, n, c); -if (object) { -/* -* Return the object even if -* put_mems_allowed indicated that -* the cpuset mems_allowed was -* updated in parallel. It's a -* harmless race between the alloc -* and the cpuset update. -*/ -put_mems_allowed (cpuset_mems_cookie); +if (object)return object; - } } } - }while (!put_mems_allowed (cpuset_mems_cookie)); + }while (read_mems_allowed_retry (cpuset_mems_cookie));#endif return NULL ; }
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Peter Zijlstra
maintainerRe: [PATCH] mm: Optimize put_mems_allowed() usage
Anybody care to pick this one up? Andrew?
Quoted 277 lines · Peter Zijlstra
Subject: mm: Optimize
put_mems_allowed()usage
From: Peter Zijlstra <[email protected]>
Date: Mon Mar 26 14:13:05 CEST 2012Since
put_mems_allowed()is strictly optional, its a seqcount retry, we don't need to evaluate the function if the allocation was in fact successful, saving asmp_rmbsome loads and comparisons on some relative fast-paths.Since the naming, get/put_mems_allowed() does suggest a mandatory pairing, rename the interface, as suggested by Mel, to resemble the seqcount interface.
This gives us:
read_mems_allowed_begin()andread_mems_allowed_retry(), where it is important to note that the return value of the latter call is inverted from its previous incarnation.Acked-by Mel Gorman <[email protected]>Signed-off-by Peter Zijlstra <[email protected]>--- include/linux/cpuset.h | 27 ++++++++++++++------------- kernel/cpuset.c | 2 +- mm/filemap.c | 4 ++-- mm/hugetlb.c | 4 ++-- mm/mempolicy.c | 14 +++++++------- mm/page_alloc.c | 8 ++++---- mm/slab.c | 4 ++-- mm/slub.c | 16 +++------------- 8 files changed, 35 insertions(+), 44 deletions(-)include/linux/cpuset.h +14
-13@@ -89,25 +89,26 @@ extern void rebuild_sched_domains(void);extern void cpuset_print_task_mems_allowed (struct task_struct *p);/* -* get_mems_allowed is required when making decisions involving mems_allowed -* such as during page allocation. mems_allowed can be updated in parallel -* and depending on the new value an operation can fail potentially causing -* process failure. A retry loop with get_mems_allowed and put_mems_allowed -* prevents these artificial failures. +* read_mems_allowed_begin is required when making decisions involving +* mems_allowed such as during page allocation. mems_allowed can be updated in +* parallel and depending on the new value an operation can fail potentially +* causing process failure. A retry loop with read_mems_allowed_begin and +* read_mems_allowed_retry prevents these artificial failures. */ -static inline unsigned int get_mems_allowed (void ) +static inline unsigned int read_mems_allowed_begin (void ) {return read_seqcount_begin (¤t->mems_allowed_seq); }/* -* If this returns false, the operation that took place after get_mems_allowed -* may have failed. It is up to the caller to retry the operation if +* If this returns true, the operation that took place after +* read_mems_allowed_begin may have failed artificially due to a concurrent +* update of mems_allowed. It is up to the caller to retry the operation if * appropriate. */ -static inline bool put_mems_allowed (unsigned int seq) +static inline bool read_mems_allowed_retry (unsigned int seq) { -return !read_seqcount_retry (¤t->mems_allowed_seq, seq); +return read_seqcount_retry (¤t->mems_allowed_seq, seq); }static inline void set_mems_allowed (nodemask_t nodemask)@@ -225,14 +226,14 @@ static inline void set_mems_allowed(node { } -static inline unsigned int get_mems_allowed (void ) +static inline unsigned int read_mems_allowed_begin (void ) {return 0 ; } -static inline bool put_mems_allowed (unsigned int seq) +static inline bool read_mems_allowed_retry (unsigned int seq) { -return true; +return false; }#endif /* !CONFIG_CPUSETS */ kernel/cpuset.c +1
-1@@ -976,7 +976,7 @@ static void cpuset_change_task_nodemask(task_lock (tsk);/* * Determine if a loop is necessary if another thread is doing -* get_mems_allowed(). If at least one node remains unchanged and +* read_mems_allowed_begin(). If at least one node remains unchanged and * tsk does not have a mempolicy, then an empty nodemask will not be * possible when mems_allowed is larger than a word. */ mm/filemap.c +2
-2@@ -501,10 +501,10 @@ struct page *__page_cache_alloc(gfp_t gfif (cpuset_do_page_mem_spread ()) {unsigned int cpuset_mems_cookie;do { - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin (); n =cpuset_mem_spread_node (); page =alloc_pages_exact_node (n, gfp,0 ); - }while (!put_mems_allowed (cpuset_mems_cookie) && !page); + }while (!page &&read_mems_allowed_retry (cpuset_mems_cookie));return page; }mm/hugetlb.c +2
-2@@ -541,7 +541,7 @@ static struct page *dequeue_huge_page_vmunsigned int cpuset_mems_cookie; retry_cpuset: - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin (); zonelist =huge_zonelist (vma, address, htlb_alloc_mask, &mpol, &nodemask);/* @@ -570,7 +570,7 @@ static struct page *dequeue_huge_page_vm }mpol_cond_put (mpol); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page;mm/mempolicy.c +7
-7@@ -1692,7 +1692,7 @@ int node_random(const nodemask_t *maskp)* If the effective policy is 'BIND, returns a pointer to the mempolicy's * @nodemask for filtering the zonelist. * -* Must be protected by get_mems_allowed() +* Must be protected by read_mems_allowed_begin() */ struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr, gfp_t gfp_flags, struct mempolicy **mpol,@@ -1857,7 +1857,7 @@ alloc_pages_vma(gfp_t gfp, int order, st retry_cpuset: pol =get_vma_policy (current, vma, addr); - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin ();if (unlikely (pol->mode ==MPOL_INTERLEAVE )) {unsigned nid;@@ -1865,7 +1865,7 @@ alloc_pages_vma(gfp_t gfp, int order, st nid =interleave_nid (pol, vma, addr,PAGE_SHIFT + order);mpol_cond_put (pol); page =alloc_page_interleave (gfp, order, nid); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page;@@ -1878,7 +1878,7 @@ alloc_pages_vma(gfp_t gfp, int order, ststruct page *page =__alloc_pages_nodemask (gfp, order, zl,policy_nodemask (gfp, pol));__mpol_put (pol); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page; }@@ -1887,7 +1887,7 @@ alloc_pages_vma(gfp_t gfp, int order, st*/ page =__alloc_pages_nodemask (gfp, order, zl,policy_nodemask (gfp, pol)); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page; }@@ -1921,7 +1921,7 @@ struct page *alloc_pages_current(gfp_t g pol = &default_policy; retry_cpuset: - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin ();/* * No reference counting needed for current->mempolicy @@ -1934,7 +1934,7 @@ struct page *alloc_pages_current(gfp_t gpolicy_zonelist (gfp, pol,numa_node_id ()), policy_nodemask(gfp, pol)); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page;mm/page_alloc.c +4
-4@@ -2402,7 +2402,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, ureturn NULL ; retry_cpuset: - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin ();/* The preferred zone is used for statistics later */ first_zones_zonelist(zonelist, high_zoneidx,@@ -2429,7 +2429,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, u* the mask is being updated. If a page allocation is about to fail, * check if the cpuset changed during allocation and if so, retry. */ -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page;@@ -2651,9 +2651,9 @@ bool skip_free_areas_node(unsigned int f goto out;do { - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin (); ret = !node_isset (nid, cpuset_current_mems_allowed); - }while (!put_mems_allowed (cpuset_mems_cookie)); + }while (read_mems_allowed_retry (cpuset_mems_cookie)); out:return ret; }mm/slab.c +2
-2@@ -3318,7 +3318,7 @@ static void *fallback_alloc(struct kmem_ local_flags = flags & (GFP_CONSTRAINT_MASK |GFP_RECLAIM_MASK ); retry_cpuset: - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin (); zonelist =node_zonelist (slab_node (current->mempolicy), flags); retry:@@ -3374,7 +3374,7 @@ static void *fallback_alloc(struct kmem_ } } -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !obj)) +if (unlikely (!obj &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return obj; }mm/slub.c +1
-11@@ -1606,7 +1606,7 @@ static struct page *get_any_partial(strureturn NULL ; do { - cpuset_mems_cookie = get_mems_allowed(); + cpuset_mems_cookie = read_mems_allowed_begin(); zonelist = node_zonelist(slab_node(current->mempolicy), flags); for_each_zone_zonelist(zone, z, zonelist, high_zoneidx) { struct kmem_cache_node *n;@@ -1616,21 +1616,11 @@ static struct page *get_any_partial(struif (n &&cpuset_zone_allowed_hardwall (zone, flags) && n->nr_partial > s->min_partial) { object =get_partial_node (s, n, c); -if (object) { -/* -* Return the object even if -* put_mems_allowed indicated that -* the cpuset mems_allowed was -* updated in parallel. It's a -* harmless race between the alloc -* and the cpuset update. -*/ -put_mems_allowed (cpuset_mems_cookie);
mm/slub.c static struct page *get_any_partial(stru · line 1626
Quoted 10 lines
mm/slub.c +2
-2+if (object) return object; - } } } - }while (!put_mems_allowed (cpuset_mems_cookie)); + }while (read_mems_allowed_retry (cpuset_mems_cookie));#endif return NULL ; }
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Andrew Morton
maintainerRe: [PATCH] mm: Optimize put_mems_allowed() usage
In reply to Peter ZijlstraQuoted 42 lines · On Tue, 27 Mar 2012 15:14:30 +0200 Peter Zijlstra <[email protected]>…
Subject: mm: Optimize
put_mems_allowed()usage
From: Peter Zijlstra <[email protected]>
Date: Mon Mar 26 14:13:05 CEST 2012Since
put_mems_allowed()is strictly optional, its a seqcount retry, we don't need to evaluate the function if the allocation was in fact successful, saving asmp_rmbsome loads and comparisons on some relative fast-paths.Since the naming, get/put_mems_allowed() does suggest a mandatory pairing, rename the interface, as suggested by Mel, to resemble the seqcount interface.
This gives us:
read_mems_allowed_begin()andread_mems_allowed_retry(), where it is important to note that the return value of the latter call is inverted from its previous incarnation....
mm/slub.c +1
-8@@ -1606,7 +1606,7 @@ static struct page *get_any_partial(strureturn NULL ; do { - cpuset_mems_cookie = get_mems_allowed(); + cpuset_mems_cookie = read_mems_allowed_begin(); zonelist = node_zonelist(slab_node(current->mempolicy), flags); for_each_zone_zonelist(zone, z, zonelist, high_zoneidx) { struct kmem_cache_node *n;@@ -1616,21 +1616,11 @@ static struct page *get_any_partial(struif (n &&cpuset_zone_allowed_hardwall (zone, flags) && n->nr_partial > s->min_partial) { object =get_partial_node (s, n, c); -if (object) { -/* -* Return the object even if -* put_mems_allowed indicated that -* the cpuset mems_allowed was -* updated in parallel. It's a -* harmless race between the alloc
mm/slub.c static struct page *get_any_partial(stru · line 1623
Quoted 10 lines
mm/slub.c +2
-5-* and the cpuset update. -*/ -put_mems_allowed (cpuset_mems_cookie); +if (object)return object; - } } } - }while (!put_mems_allowed (cpuset_mems_cookie)); + }while (read_mems_allowed_retry (cpuset_mems_cookie));
I do think it was a bad idea to remove that comment. As it stands, the reader will be wondering why we did the read_mems_allowed_begin() at all, and whether failing to check for a change is a bug.
mm/slub.c renamed from mm/slub.c~mm-optimize-put_mems_allowed-usage-fix +9 -1
@@ -1624,8 +1624,16 @@ static struct page *get_any_partial(struif (n &&cpuset_zone_allowed_hardwall (zone, flags) && n->nr_partial > s->min_partial) { object =get_partial_node (s, n, c); -if (object) +if (object) { +/* +* Don't check read_mems_allowed_retry() +* here - if mems_allowed was updated in +* parallel, that was a harmless race +* between allocation and the cpuset +* update +*/ return object; + } } } }while (read_mems_allowed_retry (cpuset_mems_cookie));
_
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH] mm: Optimize put_mems_allowed() usage
Quoted 14 lines · Andrew Morton
I do think it was a bad idea to remove that comment. As it stands, the reader will be wondering why we did the
read_mems_allowed_begin()at all, and whether failing to check for a change is a bug.mm/slub.c renamed from mm/slub.c~mm-optimize-put_mems_allowed-usage-fix +3
-1@@ -1624,8 +1624,16 @@ static struct page *get_any_partial(struif (n &&cpuset_zone_allowed_hardwall (zone, flags) && n->nr_partial > s->min_partial) { object =get_partial_node (s, n, c); -if (object) +if (object) { +/* +* Don't check read_mems_allowed_retry()
mm/slub.c static struct page *get_any_partial(stru · line 1639
Quoted 10 lines
mm/slub.c renamed from mm/slub.c~mm-optimize-put_mems_allowed-usage-fix +6
-0+* here - if mems_allowed was updated in +* parallel, that was a harmless race +* between allocation and the cpuset +* update +*/ return object; + } } } }while (read_mems_allowed_retry (cpuset_mems_cookie));
OK, it seemed weird to have that comment in this one place whilst it is the general pattern of this construct.
The whole read_mems_allowed_retry() should only ever be attempted in case the allocation failed.
But sure..
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
[tip:sched/numa] mm: Optimize put_mems_allowed() usage
In reply to Peter ZijlstraCommit-ID: 2ab41dd599228532afc0d5920f82e6378ec1c5f3 Gitweb: http://git.kernel.org/tip/2ab41dd599228532afc0d5920f82e6378ec1c5f3 Author: Peter Zijlstra <[email protected]> AuthorDate: Mon, 26 Mar 2012 14:13:05 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Thu, 17 May 2012 14:06:12 +0200
mm: Optimize put_mems_allowed() usage
Since put_mems_allowed() is strictly optional, its a seqcount retry, we don't need to evaluate the function if the allocation was in fact successful, saving a smp_rmb some loads and comparisons on some relative fast-paths.
Since the naming, get/put_mems_allowed() does suggest a mandatory pairing, rename the interface, as suggested by Mel, to resemble the seqcount interface.
This gives us: read_mems_allowed_begin() and read_mems_allowed_retry(), where it is important to note that the return value of the latter call is inverted from its previous incarnation.
--- include/linux/cpuset.h | 27 ++++++++++++++------------- kernel/cpuset.c | 2 +- mm/filemap.c | 4 ++-- mm/hugetlb.c | 4 ++-- mm/mempolicy.c | 14 +++++++------- mm/page_alloc.c | 8 ++++---- mm/slab.c | 4 ++-- mm/slub.c | 16 +++------------- 8 files changed, 35 insertions(+), 44 deletions(-)
include/linux/cpuset.h +14 -13
@@ -89,25 +89,26 @@ extern void rebuild_sched_domains(void);extern void cpuset_print_task_mems_allowed (struct task_struct *p);/* -* get_mems_allowed is required when making decisions involving mems_allowed -* such as during page allocation. mems_allowed can be updated in parallel -* and depending on the new value an operation can fail potentially causing -* process failure. A retry loop with get_mems_allowed and put_mems_allowed -* prevents these artificial failures. +* read_mems_allowed_begin is required when making decisions involving +* mems_allowed such as during page allocation. mems_allowed can be updated in +* parallel and depending on the new value an operation can fail potentially +* causing process failure. A retry loop with read_mems_allowed_begin and +* read_mems_allowed_retry prevents these artificial failures. */ -static inline unsigned int get_mems_allowed (void ) +static inline unsigned int read_mems_allowed_begin (void ) {return read_seqcount_begin (¤t->mems_allowed_seq); }/* -* If this returns false, the operation that took place after get_mems_allowed -* may have failed. It is up to the caller to retry the operation if +* If this returns true, the operation that took place after +* read_mems_allowed_begin may have failed artificially due to a concurrent +* update of mems_allowed. It is up to the caller to retry the operation if * appropriate. */ -static inline bool put_mems_allowed (unsigned int seq) +static inline bool read_mems_allowed_retry (unsigned int seq) { -return !read_seqcount_retry (¤t->mems_allowed_seq, seq); +return read_seqcount_retry (¤t->mems_allowed_seq, seq); }static inline void set_mems_allowed (nodemask_t nodemask)
@@ -223,14 +224,14 @@ static inline void set_mems_allowed(nodemask_t nodemask) { } -static inline unsigned int get_mems_allowed (void ) +static inline unsigned int read_mems_allowed_begin (void ) {return 0 ; } -static inline bool put_mems_allowed (unsigned int seq) +static inline bool read_mems_allowed_retry (unsigned int seq) { -return true; +return false; }#endif /* !CONFIG_CPUSETS */
kernel/cpuset.c +1 -1
@@ -976,7 +976,7 @@ static void cpuset_change_task_nodemask(struct task_struct *tsk,task_lock (tsk);/* * Determine if a loop is necessary if another thread is doing -* get_mems_allowed(). If at least one node remains unchanged and +* read_mems_allowed_begin(). If at least one node remains unchanged and * tsk does not have a mempolicy, then an empty nodemask will not be * possible when mems_allowed is larger than a word. */
mm/filemap.c +2 -2
@@ -501,10 +501,10 @@ struct page *__page_cache_alloc(gfp_t gfp)if (cpuset_do_page_mem_spread ()) {unsigned int cpuset_mems_cookie;do { - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin (); n =cpuset_mem_spread_node (); page =alloc_pages_exact_node (n, gfp,0 ); - }while (!put_mems_allowed (cpuset_mems_cookie) && !page); + }while (!page &&read_mems_allowed_retry (cpuset_mems_cookie));return page; }
mm/hugetlb.c +2 -2
@@ -541,7 +541,7 @@ static struct page *dequeue_huge_page_vma(struct hstate *h,unsigned int cpuset_mems_cookie; retry_cpuset: - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin (); zonelist =huge_zonelist (vma, address, htlb_alloc_mask, &mpol, &nodemask);/*
@@ -570,7 +570,7 @@ retry_cpuset: }mpol_cond_put (mpol); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page;
mm/mempolicy.c +7 -7
@@ -1695,7 +1695,7 @@ int node_random(const nodemask_t *maskp)* If the effective policy is 'BIND, returns a pointer to the mempolicy's * @nodemask for filtering the zonelist. * -* Must be protected by get_mems_allowed() +* Must be protected by read_mems_allowed_begin() */ struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr, gfp_t gfp_flags, struct mempolicy **mpol,
@@ -1860,7 +1860,7 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma, retry_cpuset: pol =get_vma_policy (current, vma, addr); - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin ();if (unlikely (pol->mode ==MPOL_INTERLEAVE )) {unsigned nid;
@@ -1868,7 +1868,7 @@ retry_cpuset: nid =interleave_nid (pol, vma, addr,PAGE_SHIFT + order);mpol_cond_put (pol); page =alloc_page_interleave (gfp, order, nid); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page;
@@ -1881,7 +1881,7 @@ retry_cpuset:struct page *page =__alloc_pages_nodemask (gfp, order, zl,policy_nodemask (gfp, pol));__mpol_put (pol); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page; }
@@ -1890,7 +1890,7 @@ retry_cpuset:*/ page =__alloc_pages_nodemask (gfp, order, zl,policy_nodemask (gfp, pol)); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page; }
@@ -1924,7 +1924,7 @@ struct page *alloc_pages_current(gfp_t gfp, unsigned order) pol = &default_policy; retry_cpuset: - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin ();/* * No reference counting needed for current->mempolicy
@@ -1937,7 +1937,7 @@ retry_cpuset:policy_zonelist (gfp, pol,numa_node_id ()), policy_nodemask(gfp, pol)); -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page;
mm/page_alloc.c +4 -4
@@ -2442,7 +2442,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,return NULL ; retry_cpuset: - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin ();/* The preferred zone is used for statistics later */ first_zones_zonelist(zonelist, high_zoneidx,
@@ -2469,7 +2469,7 @@ out:* the mask is being updated. If a page allocation is about to fail, * check if the cpuset changed during allocation and if so, retry. */ -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) +if (unlikely (!page &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return page;
@@ -2691,9 +2691,9 @@ bool skip_free_areas_node(unsigned int flags, int nid) goto out;do { - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin (); ret = !node_isset (nid, cpuset_current_mems_allowed); - }while (!put_mems_allowed (cpuset_mems_cookie)); + }while (read_mems_allowed_retry (cpuset_mems_cookie)); out:return ret; }
mm/slab.c +2 -2
@@ -3367,7 +3367,7 @@ static void *fallback_alloc(struct kmem_cache *cache, gfp_t flags) local_flags = flags & (GFP_CONSTRAINT_MASK |GFP_RECLAIM_MASK ); retry_cpuset: - cpuset_mems_cookie =get_mems_allowed (); + cpuset_mems_cookie =read_mems_allowed_begin (); zonelist =node_zonelist (slab_node (current->mempolicy), flags); retry:
@@ -3423,7 +3423,7 @@ retry: } } -if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !obj)) +if (unlikely (!obj &&read_mems_allowed_retry (cpuset_mems_cookie))) goto retry_cpuset;return obj; }
mm/slub.c +3 -13
@@ -1613,7 +1613,7 @@ static struct page *get_any_partial(struct kmem_cache *s, gfp_t flags,return NULL ; do { - cpuset_mems_cookie = get_mems_allowed(); + cpuset_mems_cookie = read_mems_allowed_begin(); zonelist = node_zonelist(slab_node(current->mempolicy), flags); for_each_zone_zonelist(zone, z, zonelist, high_zoneidx) { struct kmem_cache_node *n;
@@ -1623,21 +1623,11 @@ static struct page *get_any_partial(struct kmem_cache *s, gfp_t flags,if (n &&cpuset_zone_allowed_hardwall (zone, flags) && n->nr_partial > s->min_partial) { object =get_partial_node (s, n, c); -if (object) { -/* -* Return the object even if -* put_mems_allowed indicated that -* the cpuset mems_allowed was -* updated in parallel. It's a -* harmless race between the alloc -* and the cpuset update. -*/ -put_mems_allowed (cpuset_mems_cookie); +if (object)return object; - } } } - }while (!put_mems_allowed (cpuset_mems_cookie)); + }while (read_mems_allowed_retry (cpuset_mems_cookie));#endif return NULL ; }
Peter Zijlstra
maintainerQuoted 22 lines · Mel Gorman
mm/mempolicy.c +6
-2@@ -1843,18 +1843,24 @@ struct page * alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma, unsigned long addr, int node) { - struct mempolicy *pol = get_vma_policy(current, vma, addr); + struct mempolicy *pol;struct zonelist *zl;struct page *page; +unsigned int cpuset_mems_cookie; + +retry_cpuset: + pol =get_vma_policy (current, vma, addr); + cpuset_mems_cookie =get_mems_allowed (); -get_mems_allowed ();if (unlikely (pol->mode ==MPOL_INTERLEAVE )) {unsigned nid;
mm/mempolicy.c struct page * · line 1866
Quoted 10 lines
mm/mempolicy.c +3
-1nid =interleave_nid (pol, vma, addr,PAGE_SHIFT + order);mpol_cond_put (pol); page =alloc_page_interleave (gfp, order, nid); -put_mems_allowed (); +if (unlikely (!put_mems_allowed (cpuset_mems_cookie) && !page)) + goto retry_cpuset; +return page; } zl =policy_zonelist (gfp, pol, node);
So I think this patch is broken (still). Suppose we have an INTERLEAVE mempol like 0x3 and change it to 0xc.
Original: 0x3
Rebind Step 1: 0xf /* set bits */
Rebind Step 2: 0xc /* clear bits */
Now look at what can happen with offset_il_node() when its ran concurrently with step 2:
nnodes = nodes_weight(pol->v.nodes); /* observes 0xf and returns 4 */
/* now we clear the actual bits */
target = (unsigned int)off % nnodes; /* assume target >= 2 */ c = 0;
do {
nid = next_node(nid, pol->v.nodes); c++;
} while (c <= target);
/* here nid := MAX_NUMNODES */
This nid is then blindly inserted into node_zonelist() which does an NODE_DATA() array access out of bounds and off we go.
This would suggest we put the whole seqcount thing inside offset_il_node().
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Peter Zijlstra
maintainerQuoted 21 lines · Peter Zijlstra
So I think this patch is broken (still). Suppose we have an INTERLEAVE mempol like
0x3and change it to0xc.Original:
0x3
Rebind Step 1:0xf/* set bits */
Rebind Step 2:0xc/* clear bits */Now look at what can happen with
offset_il_node()when its ran concurrently with step 2:nnodes =
nodes_weight(pol->v.nodes); /* observes0xfand returns 4 *//* now we clear the actual bits */
target = (unsigned int)off % nnodes; /* assume target >= 2 */ c = 0;
do {nid = next_node(nid, pol->v.nodes); c++;} while (c <= target);
Quoted 8 lines
/* here nid :=
MAX_NUMNODES*/This nid is then blindly inserted into
node_zonelist()which does anNODE_DATA()array access out of bounds and off we go.This would suggest we put the whole seqcount thing inside
offset_il_node().
Oh bloody grrr. Its not directly related at all, the patch in question fixes a cpuset task_struct::mems_allowed problem while the above is a mempolicy issue and of course the cpuset and mempolicy code are completely bloody different :/
So I guess the quick and ugly solution is something like the below.
mm/mempolicy.c +10 -8
@@ -1762,19 +1762,21 @@ unsigned slab_node(void)static unsigned offset_il_node (struct mempolicy *pol,struct vm_area_struct *vma,unsigned long off) { -unsigned nnodes =nodes_weight (pol->v.nodes); -unsigned target; -int c; -int nid =-1 ; +unsigned nnodes, target; +int c, nid; +again: + nnodes =nodes_weight (pol->v.nodes);if (!nnodes)return numa_node_id (); + target = (unsigned int )off % nnodes; - c =0 ; -do { +for (c =0 , nid =-1 ; c <= target; c++) nid =next_node (nid, pol->v.nodes); - c++; - }while (c <= target); + +if (unlikely ((unsigned )nid >=MAX_NUMNODES )) + goto again; +return nid; }
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 1 line · Peter Zijlstra
So I guess the quick and ugly solution is something like the below.
This still crashes :)
Quoted 18 lines
mm/mempolicy.c +5
-4@@ -1762,19 +1762,21 @@ unsigned slab_node(void)static unsigned offset_il_node (struct mempolicy *pol,struct vm_area_struct *vma,unsigned long off) { -unsigned nnodes =nodes_weight (pol->v.nodes); -unsigned target; -int c; -int nid =-1 ; +unsigned nnodes, target; +int c, nid; +again: + nnodes =nodes_weight (pol->v.nodes);if (!nnodes)return numa_node_id (); +
mm/mempolicy.c unsigned slab_node(void) · line 1778
Quoted 10 lines
mm/mempolicy.c +4
-4target = (unsigned int )off % nnodes; - c =0 ; -do { +for (c =0 , nid =-1 ; c <= target; c++) nid =next_node (nid, pol->v.nodes); - c++; - }while (c <= target); + +if (unlikely ((unsigned )nid >=MAX_NUMNODES )) + goto again;
I'll go kick off a compile that replaces the conditional above with:
if (unlikely(!node_online(nid))) goto again;
Quoted 2 lines
mm/mempolicy.c +0
-0return nid; }
Signature · 7 lines
All rights reversed -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Mel Gorman
reviewerQuoted 2 lines · Peter Zijlstra
Quoted 1 line · Peter Zijlstra
So I think this patch is broken (still).
I am assuming the lack of complaints is that it is not a heavily executed path. I expect that you (and Rik) are hitting this as part of automatic NUMA balancing. Still a bug, just slightly less urgent if NUMA balancing is the reproduction case.
Quoted 56 lines
Quoted 21 lines
Suppose we have an
INTERLEAVE mempol like0x3and change it to0xc.Original:
0x3
Rebind Step 1:0xf/* set bits */
Rebind Step 2:0xc/* clear bits */Now look at what can happen with
offset_il_node()when its ran concurrently with step 2:nnodes =
nodes_weight(pol->v.nodes); /* observes0xfand returns 4 *//* now we clear the actual bits */
target = (unsigned int)off % nnodes; /* assume target >= 2 */ c = 0;
do {nid = next_node(nid, pol->v.nodes); c++;} while (c <= target);
Quoted 8 lines
/* here nid :=
MAX_NUMNODES*/This nid is then blindly inserted into
node_zonelist()which does anNODE_DATA()array access out of bounds and off we go.This would suggest we put the whole seqcount thing inside
offset_il_node().Oh bloody grrr. Its not directly related at all, the patch in question fixes a cpuset
task_struct::mems_allowedproblem while the above is a mempolicy issue and of course the cpuset and mempolicy code are completely bloody different :/So I guess the quick and ugly solution is something like the below.
mm/mempolicy.c +5
-4@@ -1762,19 +1762,21 @@ unsigned slab_node(void)static unsigned offset_il_node (struct mempolicy *pol,struct vm_area_struct *vma,unsigned long off) { -unsigned nnodes =nodes_weight (pol->v.nodes); -unsigned target; -int c; -int nid =-1 ; +unsigned nnodes, target; +int c, nid; +again: + nnodes =nodes_weight (pol->v.nodes);if (!nnodes)return numa_node_id (); + target = (unsigned int )off % nnodes;
mm/mempolicy.c unsigned slab_node(void) · line 1779
Quoted 10 lines
mm/mempolicy.c +5
-4- c =0 ; -do { +for (c =0 , nid =-1 ; c <= target; c++) nid =next_node (nid, pol->v.nodes); - c++; - }while (c <= target); + +if (unlikely ((unsigned )nid >=MAX_NUMNODES )) + goto again; +
MAX_NUMNODES is unrelated to anything except that it might prevent a crash and even then nr_online_nodes is probably what you wanted and even that assumes the NUMA node numbering is contiguous. The real concern is whether the updated mask is an allowed target for the updated memory policy. If it's not then "nid" can be pointing off the deep end somewhere. With this conversion to a for loop there is race after you check nnodes where target gets set to 0 and then return a nid of -1 which I suppose will just blow up differently but it's fixable.
This? Untested. Fixes implicit types while it's there. Note the use of first node and (c < target) to guarantee nid gets set and that the first potential node is still used as an interleave target.
mm/mempolicy.c +13 -11
@@ -1755,22 +1755,24 @@ unsigned slab_node(void) }/* Do static interleaving for a VMA with known offset. */ -static unsigned offset_il_node (struct mempolicy *pol, +static unsigned int offset_il_node (struct mempolicy *pol,struct vm_area_struct *vma,unsigned long off) { -unsigned nnodes =nodes_weight (pol->v.nodes); -unsigned target; -int c; -int nid =-1 ; +unsigned int nr_nodes, target; +int i, nid; -if (!nnodes) +again: + nr_nodes =nodes_weight (pol->v.nodes); +if (!nr_nodes)return numa_node_id (); - target = (unsigned int )off % nnodes; - c =0 ; -do { + target = (unsigned int )off % nr_nodes; +for (i =0 , nid =first_node (pol->v.nodes); i < target; i++) nid =next_node (nid, pol->v.nodes); - c++; - }while (c <= target); + +/* Policy nodemask can potentially update in parallel */ +if (unlikely (!node_isset (nid, pol->v.nodes))) + goto again; +return nid; }
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Peter Zijlstra
maintainerQuoted 8 lines · Mel Gorman
Quoted 2 lines · Peter Zijlstra
Quoted 1 line · Peter Zijlstra
So I think this patch is broken (still).
I am assuming the lack of complaints is that it is not a heavily executed path. I expect that you (and Rik) are hitting this as part of automatic NUMA balancing. Still a bug, just slightly less urgent if NUMA balancing is the reproduction case.
I thought it was, we crashed somewhere suspiciously close, but no. You need shared mpols for this to actually trigger and the NUMA stuff doesn't use that.
Quoted 7 lines
mm/mempolicy.c unsigned slab_node(void) · line 1775
Quoted 3 lines
mm/mempolicy.c +3
-0+if (unlikely ((unsigned )nid >=MAX_NUMNODES )) + goto again; +
MAX_NUMNODESis unrelated to anything except that it might prevent a crash and even thennr_online_nodesis probably what you wanted and even that assumes the NUMA node numbering is contiguous.
I used whatever nodemask.h did to detect end-of-bitmap and they use MAX_NUMNODES. See __next_node() and for_each_node() like.
MAX_NUMNODES doesn't assume contiguous numbers since its the actual size of the bitmap, nr_online_nodes would hoever.
Quoted 6 lines
The real concern is whether
the updated mask is an allowed target for the updated memory policy. If it's not then "nid" can be pointing off the deep end somewhere. With this conversion to a for loop there is race after you check nnodes where target gets set to 0 and then return a nid of -1 which I suppose will just blow up differently but it's fixable.
But but but, I did i <= target, which will match when target == 0 so you'll get at least a single iteration and nid will be set.
Quoted 33 lines
This? Untested. Fixes implicit types while it's there. Note the use of first node and (c < target) to guarantee nid gets set and that the first potential node is still used as an interleave target.
mm/mempolicy.c +8
-9@@ -1755,22 +1755,24 @@ unsigned slab_node(void) }/* Do static interleaving for a VMA with known offset. */ -static unsigned offset_il_node (struct mempolicy *pol, +static unsigned int offset_il_node (struct mempolicy *pol,struct vm_area_struct *vma,unsigned long off) { -unsigned nnodes =nodes_weight (pol->v.nodes); -unsigned target; -int c; -int nid =-1 ; +unsigned int nr_nodes, target; +int i, nid; -if (!nnodes) +again: + nr_nodes =nodes_weight (pol->v.nodes); +if (!nr_nodes)return numa_node_id (); - target = (unsigned int )off % nnodes; - c =0 ; - do { + target = (unsigned int )off % nr_nodes; +for (i =0 , nid =first_node (pol->v.nodes); i < target; i++)
mm/mempolicy.c unsigned slab_node(void) · line 1777
Quoted 10 lines
mm/mempolicy.c +5
-2nid =next_node (nid, pol->v.nodes); - c++; - }while (c <= target); + +/* Policy nodemask can potentially update in parallel */ +if (unlikely (!node_isset (nid, pol->v.nodes))) + goto again; +return nid; }
So I explicitly didn't use the node_isset() test because that's more likely to trigger than the nid >= MAX_NUMNODES test. Its fine to return a node that isn't actually part of the mask anymore -- a race is a race anyway.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Peter Zijlstra
maintainerQuoted 36 lines · Peter Zijlstra
Quoted 36 lines
mm/mempolicy.c +12
-11@@ -1755,22 +1755,24 @@ unsigned slab_node(void) }/* Do static interleaving for a VMA with known offset. */ -static unsigned offset_il_node (struct mempolicy *pol, +static unsigned int offset_il_node (struct mempolicy *pol,struct vm_area_struct *vma,unsigned long off) { -unsigned nnodes =nodes_weight (pol->v.nodes); -unsigned target; -int c; -int nid =-1 ; +unsigned int nr_nodes, target; +int i, nid; -if (!nnodes) +again: + nr_nodes =nodes_weight (pol->v.nodes); +if (!nr_nodes)return numa_node_id (); - target = (unsigned int )off % nnodes; - c =0 ; -do { + target = (unsigned int )off % nr_nodes; +for (i =0 , nid =first_node (pol->v.nodes); i < target; i++) nid =next_node (nid, pol->v.nodes); - c++; - }while (c <= target); + +/* Policy nodemask can potentially update in parallel */ +if (unlikely (!node_isset (nid, pol->v.nodes))) + goto again;
Quoted 8 lines
mm/mempolicy.c unsigned slab_node(void) · line 1777
Quoted 3 lines
mm/mempolicy.c +1
-0+return nid; }So I explicitly didn't use the
node_isset()test because that's more likely to trigger than the nid >=MAX_NUMNODEStest. Its fine to return a node that isn't actually part of the mask anymore -- a race is a race anyway.
Oh more importantly, if nid does indeed end up being >= MAX_NUMNODES as is possible with next_node() the node_isset() test will be out-of-bounds and can crash itself.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Mel Gorman
reviewerQuoted 5 lines · Peter Zijlstra
Quoted 4 lines · Mel Gorman
Quoted 2 lines · Peter Zijlstra
Quoted 1 line · Peter Zijlstra
So I think this patch is broken (still).
Quoted 9 lines
Quoted 4 lines
I am assuming the lack of complaints is that it is not a heavily executed path. I expect that you (and Rik) are hitting this as part of automatic NUMA balancing. Still a bug, just slightly less urgent if NUMA balancing is the reproduction case.
I thought it was, we crashed somewhere suspiciously close, but no. You need shared mpols for this to actually trigger and the NUMA stuff doesn't use that.
Ah, so this is a red herring?
Quoted 11 lines
Quoted 7 lines
mm/mempolicy.c unsigned slab_node(void) · line 1775
Quoted 3 lines
mm/mempolicy.c +3
-0+if (unlikely ((unsigned )nid >=MAX_NUMNODES )) + goto again; +
MAX_NUMNODESis unrelated to anything except that it might prevent a crash and even thennr_online_nodesis probably what you wanted and even that assumes the NUMA node numbering is contiguous.I used whatever
nodemask.hdid to detect end-of-bitmap and they useMAX_NUMNODES. See__next_node()andfor_each_node()like.
The check does prevent us going off the end of the bitmap but does not necessarily return an online node.
Quoted 3 lines
MAX_NUMNODESdoesn't assume contiguous numbers since its the actual size of the bitmap,nr_online_nodeswould hoever.
I intended to say nr_node_ids, the same size as buffers such as the task_numa_buffers. If we ever return a nid > nr_node_ids here then task_numa_fault would corrupt memory. However, it should be possible for node_weight to exceed nr_node_ids except maybe during node hot-remove so it's not the problem.
Quoted 10 lines
Quoted 6 lines
The real concern is whether
the updated mask is an allowed target for the updated memory policy. If it's not then "nid" can be pointing off the deep end somewhere. With this conversion to a for loop there is race after you check nnodes where target gets set to 0 and then return a nid of -1 which I suppose will just blow up differently but it's fixable.But but but, I did i <= target, which will match when target == 0 so you'll get at least a single iteration and nid will be set.
True.
Quoted 40 lines
Quoted 40 lines
This? Untested. Fixes implicit types while it's there. Note the use of first node and (c < target) to guarantee nid gets set and that the first potential node is still used as an interleave target.
mm/mempolicy.c +12
-11@@ -1755,22 +1755,24 @@ unsigned slab_node(void) }/* Do static interleaving for a VMA with known offset. */ -static unsigned offset_il_node (struct mempolicy *pol, +static unsigned int offset_il_node (struct mempolicy *pol,struct vm_area_struct *vma,unsigned long off) { -unsigned nnodes =nodes_weight (pol->v.nodes); -unsigned target; -int c; -int nid =-1 ; +unsigned int nr_nodes, target; +int i, nid; -if (!nnodes) +again: + nr_nodes =nodes_weight (pol->v.nodes); +if (!nr_nodes)return numa_node_id (); - target = (unsigned int )off % nnodes; - c =0 ; -do { + target = (unsigned int )off % nr_nodes; +for (i =0 , nid =first_node (pol->v.nodes); i < target; i++) nid =next_node (nid, pol->v.nodes); - c++; - }while (c <= target); + +/* Policy nodemask can potentially update in parallel */ +if (unlikely (!node_isset (nid, pol->v.nodes))) + goto again;
Quoted 8 lines
mm/mempolicy.c unsigned slab_node(void) · line 1777
Quoted 3 lines
mm/mempolicy.c +1
-0+return nid; }So I explicitly didn't use the
node_isset()test because that's more likely to trigger than the nid >=MAX_NUMNODEStest. Its fine to return a node that isn't actually part of the mask anymore -- a race is a race anyway.
Yeah and as long as it's < nr_node_ids it should be ok within the task numa fault handling as well.
Signature · 8 lines
Mel Gorman SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Peter Zijlstra
maintainerQuoted 6 lines · Mel Gorman
Quoted 4 lines
I thought it was, we crashed somewhere suspiciously close, but no. You need shared mpols for this to actually trigger and the NUMA stuff doesn't use that.
Ah, so this is a red herring?
Yeah, but I still think its an actual bug. It seems the easiest way to trigger this would be to:
create a task that constantly allocates pages
have said task have an MPOL_INTERLEAVE task policy
put said task into a cpuset
using a different task (your shell for example) flip the cpuset's
mems_allowed back and forth.
This would have the shell task constantly rebind (in two steps) our allocating task's INTERLEAVE policy.
Quoted 6 lines
Quoted 3 lines
I used whatever
nodemask.hdid to detect end-of-bitmap and they useMAX_NUMNODES. See__next_node()andfor_each_node()like.The check does prevent us going off the end of the bitmap but does not necessarily return an online node.
Right, but its guaranteed to return a 'valid' node. I don't think it returning an offline node is a problem, we'll find it empty and fail the page allocation.
Quoted 9 lines
Quoted 3 lines
MAX_NUMNODESdoesn't assume contiguous numbers since its the actual size of the bitmap,nr_online_nodeswould hoever.I intended to say
nr_node_ids, the same size as buffers such as thetask_numa_buffers. If we ever return a nid >nr_node_idshere thentask_numa_faultwould corrupt memory. However, it should be possible fornode_weightto exceednr_node_idsexcept maybe during node hot-remove so it's not the problem.
The nodemask situation seems somewhat more confused than the cpumask case; how would we ever return a nid > nr_node_ids? Corrupt nodemask?
In the cpumask case we use the runtime limit nr_cpu_ids for all bitmap operations, arguably we should make the nodemask stuff do the same.
Less bits to iterate is always good; a MAX_NUMNODES=64 (x86_64-defconfig) will still iterate all 64 bits, even though its unlikely to have more than 1 let alone more than 8 nodes.
Quoted 7 lines
Quoted 4 lines
So I explicitly didn't use the
node_isset()test because that's more likely to trigger than the nid >=MAX_NUMNODEStest. Its fine to return a node that isn't actually part of the mask anymore -- a race is a race anyway.Yeah and as long as it's <
nr_node_idsit should be ok within the task numa fault handling as well.
Right, I'm just a tad confused on how we could ever get a nid >= nr_node_ids except from a prior bug (corrupted nodemask).
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Mel Gorman
reviewerQuoted 9 lines · Peter Zijlstra
Quoted 5 lines
I intended to say
nr_node_ids, the same size as buffers such as thetask_numa_buffers. If we ever return a nid >nr_node_idshere thentask_numa_faultwould corrupt memory. However, it should be possible fornode_weightto exceednr_node_idsexcept maybe during node hot-remove so it's not the problem.The nodemask situation seems somewhat more confused than the cpumask case; how would we ever return a nid >
nr_node_ids? Corrupt nodemask?
ARGH. I meant impossible. The exact bloody opposite of what I wrote.
Signature · 2 lines
Mel Gorman SUSE Labs