kfence: optimize timer scheduling

We have observed that mostly-idle systems with KFENCE enabled wake up otherwise idle CPUs, preventing such to enter a lower power state. Debugging revealed that KFENCE spends too much active time in toggle_allocation_gate().

While the first version of KFENCE was using all the right bits to be scheduling optimal, and thus power efficient, by simply using wait_event() + wake_up(), that code was unfortunately removed.

As KFENCE was exposed to various different configs and tests, the scheduling optimal code slowly disappeared. First because of hung task warnings, and finally because of deadlocks when an allocation is made by timer code with debug objects enabled. Clearly, the "fixes" were not too friendly for devices that want to be power efficient.

Therefore, let's try a little harder to fix the hung task and deadlock problems that we have with wait_event() + wake_up(), while remaining as scheduling friendly and power efficient as possible.

Crucially, we need to defer the wake_up() to an irq_work, avoiding any potential for deadlock.

The result with this series is that on the devices where we observed a power regression, power usage returns back to baseline levels.

Marco Elver (3):
kfence: await for allocation using wait_event
kfence: maximize allocation wait timeout duration
kfence: use power-efficient work queue to run delayed work

 lib/Kconfig.kfence |  1 +
 mm/kfence/core.c   | 71 +++++++++++++++++++++++++++++++++++-----------
 2 files changed, 55 insertions(+), 17 deletions(-)

2.31.1.368.gbe11c130af-goog

On mostly-idle systems, we have observed that toggle_allocation_gate() is a cause of frequent wake-ups, preventing an otherwise idle CPU to go into a lower power state.

A late change in KFENCE's development, due to a potential deadlock [1], required changing the scheduling-friendly wait_event_timeout() and wake_up() to an open-coded wait-loop using schedule_timeout().
[1] https://lkml.kernel.org/r/[email protected]

To avoid unnecessary wake-ups, switch to using wait_event_timeout().

Unfortunately, we still cannot use a version with direct wake_up() in __kfence_alloc() due to the same potential for deadlock as in [1]. Instead, add a level of indirection via an irq_work that is scheduled if we determine that the kfence_timer requires a wake_up().

---
 lib/Kconfig.kfence |  1 +
 mm/kfence/core.c   | 58 +++++++++++++++++++++++++++++++++-------------
 2 files changed, 43 insertions(+), 16 deletions(-)

index 78f50ccb3b45..e641add33947 100644
@@ -7,6 +7,7 @@ menuconfig KFENCE
 	bool "KFENCE: low-overhead sampling-based memory safety error detector"
 	depends on HAVE_ARCH_KFENCE && (SLAB || SLUB)
 	select STACKTRACE
+	select IRQ_WORK
 	help
 	  KFENCE is a low-overhead sampling-based detector of heap out-of-bounds
 	  access, use-after-free, and invalid-free errors. KFENCE is designed

2.31.1.368.gbe11c130af-goog

Yes it is. We want to avoid the unconditional irq_work in __kfence_alloc(). When the system is under load doing frequent allocations, at least in my tests this avoids the irq_work almost always. Without the irq_work you'd be correct of course.

And in case this is about the smp_mb() here, yes it definitely is required. We *must* order the write of kfence_timer_waiting *before* the check of kfence_allocation_gate, which wait_event() does before anything else (including changing the state). Otherwise the write may be reordered after the read, and we could potentially never wake up because __kfence_alloc() not waking us.

This is documented in __kfence_alloc().

One of the reasons why wait_event() checks the wait condition before anything else is no waker can help waiter before waiter gets themselves on the wait queue head list. Nor can waker without scheduling on the waiter side, even if the waiter is sitting on the list. So the mb cannot make sense without scheduling, let alone the mb in wait_event().

You are right of course. I just went and expanded wait_event():

	do {
		if (atomic_read(&kfence_allocation_gate))
			break;
		init_wait_entry(...);
		for (;;) {
			long __int = prepare_to_wait_event(...);
			if (atomic_read(&kfence_allocation_gate))
				break;
			...
			schedule();
		}
		finish_wait(...);
	} while (0);

I just kept looking at the first check. Before the wait entry setup and finally the second re-check after the mb() in prepare_to_wait_event(). So removing the smp_mb() is indeed fine given the second re-check is ordered after the write per state change mb().

And then I just saw we should just use waitqueue_active() anyway, which documents this, too.

I'll send a v2.

Thank you!

-- Marco

The allocation wait timeout was initially added because of warnings due
to CONFIG_DETECT_HUNG_TASK=y [1]. While the 1 sec timeout is sufficient
to resolve the warnings (given the hung task timeout must be 1 sec or
larger) it may cause unnecessary wake-ups if the system is idle.
[1] https://lkml.kernel.org/r/CADYN=9J0DQhizAGB0-jz4HOBBh+05kMBXb4c0cXMS7Qi5NAJiw@mail.gmail.com

Fix it by computing the timeout duration in terms of the current sysctl_hung_task_timeout_secs value.

---
 mm/kfence/core.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

index 5f0a56041549..73e7b621fb36 100644
@@ -20,6 +20,7 @@
 #include <linux/moduleparam.h>
 #include <linux/random.h>
 #include <linux/rcupdate.h>
+#include <linux/sched/sysctl.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>

@@ -626,7 +627,16 @@ static void toggle_allocation_gate(struct work_struct *work)
 
 	WRITE_ONCE(kfence_timer_waiting, true);
 	smp_mb(); /* See comment in __kfence_alloc(). */
-	wait_event_timeout(allocation_wait, atomic_read(&kfence_allocation_gate), HZ);
+	if (sysctl_hung_task_timeout_secs) {
+		/*
+		 * During low activity with no allocations we might wait a
+		 * while; let's avoid the hung task warning.
+		 */
+		wait_event_timeout(allocation_wait, atomic_read(&kfence_allocation_gate),
+				   sysctl_hung_task_timeout_secs * HZ / 2);
+	} else {
+		wait_event(allocation_wait, atomic_read(&kfence_allocation_gate));
+	}
 	smp_store_release(&kfence_timer_waiting, false); /* Order after wait_event(). */
 
 	/* Disable static key and reset timer. */

2.31.1.368.gbe11c130af-goog

Use the power-efficient work queue, to avoid the pathological case where we keep pinning ourselves on the same possibly idle CPU on systems that want to be power-efficient [1].
[1] https://lwn.net/Articles/731052/

---
 mm/kfence/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 73e7b621fb36..7e20cd9690a2 100644
@@ -642,7 +642,8 @@ static void toggle_allocation_gate(struct work_struct *work)
 	/* Disable static key and reset timer. */
 	static_branch_disable(&kfence_allocation_key);
 #endif
-	schedule_delayed_work(&kfence_timer, msecs_to_jiffies(kfence_sample_interval));
+	queue_delayed_work(system_power_efficient_wq, &kfence_timer,
+			   msecs_to_jiffies(kfence_sample_interval));
 }
 static DECLARE_DELAYED_WORK(kfence_timer, toggle_allocation_gate);
 

@@ -671,7 +672,7 @@ void __init kfence_init(void)
 	}
 
 	WRITE_ONCE(kfence_enabled, true);
-	schedule_delayed_work(&kfence_timer, 0);
+	queue_delayed_work(system_power_efficient_wq, &kfence_timer, 0);
 	pr_info("initialized - using %lu bytes for %d objects at 0x%p-0x%p\n", KFENCE_POOL_SIZE,
 		CONFIG_KFENCE_NUM_OBJECTS, (void *)__kfence_pool,
 		(void *)(__kfence_pool + KFENCE_POOL_SIZE));

2.31.1.368.gbe11c130af-goog