Use local_lock for pcp protection and reduce stat overhead
Subsystems (6)
v3 cover Use local_lock for pcp protection and reduce stat overhead
Changelog since v2
o Fix zonestats initialisation
o Merged memory hotplug fix separately
o Embed local_lock within per_cpu_pages
This series requires patches in Andrew's tree so for convenience, it's also available at
git://git.kernel.org/pub/scm/linux/kernel/git/mel/linux.git mm-percpu-local_lock-v3r6
The PCP (per-cpu page allocator in page_alloc.c) shares locking requirements with vmstat and the zone lock which is inconvenient and causes some issues. For example, the PCP list and vmstat share the same per-cpu space meaning that it's possible that vmstat updates dirty cache lines holding per-cpu lists across CPUs unless padding is used. Second, PREEMPT_RT does not want to disable IRQs for too long in the page allocator.
This series splits the locking requirements and uses locks types more suitable for PREEMPT_RT, reduces the time when special locking is required for stats and reduces the time when IRQs need to be disabled on !PREEMPT_RT kernels.
Why local_lock? PREEMPT_RT considers the following sequence to be unsafe as documented in Documentation/locking/locktypes.rst
local_irq_disable(); spin_lock(&lock);
The pcp allocator has this sequence for rmqueue_pcplist (local_irq_save) -> __rmqueue_pcplist -> rmqueue_bulk (spin_lock). While it's possible to separate this out, it generally means there are points where we enable IRQs and reenable them again immediately. To prevent a migration and the per-cpu pointer going stale, migrate_disable is also needed. That is a custom lock that is similar, but worse, than local_lock. Furthermore, on PREEMPT_RT, it's undesirable to leave IRQs disabled for too long. By converting to local_lock which disables migration on PREEMPT_RT, the locking requirements can be separated and start moving the protections for PCP, stats and the zone lock to PREEMPT_RT-safe equivalent locking. As a bonus, local_lock also means that PROVE_LOCKING does something useful.
After that, it's obvious that zone_statistics incurs too much overhead and leaves IRQs disabled for longer than necessary on !PREEMPT_RT kernels. zone_statistics uses perfectly accurate counters requiring IRQs be disabled for parallel RMW sequences when inaccurate ones like vm_events would do. The series makes the NUMA statistics (NUMA_HIT and friends) inaccurate counters that then require no special protection on !PREEMPT_RT.
The bulk page allocator can then do stat updates in bulk with IRQs enabled which should improve the efficiency. Technically, this could have been done without the local_lock and vmstat conversion work and the order simply reflects the timing of when different series were implemented.
Finally, there are places where we conflate IRQs being disabled for the PCP with the IRQ-safe zone spinlock. The remainder of the series reduces the scope of what is protected by disabled IRQs on !PREEMPT_RT kernels. By the end of the series, page_alloc.c does not call local_irq_save so the locking scope is a bit clearer. The one exception is that modifying NR_FREE_PAGES still happens in places where it's known the IRQs are disabled as it's harmless for PREEMPT_RT and would be expensive to split the locking there.
No performance data is included because despite the overhead of the stats, it's within the noise for most workloads on !PREEMPT_RT. However, Jesper Dangaard Brouer ran a page allocation microbenchmark on a E5-1650 v4 @ 3.60GHz CPU on the first version of this series. Focusing on the array variant of the bulk page allocator reveals the following.
(CPU: Intel(R) Xeon(R) CPU E5-1650 v4 @ 3.60GHz) ARRAY variant: time_bulk_page_alloc_free_array: step=bulk size
Baseline Patched 1 56.383 54.225 (+3.83%) 2 40.047 35.492 (+11.38%) 3 37.339 32.643 (+12.58%) 4 35.578 30.992 (+12.89%) 8 33.592 29.606 (+11.87%) 16 32.362 28.532 (+11.85%) 32 31.476 27.728 (+11.91%) 64 30.633 27.252 (+11.04%) 128 30.596 27.090 (+11.46%)
While this is a positive outcome, the series is more likely to be interesting to the RT people in terms of getting parts of the PREEMPT_RT tree into mainline.
drivers/base/node.c | 18 +-- include/linux/mmzone.h | 58 ++++++-- include/linux/vmstat.h | 65 +++++---- mm/mempolicy.c | 2 +- mm/page_alloc.c | 302 +++++++++++++++++++++++++---------------- mm/vmstat.c | 250 ++++++++++++---------------------- 6 files changed, 370 insertions(+), 325 deletions(-)
Signature · 27 lines
2.26.2
Mel Gorman (11):
mm/page_alloc: Split per cpu page lists and zone stats
mm/page_alloc: Convert per-cpu list protection to local_lock
mm/vmstat: Convert NUMA statistics to basic NUMA counters
mm/vmstat: Inline NUMA event counter updates
mm/page_alloc: Batch the accounting updates in the bulk allocator
mm/page_alloc: Reduce duration that IRQs are disabled for VM counters
mm/page_alloc: Remove duplicate checks if migratetype should be
isolated
mm/page_alloc: Explicitly acquire the zone lock in __free_pages_ok
mm/page_alloc: Avoid conflating IRQs disabled with zone->lock
mm/page_alloc: Update PGFREE outside the zone lock in __free_pages_ok
mm/page_alloc: Embed per_cpu_pages locking within the per-cpu
structure
drivers/base/node.c | 18 +--
include/linux/mmzone.h | 58 ++++++--
include/linux/vmstat.h | 65 +++++----
mm/mempolicy.c | 2 +-
mm/page_alloc.c | 302 +++++++++++++++++++++++++----------------
mm/vmstat.c | 250 ++++++++++++----------------------
6 files changed, 370 insertions(+), 325 deletions(-)
--
2.26.2PATCH v3 1/11 mm/page_alloc: Split per cpu page lists and zone stats
The per-cpu page allocator lists and the per-cpu vmstat deltas are stored in the same struct per_cpu_pages even though vmstats have no direct impact on the per-cpu page lists. This is inconsistent because the vmstats for a node are stored on a dedicated structure. The bigger issue is that the per_cpu_pages structure is not cache-aligned and stat updates either cache conflict with adjacent per-cpu lists incurring a runtime cost or padding is required incurring a memory cost.
This patch splits the per-cpu pagelists and the vmstat deltas into separate structures. It's mostly a mechanical conversion but some variable renaming is done to clearly distinguish the per-cpu pages structure (pcp) from the vmstats (pzstats).
Superficially, this appears to increase the size of the per_cpu_pages structure but the movement of expire fills a structure hole so there is no impact overall.
--- include/linux/mmzone.h | 18 ++++---- include/linux/vmstat.h | 8 ++-- mm/page_alloc.c | 85 ++++++++++++++++++++----------------- mm/vmstat.c | 96 ++++++++++++++++++++++-------------------- 4 files changed, 111 insertions(+), 96 deletions(-)
include/linux/mmzone.h +10 -8
@@ -341,20 +341,21 @@ struct per_cpu_pages {int count;/* number of pages in the list */ int high;/* high watermark, emptying needed */ int batch;/* chunk size for buddy add/remove */ +#ifdef CONFIG_NUMA +int expire;/* When 0, remote pagesets are drained */ +#endif /* Lists of pages, one per migrate type stored on the pcp-lists */ struct list_head lists[MIGRATE_PCPTYPES ]; }; -struct per_cpu_pageset { -struct per_cpu_pages pcp; -#ifdef CONFIG_NUMA -s8 expire; -u16 vm_numa_stat_diff[NR_VM_NUMA_STAT_ITEMS ]; -#endif +struct per_cpu_zonestat {#ifdef CONFIG_SMP -s8 stat_threshold;s8 vm_stat_diff[NR_VM_ZONE_STAT_ITEMS ]; +s8 stat_threshold; +#endif +#ifdef CONFIG_NUMA +u16 vm_numa_stat_diff[NR_VM_NUMA_STAT_ITEMS ];#endif };
@@ -470,7 +471,8 @@ struct zone {int node;#endif struct pglist_data *zone_pgdat; -struct per_cpu_pageset __percpu *pageset; +struct per_cpu_pages __percpu *per_cpu_pageset; +struct per_cpu_zonestat __percpu *per_cpu_zonestats;/* * the high and batch values are copied to individual pagesets for * faster access
include/linux/vmstat.h +4 -4
@@ -163,7 +163,7 @@ static inline unsigned long zone_numa_state_snapshot(struct zone *zone,int cpu;for_each_online_cpu (cpu) - x +=per_cpu_ptr (zone->pageset, cpu)->vm_numa_stat_diff[item]; + x +=per_cpu_ptr (zone->per_cpu_zonestats, cpu)->vm_numa_stat_diff[item];return x; }
@@ -236,7 +236,7 @@ static inline unsigned long zone_page_state_snapshot(struct zone *zone,#ifdef CONFIG_SMP int cpu;for_each_online_cpu (cpu) - x +=per_cpu_ptr (zone->pageset, cpu)->vm_stat_diff[item]; + x +=per_cpu_ptr (zone->per_cpu_zonestats, cpu)->vm_stat_diff[item];if (x <0 ) x =0 ;
@@ -291,7 +291,7 @@ struct ctl_table;int vmstat_refresh (struct ctl_table *,int write,void *buffer,size_t *lenp,loff_t *ppos); -void drain_zonestat (struct zone *zone,struct per_cpu_pageset *); +void drain_zonestat (struct zone *zone,struct per_cpu_zonestat *);int calculate_pressure_threshold (struct zone *zone);int calculate_normal_threshold (struct zone *zone);
@@ -399,7 +399,7 @@ static inline void cpu_vm_stats_fold(int cpu) { }static inline void quiet_vmstat (void ) { }static inline void drain_zonestat (struct zone *zone, -struct per_cpu_pageset *pset) { } +struct per_cpu_zonestat *pzstats) { }#endif /* CONFIG_SMP */ static inline void __mod_zone_freepage_state(struct zone *zone, int nr_pages,
mm/page_alloc.c +47 -38
212 lines not on this page · raw message
mm/vmstat.c +50 -46
250 lines not on this page · raw message
Signature · 1 line
2.26.2
PATCH v3 2/11 mm/page_alloc: Convert per-cpu list protection to local_lock
In reply to Mel GormanThere is a lack of clarity of what exactly local_irq_save/local_irq_restore protects in page_alloc.c . It conflates the protection of per-cpu page allocation structures with per-cpu vmstat deltas.
This patch protects the PCP structure using local_lock which for most configurations is identical to IRQ enabling/disabling. The scope of the lock is still wider than it should be but this is decreased later.
It is possible for the local_lock to be embedded safely within struct per_cpu_pages but it adds complexity to free_unref_page_list so it is implemented as a separate patch later in the series.
--- include/linux/mmzone.h | 2 ++ mm/page_alloc.c | 50 +++++++++++++++++++++++++++++------------- 2 files changed, 37 insertions(+), 15 deletions(-)
include/linux/mmzone.h +2 -0
@@ -20,6 +20,7 @@#include <linux/atomic.h> #include <linux/mm_types.h> #include <linux/page-flags.h> +#include <linux/local_lock.h> #include <asm/page.h> /* Free memory management - zoned buddy allocator. */
@@ -337,6 +338,7 @@ enum zone_watermarks {#define high_wmark_pages (z) (z->_watermark[WMARK_HIGH] + z->watermark_boost)#define wmark_pages (z, i) (z->_watermark[i] + z->watermark_boost) +/* Fields and list protected by pagesets local_lock in page_alloc.c */ struct per_cpu_pages {int count;/* number of pages in the list */ int high;/* high watermark, emptying needed */
mm/page_alloc.c +35 -15
145 lines not on this page · raw message
Signature · 1 line
2.26.2
PATCH v3 3/11 mm/vmstat: Convert NUMA statistics to basic NUMA counters
In reply to Mel GormanNUMA statistics are maintained on the zone level for hits, misses, foreign etc but nothing relies on them being perfectly accurate for functional correctness. The counters are used by userspace to get a general overview of a workloads NUMA behaviour but the page allocator incurs a high cost to maintain perfect accuracy similar to what is required for a vmstat like NR_FREE_PAGES. There even is a sysctl vm.numa_stat to allow userspace to turn off the collection of NUMA statistics like NUMA_HIT.
This patch converts NUMA_HIT and friends to be NUMA events with similar accuracy to VM events. There is a possibility that slight errors will be introduced but the overall trend as seen by userspace will be similar. Note that while these counters could be maintained at the node level that it would have a user-visible impact.
--- drivers/base/node.c | 18 +++-- include/linux/mmzone.h | 11 ++- include/linux/vmstat.h | 42 +++++----- mm/mempolicy.c | 2 +- mm/page_alloc.c | 12 +-- mm/vmstat.c | 175 ++++++++++++----------------------------- 6 files changed, 93 insertions(+), 167 deletions(-)
drivers/base/node.c +10 -8
@@ -484,6 +484,7 @@ static DEVICE_ATTR(meminfo, 0444, node_read_meminfo, NULL);static ssize_t node_read_numastat (struct device *dev,struct device_attribute *attr,char *buf) { +fold_vm_numa_events ();return sysfs_emit (buf,"numa_hit %lu\n" "numa_miss %lu\n"
@@ -491,12 +492,12 @@ static ssize_t node_read_numastat(struct device *dev,"interleave_hit %lu\n" "local_node %lu\n" "other_node %lu\n" , -sum_zone_numa_state (dev->id,NUMA_HIT ), -sum_zone_numa_state (dev->id,NUMA_MISS ), -sum_zone_numa_state (dev->id,NUMA_FOREIGN ), -sum_zone_numa_state (dev->id,NUMA_INTERLEAVE_HIT ), -sum_zone_numa_state (dev->id,NUMA_LOCAL ), - sum_zone_numa_state(dev->id, NUMA_OTHER)); +sum_zone_numa_event_state (dev->id,NUMA_HIT ), +sum_zone_numa_event_state (dev->id,NUMA_MISS ), +sum_zone_numa_event_state (dev->id,NUMA_FOREIGN ), +sum_zone_numa_event_state (dev->id,NUMA_INTERLEAVE_HIT ), +sum_zone_numa_event_state (dev->id,NUMA_LOCAL ), + sum_zone_numa_event_state(dev->id, NUMA_OTHER)); } static DEVICE_ATTR(numastat, 0444, node_read_numastat, NULL);
@@ -514,10 +515,11 @@ static ssize_t node_read_vmstat(struct device *dev, sum_zone_node_page_state(nid, i));#ifdef CONFIG_NUMA -for (i =0 ; i <NR_VM_NUMA_STAT_ITEMS ; i++) +fold_vm_numa_events (); +for (i =0 ; i <NR_VM_NUMA_EVENT_ITEMS ; i++) len +=sysfs_emit_at (buf, len,"%s %lu\n" ,numa_stat_name (i), -sum_zone_numa_state (nid, i)); +sum_zone_numa_event_state (nid, i));#endif for (i =0 ; i <NR_VM_NODE_STAT_ITEMS ; i++) {
include/linux/mmzone.h +7 -4
@@ -135,10 +135,10 @@ enum numa_stat_item {NUMA_INTERLEAVE_HIT ,/* interleaver preferred this zone */ NUMA_LOCAL ,/* allocation from local node */ NUMA_OTHER ,/* allocation from other node */ -NR_VM_NUMA_STAT_ITEMS +NR_VM_NUMA_EVENT_ITEMS };#else -#define NR_VM_NUMA_STAT_ITEMS 0 +#define NR_VM_NUMA_EVENT_ITEMS 0#endif enum zone_stat_item {
@@ -357,7 +357,10 @@ struct per_cpu_zonestat {s8 stat_threshold;#endif #ifdef CONFIG_NUMA -u16 vm_numa_stat_diff[NR_VM_NUMA_STAT_ITEMS ]; +u16 vm_numa_stat_diff[NR_VM_NUMA_EVENT_ITEMS ]; +#endif +#ifdef CONFIG_NUMA +unsigned long vm_numa_event[NR_VM_NUMA_EVENT_ITEMS ];#endif };
@@ -609,7 +612,7 @@ struct zone {ZONE_PADDING (_pad3_)/* Zone statistics */ atomic_long_t vm_stat[NR_VM_ZONE_STAT_ITEMS]; -atomic_long_t vm_numa_stat[NR_VM_NUMA_STAT_ITEMS ]; +atomic_long_t vm_numa_events[NR_VM_NUMA_EVENT_ITEMS ]; }____cacheline_internodealigned_in_smp ;enum pgdat_flags {
include/linux/vmstat.h +19 -23
93 lines not on this page · raw message
mm/mempolicy.c +1 -1
@@ -2146,7 +2146,7 @@ static struct page *alloc_page_interleave(gfp_t gfp, unsigned order,return page;if (page &&page_to_nid (page) == nid) {preempt_disable (); -__inc_numa_state (page_zone (page),NUMA_INTERLEAVE_HIT ); +__count_numa_event (page_zone (page),NUMA_INTERLEAVE_HIT );preempt_enable (); }return page;
mm/page_alloc.c +6 -6
@@ -3424,12 +3424,12 @@ static inline void zone_statistics(struct zone *preferred_zone, struct zone *z) local_stat =NUMA_OTHER ;if (zone_to_nid (z) ==zone_to_nid (preferred_zone)) -__inc_numa_state (z,NUMA_HIT ); +__count_numa_event (z,NUMA_HIT );else { -__inc_numa_state (z,NUMA_MISS ); -__inc_numa_state (preferred_zone,NUMA_FOREIGN ); +__count_numa_event (z,NUMA_MISS ); +__count_numa_event (preferred_zone,NUMA_FOREIGN ); } -__inc_numa_state (z, local_stat); +__count_numa_event (z, local_stat);#endif }
@@ -6700,8 +6700,8 @@ void __init setup_per_cpu_pageset(void)*/ for_each_possible_cpu (cpu) {struct per_cpu_zonestat *pzstats = &per_cpu (boot_zonestats, cpu); -memset (pzstats->vm_numa_stat_diff,0 , -sizeof (pzstats->vm_numa_stat_diff)); +memset (pzstats->vm_numa_event,0 , +sizeof (pzstats->vm_numa_event)); }#endif
mm/vmstat.c +50 -125
338 lines not on this page · raw message
Signature · 1 line
2.26.2
PATCH v3 4/11 mm/vmstat: Inline NUMA event counter updates
In reply to Mel Gorman__count_numa_event is small enough to be treated similarly to __count_vm_event so inline it.
--- include/linux/vmstat.h | 9 +++++++++ mm/vmstat.c | 9 --------- 2 files changed, 9 insertions(+), 9 deletions(-)
include/linux/vmstat.h +9 -0
@@ -237,6 +237,15 @@ static inline unsigned long zone_page_state_snapshot(struct zone *zone, }#ifdef CONFIG_NUMA +/* See __count_vm_event comment on why raw_cpu_inc is used. */ +static inline void +__count_numa_event (struct zone *zone,enum numa_stat_item item) +{ +struct per_cpu_zonestat __percpu *pzstats = zone->per_cpu_zonestats; + +raw_cpu_inc (pzstats->vm_numa_event[item]); +} +extern void __count_numa_event (struct zone *zone,enum numa_stat_item item);extern unsigned long sum_zone_node_page_state (int node,enum zone_stat_item item);
mm/vmstat.c +0 -9
@@ -902,15 +902,6 @@ void drain_zonestat(struct zone *zone, struct per_cpu_zonestat *pzstats)#endif #ifdef CONFIG_NUMA -/* See __count_vm_event comment on why raw_cpu_inc is used. */ -void __count_numa_event (struct zone *zone, -enum numa_stat_item item) -{ -struct per_cpu_zonestat __percpu *pzstats = zone->per_cpu_zonestats; - -raw_cpu_inc (pzstats->vm_numa_event[item]); -} -/* * Determine the per node value of a stat item. This function * is called frequently in a NUMA machine, so try to be as
Signature · 1 line
2.26.2
Re: [PATCH 04/11] mm/vmstat: Inline NUMA event counter updates
Quoted 4 lines · Mel Gorman
__count_numa_eventis small enough to be treated similarly to__count_vm_eventso inline it.Signed-off-by Mel Gorman <[email protected]>
Quoted 46 lines
--- include/linux/vmstat.h | 9 +++++++++ mm/vmstat.c | 9 --------- 2 files changed, 9 insertions(+), 9 deletions(-)include/linux/vmstat.h +9
-0@@ -237,6 +237,15 @@ static inline unsigned long zone_page_state_snapshot(struct zone *zone, }#ifdef CONFIG_NUMA +/* See __count_vm_event comment on why raw_cpu_inc is used. */ +static inline void +__count_numa_event (struct zone *zone,enum numa_stat_item item) +{ +struct per_cpu_zonestat __percpu *pzstats = zone->per_cpu_zonestats; + +raw_cpu_inc (pzstats->vm_numa_event[item]); +} +extern void __count_numa_event (struct zone *zone,enum numa_stat_item item);extern unsigned long sum_zone_node_page_state (int node,enum zone_stat_item item);mm/vmstat.c +0
-9@@ -902,15 +902,6 @@ void drain_zonestat(struct zone *zone, struct per_cpu_zonestat *pzstats)#endif #ifdef CONFIG_NUMA -/* See __count_vm_event comment on why raw_cpu_inc is used. */ -void __count_numa_event (struct zone *zone, -enum numa_stat_item item) -{ -struct per_cpu_zonestat __percpu *pzstats = zone->per_cpu_zonestats; - -raw_cpu_inc (pzstats->vm_numa_event[item]); -} -/* * Determine the per node value of a stat item. This function * is called frequently in a NUMA machine, so try to be as
Re: [PATCH 04/11] mm/vmstat: Inline NUMA event counter updates
Quoted 20 lines · Vlastimil Babka
Quoted 4 lines · Mel Gorman
__count_numa_eventis small enough to be treated similarly to__count_vm_eventso inline it.Signed-off-by Mel Gorman <[email protected]>Acked-by Vlastimil Babka <[email protected]>Quoted 12 lines
--- include/linux/vmstat.h | 9 +++++++++ mm/vmstat.c | 9 --------- 2 files changed, 9 insertions(+), 9 deletions(-)include/linux/vmstat.h +0
-0@@ -237,6 +237,15 @@ static inline unsigned long zone_page_state_snapshot(struct zone *zone, }
Quoted 11 lines
include/linux/vmstat.h static inline unsigned long zone_page_state_snapshot(struct zone *zone, · line 249
Quoted 11 lines
include/linux/vmstat.h +9
-0#ifdef CONFIG_NUMA +/* See __count_vm_event comment on why raw_cpu_inc is used. */ +static inline void +__count_numa_event (struct zone *zone,enum numa_stat_item item) +{ +struct per_cpu_zonestat __percpu *pzstats = zone->per_cpu_zonestats; + +raw_cpu_inc (pzstats->vm_numa_event[item]); +} +extern void __count_numa_event (struct zone *zone,enum numa_stat_item item);
Ah, but the line above should be removed.
Quoted 24 lines
Quoted 23 lines
include/linux/vmstat.h +0
-0extern unsigned long sum_zone_node_page_state (int node,enum zone_stat_item item);mm/vmstat.c +0
-9@@ -902,15 +902,6 @@ void drain_zonestat(struct zone *zone, struct per_cpu_zonestat *pzstats)#endif #ifdef CONFIG_NUMA -/* See __count_vm_event comment on why raw_cpu_inc is used. */ -void __count_numa_event (struct zone *zone, -enum numa_stat_item item) -{ -struct per_cpu_zonestat __percpu *pzstats = zone->per_cpu_zonestats; - -raw_cpu_inc (pzstats->vm_numa_event[item]); -} -/* * Determine the per node value of a stat item. This function * is called frequently in a NUMA machine, so try to be as
Re: [PATCH 04/11] mm/vmstat: Inline NUMA event counter updates
Quoted 21 lines · Vlastimil Babka
Quoted 20 lines · Vlastimil Babka
Quoted 4 lines · Mel Gorman
__count_numa_eventis small enough to be treated similarly to__count_vm_eventso inline it.Signed-off-by Mel Gorman <[email protected]>Acked-by Vlastimil Babka <[email protected]>Quoted 12 lines
--- include/linux/vmstat.h | 9 +++++++++ mm/vmstat.c | 9 --------- 2 files changed, 9 insertions(+), 9 deletions(-)include/linux/vmstat.h +0
-0@@ -237,6 +237,15 @@ static inline unsigned long zone_page_state_snapshot(struct zone *zone, }
Quoted 14 lines
Quoted 11 lines
include/linux/vmstat.h static inline unsigned long zone_page_state_snapshot(struct zone *zone, · line 249
Quoted 11 lines
include/linux/vmstat.h +9
-0#ifdef CONFIG_NUMA +/* See __count_vm_event comment on why raw_cpu_inc is used. */ +static inline void +__count_numa_event (struct zone *zone,enum numa_stat_item item) +{ +struct per_cpu_zonestat __percpu *pzstats = zone->per_cpu_zonestats; + +raw_cpu_inc (pzstats->vm_numa_event[item]); +} +extern void __count_numa_event (struct zone *zone,enum numa_stat_item item);Ah, but the line above should be removed.
Yes it should!
Signature · 2 lines
Mel Gorman SUSE Labs
PATCH v3 5/11 mm/page_alloc: Batch the accounting updates in the bulk allocator
In reply to Mel GormanNow that the zone_statistics are simple counters that do not require special protection, the bulk allocator accounting updates can be batch updated without adding too much complexity with protected RMW updates or using xchg.
--- include/linux/vmstat.h | 8 ++++++++ mm/page_alloc.c | 30 +++++++++++++----------------- 2 files changed, 21 insertions(+), 17 deletions(-)
include/linux/vmstat.h +8 -0
@@ -246,6 +246,14 @@ __count_numa_event(struct zone *zone, enum numa_stat_item item)raw_cpu_inc (pzstats->vm_numa_event[item]); } +static inline void +__count_numa_events (struct zone *zone,enum numa_stat_item item,long delta) +{ +struct per_cpu_zonestat __percpu *pzstats = zone->per_cpu_zonestats; + +raw_cpu_add (pzstats->vm_numa_event[item], delta); +} +extern void __count_numa_event (struct zone *zone,enum numa_stat_item item);extern unsigned long sum_zone_node_page_state (int node,enum zone_stat_item item);
mm/page_alloc.c +13 -17
@@ -3411,7 +3411,8 @@ void __putback_isolated_page(struct page *page, unsigned int order, int mt)* * Must be called with interrupts disabled. */ -static inline void zone_statistics (struct zone *preferred_zone,struct zone *z) +static inline void zone_statistics (struct zone *preferred_zone,struct zone *z, +long nr_account) {#ifdef CONFIG_NUMA enum numa_stat_item local_stat =NUMA_LOCAL ;
@@ -3424,12 +3425,12 @@ static inline void zone_statistics(struct zone *preferred_zone, struct zone *z) local_stat =NUMA_OTHER ;if (zone_to_nid (z) ==zone_to_nid (preferred_zone)) -__count_numa_event (z,NUMA_HIT ); +__count_numa_events (z,NUMA_HIT , nr_account);else { -__count_numa_event (z,NUMA_MISS ); -__count_numa_event (preferred_zone,NUMA_FOREIGN ); +__count_numa_events (z,NUMA_MISS , nr_account); +__count_numa_events (preferred_zone,NUMA_FOREIGN , nr_account); } -__count_numa_event (z, local_stat); +__count_numa_events (z, local_stat, nr_account);#endif }
@@ -3475,7 +3476,7 @@ static struct page *rmqueue_pcplist(struct zone *preferred_zone, page =__rmqueue_pcplist (zone, migratetype, alloc_flags, pcp, list);if (page) {__count_zid_vm_events (PGALLOC ,page_zonenum (page),1 ); -zone_statistics (preferred_zone, zone); +zone_statistics (preferred_zone, zone,1 ); }local_unlock_irqrestore (&pagesets.lock, flags);return page;
@@ -3536,7 +3537,7 @@ struct page *rmqueue(struct zone *preferred_zone, get_pcppage_migratetype(page));__count_zid_vm_events (PGALLOC ,page_zonenum (page),1 << order); -zone_statistics (preferred_zone, zone); +zone_statistics (preferred_zone, zone,1 );local_irq_restore (flags); out:
@@ -5019,7 +5020,7 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,struct alloc_context ac;gfp_t alloc_gfp;unsigned int alloc_flags =ALLOC_WMARK_LOW ; -int nr_populated =0 ; +int nr_populated =0 , nr_account =0 ;if (unlikely (nr_pages <=0 ))return 0 ;
@@ -5092,15 +5093,7 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid, goto failed_irq;break ; } - -/* -* Ideally this would be batched but the best way to do -* that cheaply is to first convert zone_statistics to -* be inaccurate per-cpu counter like vm_events to avoid -* a RMW cycle then do the accounting with IRQs enabled. -*/ -__count_zid_vm_events (PGALLOC ,zone_idx (zone),1 ); -zone_statistics (ac.preferred_zoneref->zone, zone); + nr_account++;prep_new_page (page,0 , gfp,0 );if (page_list)
@@ -5110,6 +5103,9 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid, nr_populated++; } +__count_zid_vm_events (PGALLOC ,zone_idx (zone), nr_account); +zone_statistics (ac.preferred_zoneref->zone, zone, nr_account); +local_unlock_irqrestore (&pagesets.lock, flags);return nr_populated;
Signature · 1 line
2.26.2
Re: [PATCH 05/11] mm/page_alloc: Batch the accounting updates in the bulk allocator
Quoted 6 lines · Mel Gorman
Now that the
zone_statisticsare simple counters that do not require special protection, the bulk allocator accounting updates can be batch updated without adding too much complexity with protected RMW updates or using xchg.Signed-off-by Mel Gorman <[email protected]>
Quoted 110 lines
--- include/linux/vmstat.h | 8 ++++++++ mm/page_alloc.c | 30 +++++++++++++----------------- 2 files changed, 21 insertions(+), 17 deletions(-)include/linux/vmstat.h +8
-0@@ -246,6 +246,14 @@ __count_numa_event(struct zone *zone, enum numa_stat_item item)raw_cpu_inc (pzstats->vm_numa_event[item]); } +static inline void +__count_numa_events (struct zone *zone,enum numa_stat_item item,long delta) +{ +struct per_cpu_zonestat __percpu *pzstats = zone->per_cpu_zonestats; + +raw_cpu_add (pzstats->vm_numa_event[item], delta); +} +extern void __count_numa_event (struct zone *zone,enum numa_stat_item item);extern unsigned long sum_zone_node_page_state (int node,enum zone_stat_item item);mm/page_alloc.c +13
-17@@ -3411,7 +3411,8 @@ void __putback_isolated_page(struct page *page, unsigned int order, int mt)* * Must be called with interrupts disabled. */ -static inline void zone_statistics (struct zone *preferred_zone,struct zone *z) +static inline void zone_statistics (struct zone *preferred_zone,struct zone *z, +long nr_account) {#ifdef CONFIG_NUMA enum numa_stat_item local_stat =NUMA_LOCAL ;@@ -3424,12 +3425,12 @@ static inline void zone_statistics(struct zone *preferred_zone, struct zone *z) local_stat =NUMA_OTHER ;if (zone_to_nid (z) ==zone_to_nid (preferred_zone)) -__count_numa_event (z,NUMA_HIT ); +__count_numa_events (z,NUMA_HIT , nr_account);else { -__count_numa_event (z,NUMA_MISS ); -__count_numa_event (preferred_zone,NUMA_FOREIGN ); +__count_numa_events (z,NUMA_MISS , nr_account); +__count_numa_events (preferred_zone,NUMA_FOREIGN , nr_account); } -__count_numa_event (z, local_stat); +__count_numa_events (z, local_stat, nr_account);#endif }@@ -3475,7 +3476,7 @@ static struct page *rmqueue_pcplist(struct zone *preferred_zone, page =__rmqueue_pcplist (zone, migratetype, alloc_flags, pcp, list);if (page) {__count_zid_vm_events (PGALLOC ,page_zonenum (page),1 ); -zone_statistics (preferred_zone, zone); +zone_statistics (preferred_zone, zone,1 ); }local_unlock_irqrestore (&pagesets.lock, flags);return page;@@ -3536,7 +3537,7 @@ struct page *rmqueue(struct zone *preferred_zone, get_pcppage_migratetype(page));__count_zid_vm_events (PGALLOC ,page_zonenum (page),1 << order); -zone_statistics (preferred_zone, zone); +zone_statistics (preferred_zone, zone,1 );local_irq_restore (flags); out:@@ -5019,7 +5020,7 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,struct alloc_context ac;gfp_t alloc_gfp;unsigned int alloc_flags =ALLOC_WMARK_LOW ; -int nr_populated =0 ; +int nr_populated =0 , nr_account =0 ;if (unlikely (nr_pages <=0 ))return 0 ;@@ -5092,15 +5093,7 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid, goto failed_irq;break ; } - -/* -* Ideally this would be batched but the best way to do -* that cheaply is to first convert zone_statistics to -* be inaccurate per-cpu counter like vm_events to avoid -* a RMW cycle then do the accounting with IRQs enabled. -*/ -__count_zid_vm_events (PGALLOC ,zone_idx (zone),1 ); -zone_statistics (ac.preferred_zoneref->zone, zone); + nr_account++;prep_new_page (page,0 , gfp,0 );if (page_list)@@ -5110,6 +5103,9 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid, nr_populated++; } +__count_zid_vm_events (PGALLOC ,zone_idx (zone), nr_account); +zone_statistics (ac.preferred_zoneref->zone, zone, nr_account); +local_unlock_irqrestore (&pagesets.lock, flags);return nr_populated;
PATCH v3 6/11 mm/page_alloc: Reduce duration that IRQs are disabled for VM counters
In reply to Mel GormanIRQs are left disabled for the zone and node VM event counters. This is unnecessary as the affected counters are allowed to race for preemmption and IRQs.
This patch reduces the scope of IRQs being disabled
via local_[lock|unlock]_irq on !PREEMPT_RT kernels. One __mod_zone_freepage_state is still called with IRQs disabled. While this could be moved out, it's not free on all architectures as some require IRQs to be disabled for mod_zone_page_state on !PREEMPT_RT kernels.
--- mm/page_alloc.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
mm/page_alloc.c +6 -6
@@ -3474,11 +3474,11 @@ static struct page *rmqueue_pcplist(struct zone *preferred_zone, pcp =this_cpu_ptr (zone->per_cpu_pageset); list = &pcp->lists[migratetype]; page =__rmqueue_pcplist (zone, migratetype, alloc_flags, pcp, list); +local_unlock_irqrestore (&pagesets.lock, flags);if (page) {__count_zid_vm_events (PGALLOC ,page_zonenum (page),1 );zone_statistics (preferred_zone, zone,1 ); } -local_unlock_irqrestore (&pagesets.lock, flags);return page; }
@@ -3530,15 +3530,15 @@ struct page *rmqueue(struct zone *preferred_zone,if (!page) page =__rmqueue (zone, order, migratetype, alloc_flags); }while (page &&check_new_pages (page, order)); -spin_unlock (&zone->lock);if (!page) goto failed; +__mod_zone_freepage_state (zone, -(1 << order),get_pcppage_migratetype (page)); +spin_unlock_irqrestore (&zone->lock, flags);__count_zid_vm_events (PGALLOC ,page_zonenum (page),1 << order);zone_statistics (preferred_zone, zone,1 ); -local_irq_restore (flags); out:/* Separate test+clear to avoid unnecessary atomics */
@@ -3551,7 +3551,7 @@ struct page *rmqueue(struct zone *preferred_zone,return page; failed: -local_irq_restore (flags); +spin_unlock_irqrestore (&zone->lock, flags);return NULL ; }
@@ -5103,11 +5103,11 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid, nr_populated++; } +local_unlock_irqrestore (&pagesets.lock, flags); +__count_zid_vm_events (PGALLOC ,zone_idx (zone), nr_account);zone_statistics (ac.preferred_zoneref->zone, zone, nr_account); -local_unlock_irqrestore (&pagesets.lock, flags); -return nr_populated; failed_irq:
Signature · 1 line
2.26.2
Re: [PATCH 06/11] mm/page_alloc: Reduce duration that IRQs are disabled for VM counters
Quoted 11 lines · Mel Gorman
IRQs are left disabled for the zone and node VM event counters. This is unnecessary as the affected counters are allowed to race for preemmption and IRQs.
This patch reduces the scope of IRQs being disabled
via local_[lock|unlock]_irq on !PREEMPT_RT kernels. One__mod_zone_freepage_stateis still called with IRQs disabled. While this could be moved out, it's not free on all architectures as some require IRQs to be disabled formod_zone_page_stateon !PREEMPT_RT kernels.Signed-off-by Mel Gorman <[email protected]>
Nice!
Quoted 63 lines
--- mm/page_alloc.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)mm/page_alloc.c +6
-6@@ -3474,11 +3474,11 @@ static struct page *rmqueue_pcplist(struct zone *preferred_zone, pcp =this_cpu_ptr (zone->per_cpu_pageset); list = &pcp->lists[migratetype]; page =__rmqueue_pcplist (zone, migratetype, alloc_flags, pcp, list); +local_unlock_irqrestore (&pagesets.lock, flags);if (page) {__count_zid_vm_events (PGALLOC ,page_zonenum (page),1 );zone_statistics (preferred_zone, zone,1 ); } -local_unlock_irqrestore (&pagesets.lock, flags);return page; }@@ -3530,15 +3530,15 @@ struct page *rmqueue(struct zone *preferred_zone,if (!page) page =__rmqueue (zone, order, migratetype, alloc_flags); }while (page &&check_new_pages (page, order)); -spin_unlock (&zone->lock);if (!page) goto failed; +__mod_zone_freepage_state (zone, -(1 << order),get_pcppage_migratetype (page)); +spin_unlock_irqrestore (&zone->lock, flags);__count_zid_vm_events (PGALLOC ,page_zonenum (page),1 << order);zone_statistics (preferred_zone, zone,1 ); -local_irq_restore (flags); out:/* Separate test+clear to avoid unnecessary atomics */ @@ -3551,7 +3551,7 @@ struct page *rmqueue(struct zone *preferred_zone,return page; failed: -local_irq_restore (flags); +spin_unlock_irqrestore (&zone->lock, flags);return NULL ; }@@ -5103,11 +5103,11 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid, nr_populated++; } +local_unlock_irqrestore (&pagesets.lock, flags); +__count_zid_vm_events (PGALLOC ,zone_idx (zone), nr_account);zone_statistics (ac.preferred_zoneref->zone, zone, nr_account); -local_unlock_irqrestore (&pagesets.lock, flags); -return nr_populated; failed_irq:
PATCH v3 7/11 mm/page_alloc: Remove duplicate checks if migratetype should be isolated
In reply to Mel GormanBoth free_pcppages_bulk() and free_one_page() have very similar checks about whether a page's migratetype has changed under the zone lock. Use a common helper.
--- mm/page_alloc.c | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-)
mm/page_alloc.c +22 -10
@@ -1354,6 +1354,23 @@ static inline void prefetch_buddy(struct page *page)prefetch (buddy); } +/* +* The migratetype of a page may have changed due to isolation so check. +* Assumes the caller holds the zone->lock to serialise against page +* isolation. +*/ +static inline int +check_migratetype_isolated (struct zone *zone,struct page *page,unsigned long pfn,int migratetype) +{ +/* If isolating, check if the migratetype has changed */ +if (unlikely (has_isolate_pageblock (zone) || +is_migrate_isolate (migratetype))) { + migratetype =get_pfnblock_migratetype (page, pfn); + } + +return migratetype; +} +/* * Frees a number of pages from the PCP lists * Assumes all pages on list are in same zone, and of same order.
@@ -1371,7 +1388,6 @@ static void free_pcppages_bulk(struct zone *zone, int count,int migratetype =0 ;int batch_free =0 ;int prefetch_nr =READ_ONCE (pcp->batch); -bool isolated_pageblocks;struct page *page, *tmp;LIST_HEAD (head);
@@ -1433,21 +1449,20 @@ static void free_pcppages_bulk(struct zone *zone, int count,* both PREEMPT_RT and non-PREEMPT_RT configurations. */ spin_lock (&zone->lock); - isolated_pageblocks =has_isolate_pageblock (zone);/* * Use safe version since after __free_one_page(), * page->lru.next will not point to original list. */ list_for_each_entry_safe (page, tmp, &head, lru) { +unsigned long pfn =page_to_pfn (page);int mt =get_pcppage_migratetype (page); +/* MIGRATE_ISOLATE page should not go to pcplists */ VM_BUG_ON_PAGE (is_migrate_isolate (mt), page); -/* Pageblock could have been isolated meanwhile */ -if (unlikely (isolated_pageblocks)) - mt =get_pageblock_migratetype (page); -__free_one_page (page,page_to_pfn (page), zone,0 , mt,FPI_NONE ); + mt =check_migratetype_isolated (zone, page, pfn, mt); +__free_one_page (page, pfn, zone,0 , mt,FPI_NONE );trace_mm_page_pcpu_drain (page,0 , mt); }spin_unlock (&zone->lock);
@@ -1459,10 +1474,7 @@ static void free_one_page(struct zone *zone, int migratetype, fpi_t fpi_flags) {spin_lock (&zone->lock); -if (unlikely (has_isolate_pageblock (zone) || -is_migrate_isolate (migratetype))) { - migratetype =get_pfnblock_migratetype (page, pfn); - } + migratetype =check_migratetype_isolated (zone, page, pfn, migratetype);__free_one_page (page, pfn, zone, order, migratetype, fpi_flags);spin_unlock (&zone->lock); }
Signature · 1 line
2.26.2
Re: [PATCH 07/11] mm/page_alloc: Remove duplicate checks if migratetype should be isolated
Quoted 5 lines · Mel Gorman
Both
free_pcppages_bulk()andfree_one_page()have very similar checks about whether a page's migratetype has changed under the zone lock. Use a common helper.Signed-off-by Mel Gorman <[email protected]>
Seems like for free_pcppages_bulk() this patch makes it check for each page on the pcplist
- zone->nr_isolate_pageblock != 0 instead of local bool (the performance might be the same I guess on modern cpu though)
- is_migrate_isolate(migratetype) for a migratetype obtained by get_pcppage_migratetype() which cannot be migrate_isolate so the check is useless.
As such it doesn't seem a worthwhile cleanup to me considering all the other microoptimisations?
Quoted 79 lines
--- mm/page_alloc.c | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-)mm/page_alloc.c +22
-10@@ -1354,6 +1354,23 @@ static inline void prefetch_buddy(struct page *page)prefetch (buddy); } +/* +* The migratetype of a page may have changed due to isolation so check. +* Assumes the caller holds the zone->lock to serialise against page +* isolation. +*/ +static inline int +check_migratetype_isolated (struct zone *zone,struct page *page,unsigned long pfn,int migratetype) +{ +/* If isolating, check if the migratetype has changed */ +if (unlikely (has_isolate_pageblock (zone) || +is_migrate_isolate (migratetype))) { + migratetype =get_pfnblock_migratetype (page, pfn); + } + +return migratetype; +} +/* * Frees a number of pages from the PCP lists * Assumes all pages on list are in same zone, and of same order. @@ -1371,7 +1388,6 @@ static void free_pcppages_bulk(struct zone *zone, int count,int migratetype =0 ;int batch_free =0 ;int prefetch_nr =READ_ONCE (pcp->batch); -bool isolated_pageblocks;struct page *page, *tmp;LIST_HEAD (head);@@ -1433,21 +1449,20 @@ static void free_pcppages_bulk(struct zone *zone, int count,* both PREEMPT_RT and non-PREEMPT_RT configurations. */ spin_lock (&zone->lock); - isolated_pageblocks =has_isolate_pageblock (zone);/* * Use safe version since after __free_one_page(), * page->lru.next will not point to original list. */ list_for_each_entry_safe (page, tmp, &head, lru) { +unsigned long pfn =page_to_pfn (page);int mt =get_pcppage_migratetype (page); +/* MIGRATE_ISOLATE page should not go to pcplists */ VM_BUG_ON_PAGE (is_migrate_isolate (mt), page); -/* Pageblock could have been isolated meanwhile */ -if (unlikely (isolated_pageblocks)) - mt =get_pageblock_migratetype (page); -__free_one_page (page,page_to_pfn (page), zone,0 , mt,FPI_NONE ); + mt =check_migratetype_isolated (zone, page, pfn, mt); +__free_one_page (page, pfn, zone,0 , mt,FPI_NONE );trace_mm_page_pcpu_drain (page,0 , mt); }spin_unlock (&zone->lock);@@ -1459,10 +1474,7 @@ static void free_one_page(struct zone *zone, int migratetype, fpi_t fpi_flags) {spin_lock (&zone->lock); -if (unlikely (has_isolate_pageblock (zone) || -is_migrate_isolate (migratetype))) { - migratetype =get_pfnblock_migratetype (page, pfn); - } + migratetype =check_migratetype_isolated (zone, page, pfn, migratetype);__free_one_page (page, pfn, zone, order, migratetype, fpi_flags);spin_unlock (&zone->lock); }
Re: [PATCH 07/11] mm/page_alloc: Remove duplicate checks if migratetype should be isolated
Quoted 14 lines · Vlastimil Babka
Quoted 5 lines · Mel Gorman
Both
free_pcppages_bulk()andfree_one_page()have very similar checks about whether a page's migratetype has changed under the zone lock. Use a common helper.Signed-off-by Mel Gorman <[email protected]>Seems like for
free_pcppages_bulk()this patch makes it check for each page on the pcplist
-zone->nr_isolate_pageblock!= 0 instead of local bool (the performance might be the same I guess on modern cpu though)
-is_migrate_isolate(migratetype)for a migratetype obtained byget_pcppage_migratetype()which cannot bemigrate_isolateso the check is useless.
Quoted 3 lines
As such it doesn't seem a worthwhile cleanup to me considering all the other microoptimisations?
The patch was a preparation patch for the rest of the series to avoid code duplication and to consolidate checks together in one place to determine if they are even correct.
Until zone_pcp_disable() came along, it was possible to have isolated PCP pages in the lists even though zone->nr_isolate_pageblock could be 0 during memory hot-remove so the split in free_pcppages_bulk was not necessarily correct at all times.
The remaining problem is alloc_contig_pages, it does not disable PCPs so both checks are necessary. If that also disabled PCPs then check_migratetype_isolated could be deleted but the cost to alloc_contig_pages might be too high.
I'll delete this patch for now because it's relatively minor and there should be other ways of keeping the code duplication down.
Signature · 2 lines
Mel Gorman SUSE Labs
Re: [PATCH 07/11] mm/page_alloc: Remove duplicate checks if migratetype should be isolated
Quoted 28 lines · Mel Gorman
Quoted 14 lines · Vlastimil Babka
Quoted 5 lines · Mel Gorman
Both
free_pcppages_bulk()andfree_one_page()have very similar checks about whether a page's migratetype has changed under the zone lock. Use a common helper.Signed-off-by Mel Gorman <[email protected]>Seems like for
free_pcppages_bulk()this patch makes it check for each page on the pcplist
-zone->nr_isolate_pageblock!= 0 instead of local bool (the performance might be the same I guess on modern cpu though)
-is_migrate_isolate(migratetype)for a migratetype obtained byget_pcppage_migratetype()which cannot bemigrate_isolateso the check is useless.Quoted 3 lines
As such it doesn't seem a worthwhile cleanup to me considering all the other microoptimisations?
The patch was a preparation patch for the rest of the series to avoid code duplication and to consolidate checks together in one place to determine if they are even correct.
Until
zone_pcp_disable()came along, it was possible to have isolated PCP pages in the lists even thoughzone->nr_isolate_pageblockcould be 0 during memory hot-remove so the split infree_pcppages_bulkwas not necessarily correct at all times.
Quoted 4 lines
The remaining problem is
alloc_contig_pages, it does not disable PCPs so both checks are necessary. If that also disabled PCPs thencheck_migratetype_isolatedcould be deleted but the cost toalloc_contig_pagesmight be too high.
I see. Well that's unfortunate if checking zone->nr_isolate_pageblock is not sufficient, as it was supposed to be :(
But I don't think the check_migratetype_isolated() check was helping in such scenario as it was, anyway.
It's testing this:
+ if (unlikely(has_isolate_pageblock(zone) ||
+ is_migrate_isolate(migratetype))) {
In the context of free_one_page(), the 'migratetype' variable holds a value that's read from pageblock in one of the callers of free_one_page():
migratetype = get_pcppage_migratetype(page);
and because it's read outside of zone lock, it might be a MIGRATE_ISOLATE even though after we obtain the zone lock, we might find out it's not anymore. This is explained in commit ad53f92eb416 ("mm/page_alloc: fix incorrect isolation behavior by rechecking migratetype") as scenario 1.
However, in the context of free_pcppages_bulk(), the migratetype we are checking in check_migratetype_isolated() is this one:
int mt = get_pcppage_migratetype(page);
That was the one determined while adding the page to pcplist, and is stored in the struct page and we know it's not MIGRATE_ISOLATE otherwise the page would not go to pcplist. But by rechecking this stored value, we would not be finding the case where the underlying pageblock's migratetype would change to MIGRATE_ISOLATE, anyway...
Quoted 3 lines
I'll delete this patch for now because it's relatively minor and there should be other ways of keeping the code duplication down.
PATCH v3 8/11 mm/page_alloc: Explicitly acquire the zone lock in __free_pages_ok
In reply to Mel Gorman__free_pages_ok() disables IRQs before calling a common helper free_one_page() that acquires the zone lock. This is not safe according to Documentation/locking/locktypes.rst and in this context, IRQ disabling is not protecting a per_cpu_pages structure either or a local_lock would be used.
This patch explicitly acquires the lock with spin_lock_irqsave instead of relying on a helper. This removes the last instance of local_irq_save() in page_alloc.c.
--- mm/page_alloc.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
mm/page_alloc.c +5 -8
@@ -1559,21 +1559,18 @@ static void __free_pages_ok(struct page *page, unsigned int order,unsigned long flags;int migratetype;unsigned long pfn =page_to_pfn (page); +struct zone *zone =page_zone (page);if (!free_pages_prepare (page, order, true))return ; migratetype =get_pfnblock_migratetype (page, pfn); -/* -* TODO FIX: Disable IRQs before acquiring IRQ-safe zone->lock -* and protect vmstat updates. -*/ -local_irq_save (flags); +spin_lock_irqsave (&zone->lock, flags);__count_vm_events (PGFREE ,1 << order); -free_one_page (page_zone (page), page, pfn, order, migratetype, - fpi_flags); -local_irq_restore (flags); + migratetype =check_migratetype_isolated (zone, page, pfn, migratetype); +__free_one_page (page, pfn, zone, order, migratetype, fpi_flags); +spin_unlock_irqrestore (&zone->lock, flags); }void __free_pages_core (struct page *page,unsigned int order)
Signature · 1 line
2.26.2
Re: [PATCH 08/11] mm/page_alloc: Explicitly acquire the zone lock in __free_pages_ok
Quoted 9 lines · Mel Gorman
__free_pages_ok()disables IRQs before calling a common helperfree_one_page()that acquires the zone lock. This is not safe according toDocumentation/locking/locktypes.rstand in this context, IRQ disabling is not protecting aper_cpu_pagesstructure either or alocal_lockwould be used.This patch explicitly acquires the lock with
spin_lock_irqsaveinstead of relying on a helper. This removes the last instance oflocal_irq_save()inpage_alloc.c.
\o/
Quoted 1 line
Signed-off-by Mel Gorman <[email protected]>
Quoted 36 lines
--- mm/page_alloc.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)mm/page_alloc.c +5
-8@@ -1559,21 +1559,18 @@ static void __free_pages_ok(struct page *page, unsigned int order,unsigned long flags;int migratetype;unsigned long pfn =page_to_pfn (page); +struct zone *zone =page_zone (page);if (!free_pages_prepare (page, order, true))return ; migratetype =get_pfnblock_migratetype (page, pfn); -/* -* TODO FIX: Disable IRQs before acquiring IRQ-safe zone->lock -* and protect vmstat updates. -*/ -local_irq_save (flags); +spin_lock_irqsave (&zone->lock, flags);__count_vm_events (PGFREE ,1 << order); -free_one_page (page_zone (page), page, pfn, order, migratetype, - fpi_flags); -local_irq_restore (flags); + migratetype =check_migratetype_isolated (zone, page, pfn, migratetype); +__free_one_page (page, pfn, zone, order, migratetype, fpi_flags); +spin_unlock_irqrestore (&zone->lock, flags); }void __free_pages_core (struct page *page,unsigned int order)
PATCH v3 9/11 mm/page_alloc: Avoid conflating IRQs disabled with zone->lock
In reply to Mel GormanHistorically when freeing pages, free_one_page() assumed that callers had IRQs disabled and the zone->lock could be acquired with spin_lock(). This confuses the scope of what local_lock_irq is protecting and what zone->lock is protecting in free_unref_page_list in particular.
This patch uses spin_lock_irqsave() for the zone->lock in free_one_page() instead of relying on callers to have disabled IRQs. free_unref_page_commit() is changed to only deal with PCP pages protected by the local lock. free_unref_page_list() then first frees isolated pages to the buddy lists with free_one_page() and frees the rest of the pages to the PCP via free_unref_page_commit(). The end result is that free_one_page() is no longer depending on side-effects of local_lock to be correct.
Note that this may incur a performance penalty while memory hot-remove is running but that is not a common operation.
--- mm/page_alloc.c | 67 ++++++++++++++++++++++++++++++------------------- 1 file changed, 41 insertions(+), 26 deletions(-)
mm/page_alloc.c +41 -26
@@ -1473,10 +1473,12 @@ static void free_one_page(struct zone *zone,unsigned int order, int migratetype, fpi_t fpi_flags) { - spin_lock(&zone->lock); + unsigned long flags; + +spin_lock_irqsave (&zone->lock, flags); migratetype =check_migratetype_isolated (zone, page, pfn, migratetype);__free_one_page (page, pfn, zone, order, migratetype, fpi_flags); -spin_unlock (&zone->lock); +spin_unlock_irqrestore (&zone->lock, flags); } static void __meminit __init_single_page(struct page *page, unsigned long pfn,
@@ -3238,31 +3240,13 @@ static bool free_unref_page_prepare(struct page *page, unsigned long pfn)return true; } -static void free_unref_page_commit (struct page *page,unsigned long pfn) +static void free_unref_page_commit (struct page *page,unsigned long pfn, +int migratetype) {struct zone *zone =page_zone (page);struct per_cpu_pages *pcp; -int migratetype; - migratetype =get_pcppage_migratetype (page);__count_vm_event (PGFREE ); - -/* -* We only track unmovable, reclaimable and movable on pcp lists. -* Free ISOLATE pages back to the allocator because they are being -* offlined but treat HIGHATOMIC as movable pages so we can get those -* areas back if necessary. Otherwise, we may have to free -* excessively into the page allocator -*/ -if (migratetype >=MIGRATE_PCPTYPES ) { -if (unlikely (is_migrate_isolate (migratetype))) { -free_one_page (zone, page, pfn,0 , migratetype, -FPI_NONE ); -return ; - } - migratetype =MIGRATE_MOVABLE ; - } - pcp =this_cpu_ptr (zone->per_cpu_pageset);list_add (&page->lru, &pcp->lists[migratetype]); pcp->count++;
@@ -3277,12 +3261,29 @@ void free_unref_page(struct page *page) {unsigned long flags;unsigned long pfn =page_to_pfn (page); +int migratetype;if (!free_unref_page_prepare (page, pfn))return ; +/* +* We only track unmovable, reclaimable and movable on pcp lists. +* Place ISOLATE pages on the isolated list because they are being +* offlined but treat HIGHATOMIC as movable pages so we can get those +* areas back if necessary. Otherwise, we may have to free +* excessively into the page allocator +*/ + migratetype =get_pcppage_migratetype (page); +if (unlikely (migratetype >=MIGRATE_PCPTYPES )) { +if (unlikely (is_migrate_isolate (migratetype))) { +free_one_page (page_zone (page), page, pfn,0 , migratetype,FPI_NONE ); +return ; + } + migratetype =MIGRATE_MOVABLE ; + } +local_lock_irqsave (&pagesets.lock, flags); -free_unref_page_commit (page, pfn); +free_unref_page_commit (page, pfn, migratetype);local_unlock_irqrestore (&pagesets.lock, flags); }
@@ -3294,6 +3295,7 @@ void free_unref_page_list(struct list_head *list)struct page *page, *next;unsigned long flags, pfn;int batch_count =0 ; +int migratetype;/* Prepare pages for freeing */ list_for_each_entry_safe (page, next, list, lru) {
@@ -3301,15 +3303,28 @@ void free_unref_page_list(struct list_head *list)if (!free_unref_page_prepare (page, pfn))list_del (&page->lru);set_page_private (page, pfn); + +/* +* Free isolated pages directly to the allocator, see +* comment in free_unref_page. +*/ + migratetype =get_pcppage_migratetype (page); +if (unlikely (migratetype >=MIGRATE_PCPTYPES )) { +if (unlikely (is_migrate_isolate (migratetype))) { +free_one_page (page_zone (page), page, pfn,0 , + migratetype,FPI_NONE ); +list_del (&page->lru); + } + } }local_lock_irqsave (&pagesets.lock, flags);list_for_each_entry_safe (page, next, list, lru) { -unsigned long pfn =page_private (page); - + pfn =page_private (page);set_page_private (page,0 ); + migratetype =get_pcppage_migratetype (page);trace_mm_page_free_batched (page); -free_unref_page_commit (page, pfn); +free_unref_page_commit (page, pfn, migratetype);/* * Guard against excessive IRQ disabled times when we get
Signature · 1 line
2.26.2
Re: [PATCH 09/11] mm/page_alloc: Avoid conflating IRQs disabled with zone->lock
Quoted 14 lines · Mel Gorman
Historically when freeing pages,
free_one_page()assumed that callers had IRQs disabled and thezone->lockcould be acquired withspin_lock(). This confuses the scope of whatlocal_lock_irqis protecting and whatzone->lockis protecting infree_unref_page_listin particular.This patch uses
spin_lock_irqsave()for thezone->lockinfree_one_page()instead of relying on callers to have disabled IRQs.free_unref_page_commit()is changed to only deal with PCP pages protected by the local lock.free_unref_page_list()then first frees isolated pages to the buddy lists withfree_one_page()and frees the rest of the pages to the PCP viafree_unref_page_commit(). The end result is thatfree_one_page()is no longer depending on side-effects oflocal_lockto be correct.
Quoted 4 lines
Note that this may incur a performance penalty while memory hot-remove is running but that is not a common operation.
Signed-off-by Mel Gorman <[email protected]>
A nit below:
mm/page_alloc.c void free_unref_page_list(struct list_head *list) · line 3305
Quoted 12 lines
mm/page_alloc.c +1
-0@@ -3294,6 +3295,7 @@ void free_unref_page_list(struct list_head *list)struct page *page, *next;unsigned long flags, pfn;int batch_count =0 ; +int migratetype;/* Prepare pages for freeing */ list_for_each_entry_safe (page, next, list, lru) {@@ -3301,15 +3303,28 @@ void free_unref_page_list(struct list_head *list)if (!free_unref_page_prepare (page, pfn))list_del (&page->lru);set_page_private (page, pfn);
Should probably move this below so we don't set private for pages that then go through free_one_page()? Doesn't seem to be a bug, just unneccessary.
Quoted 29 lines
mm/page_alloc.c +16
-3+ +/* +* Free isolated pages directly to the allocator, see +* comment in free_unref_page. +*/ + migratetype =get_pcppage_migratetype (page); +if (unlikely (migratetype >=MIGRATE_PCPTYPES )) { +if (unlikely (is_migrate_isolate (migratetype))) { +free_one_page (page_zone (page), page, pfn,0 , + migratetype,FPI_NONE ); +list_del (&page->lru); + } + } }local_lock_irqsave (&pagesets.lock, flags);list_for_each_entry_safe (page, next, list, lru) { -unsigned long pfn =page_private (page); - + pfn =page_private (page);set_page_private (page,0 ); + migratetype =get_pcppage_migratetype (page);trace_mm_page_free_batched (page); -free_unref_page_commit (page, pfn); +free_unref_page_commit (page, pfn, migratetype);/* * Guard against excessive IRQ disabled times when we get
Re: [PATCH 09/11] mm/page_alloc: Avoid conflating IRQs disabled with zone->lock
Quoted 6 lines · Vlastimil Babka
Quoted 6 lines
(quoted diff) +1
-0@@ -3294,6 +3295,7 @@ void free_unref_page_list(struct list_head *list) struct page *page, *next; unsigned long flags, pfn; int batch_count = 0; + int migratetype;
Quoted 10 lines
Review void free_unref_page_list(struct list_head *list) · line 3305
Quoted 6 lines
(quoted diff) +0
-0/* Prepare pages for freeing */ list_for_each_entry_safe(page, next, list, lru) {@@ -3301,15 +3303,28 @@ void free_unref_page_list(struct list_head *list) if (!free_unref_page_prepare(page, pfn)) list_del(&page->lru); set_page_private(page, pfn);Should probably move this below so we don't set private for pages that then go through
free_one_page()? Doesn't seem to be a bug, just unneccessary.
Sure.
mm/page_alloc.c +2 -1
@@ -3293,7 +3293,6 @@ void free_unref_page_list(struct list_head *list) pfn =page_to_pfn (page);if (!free_unref_page_prepare (page, pfn))list_del (&page->lru); -set_page_private (page, pfn);/* * Free isolated pages directly to the allocator, see
@@ -3307,6 +3306,8 @@ void free_unref_page_list(struct list_head *list)list_del (&page->lru); } } + +set_page_private (page, pfn); }local_lock_irqsave (&pagesets.lock, flags);
Signature · 2 lines
Mel Gorman SUSE Labs
PATCH v3 10/11 mm/page_alloc: Update PGFREE outside the zone lock in __free_pages_ok
In reply to Mel GormanVM events do not need explicit protection by disabling IRQs so update the counter with IRQs enabled in __free_pages_ok.
--- mm/page_alloc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
mm/page_alloc.c +2 -1
@@ -1569,10 +1569,11 @@ static void __free_pages_ok(struct page *page, unsigned int order, migratetype =get_pfnblock_migratetype (page, pfn);spin_lock_irqsave (&zone->lock, flags); -__count_vm_events (PGFREE ,1 << order); migratetype =check_migratetype_isolated (zone, page, pfn, migratetype);__free_one_page (page, pfn, zone, order, migratetype, fpi_flags);spin_unlock_irqrestore (&zone->lock, flags); + +__count_vm_events (PGFREE ,1 << order); }void __free_pages_core (struct page *page,unsigned int order)
Signature · 1 line
2.26.2
Re: [PATCH 10/11] mm/page_alloc: Update PGFREE outside the zone lock in __free_pages_ok
Quoted 4 lines · Mel Gorman
VM events do not need explicit protection by disabling IRQs so update the counter with IRQs enabled in
__free_pages_ok.Signed-off-by Mel Gorman <[email protected]>
Quoted 22 lines
--- mm/page_alloc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)mm/page_alloc.c +2
-1@@ -1569,10 +1569,11 @@ static void __free_pages_ok(struct page *page, unsigned int order, migratetype =get_pfnblock_migratetype (page, pfn);spin_lock_irqsave (&zone->lock, flags); -__count_vm_events (PGFREE ,1 << order); migratetype =check_migratetype_isolated (zone, page, pfn, migratetype);__free_one_page (page, pfn, zone, order, migratetype, fpi_flags);spin_unlock_irqrestore (&zone->lock, flags); + +__count_vm_events (PGFREE ,1 << order); }void __free_pages_core (struct page *page,unsigned int order)
PATCH v3 11/11 mm/page_alloc: Embed per_cpu_pages locking within the per-cpu structure
In reply to Mel Gormanstruct per_cpu_pages is protected by the pagesets lock but it can be embedded within struct per_cpu_pages at a minor cost. This is possible because per-cpu lookups are based on offsets. Paraphrasing an explanation from Peter Ziljstra
The whole thing relies on:
&per_cpu_ptr(msblk->stream, cpu)->lock == per_cpu_ptr(&msblk->stream->lock, cpu)
Which is true because the lhs:
(local_lock_t *)((zone->per_cpu_pages + per_cpu_offset(cpu)) + offsetof(struct per_cpu_pages, lock))
and the rhs:
(local_lock_t *)((zone->per_cpu_pages + offsetof(struct per_cpu_pages, lock)) + per_cpu_offset(cpu))
are identical, because addition is associative.
More details are included in mmzone.h. This embedding is not completely free for three reasons.
1. As local_lock does not return a per-cpu structure, the PCP has to be looked up twice -- first to acquire the lock and again to get the PCP pointer.
2. For PREEMPT_RT and CONFIG_DEBUG_LOCK_ALLOC, local_lock is potentially a spinlock or has lock-specific tracking. In both cases, it becomes necessary to release/acquire different locks when freeing a list of pages in free_unref_page_list.
3. For most kernel configurations, local_lock_t is empty and no storage is required. By embedding the lock, the memory consumption on PREEMPT_RT and CONFIG_DEBUG_LOCK_ALLOC is higher.
--- include/linux/mmzone.h | 31 ++++++++++++++++- mm/page_alloc.c | 78 +++++++++++++++++++++++++++--------------- 2 files changed, 81 insertions(+), 28 deletions(-)
include/linux/mmzone.h +30 -1
@@ -338,8 +338,37 @@ enum zone_watermarks {#define high_wmark_pages (z) (z->_watermark[WMARK_HIGH] + z->watermark_boost)#define wmark_pages (z, i) (z->_watermark[i] + z->watermark_boost) -/* Fields and list protected by pagesets local_lock in page_alloc.c */ +/* +* struct per_cpu_pages is a per-cpu structure protected by a lock +* embedded within the structure. This is subtle because ordinarily +* that would imply the PCP structure must first be located when +* the task is not yet pinned to the CPU e.g. +* +* migrate_disable(); +* pcp = this_cpu_ptr(zone->per_cpu_pageset); +* local_lock_irqsave(&pcp->lock, flags); +* +* However, because per-cpu addresses are calculated based on offsets, +* the following is true +* +* &per_cpu_ptr(zone->per_cpu_pageset, cpu)->lock == per_cpu_ptr(&zone->per_cpu_pageset->lock, cpu) +* +* This is true because the LHS is +* +* (local_lock_t *)((zone->per_cpu_pageset + per_cpu_offset(cpu)) + offsetof(struct per_cpu_pageset, lock)) +* +* while the RHS is +* +* (local_lock_t *)((zone->per_cpu_pageset + offsetof(struct per_cpu_pageset, lock)) + per_cpu_offset(cpu)) +* +* local_lock will first disable preempt, migration or IRQs depending on +* the kernel configuration prior to the per_cpu_offset is calculated. +* Hence, the following is safe for both PREEMPT_RT and !PREEMPT_RT. +* +* local_lock_irqsave(&zone->per_cpu_pageset->lock, flags); +*/ struct per_cpu_pages { +local_lock_t lock;int count;/* number of pages in the list */ int high;/* high watermark, emptying needed */ int batch;/* chunk size for buddy add/remove */
mm/page_alloc.c +51 -27
188 lines not on this page · raw message
Signature · 1 line
2.26.2
Re: [PATCH 11/11] mm/page_alloc: Embed per_cpu_pages locking within the per-cpu structure
Quoted 22 lines · Mel Gorman
struct
per_cpu_pagesis protected by the pagesets lock but it can be embedded within structper_cpu_pagesat a minor cost. This is possible because per-cpu lookups are based on offsets. Paraphrasing an explanation from Peter ZiljstraThe whole thing relies on:
&per_cpu_ptr(msblk->stream, cpu)->lock == per_cpu_ptr(&msblk->stream->lock, cpu)Which is true because the lhs:
(local_lock_t *)((zone->per_cpu_pages + per_cpu_offset(cpu)) + offsetof(struct per_cpu_pages, lock))and the rhs:
(local_lock_t *)((zone->per_cpu_pages + offsetof(struct per_cpu_pages, lock)) + per_cpu_offset(cpu))are identical, because addition is associative.
More details are included in
mmzone.h. This embedding is not completely free for three reasons.
Quoted 8 lines
1. As
local_lockdoes not return a per-cpu structure, the PCP has to be looked up twice -- first to acquire the lock and again to get the PCP pointer.2. For
PREEMPT_RTandCONFIG_DEBUG_LOCK_ALLOC,local_lockis potentially a spinlock or has lock-specific tracking. In both cases, it becomes necessary to release/acquire different locks when freeing a list of pages infree_unref_page_list.
Looks like this pattern could benefit from a local_lock API helper that would do the right thing? It probably couldn't optimize much the CONFIG_PREEMPT_RT case which would need to be unlock/lock in any case, but CONFIG_DEBUG_LOCK_ALLOC could perhaps just keep the IRQ's disabled and just note the change of what's acquired?
Quoted 3 lines
3. For most kernel configurations,
local_lock_tis empty and no storage is required. By embedding the lock, the memory consumption onPREEMPT_RTandCONFIG_DEBUG_LOCK_ALLOCis higher.
But I wonder, is there really a benefit to this increased complexity? Before the patch we had "pagesets" - a local_lock that protects all zones' pcplists. Now each zone's pcplists have own local_lock. On !PREEMPT_RT we will never take the locks of multiple zones from the same CPU in parallel, because we use local_lock_irqsave(). Can that parallelism happen on PREEMPT_RT, because that could perhaps justify the change?
Quoted 3 lines
Suggested-by Peter Zijlstra <[email protected]>Signed-off-by Mel Gorman <[email protected]>---
Re: [PATCH 11/11] mm/page_alloc: Embed per_cpu_pages locking within the per-cpu structure
Quoted 32 lines · Vlastimil Babka
Quoted 22 lines · Mel Gorman
struct
per_cpu_pagesis protected by the pagesets lock but it can be embedded within structper_cpu_pagesat a minor cost. This is possible because per-cpu lookups are based on offsets. Paraphrasing an explanation from Peter ZiljstraThe whole thing relies on:
&per_cpu_ptr(msblk->stream, cpu)->lock == per_cpu_ptr(&msblk->stream->lock, cpu)Which is true because the lhs:
(local_lock_t *)((zone->per_cpu_pages + per_cpu_offset(cpu)) + offsetof(struct per_cpu_pages, lock))and the rhs:
(local_lock_t *)((zone->per_cpu_pages + offsetof(struct per_cpu_pages, lock)) + per_cpu_offset(cpu))are identical, because addition is associative.
More details are included in
mmzone.h. This embedding is not completely free for three reasons.Quoted 8 lines
1. As
local_lockdoes not return a per-cpu structure, the PCP has to be looked up twice -- first to acquire the lock and again to get the PCP pointer.2. For
PREEMPT_RTandCONFIG_DEBUG_LOCK_ALLOC,local_lockis potentially a spinlock or has lock-specific tracking. In both cases, it becomes necessary to release/acquire different locks when freeing a list of pages infree_unref_page_list.
Quoted 6 lines
Looks like this pattern could benefit from a
local_lockAPI helper that would do the right thing? It probably couldn't optimize much theCONFIG_PREEMPT_RTcase which would need to be unlock/lock in any case, butCONFIG_DEBUG_LOCK_ALLOCcould perhaps just keep the IRQ's disabled and just note the change of what's acquired?
A helper could potentially be used but right now, there is only one call-site that needs this type of care so it may be overkill. A helper was proposed that can lookup and lock a per-cpu structure which is generally useful but does not suit the case where different locks need to be acquired.
Quoted 11 lines
Quoted 3 lines
3. For most kernel configurations,
local_lock_tis empty and no storage is required. By embedding the lock, the memory consumption onPREEMPT_RTandCONFIG_DEBUG_LOCK_ALLOCis higher.But I wonder, is there really a benefit to this increased complexity? Before the patch we had "pagesets" - a
local_lockthat protects all zones' pcplists. Now each zone's pcplists have ownlocal_lock. On !PREEMPT_RT we will never take the locks of multiple zones from the same CPU in parallel, because we uselocal_lock_irqsave(). Can that parallelism happen onPREEMPT_RT, because that could perhaps justify the change?
I don't think PREEMPT_RT gets additional parallelism because it's still a per-cpu structure that is being protected. The difference is whether we are protecting the CPU-N index for all per_cpu_pages or just one. The patch exists because it was asked why the lock was not embedded within the structure it's protecting. I initially thought that was unsafe and I was wrong as explained in the changelog. But now that I find it *can* be done but it's a bit ugly so I put it at the end of the series so it can be dropped if necessary.
Signature · 2 lines
Mel Gorman SUSE Labs