On Wed, 21 Aug 2024 at 22:43, Michal Wajdeczko
<michal.wajdeczko@xxxxxxxxx> wrote:
>
> Currently, the 'static stub' API only allows function redirection
> for calls made from the kthread of the current test, which prevents
> the use of this functionalty when the tested code is also used by
> other threads, outside of the KUnit test, like from the workqueue.
>
> Add another set of macros to allow redirection to the replacement
> functions, which, unlike the KUNIT_STATIC_STUB_REDIRECT, will
> affect all calls done during the test execution.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> ---
> Cc: David Gow <davidgow@xxxxxxxxxx>
> Cc: Daniel Latypov <dlatypov@xxxxxxxxxx>
> Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> ---
Thanks for sending this in: we really do need a way to use stubs from
other threads.
In general, I think there are two possible ways of implementing that:
- Make a version of the stubs which don't need a KUnit context (i.e.,
this patch), or
- Have a way of extending the KUnit context to other threads.
Personally, I'd prefer the latter if it were feasible, as it is both
cleaner from a user point of view (we don't need two variants of the
same thing), and extends more easily to features beyond stubs.
However, there are a few downsides:
- We'd need to find a way of handling the case where the test function
returns while there's still a background thread happening.
- In general, KUnit would need to be audited for thread safety for
those macros (I don't think it's too bad, but worth checking),
- We'd need a way of passing the KUnit context to the new thread,
which might be difficult to make pleasant.
- We'd need to handle cases where a thread is only partly running test
code, or otherwise doesn't create its threads directly (e.g.,
workqueues, rcu, etc)
- What should happen if an assertion fails on another thread — does
the failing thread quit, does the original thread quit, both?
In short, it's a complicated enough situation that I doubt we'd solve
all of those problems cleanly or quickly, so this patch is probably
the better option. Regardless, we need a story for what the thread
safety of this is -- can you activate/deactivate stubs while the
stubbed function is being called. (My gut feeling is "this should be
possible, but is probably ill-advised" is what we should aim for,
which probably requires making sure the stub update is done
atomically.
More rambling below, but I think this is probably good once the
atomic/thread-safety stuff is either sorted out or at least
documented. As for the name, how about KUNIT_GLOBAL_STUB_REDIRECT()?
I'm okay with FIXED_STUB if you prefer it, though.
Cheers,
-- David
> include/kunit/static_stub.h | 80 +++++++++++++++++++++++++++++++++++++
> lib/kunit/static_stub.c | 21 ++++++++++
> 2 files changed, 101 insertions(+)
>
> diff --git a/include/kunit/static_stub.h b/include/kunit/static_stub.h
> index bf940322dfc0..3dd98c8f3f1f 100644
> --- a/include/kunit/static_stub.h
> +++ b/include/kunit/static_stub.h
> @@ -12,6 +12,7 @@
>
> /* If CONFIG_KUNIT is not enabled, these stubs quietly disappear. */
> #define KUNIT_STATIC_STUB_REDIRECT(real_fn_name, args...) do {} while (0)
> +#define KUNIT_FIXED_STUB_REDIRECT(stub, args...) do {} while (0)
>
> #else
>
> @@ -109,5 +110,84 @@ void __kunit_activate_static_stub(struct kunit *test,
> */
> void kunit_deactivate_static_stub(struct kunit *test, void *real_fn_addr);
>
> +/**
> + * KUNIT_FIXED_STUB_REDIRECT() - Call a fixed function stub if activated.
> + * @stub: The location of the function stub pointer
> + * @args: All of the arguments passed to this stub
> + *
> + * This is a function prologue which is used to allow calls to the current
> + * function to be redirected if a KUnit is running. If the stub is NULL or
> + * the KUnit is not running the function will continue execution as normal.
> + *
> + * The function stub pointer must be stored in a place that is accessible both
> + * from the test code that will activate this stub and from the function where
> + * we will do the redirection.
> + *
> + * Unlike the KUNIT_STATIC_STUB_REDIRECT(), this redirection will work
> + * even if the caller is not in a KUnit context (like a worker thread).
> + *
> + * Example:
> + *
> + * .. code-block:: c
> + *
> + * static int (*func_stub)(int n);
> + *
> + * int real_func(int n)
> + * {
> + * KUNIT_FIXED_STUB_REDIRECT(func_stub, n);
> + * return n + 1;
> + * }
> + *
> + * int replacement_func(int n)
> + * {
> + * return n + 100;
> + * }
> + *
> + * void example_test(struct kunit *test)
> + * {
> + * KUNIT_EXPECT_EQ(test, real_func(1), 2);
> + * func_stub = replacement_func;
> + * KUNIT_EXPECT_EQ(test, real_func(1), 101);
> + * }
> + */
> +#define KUNIT_FIXED_STUB_REDIRECT(stub, args...) do { \
> + typeof(stub) replacement = (stub); \
> + if (kunit_is_running()) { \
> + if (unlikely(replacement)) { \
> + pr_info(KUNIT_SUBTEST_INDENT "# %s: calling stub %ps\n", \
I suspect we want to at least make the logging here optional,
particularly since it doesn't go into the test log.
> + __func__, replacement); \
> + return replacement(args); \
> + } \
> + } \
> +} while (0)
> +
> +void __kunit_activate_fixed_stub(struct kunit *test, void *stub_ptr, void *replacement_func);
> +
> +/**
> + * kunit_activate_fixed_stub() - Setup a fixed function stub.
> + * @test: Test case that wants to activate a fixed function stub
> + * @stub: The location of the function stub pointer
> + * @replacement: The replacement function
> + *
> + * This helper setups a function stub with the replacement function.
> + * It will also automatically restore stub to NULL at the test end.
> + */
> +#define kunit_activate_fixed_stub(test, stub, replacement) do { \
Do we want to actually hook this into the struct kunit here? I suspect
we do, but it does mean that this has to either be called from the
main thread, or the struct kunit* needs to be passed around.
> + typecheck_pointer(stub); \
> + typecheck_fn(typeof(stub), replacement); \
> + typeof(stub) *stub_ptr = &(stub); \
> + __kunit_activate_fixed_stub((test), stub_ptr, (replacement)); \
> +} while (0)
> +
> +/**
> + * kunit_deactivate_fixed_stub() - Disable a fixed function stub.
> + * @test: Test case that wants to deactivate a fixed function stub (unused for now)
> + * @stub: The location of the function stub pointer
> + */
> +#define kunit_deactivate_fixed_stub(test, stub) do { \
> + typecheck(struct kunit *, (test)); \
> + (stub) = NULL; \
> +} while (0)
> +
> #endif
> #endif
> diff --git a/lib/kunit/static_stub.c b/lib/kunit/static_stub.c
> index 92b2cccd5e76..1b50cf457e89 100644
> --- a/lib/kunit/static_stub.c
> +++ b/lib/kunit/static_stub.c
> @@ -121,3 +121,24 @@ void __kunit_activate_static_stub(struct kunit *test,
> }
> }
> EXPORT_SYMBOL_GPL(__kunit_activate_static_stub);
> +
> +static void nullify_stub_ptr(void *data)
> +{
> + void **ptr = data;
> +
> + *ptr = NULL;
As below, does this need to be atomic?
> +}
> +
> +/*
> + * Helper function for kunit_activate_static_stub(). The macro does
> + * typechecking, so use it instead.
> + */
> +void __kunit_activate_fixed_stub(struct kunit *test, void *stub_ptr, void *replacement_func)
> +{
> + void **stub = stub_ptr;
> +
> + KUNIT_ASSERT_NOT_NULL(test, stub_ptr);
> + *stub = replacement_func;
Do we need to do something here to make this atomic? At the very
least, I think we want to READ_ONCE()/WRITE_ONCE() these, but even
then we could end up with torn writes, I think.
In general, though, what's the rule around activating a stub from a
different thread to it being called? I thinki we definitely want to
allow it, but _maybe_ we can get away with saying that the stub can't
be activated/deactivated concurrently with being used?
> + KUNIT_ASSERT_EQ(test, 0, kunit_add_action_or_reset(test, nullify_stub_ptr, stub_ptr));
Again, we probably need to emphasise that any work done on another
thread needs to be complete before the test ends on the main thread.
This isn't specific to this feature, though.
> +}
> +EXPORT_SYMBOL_GPL(__kunit_activate_fixed_stub);
> --
> 2.43.0
>
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
