Drain remote per-cpu directly v3

The page allocator uses page->lru for storing pages on either buddy or
PCP lists. Create page->buddy_list and page->pcp_list as a union with page->lru. This is simply to clarify what type of list a page is on in the page allocator.

No functional change intended.

[minchan: Fix page lru fields in macros]

Signed-off-by Mel Gorman <[email protected]>

Tested-by Minchan Kim <[email protected]>

Acked-by Minchan Kim <[email protected]>

---

The page allocator uses page->lru for storing pages on either buddy or PCP lists. Create page->buddy_list and page->pcp_list as a union with page->lru. This is simply to clarify what type of list a page is on in the page allocator.

No functional change intended.

[minchan: Fix page lru fields in macros]

Signed-off-by Mel Gorman <[email protected]>

Tested-by Minchan Kim <[email protected]>

Acked-by Minchan Kim <[email protected]>

The per_cpu_pages is cache-aligned on a standard x86-64 distribution configuration but a later patch will add a new field which would push the structure into the next cache line. Use only one list to store THP-sized pages on the per-cpu list. This assumes that the vast majority of THP-sized allocations are GFP_MOVABLE but even if it was another type, it would not contribute to serious fragmentation that potentially causes a later THP allocation failure. Align per_cpu_pages on the cacheline boundary to ensure there is no false cache sharing.

After this patch, the structure sizing is;

struct per_cpu_pages {

        int                        count;                /*     0     4 */
        int                        high;                 /*     4     4 */
        int                        batch;                /*     8     4 */
        short int                  free_factor;          /*    12     2 */
        short int                  expire;               /*    14     2 */
        struct list_head           lists[13];            /*    16   208 */

        /* size: 256, cachelines: 4, members: 6 */
        /* padding: 32 */
} __attribute__((__aligned__(64)));

Signed-off-by Mel Gorman <[email protected]>

Tested-by Minchan Kim <[email protected]>

Acked-by Minchan Kim <[email protected]>

---
 include/linux/mmzone.h | 11 +++++++----
 mm/page_alloc.c        |  4 ++--
 2 files changed, 9 insertions(+), 6 deletions(-)

include/linux/mmzone.h +7 -4

index 962b14d403e8..abe530748de6 100644
@@ -358,15 +358,18 @@ enum zone_watermarks {
 };
 
 /*
- * One per migratetype for each PAGE_ALLOC_COSTLY_ORDER plus one additional
- * for pageblock size for THP if configured.
+ * One per migratetype for each PAGE_ALLOC_COSTLY_ORDER. One additional list
+ * for THP which will usually be GFP_MOVABLE. Even if it is another type,
+ * it should not contribute to serious fragmentation causing THP allocation
+ * failures.
  */
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 #define NR_PCP_THP 1
 #else
 #define NR_PCP_THP 0
 #endif
-#define NR_PCP_LISTS (MIGRATE_PCPTYPES * (PAGE_ALLOC_COSTLY_ORDER + 1 + NR_PCP_THP))
+#define NR_LOWORDER_PCP_LISTS (MIGRATE_PCPTYPES * (PAGE_ALLOC_COSTLY_ORDER + 1))
+#define NR_PCP_LISTS (NR_LOWORDER_PCP_LISTS + NR_PCP_THP)
 
 /*
  * Shift to encode migratetype and order in the same integer, with order

@@ -392,7 +395,7 @@ struct per_cpu_pages {
 
 	/* Lists of pages, one per migrate type stored on the pcp-lists */
 	struct list_head lists[NR_PCP_LISTS];
-};
+} ____cacheline_aligned_in_smp;
 
 struct per_cpu_zonestat {
 #ifdef CONFIG_SMP

mm/page_alloc.c +2 -2

index f58f85fdb05f..5851ee88a89c 100644
@@ -648,7 +648,7 @@ static inline unsigned int order_to_pindex(int migratetype, int order)
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 	if (order > PAGE_ALLOC_COSTLY_ORDER) {
 		VM_BUG_ON(order != pageblock_order);
-		base = PAGE_ALLOC_COSTLY_ORDER + 1;
+		return NR_LOWORDER_PCP_LISTS;
 	}
 #else
 	VM_BUG_ON(order > PAGE_ALLOC_COSTLY_ORDER);

@@ -662,7 +662,7 @@ static inline int pindex_to_order(unsigned int pindex)
 	int order = pindex / MIGRATE_PCPTYPES;
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
-	if (order > PAGE_ALLOC_COSTLY_ORDER)
+	if (pindex == NR_LOWORDER_PCP_LISTS)
 		order = pageblock_order;
 #else
 	VM_BUG_ON(order > PAGE_ALLOC_COSTLY_ORDER);

This is a preparation page to allow the buddy removal code to be reused
in a later patch.

No functional change.

Signed-off-by Mel Gorman <[email protected]>

Tested-by Minchan Kim <[email protected]>

Acked-by Minchan Kim <[email protected]>

---

This is a preparation page to allow the buddy removal code to be reused in a later patch.

No functional change.

Signed-off-by Mel Gorman <[email protected]>

Tested-by Minchan Kim <[email protected]>

Acked-by Minchan Kim <[email protected]>

This is a preparation page to allow the buddy removal code to be reused in a later patch.

No functional change.

Signed-off-by Mel Gorman <[email protected]>

Tested-by Minchan Kim <[email protected]>

Acked-by Minchan Kim <[email protected]>

---

Quoted 9 lines · Mel Gorman

This is a preparation page to allow the buddy removal code to be reused in a later patch.

No functional change.

Signed-off-by Mel Gorman <[email protected]>

Tested-by Minchan Kim <[email protected]>

Acked-by Minchan Kim <[email protected]>

---

I see this splat when this patch is applied on 5.10.107 kernel:

I could resolve it by applying this patch:

	diff --git a/mm/vmstat.c b/mm/vmstat.c
	index 80c1e0a0f094e..92fb0c08296ef 100644
	--- a/mm/vmstat.c
	+++ b/mm/vmstat.c
	@@ -957,11 +957,11 @@ void __inc_numa_state(struct zone *zone,
		u16 __percpu *p = pcp->vm_numa_stat_diff + item;
		u16 v;

	-       v = __this_cpu_inc_return(*p);

	+       v = this_cpu_inc_return(*p);

		if (unlikely(v > NUMA_STATS_THRESHOLD)) {
			zone_numa_state_add(v, zone, item);
	-               __this_cpu_write(*p, 0);
	+               this_cpu_write(*p, 0);
		}
	 }

Quoted 13 lines · Qais Yousef

Quoted 9 lines · Mel Gorman

This is a preparation page to allow the buddy removal code to be reused in a later patch.

No functional change.

Signed-off-by Mel Gorman <[email protected]>

Tested-by Minchan Kim <[email protected]>

Acked-by Minchan Kim <[email protected]>

---

I see this splat when this patch is applied on 5.10.107 kernel:

<SNIP>

Quoted 11 lines

I could resolve it by applying this patch:

	diff --git a/mm/vmstat.c b/mm/vmstat.c
	index 80c1e0a0f094e..92fb0c08296ef 100644
	--- a/mm/vmstat.c
	+++ b/mm/vmstat.c
	@@ -957,11 +957,11 @@ void __inc_numa_state(struct zone *zone,
		u16 __percpu *p = pcp->vm_numa_stat_diff + item;
		u16 v;

	-       v = __this_cpu_inc_return(*p);

Quoted 9 lines

	+       v = this_cpu_inc_return(*p);

		if (unlikely(v > NUMA_STATS_THRESHOLD)) {
			zone_numa_state_add(v, zone, item);
	-               __this_cpu_write(*p, 0);
	+               this_cpu_write(*p, 0);
		}
	 }

5.18 does not have __inc_numa_state() so it's likely you are missing backports, probably f19298b9516c1a031b34b4147773457e3efe743b at minimum.

The VM_BUG_ON check for a valid page can be avoided with a simple change in the flow. The ZONE_BOOSTED_WATERMARK is unlikely in general and even more unlikely if the page allocation failed so mark the branch unlikely.

Signed-off-by Mel Gorman <[email protected]>

Tested-by Minchan Kim <[email protected]>

Acked-by Minchan Kim <[email protected]>

---

The VM_BUG_ON check for a valid page can be avoided with a simple change in the flow. The ZONE_BOOSTED_WATERMARK is unlikely in general and even more unlikely if the page allocation failed so mark the branch unlikely.

Signed-off-by Mel Gorman <[email protected]>

Tested-by Minchan Kim <[email protected]>

Acked-by Minchan Kim <[email protected]>

---
 mm/page_alloc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

mm/page_alloc.c +5 -4

index 1c4c54503a5d..b543333dce8f 100644
@@ -3765,17 +3765,18 @@ struct page *rmqueue(struct zone *preferred_zone,
 
 	page = rmqueue_buddy(preferred_zone, zone, order, alloc_flags,
 							migratetype);
-	if (unlikely(!page))
-		return NULL;
 
 out:
 	/* Separate test+clear to avoid unnecessary atomics */
-	if (test_bit(ZONE_BOOSTED_WATERMARK, &zone->flags)) {
+	if (unlikely(test_bit(ZONE_BOOSTED_WATERMARK, &zone->flags))) {
 		clear_bit(ZONE_BOOSTED_WATERMARK, &zone->flags);
 		wakeup_kswapd(zone, 0, 0, zone_idx(zone));
 	}
 
-	VM_BUG_ON_PAGE(page && bad_range(zone, page), page);
+	if (unlikely(!page))
+		return NULL;
+
+	VM_BUG_ON_PAGE(bad_range(zone, page), page);
 	return page;
 }
 

Quoted 4 lines · Mel Gorman

The VM_BUG_ON check for a valid page can be avoided with a simple change in the flow. The ZONE_BOOSTED_WATERMARK is unlikely in general and even more unlikely if the page allocation failed so mark the branch unlikely.

Hm, so that makes a DEBUG_VM config avoid the check. On the other hand, it puts it on the path returning from rmqueue_pcplist() for all configs, and that should be the fast path. So unless things further change in the following patches, it doesn't seem that useful?

Quoted 11 lines · Vlastimil Babka

Quoted 4 lines · Mel Gorman

The VM_BUG_ON check for a valid page can be avoided with a simple change in the flow. The ZONE_BOOSTED_WATERMARK is unlikely in general and even more unlikely if the page allocation failed so mark the branch unlikely.

Hm, so that makes a DEBUG_VM config avoid the check. On the other hand, it puts it on the path returning from rmqueue_pcplist() for all configs, and that should be the fast path. So unless things further change in the following patches, it doesn't seem that useful?

You're right -- the fast path ends up with both a if
(page) and if (!page) checks. Andrew, can you drop the patch mm-page_alloc-remove-unnecessary-page-==-null-check-in-rmqueue.patch from your tree please?

Originally the flow was important when I was writing the patch and later became unnecessary. However, it reminded me of another problem I thought of when writing this and then forgotten to note it in the changelog. If the page allocation fails then ZONE_BOOSTED_WATERMARK should still be tested and cleared before waking kswapd. It could happen if an allocation attempt tried to fallback to another migratetype and still fail to find a suitable page. This is true whether going through the PCP lists or not.

So what do you think of me adding this patch to a follow-up series?

--8<--
mm/page_alloc: Remove mistaken page == NULL check in rmqueue

If a page allocation fails, the ZONE_BOOSTER_WATERMARK should be tested, cleared and kswapd woken whether the allocation attempt was via the PCP or directly via the buddy list.

Remove the page == NULL so the ZONE_BOOSTED_WATERMARK bit is checked unconditionally. As it is unlikely that ZONE_BOOSTED_WATERMARK is set, mark the branch accordingly.

Signed-off-by Mel Gorman <[email protected]>

---
 mm/page_alloc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

mm/page_alloc.c +1 -3

index 1c4c54503a5d..61d5bc2efffe 100644
@@ -3765,12 +3765,10 @@ struct page *rmqueue(struct zone *preferred_zone,
 
 	page = rmqueue_buddy(preferred_zone, zone, order, alloc_flags,
 							migratetype);
-	if (unlikely(!page))
-		return NULL;
 
 out:
 	/* Separate test+clear to avoid unnecessary atomics */
-	if (test_bit(ZONE_BOOSTED_WATERMARK, &zone->flags)) {
+	if (unlikely(test_bit(ZONE_BOOSTED_WATERMARK, &zone->flags))) {
 		clear_bit(ZONE_BOOSTED_WATERMARK, &zone->flags);
 		wakeup_kswapd(zone, 0, 0, zone_idx(zone));
 	}

Currently the PCP lists are protected by using local_lock_irqsave to prevent migration and IRQ reentrancy but this is inconvenient. Remote draining of the lists is impossible and a workqueue is required and every task allocation/free must disable then enable interrupts which is
expensive.

As preparation for dealing with both of those problems, protect the lists with a spinlock. The IRQ-unsafe version of the lock is used because IRQs are already disabled by local_lock_irqsave. spin_trylock is used in preparation for a time when local_lock could be used instead
of lock_lock_irqsave.

The per_cpu_pages still fits within the same number of cache lines after
this patch relative to before the series.

struct per_cpu_pages {
        spinlock_t                 lock;                 /*     0 4 */
        int                        count;                /*     4 4 */
        int                        high;                 /*     8 4 */
        int                        batch;                /*    12 4 */
        short int                  free_factor;          /*    16 2 */
        short int                  expire;               /*    18 2 */

        /* XXX 4 bytes hole, try to pack */

        struct list_head           lists[13];            /*    24 208 */

        /* size: 256, cachelines: 4, members: 7 */ /* sum members: 228, holes: 1, sum holes: 4 */ /* padding: 24 */
} __attribute__((__aligned__(64)));

There is overhead in the fast path due to acquiring the spinlock even though the spinlock is per-cpu and uncontended in the common case. Page
Fault Test (PFT) running on a 1-socket reported the following results on
a 1 socket machine.

                                     5.18.0-rc1               5.18.0- rc1
                                        vanilla         mm-pcpdrain- v2r1
Hmean     faults/sec-1   886331.5718 (   0.00%)   885462.7479 (  - 0.10%)
Hmean     faults/sec-3  2337706.1583 (   0.00%)  2332130.4909 *  - 0.24%*
Hmean     faults/sec-5  2851594.2897 (   0.00%)  2844123.9307 (  - 0.26%)
Hmean     faults/sec-7  3543251.5507 (   0.00%)  3516889.0442 *  - 0.74%*
Hmean     faults/sec-8  3947098.0024 (   0.00%)  3916162.8476 *  - 0.78%*
Stddev    faults/sec-1     2302.9105 (   0.00%)     2065.0845 ( 10.33%)
Stddev    faults/sec-3     7275.2442 (   0.00%)     6033.2620 ( 17.07%)
Stddev    faults/sec-5    24726.0328 (   0.00%)    12525.1026 ( 49.34%)
Stddev    faults/sec-7     9974.2542 (   0.00%)     9543.9627 ( 4.31%)
Stddev    faults/sec-8     9468.0191 (   0.00%)     7958.2607 ( 15.95%)
CoeffVar  faults/sec-1        0.2598 (   0.00%)        0.2332 ( 10.24%)
CoeffVar  faults/sec-3        0.3112 (   0.00%)        0.2587 ( 16.87%)
CoeffVar  faults/sec-5        0.8670 (   0.00%)        0.4404 ( 49.21%)
CoeffVar  faults/sec-7        0.2815 (   0.00%)        0.2714 ( 3.60%)
CoeffVar  faults/sec-8        0.2399 (   0.00%)        0.2032 ( 15.28%)

There is a small hit in the number of faults per second but given that
the results are more stable, it's borderline noise.

Signed-off-by Mel Gorman <[email protected]>

Tested-by Minchan Kim <[email protected]>

Acked-by Minchan Kim <[email protected]>

---

From: Nicolas Saenz Julienne <[email protected]>

Some setups, notably NOHZ_FULL CPUs, are too busy to handle the per-cpu drain work queued by __drain_all_pages(). So introduce a new mechanism to remotely drain the per-cpu lists. It is made possible by remotely locking 'struct per_cpu_pages' new per-cpu spinlocks. A benefit of this new scheme is that drain operations are now migration safe.

There was no observed performance degradation vs. the previous scheme. Both netperf and hackbench were run in parallel to triggering the __drain_all_pages(NULL, true) code path around ~100 times per second. The new scheme performs a bit better (~5%), although the important point here is there are no performance regressions vs. the previous mechanism. Per-cpu lists draining happens only in slow paths.

Minchan Kim tested this independently and reported;

My workload is not NOHZ CPUs but run apps under heavy memory pressure so they goes to direct reclaim and be stuck on drain_all_pages until work on workqueue run.

unit: nanosecond
max(dur) avg(dur) count(dur)
166713013 487511.77786438033 1283

From traces, system encountered the drain_all_pages 1283 times and worst case was 166ms and avg was 487us.

The other problem was alloc_contig_range in CMA. The PCP draining takes several hundred millisecond sometimes though there is no memory pressure or a few of pages to be migrated out but CPU were fully booked.

Your patch perfectly removed those wasted time.

Quoted 28 lines · On Thu, 12 May 2022 09:50:43 +0100 Mel Gorman <[email protected]>…

From: Nicolas Saenz Julienne <[email protected]>

Some setups, notably NOHZ_FULL CPUs, are too busy to handle the per-cpu drain work queued by __drain_all_pages(). So introduce a new mechanism to remotely drain the per-cpu lists. It is made possible by remotely locking 'struct per_cpu_pages' new per-cpu spinlocks. A benefit of this new scheme is that drain operations are now migration safe.

There was no observed performance degradation vs. the previous scheme. Both netperf and hackbench were run in parallel to triggering the __drain_all_pages(NULL, true) code path around ~100 times per second. The new scheme performs a bit better (~5%), although the important point here is there are no performance regressions vs. the previous mechanism. Per-cpu lists draining happens only in slow paths.

Minchan Kim tested this independently and reported;

My workload is not NOHZ CPUs but run apps under heavy memory pressure so they goes to direct reclaim and be stuck on drain_all_pages until work on workqueue run.

unit: nanosecond
max(dur) avg(dur) count(dur)
166713013 487511.77786438033 1283

From traces, system encountered the drain_all_pages 1283 times and worst case was 166ms and avg was 487us.

Quoted 6 lines

The other problem was alloc_contig_range in CMA. The PCP draining takes several hundred millisecond sometimes though there is no memory pressure or a few of pages to be migrated out but CPU were fully booked.

Your patch perfectly removed those wasted time.

I'm not getting a sense here of the overall effect upon userspace performance. As Thomas said last year in
https://lkml.kernel.org/r/87v92sgt3n.ffs@tglx

: The changelogs and the cover letter have a distinct void vs. that which : means this is just another example of 'scratch my itch' changes w/o : proper justification.

Is there more to all of this than itchiness and if so, well, you know the rest ;)

Quoted 41 lines · Andrew Morton

Quoted 28 lines · On Thu, 12 May 2022 09:50:43 +0100 Mel Gorman <[email protected]>…

From: Nicolas Saenz Julienne <[email protected]>

Some setups, notably NOHZ_FULL CPUs, are too busy to handle the per-cpu drain work queued by __drain_all_pages(). So introduce a new mechanism to remotely drain the per-cpu lists. It is made possible by remotely locking 'struct per_cpu_pages' new per-cpu spinlocks. A benefit of this new scheme is that drain operations are now migration safe.

There was no observed performance degradation vs. the previous scheme. Both netperf and hackbench were run in parallel to triggering the __drain_all_pages(NULL, true) code path around ~100 times per second. The new scheme performs a bit better (~5%), although the important point here is there are no performance regressions vs. the previous mechanism. Per-cpu lists draining happens only in slow paths.

Minchan Kim tested this independently and reported;

My workload is not NOHZ CPUs but run apps under heavy memory pressure so they goes to direct reclaim and be stuck on drain_all_pages until work on workqueue run.

unit: nanosecond
max(dur) avg(dur) count(dur)
166713013 487511.77786438033 1283

From traces, system encountered the drain_all_pages 1283 times and worst case was 166ms and avg was 487us.

Quoted 6 lines

The other problem was alloc_contig_range in CMA. The PCP draining takes several hundred millisecond sometimes though there is no memory pressure or a few of pages to be migrated out but CPU were fully booked.

Your patch perfectly removed those wasted time.

I'm not getting a sense here of the overall effect upon userspace performance. As Thomas said last year in
https://lkml.kernel.org/r/87v92sgt3n.ffs@tglx

Quoted 7 lines

: The changelogs and the cover letter have a distinct void vs. that which : means this is just another example of 'scratch my itch' changes w/o : proper justification.

Is there more to all of this than itchiness and if so, well, you know the rest ;)

I think Minchan's example is clear-cut. The draining operation can take an arbitrary amount of time waiting for the workqueue to run on each CPU and can cause severe delays under reclaim or CMA and the patch fixes it. Maybe most users won't even notice but I bet phone users do if a camera app takes too long to open.

The first paragraphs was written by Nicolas and I did not want to modify it heavily and still put his Signed-off-by on it. Maybe it could have been clearer though because "too busy" is vague when the actual intent is to avoid interfering with RT tasks. Does this sound better to you?

Some setups, notably NOHZ_FULL CPUs, may be running realtime or latency-sensitive applications that cannot tolerate interference due to per-cpu drain work queued by __drain_all_pages(). Introduce a new mechanism to remotely drain the per-cpu lists. It is made possible by remotely locking 'struct per_cpu_pages' new per-cpu spinlocks. This has two advantages, the time to drain is more predictable and other unrelated tasks are not interrupted.

You raise a very valid point with Thomas' mail and it is a concern that the local_lock is no longer strictly local. We still need preemption to be disabled between the percpu lookup and the lock acquisition but that can be done with get_cpu_var() to make the scope clear.

Quoted 69 lines · Mel Gorman

Quoted 41 lines · Andrew Morton

Quoted 28 lines · On Thu, 12 May 2022 09:50:43 +0100 Mel Gorman <[email protected]>…

From: Nicolas Saenz Julienne <[email protected]>

Some setups, notably NOHZ_FULL CPUs, are too busy to handle the per-cpu drain work queued by __drain_all_pages(). So introduce a new mechanism to remotely drain the per-cpu lists. It is made possible by remotely locking 'struct per_cpu_pages' new per-cpu spinlocks. A benefit of this new scheme is that drain operations are now migration safe.

There was no observed performance degradation vs. the previous scheme. Both netperf and hackbench were run in parallel to triggering the __drain_all_pages(NULL, true) code path around ~100 times per second. The new scheme performs a bit better (~5%), although the important point here is there are no performance regressions vs. the previous mechanism. Per-cpu lists draining happens only in slow paths.

Minchan Kim tested this independently and reported;

My workload is not NOHZ CPUs but run apps under heavy memory pressure so they goes to direct reclaim and be stuck on drain_all_pages until work on workqueue run.

unit: nanosecond
max(dur) avg(dur) count(dur)
166713013 487511.77786438033 1283

From traces, system encountered the drain_all_pages 1283 times and worst case was 166ms and avg was 487us.

Quoted 6 lines

The other problem was alloc_contig_range in CMA. The PCP draining takes several hundred millisecond sometimes though there is no memory pressure or a few of pages to be migrated out but CPU were fully booked.

Your patch perfectly removed those wasted time.

I'm not getting a sense here of the overall effect upon userspace performance. As Thomas said last year in
https://lkml.kernel.org/r/87v92sgt3n.ffs@tglx

Quoted 7 lines

: The changelogs and the cover letter have a distinct void vs. that which : means this is just another example of 'scratch my itch' changes w/o : proper justification.

Is there more to all of this than itchiness and if so, well, you know the rest ;)

I think Minchan's example is clear-cut. The draining operation can take an arbitrary amount of time waiting for the workqueue to run on each CPU and can cause severe delays under reclaim or CMA and the patch fixes it. Maybe most users won't even notice but I bet phone users do if a camera app takes too long to open.

The first paragraphs was written by Nicolas and I did not want to modify it heavily and still put his Signed-off-by on it. Maybe it could have been clearer though because "too busy" is vague when the actual intent is to avoid interfering with RT tasks. Does this sound better to you?

Some setups, notably NOHZ_FULL CPUs, may be running realtime or latency-sensitive applications that cannot tolerate interference due to per-cpu drain work queued by __drain_all_pages(). Introduce a new mechanism to remotely drain the per-cpu lists. It is made possible by remotely locking 'struct per_cpu_pages' new per-cpu spinlocks. This has two advantages, the time to drain is more predictable and other unrelated tasks are not interrupted.

Quoted 4 lines

You raise a very valid point with Thomas' mail and it is a concern that the local_lock is no longer strictly local. We still need preemption to be disabled between the percpu lookup and the lock acquisition but that can be done with get_cpu_var() to make the scope clear.

This isn't going to work in RT :(

get_cpu_var() disables preemption hampering RT spinlock use. There is more to it in Documentation/locking/locktypes.rst.

Quoted 10 lines

Quoted 4 lines

You raise a very valid point with Thomas' mail and it is a concern that the local_lock is no longer strictly local. We still need preemption to be disabled between the percpu lookup and the lock acquisition but that can be done with get_cpu_var() to make the scope clear.

This isn't going to work in RT :(

get_cpu_var() disables preemption hampering RT spinlock use. There is more to it in Documentation/locking/locktypes.rst.

Bah, you're right. A helper that called preempt_disable() on !RT and migrate_disable() on RT would work although similar to local_lock with a different name. I'll look on Monday to see how the code could be restructured to always have the get_cpu_var() call immediately before the lock acquisition. Once that is done, I'll look what sort of helper that "disables preempt/migration, lookup pcp structure, acquire lock, enable preempt/migration". It's effectively the magic trick that local_lock uses to always lock the right pcpu lock but we want the spinlock semantics for remote drain.

Changelog since v2
o More conversions from page->lru to page->[pcp_list|buddy_list]
o Additional test results in changelogs

Changelog since v1
o Fix unsafe RT locking scheme
o Use spin_trylock on UP PREEMPT_RT

This series has the same intent as Nicolas' series "mm/page_alloc: Remote per-cpu lists drain support" -- avoid interference of a high priority task due to a workqueue item draining per-cpu page lists. While many workloads can tolerate a brief interruption, it may be cause a real-time

task runnning on a NOHZ_FULL CPU to miss a deadline and at minimum,

the draining in non-deterministic.

Currently an IRQ-safe local_lock protects the page allocator per-cpu lists. The local_lock on its own prevents migration and the IRQ disabling protects from corruption due to an interrupt arriving while a page allocation is in progress. The locking is inherently unsafe for remote access unless the CPU is hot-removed.

This series adjusts the locking. A spinlock is added to struct per_cpu_pages to protect the list contents while local_lock_irq continues to prevent migration and IRQ reentry. This allows a remote CPU to safely drain a remote per-cpu list.

This series is a partial series. Follow-on work should allow the local_irq_save to be converted to a local_irq to avoid IRQs being disabled/enabled in most cases. Consequently, there are some TODO comments highlighting the places that would change if local_irq was used. However, there are enough corner cases that it deserves a series on its own separated by one kernel release and the priority right now is to avoid interference of high priority tasks.

Quoted 4 lines · On Thu, 12 May 2022 09:50:37 +0100 Mel Gorman <[email protected]>…

Changelog since v2
o More conversions from page->lru to page->[pcp_list|buddy_list]
o Additional test results in changelogs

Quoted 8 lines

Changelog since v1
o Fix unsafe RT locking scheme
o Use spin_trylock on UP PREEMPT_RT

This series has the same intent as Nicolas' series "mm/page_alloc: Remote per-cpu lists drain support" -- avoid interference of a high priority task due to a workqueue item draining per-cpu page lists. While many workloads can tolerate a brief interruption, it may be cause a real-time

s/may be/may/

Quoted 1 line

task runnning on a NOHZ_FULL CPU to miss a deadline and at minimum,

s/nnn/nn/

Quoted 1 line

the draining in non-deterministic.

s/n/s/;)

Quoted 5 lines

Currently an IRQ-safe local_lock protects the page allocator per-cpu lists. The local_lock on its own prevents migration and the IRQ disabling protects from corruption due to an interrupt arriving while a page allocation is in progress. The locking is inherently unsafe for remote access unless the CPU is hot-removed.

I don't understand the final sentence here. Which CPU and why does hot-removing it make the locking safe?

Correct.

Quoted 4 lines

Quoted 1 line

the draining in non-deterministic.

s/n/s/;)

Think that one is ok. At least spell check did not complain.

Quoted 9 lines

Quoted 5 lines

Currently an IRQ-safe local_lock protects the page allocator per-cpu lists. The local_lock on its own prevents migration and the IRQ disabling protects from corruption due to an interrupt arriving while a page allocation is in progress. The locking is inherently unsafe for remote access unless the CPU is hot-removed.

I don't understand the final sentence here. Which CPU and why does hot-removing it make the locking safe?

The sentence can be dropped because it adds little and is potentially confusing. The PCP being safe to access remotely is specific to the context of the CPU being hot-removed and there are other special corner cases like zone_pcp_disable that modifies a per-cpu structure remotely but not in a way that causes corruption.

Quoted 5 lines

The sentence can be dropped because it adds little and is potentially confusing. The PCP being safe to access remotely is specific to the context of the CPU being hot-removed and there are other special corner cases like zone_pcp_disable that modifies a per-cpu structure remotely but not in a way that causes corruption.

OK. I pasted in your para from the other email. Current 0/n blurb:

Some setups, notably NOHZ_FULL CPUs, may be running realtime or latency-sensitive applications that cannot tolerate interference due to per-cpu drain work queued by __drain_all_pages(). Introduce a new mechanism to remotely drain the per-cpu lists. It is made possible by remotely locking 'struct per_cpu_pages' new per-cpu spinlocks. This has two advantages, the time to drain is more predictable and other unrelated tasks are not interrupted.

This series has the same intent as Nicolas' series "mm/page_alloc: Remote per-cpu lists drain support" -- avoid interference of a high priority task due to a workqueue item draining per-cpu page lists. While many workloads can tolerate a brief interruption, it may cause a real-time task running on a NOHZ_FULL CPU to miss a deadline and at minimum, the draining is non-deterministic.

Currently an IRQ-safe local_lock protects the page allocator per-cpu lists. The local_lock on its own prevents migration and the IRQ disabling protects from corruption due to an interrupt arriving while a page allocation is in progress.

This series adjusts the locking. A spinlock is added to struct per_cpu_pages to protect the list contents while local_lock_irq continues to prevent migration and IRQ reentry. This allows a remote CPU to safely drain a remote per-cpu list.

This series is a partial series. Follow-on work should allow the local_irq_save to be converted to a local_irq to avoid IRQs being disabled/enabled in most cases. Consequently, there are some TODO comments highlighting the places that would change if local_irq was used. However, there are enough corner cases that it deserves a series on its own separated by one kernel release and the priority right now is to avoid interference of high priority tasks.

Changelog since v2
o More conversions from page->lru to page->[pcp_list|buddy_list]
o Additional test results in changelogs

Changelog since v1
o Fix unsafe RT locking scheme
o Use spin_trylock on UP PREEMPT_RT

This series has the same intent as Nicolas' series "mm/page_alloc:
Remote
per-cpu lists drain support" -- avoid interference of a high priority task due to a workqueue item draining per-cpu page lists. While many workloads can tolerate a brief interruption, it may be cause a real- time
task runnning on a NOHZ_FULL CPU to miss a deadline and at minimum, the draining in non-deterministic.

Currently an IRQ-safe local_lock protects the page allocator per-cpu lists.
The local_lock on its own prevents migration and the IRQ disabling protects
from corruption due to an interrupt arriving while a page allocation is
in progress. The locking is inherently unsafe for remote access unless
the CPU is hot-removed.

This series adjusts the locking. A spinlock is added to struct per_cpu_pages to protect the list contents while local_lock_irq continues
to prevent migration and IRQ reentry. This allows a remote CPU to safely
drain a remote per-cpu list.

This series is a partial series. Follow-on work should allow the local_irq_save to be converted to a local_irq to avoid IRQs being disabled/enabled in most cases. Consequently, there are some TODO comments
highlighting the places that would change if local_irq was used. However,
there are enough corner cases that it deserves a series on its own separated by one kernel release and the priority right now is to avoid
interference of high priority tasks.

Patch 1 is a cosmetic patch to clarify when page->lru is storing buddy pages
        and when it is storing per-cpu pages.

Patch 2 shrinks per_cpu_pages to make room for a spin lock. Strictly speaking
        this is not necessary but it avoids per_cpu_pages consuming another
        cache line.

Patch 3 is a preparation patch to avoid code duplication.

Patch 4 is a simple micro-optimisation that improves code flow necessary for
        a later patch to avoid code duplication.

Patch 5 uses a spin_lock to protect the per_cpu_pages contents while still
        relying on local_lock to prevent migration, stabilise the pcp lookup and prevent IRQ reentrancy.

Patch 6 remote drains per-cpu pages directly instead of using a workqueue.

 include/linux/mm_types.h |   5 +
 include/linux/mmzone.h   |  12 +-
 mm/page_alloc.c          | 348 +++++++++++++++++++++++++------------ --

Changelog since v2
o More conversions from page->lru to page->[pcp_list|buddy_list]
o Additional test results in changelogs

Changelog since v1
o Fix unsafe RT locking scheme
o Use spin_trylock on UP PREEMPT_RT

This series has the same intent as Nicolas' series "mm/page_alloc: Remote per-cpu lists drain support" -- avoid interference of a high priority task due to a workqueue item draining per-cpu page lists. While many workloads can tolerate a brief interruption, it may be cause a real-time task runnning on a NOHZ_FULL CPU to miss a deadline and at minimum, the draining in non-deterministic.

Currently an IRQ-safe local_lock protects the page allocator per-cpu lists. The local_lock on its own prevents migration and the IRQ disabling protects from corruption due to an interrupt arriving while a page allocation is in progress. The locking is inherently unsafe for remote access unless the CPU is hot-removed.

This series adjusts the locking. A spinlock is added to struct per_cpu_pages to protect the list contents while local_lock_irq continues to prevent migration and IRQ reentry. This allows a remote CPU to safely drain a remote per-cpu list.

This series is a partial series. Follow-on work should allow the local_irq_save to be converted to a local_irq to avoid IRQs being disabled/enabled in most cases. Consequently, there are some TODO comments highlighting the places that would change if local_irq was used. However, there are enough corner cases that it deserves a series on its own separated by one kernel release and the priority right now is to avoid interference of high priority tasks.

Quoted 26 lines · Mel Gorman

Changelog since v2
o More conversions from page->lru to page->[pcp_list|buddy_list]
o Additional test results in changelogs

Changelog since v1
o Fix unsafe RT locking scheme
o Use spin_trylock on UP PREEMPT_RT

This series has the same intent as Nicolas' series "mm/page_alloc: Remote per-cpu lists drain support" -- avoid interference of a high priority task due to a workqueue item draining per-cpu page lists. While many workloads can tolerate a brief interruption, it may be cause a real-time task runnning on a NOHZ_FULL CPU to miss a deadline and at minimum, the draining in non-deterministic.

Currently an IRQ-safe local_lock protects the page allocator per-cpu lists. The local_lock on its own prevents migration and the IRQ disabling protects from corruption due to an interrupt arriving while a page allocation is in progress. The locking is inherently unsafe for remote access unless the CPU is hot-removed.

This series adjusts the locking. A spinlock is added to struct per_cpu_pages to protect the list contents while local_lock_irq continues to prevent migration and IRQ reentry. This allows a remote CPU to safely drain a remote per-cpu list.

Quoted 7 lines

This series is a partial series. Follow-on work should allow the local_irq_save to be converted to a local_irq to avoid IRQs being disabled/enabled in most cases. Consequently, there are some TODO comments highlighting the places that would change if local_irq was used. However, there are enough corner cases that it deserves a series on its own separated by one kernel release and the priority right now is to avoid interference of high priority tasks.

Reverting the whole series fixed an issue that offlining a memory section blocking for hours on today's linux-next tree.

__wait_rcu_gp
synchronize_rcu at kernel/rcu/tree.c:3915 lru_cache_disable at mm/swap.c:886 __alloc_contig_migrate_range at mm/page_alloc.c:9078 isolate_single_pageblock at mm/page_isolation.c:405 start_isolate_page_range
offline_pages
memory_subsys_offline
device_offline
online_store
dev_attr_store
sysfs_kf_write
kernfs_fop_write_iter
new_sync_write
vfs_write
ksys_write
__arm64_sys_write
invoke_syscall
el0_svc_common.constprop.0
do_el0_svc
el0_svc
el0t_64_sync_handler
el0t_64_sync

For full disclosure, I have also reverted the commit 0d523026abd4 ("mm/page_alloc: fix tracepoint mm_page_alloc_zone_locked()"), so the series can be reverted cleanly. But, I can't see how the commit 0d523026abd4 could cause this issue at all.

Quoted 62 lines · Qian Cai

Quoted 26 lines · Mel Gorman

Changelog since v2
o More conversions from page->lru to page->[pcp_list|buddy_list]
o Additional test results in changelogs

Changelog since v1
o Fix unsafe RT locking scheme
o Use spin_trylock on UP PREEMPT_RT

This series has the same intent as Nicolas' series "mm/page_alloc: Remote per-cpu lists drain support" -- avoid interference of a high priority task due to a workqueue item draining per-cpu page lists. While many workloads can tolerate a brief interruption, it may be cause a real-time task runnning on a NOHZ_FULL CPU to miss a deadline and at minimum, the draining in non-deterministic.

Currently an IRQ-safe local_lock protects the page allocator per-cpu lists. The local_lock on its own prevents migration and the IRQ disabling protects from corruption due to an interrupt arriving while a page allocation is in progress. The locking is inherently unsafe for remote access unless the CPU is hot-removed.

This series adjusts the locking. A spinlock is added to struct per_cpu_pages to protect the list contents while local_lock_irq continues to prevent migration and IRQ reentry. This allows a remote CPU to safely drain a remote per-cpu list.

Quoted 7 lines

This series is a partial series. Follow-on work should allow the local_irq_save to be converted to a local_irq to avoid IRQs being disabled/enabled in most cases. Consequently, there are some TODO comments highlighting the places that would change if local_irq was used. However, there are enough corner cases that it deserves a series on its own separated by one kernel release and the priority right now is to avoid interference of high priority tasks.

Reverting the whole series fixed an issue that offlining a memory section blocking for hours on today's linux-next tree.

__wait_rcu_gp
synchronize_rcu at kernel/rcu/tree.c:3915 lru_cache_disable at mm/swap.c:886 __alloc_contig_migrate_range at mm/page_alloc.c:9078 isolate_single_pageblock at mm/page_isolation.c:405 start_isolate_page_range
offline_pages
memory_subsys_offline
device_offline
online_store
dev_attr_store
sysfs_kf_write
kernfs_fop_write_iter
new_sync_write
vfs_write
ksys_write
__arm64_sys_write
invoke_syscall
el0_svc_common.constprop.0
do_el0_svc
el0_svc
el0t_64_sync_handler
el0t_64_sync

Quoted 4 lines

For full disclosure, I have also reverted the commit 0d523026abd4 ("mm/page_alloc: fix tracepoint mm_page_alloc_zone_locked()"), so the series can be reverted cleanly. But, I can't see how the commit 0d523026abd4 could cause this issue at all.

This is halting in __lru_add_drain_all where it calls synchronize_rcu before the drain even happens. It's also an LRU drain and not PCP which is what the series affects and the allocator doesn't use rcu. In a KVM machine, I can do

$ for BANK in `(for i in {1..20}; do echo $((RANDOM%416)); done) | sort -n  | uniq`; do BEFORE=`cat /sys/devices/system/memory/memory$BANK/online`; echo 0 > /sys/devices/system/memory/memory$BANK/online; AFTER=`cat /sys/devices/system/memory/memory$BANK/online`; printf "%4d %d -> %d\n" $BANK $BEFORE $AFTER; done
   3 1 -> 0
  57 1 -> 0
  74 1 -> 0
  93 1 -> 0
 101 1 -> 0
 128 1 -> 0
 133 1 -> 0
 199 1 -> 0
 223 1 -> 0
 225 1 -> 0
 229 1 -> 0
 243 1 -> 0
 263 1 -> 0
 300 1 -> 0
 309 1 -> 0
 329 1 -> 0
 355 1 -> 0
 365 1 -> 0
 372 1 -> 0
 383 1 -> 0

It offlines 20 sections although after several attempts free -m starts reporting negative used memory so there is a bug of some description. How are you testing this exactly? Is it every time or intermittent? Are you confident that reverting the series makes the problem go away?

Quoted 95 lines · Mel Gorman

Quoted 62 lines · Qian Cai

Quoted 26 lines · Mel Gorman

Changelog since v2
o More conversions from page->lru to page->[pcp_list|buddy_list]
o Additional test results in changelogs

Changelog since v1
o Fix unsafe RT locking scheme
o Use spin_trylock on UP PREEMPT_RT

This series has the same intent as Nicolas' series "mm/page_alloc: Remote per-cpu lists drain support" -- avoid interference of a high priority task due to a workqueue item draining per-cpu page lists. While many workloads can tolerate a brief interruption, it may be cause a real-time task runnning on a NOHZ_FULL CPU to miss a deadline and at minimum, the draining in non-deterministic.

Currently an IRQ-safe local_lock protects the page allocator per-cpu lists. The local_lock on its own prevents migration and the IRQ disabling protects from corruption due to an interrupt arriving while a page allocation is in progress. The locking is inherently unsafe for remote access unless the CPU is hot-removed.

This series adjusts the locking. A spinlock is added to struct per_cpu_pages to protect the list contents while local_lock_irq continues to prevent migration and IRQ reentry. This allows a remote CPU to safely drain a remote per-cpu list.

Quoted 7 lines

This series is a partial series. Follow-on work should allow the local_irq_save to be converted to a local_irq to avoid IRQs being disabled/enabled in most cases. Consequently, there are some TODO comments highlighting the places that would change if local_irq was used. However, there are enough corner cases that it deserves a series on its own separated by one kernel release and the priority right now is to avoid interference of high priority tasks.

Reverting the whole series fixed an issue that offlining a memory section blocking for hours on today's linux-next tree.

__wait_rcu_gp
synchronize_rcu at kernel/rcu/tree.c:3915 lru_cache_disable at mm/swap.c:886 __alloc_contig_migrate_range at mm/page_alloc.c:9078 isolate_single_pageblock at mm/page_isolation.c:405 start_isolate_page_range
offline_pages
memory_subsys_offline
device_offline
online_store
dev_attr_store
sysfs_kf_write
kernfs_fop_write_iter
new_sync_write
vfs_write
ksys_write
__arm64_sys_write
invoke_syscall
el0_svc_common.constprop.0
do_el0_svc
el0_svc
el0t_64_sync_handler
el0t_64_sync

Quoted 4 lines

For full disclosure, I have also reverted the commit 0d523026abd4 ("mm/page_alloc: fix tracepoint mm_page_alloc_zone_locked()"), so the series can be reverted cleanly. But, I can't see how the commit 0d523026abd4 could cause this issue at all.

This is halting in __lru_add_drain_all where it calls synchronize_rcu before the drain even happens. It's also an LRU drain and not PCP which is what the series affects and the allocator doesn't use rcu. In a KVM machine, I can do

$ for BANK in `(for i in {1..20}; do echo $((RANDOM%416)); done) | sort -n  | uniq`; do BEFORE=`cat /sys/devices/system/memory/memory$BANK/online`; echo 0 > /sys/devices/system/memory/memory$BANK/online; AFTER=`cat /sys/devices/system/memory/memory$BANK/online`; printf "%4d %d -> %d\n" $BANK $BEFORE $AFTER; done
   3 1 -> 0
  57 1 -> 0
  74 1 -> 0
  93 1 -> 0
 101 1 -> 0
 128 1 -> 0
 133 1 -> 0
 199 1 -> 0
 223 1 -> 0
 225 1 -> 0
 229 1 -> 0
 243 1 -> 0
 263 1 -> 0
 300 1 -> 0
 309 1 -> 0
 329 1 -> 0
 355 1 -> 0
 365 1 -> 0
 372 1 -> 0
 383 1 -> 0

Quoted 4 lines

It offlines 20 sections although after several attempts free -m starts reporting negative used memory so there is a bug of some description. How are you testing this exactly? Is it every time or intermittent? Are you confident that reverting the series makes the problem go away?

Cc'ing Paul. Either reverting this series or Paul's 3 patches below from today's linux-next tree fixed the issue.

ca52639daa5b rcu-tasks: Drive synchronous grace periods from calling task 89ad98e93ce8 rcu-tasks: Move synchronize_rcu_tasks_generic() down 0d90e7225fb1 rcu-tasks: Split rcu_tasks_one_gp() from rcu_tasks_kthread()

It was reproduced by running this script below on an arm64 server. I can reproduce it every time within 5 attempts. I noticed that when it happens, we have a few rcu kthreads all are stuck in this line,

        rcuwait_wait_event(&rtp->cbs_wait,
                           (needgpcb = rcu_tasks_need_gpcb(rtp)),
                           TASK_IDLE);

rcu_tasks_kthread
rcu_tasks_rude_kthread
[rcu_tasks_trace_kthread

#!/usr/bin/env python3
# SPDX-License-Identifier: GPL-2.0

import os
import re
import subprocess

def mem_iter():
base_dir = '/sys/devices/system/memory/'
for curr_dir in os.listdir(base_dir):
if re.match(r'memory\d+', curr_dir):
yield base_dir + curr_dir

if __name__ == '__main__':
print('- Try to remove each memory section and then add it back.') for mem_dir in mem_iter():
status = f'{mem_dir}/online'
if open(status).read().rstrip() == '1':
# This could expectedly fail due to many reasons.
section = os.path.basename(mem_dir)
print(f'- Try to remove {section}.')
proc = subprocess.run([f'echo 0 | sudo tee {status}'], shell=True) if proc.returncode == 0:
print(f'- Try to add {section}.') subprocess.check_call([f'echo 1 | sudo tee {status}'], shell=True)

So does this python script somehow change the tracing state? (It does not look to me like it does, but I could easily be missing something.)

Either way, is there something else waiting for these RCU flavors? (There should not be.) Nevertheless, if so, there should be a synchronize_rcu_tasks(), synchronize_rcu_tasks_rude(), or synchronize_rcu_tasks_trace() on some other blocked task's stack somewhere.

Or maybe something sleeps waiting for an RCU Tasks * callback to be invoked. In that case (and in the above case, for that matter), at least one of these pointers would be non-NULL on some CPU:

1. rcu_tasks__percpu.cblist.head
2. rcu_tasks_rude__percpu.cblist.head
3. rcu_tasks_trace__percpu.cblist.head

The ->func field of the pointed-to structure contains a pointer to the callback function, which will help work out what is going on. (Most likely a wakeup being lost or not provided.)

Alternatively, if your system has hundreds of thousands of tasks and you have attached BPF programs to short-lived socket structures and you don't yet have the workaround, then you can see hangs. (I am working on a longer-term fix.) In the short term, applying the workaround is the right thing to do. (Adding a couple of the BPF guys on CC for their thoughts.)

Quoted 2 lines · Paul E. McKenney

So does this python script somehow change the tracing state? (It does not look to me like it does, but I could easily be missing something.)

No, I don't think so either. It pretty much just offline memory sections one at a time.

Quoted 5 lines

Either way, is there something else waiting for these RCU flavors? (There should not be.) Nevertheless, if so, there should be a synchronize_rcu_tasks(), synchronize_rcu_tasks_rude(), or synchronize_rcu_tasks_trace() on some other blocked task's stack somewhere.

There are only three blocked tasks when this happens. The kmemleak_scan() is just the victim waiting for the locks taken by the stucking offline_pages()->synchronize_rcu() task.

task:kmemleak state:D stack:25824 pid: 1033 ppid: 2 flags:0x00000008

 Call trace:
  __switch_to
  __schedule
  schedule
  percpu_rwsem_wait
  __percpu_down_read
  percpu_down_read.constprop.0
  get_online_mems

kmemleak_scan
kmemleak_scan_thread
kthread
ret_from_fork

task:cppc_fie state:D stack:23472 pid: 1848 ppid: 2 flags:0x00000008

 Call trace:
  __switch_to
  __schedule
  lockdep_recursion

task:tee state:D stack:24816 pid:16733 ppid: 16732 flags:0x0000020c

 Call trace:
  __switch_to
  __schedule
  schedule
  schedule_timeout
  __wait_for_common
  wait_for_completion
  __wait_rcu_gp
  synchronize_rcu

lru_cache_disable
__alloc_contig_migrate_range
isolate_single_pageblock
start_isolate_page_range
offline_pages
memory_subsys_offline
device_offline
online_store
dev_attr_store
sysfs_kf_write
kernfs_fop_write_iter
new_sync_write
vfs_write
ksys_write
__arm64_sys_write
invoke_syscall
el0_svc_common.constprop.0
do_el0_svc
el0_svc
el0t_64_sync_handler
el0t_64_sync

Quoted 8 lines

Or maybe something sleeps waiting for an RCU Tasks * callback to be invoked. In that case (and in the above case, for that matter), at least one of these pointers would be non-NULL on some CPU:

1. rcu_tasks__percpu.cblist.head
2. rcu_tasks_rude__percpu.cblist.head
3. rcu_tasks_trace__percpu.cblist.head

Quoted 3 lines

The ->func field of the pointed-to structure contains a pointer to the callback function, which will help work out what is going on. (Most likely a wakeup being lost or not provided.)

What would be some of the easy ways to find out those? I can't see anything interesting from the output of sysrq-t.

Quoted 5 lines

Alternatively, if your system has hundreds of thousands of tasks and you have attached BPF programs to short-lived socket structures and you don't yet have the workaround, then you can see hangs. (I am working on a longer-term fix.) In the short term, applying the workaround is the right thing to do. (Adding a couple of the BPF guys on CC for their thoughts.)

The system is pretty much idle after a fresh reboot. The only workload is to run the script.

Is the task doing offline_pages()->synchronize_rcu() doing this repeatedly? Or is there a stalled RCU grace period? (From what I can see, offline_pages() is not doing huge numbers of calls to synchronize_rcu() in any of its loops, but I freely admit that I do not know this code.)

Or is it possible that reverting those three patches simply decreases the probability of failure, rather than eliminating the failure? Such a decrease could be due to many things, for example, changes to offsets and sizes of data structures.

Do you ever see RCU CPU stall warnings?

Quoted 5 lines · Paul E. McKenney

Is the task doing offline_pages()->synchronize_rcu() doing this repeatedly? Or is there a stalled RCU grace period? (From what I can see, offline_pages() is not doing huge numbers of calls to synchronize_rcu() in any of its loops, but I freely admit that I do not know this code.)

Yes, we are running into an endless loop in isolate_single_pageblock(). There was a similar issue happened not long ago, so I am wondering if we did not solve it entirely then. Anyway, I will continue the thread over there.

https://lore.kernel.org/all/YoavU%2F+NfQIzQiDF@qian/

Quoted 4 lines

Or is it possible that reverting those three patches simply decreases the probability of failure, rather than eliminating the failure? Such a decrease could be due to many things, for example, changes to offsets and sizes of data structures.

Entirely possible. Sorry for the false alarm.

Quoted 1 line

Do you ever see RCU CPU stall warnings?

No.

Quoted 26 lines · Mel Gorman

Changelog since v2
o More conversions from page->lru to page->[pcp_list|buddy_list]
o Additional test results in changelogs

Changelog since v1
o Fix unsafe RT locking scheme
o Use spin_trylock on UP PREEMPT_RT

This series has the same intent as Nicolas' series "mm/page_alloc: Remote per-cpu lists drain support" -- avoid interference of a high priority task due to a workqueue item draining per-cpu page lists. While many workloads can tolerate a brief interruption, it may be cause a real-time task runnning on a NOHZ_FULL CPU to miss a deadline and at minimum, the draining in non-deterministic.

Currently an IRQ-safe local_lock protects the page allocator per-cpu lists. The local_lock on its own prevents migration and the IRQ disabling protects from corruption due to an interrupt arriving while a page allocation is in progress. The locking is inherently unsafe for remote access unless the CPU is hot-removed.

This series adjusts the locking. A spinlock is added to struct per_cpu_pages to protect the list contents while local_lock_irq continues to prevent migration and IRQ reentry. This allows a remote CPU to safely drain a remote per-cpu list.

Quoted 7 lines

This series is a partial series. Follow-on work should allow the local_irq_save to be converted to a local_irq to avoid IRQs being disabled/enabled in most cases. Consequently, there are some TODO comments highlighting the places that would change if local_irq was used. However, there are enough corner cases that it deserves a series on its own separated by one kernel release and the priority right now is to avoid interference of high priority tasks.

Reverting the whole series fixed an issue that offlining a memory section blocking for hours on today's linux-next tree.

__wait_rcu_gp
synchronize_rcu at kernel/rcu/tree.c:3915 lru_cache_disable at mm/swap.c:886 __alloc_contig_migrate_range at mm/page_alloc.c:9078 isolate_single_pageblock at mm/page_isolation.c:405 start_isolate_page_range
offline_pages
memory_subsys_offline
device_offline
online_store
dev_attr_store
sysfs_kf_write
kernfs_fop_write_iter
new_sync_write
vfs_write
ksys_write
__arm64_sys_write
invoke_syscall
el0_svc_common.constprop.0
do_el0_svc
el0_svc
el0t_64_sync_handler
el0t_64_sync

For full disclosure, I have also reverted the commit 0d523026abd4 ("mm/page_alloc: fix tracepoint mm_page_alloc_zone_locked()"), so the series can be reverted cleanly. But, I can't see how the commit 0d523026abd4 could cause this issue at all.

Quoted 62 lines · Qian Cai

Quoted 26 lines · Mel Gorman

Changelog since v2
o More conversions from page->lru to page->[pcp_list|buddy_list]
o Additional test results in changelogs

Changelog since v1
o Fix unsafe RT locking scheme
o Use spin_trylock on UP PREEMPT_RT

This series has the same intent as Nicolas' series "mm/page_alloc: Remote per-cpu lists drain support" -- avoid interference of a high priority task due to a workqueue item draining per-cpu page lists. While many workloads can tolerate a brief interruption, it may be cause a real-time task runnning on a NOHZ_FULL CPU to miss a deadline and at minimum, the draining in non-deterministic.

Currently an IRQ-safe local_lock protects the page allocator per-cpu lists. The local_lock on its own prevents migration and the IRQ disabling protects from corruption due to an interrupt arriving while a page allocation is in progress. The locking is inherently unsafe for remote access unless the CPU is hot-removed.

This series adjusts the locking. A spinlock is added to struct per_cpu_pages to protect the list contents while local_lock_irq continues to prevent migration and IRQ reentry. This allows a remote CPU to safely drain a remote per-cpu list.

Quoted 7 lines

This series is a partial series. Follow-on work should allow the local_irq_save to be converted to a local_irq to avoid IRQs being disabled/enabled in most cases. Consequently, there are some TODO comments highlighting the places that would change if local_irq was used. However, there are enough corner cases that it deserves a series on its own separated by one kernel release and the priority right now is to avoid interference of high priority tasks.

Reverting the whole series fixed an issue that offlining a memory section blocking for hours on today's linux-next tree.

__wait_rcu_gp
synchronize_rcu at kernel/rcu/tree.c:3915 lru_cache_disable at mm/swap.c:886 __alloc_contig_migrate_range at mm/page_alloc.c:9078 isolate_single_pageblock at mm/page_isolation.c:405 start_isolate_page_range
offline_pages
memory_subsys_offline
device_offline
online_store
dev_attr_store
sysfs_kf_write
kernfs_fop_write_iter
new_sync_write
vfs_write
ksys_write
__arm64_sys_write
invoke_syscall
el0_svc_common.constprop.0
do_el0_svc
el0_svc
el0t_64_sync_handler
el0t_64_sync

Quoted 4 lines

For full disclosure, I have also reverted the commit 0d523026abd4 ("mm/page_alloc: fix tracepoint mm_page_alloc_zone_locked()"), so the series can be reverted cleanly. But, I can't see how the commit 0d523026abd4 could cause this issue at all.

Hi Qian,

The issue is probably due to lack of the following:

https://lore.kernel.org/linux-mm/YmrWK%[email protected]/

Can you please give the patch on the URL a try?

Thanks!

Changelog since v2
o More conversions from page->lru to page->[pcp_list|buddy_list]
o Additional test results in changelogs

Changelog since v1
o Fix unsafe RT locking scheme
o Use spin_trylock on UP PREEMPT_RT

This series has the same intent as Nicolas' series "mm/page_alloc: Remote per-cpu lists drain support" -- avoid interference of a high priority task due to a workqueue item draining per-cpu page lists. While many workloads can tolerate a brief interruption, it may be cause a real-time task runnning on a NOHZ_FULL CPU to miss a deadline and at minimum, the draining in non-deterministic.

Currently an IRQ-safe local_lock protects the page allocator per-cpu lists. The local_lock on its own prevents migration and the IRQ disabling protects from corruption due to an interrupt arriving while a page allocation is in progress. The locking is inherently unsafe for remote access unless the CPU is hot-removed.

This series adjusts the locking. A spinlock is added to struct per_cpu_pages to protect the list contents while local_lock_irq continues to prevent migration and IRQ reentry. This allows a remote CPU to safely drain a remote per-cpu list.

This series is a partial series. Follow-on work should allow the local_irq_save to be converted to a local_irq to avoid IRQs being disabled/enabled in most cases. Consequently, there are some TODO comments highlighting the places that would change if local_irq was used. However, there are enough corner cases that it deserves a series on its own separated by one kernel release and the priority right now is to avoid interference of high priority tasks.

Changelog since v2
o More conversions from page->lru to page->[pcp_list|buddy_list]
o Additional test results in changelogs

Changelog since v1
o Fix unsafe RT locking scheme
o Use spin_trylock on UP PREEMPT_RT

This series has the same intent as Nicolas' series "mm/page_alloc: Remote per-cpu lists drain support" -- avoid interference of a high priority task due to a workqueue item draining per-cpu page lists. While many workloads can tolerate a brief interruption, it may be cause a real-time task runnning on a NOHZ_FULL CPU to miss a deadline and at minimum, the draining in non-deterministic.

Currently an IRQ-safe local_lock protects the page allocator per-cpu lists. The local_lock on its own prevents migration and the IRQ disabling protects from corruption due to an interrupt arriving while a page allocation is in progress. The locking is inherently unsafe for remote access unless the CPU is hot-removed.

This series adjusts the locking. A spinlock is added to struct per_cpu_pages to protect the list contents while local_lock_irq continues to prevent migration and IRQ reentry. This allows a remote CPU to safely drain a remote per-cpu list.

This series is a partial series. Follow-on work should allow the local_irq_save to be converted to a local_irq to avoid IRQs being disabled/enabled in most cases. Consequently, there are some TODO comments highlighting the places that would change if local_irq was used. However, there are enough corner cases that it deserves a series on its own separated by one kernel release and the priority right now is to avoid interference of high priority tasks.

Patch 1 is a cosmetic patch to clarify when page->lru is storing buddy pages
and when it is storing per-cpu pages.

Patch 2 shrinks per_cpu_pages to make room for a spin lock. Strictly speaking
this is not necessary but it avoids per_cpu_pages consuming another cache line.

Patch 3 is a preparation patch to avoid code duplication.

Patch 4 is a simple micro-optimisation that improves code flow necessary for
a later patch to avoid code duplication.

Patch 5 uses a spin_lock to protect the per_cpu_pages contents while still
relying on local_lock to prevent migration, stabilise the pcp lookup and prevent IRQ reentrancy.

Patch 6 remote drains per-cpu pages directly instead of using a workqueue.

Quoted 50 lines · Mel Gorman

Changelog since v2
o More conversions from page->lru to page->[pcp_list|buddy_list]
o Additional test results in changelogs

Changelog since v1
o Fix unsafe RT locking scheme
o Use spin_trylock on UP PREEMPT_RT

This series has the same intent as Nicolas' series "mm/page_alloc: Remote per-cpu lists drain support" -- avoid interference of a high priority task due to a workqueue item draining per-cpu page lists. While many workloads can tolerate a brief interruption, it may be cause a real-time task runnning on a NOHZ_FULL CPU to miss a deadline and at minimum, the draining in non-deterministic.

Currently an IRQ-safe local_lock protects the page allocator per-cpu lists. The local_lock on its own prevents migration and the IRQ disabling protects from corruption due to an interrupt arriving while a page allocation is in progress. The locking is inherently unsafe for remote access unless the CPU is hot-removed.

This series adjusts the locking. A spinlock is added to struct per_cpu_pages to protect the list contents while local_lock_irq continues to prevent migration and IRQ reentry. This allows a remote CPU to safely drain a remote per-cpu list.

This series is a partial series. Follow-on work should allow the local_irq_save to be converted to a local_irq to avoid IRQs being disabled/enabled in most cases. Consequently, there are some TODO comments highlighting the places that would change if local_irq was used. However, there are enough corner cases that it deserves a series on its own separated by one kernel release and the priority right now is to avoid interference of high priority tasks.

Patch 1 is a cosmetic patch to clarify when page->lru is storing buddy pages
and when it is storing per-cpu pages.

Patch 2 shrinks per_cpu_pages to make room for a spin lock. Strictly speaking
this is not necessary but it avoids per_cpu_pages consuming another cache line.

Patch 3 is a preparation patch to avoid code duplication.

Patch 4 is a simple micro-optimisation that improves code flow necessary for
a later patch to avoid code duplication.

Patch 5 uses a spin_lock to protect the per_cpu_pages contents while still
relying on local_lock to prevent migration, stabilise the pcp lookup and prevent IRQ reentrancy.

Quoted 1 line

Patch 6 remote drains per-cpu pages directly instead of using a workqueue.

Mel, we saw spontanous "mm_percpu_wq" crash on today's linux-next tree while running CPU offlining/onlining, and wondering if you have any thoughts?

Do you think it's related to the series and if so why? From the warning, it's not obvious to me why it would be given that it's a warning about a task not being inactive when it is expected to be.