[PATCH] kunit: Enforce task execution in {soft,hard}irq contexts

[David Gow]

The kunit_run_irq_test() helper allows a function to be run in hardirq
and softirq contexts (in addition to the task context). It does this by
running the user-provided function concurrently in the three contexts,
until either a timeout has expired or a number of iterations have
completed in the normal task context.

However, on setups where the initialisation of the hardirq and softirq
contexts (or, indeed, the scheduling of those tasks) is significantly
slower than the function execution, it's possible for that number of
iterations to be exceeded before any runs in irq contexts actually
occur. This occurs with the polyval.test_polyval_preparekey_in_irqs
test, which runs 20000 iterations of the relatively fast preparekey
function, and therefore fails often under many UML, 32-bit arm, m68k and
other environments.

Instead, ensure that the max_iterations limit counts executions in all
three contexts, and requires at least one of each. This will cause the
test to continue iterating until at least the irq contexts have been
tested, or the 1s wall-clock limit has been exceeded. This causes the
test to pass in all of my environments.

In so doing, we also update the task counters to atomic ints, to better
match both the 'int' max_iterations input, and to ensure they are
correctly updated across contexts.

Finally, we also fix a few potential assertion messages to be
less-specific to the original crypto usecases.

Fixes: b41dc83f0790 ("kunit, lib/crypto: Move run_irq_test() to common header")
Signed-off-by: David Gow <davi...@google.com>
---
include/kunit/run-in-irq-context.h | 41 ++++++++++++++++++++----------
1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/include/kunit/run-in-irq-context.h b/include/kunit/run-in-irq-context.h
index 108e96433ea4..4d25aee0de6e 100644
--- a/include/kunit/run-in-irq-context.h
+++ b/include/kunit/run-in-irq-context.h
@@ -20,8 +20,8 @@ struct kunit_irq_test_state {
bool task_func_reported_failure;
bool hardirq_func_reported_failure;
bool softirq_func_reported_failure;
- unsigned long hardirq_func_calls;
- unsigned long softirq_func_calls;
+ atomic_t hardirq_func_calls;
+ atomic_t softirq_func_calls;
struct hrtimer timer;
struct work_struct bh_work;
};
@@ -32,7 +32,7 @@ static enum hrtimer_restart kunit_irq_test_timer_func(struct hrtimer *timer)
container_of(timer, typeof(*state), timer);

WARN_ON_ONCE(!in_hardirq());
- state->hardirq_func_calls++;
+ atomic_inc(&state->hardirq_func_calls);

if (!state->func(state->test_specific_state))
state->hardirq_func_reported_failure = true;
@@ -48,7 +48,7 @@ static void kunit_irq_test_bh_work_func(struct work_struct *work)
container_of(work, typeof(*state), bh_work);

WARN_ON_ONCE(!in_serving_softirq());
- state->softirq_func_calls++;
+ atomic_inc(&state->softirq_func_calls);

if (!state->func(state->test_specific_state))
state->softirq_func_reported_failure = true;
@@ -59,7 +59,10 @@ static void kunit_irq_test_bh_work_func(struct work_struct *work)
* hardirq context concurrently, and reports a failure to KUnit if any
* invocation of @func in any context returns false. @func is passed
* @test_specific_state as its argument. At most 3 invocations of @func will
- * run concurrently: one in each of task, softirq, and hardirq context.
+ * run concurrently: one in each of task, softirq, and hardirq context. @func
+ * will continue running until either @max_iterations calls have been made (so
+ * long as at least one each runs in task, softirq, and hardirq contexts), or
+ * one second has passed.
*
* The main purpose of this interrupt context testing is to validate fallback
* code paths that run in contexts where the normal code path cannot be used,
@@ -85,6 +88,10 @@ static inline void kunit_run_irq_test(struct kunit *test, bool (*func)(void *),
.test_specific_state = test_specific_state,
};
unsigned long end_jiffies;
+ int hardirq_calls, softirq_calls;
+ bool allctx = false;
+
+ max_iterations = 1;

/*
* Set up a hrtimer (the way we access hardirq context) and a work
@@ -94,14 +101,22 @@ static inline void kunit_run_irq_test(struct kunit *test, bool (*func)(void *),
CLOCK_MONOTONIC, HRTIMER_MODE_REL_HARD);
INIT_WORK_ONSTACK(&state.bh_work, kunit_irq_test_bh_work_func);

- /* Run for up to max_iterations or 1 second, whichever comes first. */
+ /* Run for up to max_iterations (including at least one task, softirq,
+ * and hardirq), or 1 second, whichever comes first.
+ */
end_jiffies = jiffies + HZ;
hrtimer_start(&state.timer, KUNIT_IRQ_TEST_HRTIMER_INTERVAL,
HRTIMER_MODE_REL_HARD);
- for (int i = 0; i < max_iterations && !time_after(jiffies, end_jiffies);
- i++) {
+ for (int task_calls = 0, calls = 0;
+ ((calls < max_iterations) || !allctx) && !time_after(jiffies, end_jiffies);
+ task_calls++) {
if (!func(test_specific_state))
state.task_func_reported_failure = true;
+
+ hardirq_calls = atomic_read(&state.hardirq_func_calls);
+ softirq_calls = atomic_read(&state.softirq_func_calls);
+ calls = task_calls + hardirq_calls + softirq_calls;
+ allctx = (task_calls > 0) && (hardirq_calls > 0) && (softirq_calls > 0);
}

/* Cancel the timer and work. */
@@ -109,21 +124,21 @@ static inline void kunit_run_irq_test(struct kunit *test, bool (*func)(void *),
flush_work(&state.bh_work);

/* Sanity check: the timer and BH functions should have been run. */
- KUNIT_EXPECT_GT_MSG(test, state.hardirq_func_calls, 0,
+ KUNIT_EXPECT_GT_MSG(test, atomic_read(&state.hardirq_func_calls), 0,
"Timer function was not called");
- KUNIT_EXPECT_GT_MSG(test, state.softirq_func_calls, 0,
+ KUNIT_EXPECT_GT_MSG(test, atomic_read(&state.softirq_func_calls), 0,
"BH work function was not called");

/* Check for incorrect hash values reported from any context. */
KUNIT_EXPECT_FALSE_MSG(
test, state.task_func_reported_failure,
- "Incorrect hash values reported from task context");
+ "Failure reported from task context");
KUNIT_EXPECT_FALSE_MSG(
test, state.hardirq_func_reported_failure,
- "Incorrect hash values reported from hardirq context");
+ "Failure reported from hardirq context");
KUNIT_EXPECT_FALSE_MSG(
test, state.softirq_func_reported_failure,
- "Incorrect hash values reported from softirq context");
+ "Failure reported from softirq context");
}

#endif /* _KUNIT_RUN_IN_IRQ_CONTEXT_H */
--
2.52.0.322.g1dd061c0dc-goog

[David Gow]

Changes since v1:
https://lore.kernel.org/all/20251219080850.9...@google.com/
- Remove a leftover debug line which forced max_iterations to 1.


include/kunit/run-in-irq-context.h | 39 ++++++++++++++++++++----------
1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/include/kunit/run-in-irq-context.h b/include/kunit/run-in-irq-context.h
index 108e96433ea4..84694f383e37 100644

@@ -85,6 +88,8 @@ static inline void kunit_run_irq_test(struct kunit *test, bool (*func)(void *),

.test_specific_state = test_specific_state,
};
unsigned long end_jiffies;
+ int hardirq_calls, softirq_calls;
+ bool allctx = false;

/*
* Set up a hrtimer (the way we access hardirq context) and a work

@@ -94,14 +99,22 @@ static inline void kunit_run_irq_test(struct kunit *test, bool (*func)(void *),

CLOCK_MONOTONIC, HRTIMER_MODE_REL_HARD);
INIT_WORK_ONSTACK(&state.bh_work, kunit_irq_test_bh_work_func);

- /* Run for up to max_iterations or 1 second, whichever comes first. */
+ /* Run for up to max_iterations (including at least one task, softirq,
+ * and hardirq), or 1 second, whichever comes first.
+ */
end_jiffies = jiffies + HZ;
hrtimer_start(&state.timer, KUNIT_IRQ_TEST_HRTIMER_INTERVAL,
HRTIMER_MODE_REL_HARD);
- for (int i = 0; i < max_iterations && !time_after(jiffies, end_jiffies);
- i++) {
+ for (int task_calls = 0, calls = 0;
+ ((calls < max_iterations) || !allctx) && !time_after(jiffies, end_jiffies);
+ task_calls++) {
if (!func(test_specific_state))
state.task_func_reported_failure = true;
+
+ hardirq_calls = atomic_read(&state.hardirq_func_calls);
+ softirq_calls = atomic_read(&state.softirq_func_calls);
+ calls = task_calls + hardirq_calls + softirq_calls;
+ allctx = (task_calls > 0) && (hardirq_calls > 0) && (softirq_calls > 0);
}

/* Cancel the timer and work. */

@@ -109,21 +122,21 @@ static inline void kunit_run_irq_test(struct kunit *test, bool (*func)(void *),

[David Gow]

.. snip ..

> @@ -85,6 +88,10 @@ static inline void kunit_run_irq_test(struct kunit *test, bool (*func)(void *),
> .test_specific_state = test_specific_state,
> };
> unsigned long end_jiffies;
> + int hardirq_calls, softirq_calls;
> + bool allctx = false;
> +
> + max_iterations = 1;

Whoops! Left this debug line in.

v2 fixes it: https://lore.kernel.org/all/20251219085259.1...@google.com/

-- David

[Eric Biggers]

Thanks! I'd like to take this through libcrypto-fixes, if that's okay
with the KUnit folks (acks would be appreciated). kunit_run_irq_test()
is a recently-added helper function used by the crypto and CRC tests.

For the Fixes commit, we should use the initial addition of this code:

Fixes: 950a81224e8b ("lib/crypto: tests: Add hash-test-template.h and gen-hash-testvecs.py")

- Eric

[David Gow]

No worries: I don't think there are any other tests using this at the
moment; certainly nothing that needs the fix.

> For the Fixes commit, we should use the initial addition of this code:
>
> Fixes: 950a81224e8b ("lib/crypto: tests: Add hash-test-template.h and gen-hash-testvecs.py")

Sounds good: I'm sure it'll be a tiny bit trickier to backport this
(and, e.g, the changes to the error messages aren't useful) to older
branches (and certainly I never saw any issues prior to the polyval
tests), but it is technically more correct, which is the best kind of
correct. :-)

Cheers,
-- David