On 22.08.2024 08:14, David Gow wrote:
> 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.
v2 is trying to wait until all active calls finishes before changing or
deactivating this particular stub, I've also added simple test for it:
KTAP version 1
1..1
KTAP version 1
# Subtest: kunit_global_stub
# module: kunit_test
1..2
# kunit_global_stub_test_slow_deactivate: real_func: redirecting to
slow_replacement_func
# kunit_global_stub_test_slow_deactivate: real_func: redirecting to
slow_replacement_func
# kunit_global_stub_test_slow_deactivate: waiting for
slow_replacement_func
# kunit_global_stub_test_slow_deactivate.speed: slow
ok 1 kunit_global_stub_test_slow_deactivate
# kunit_global_stub_test_slow_replace: real_func: redirecting to
slow_replacement_func
# kunit_global_stub_test_slow_replace: real_func: redirecting to
slow_replacement_func
# kunit_global_stub_test_slow_replace: waiting for slow_replacement_func
# kunit_global_stub_test_slow_replace: real_func: redirecting to
other_replacement_func
# kunit_global_stub_test_slow_replace.speed: slow
ok 2 kunit_global_stub_test_slow_replace
# kunit_global_stub: pass:2 fail:0 skip:0 total:2
# Totals: pass:2 fail:0 skip:0 total:2
>
> 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.
renamed to KUNIT_GLOBAL_STUB_REDIRECT
>
> 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.
as in v2 there is a dedicated struct/union to hold the address and the
type of the replacement function, I've also added there a kunit* so is
now everything is logged to 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
yes, as we want automatic deactivation of the stub to avoid leaving
active stub after the test ends (may be important if using KUnit for
some live testing, like we do in Xe)
> 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.
we usually enable/activate stubs from the test case code, where kunit*
is already there, so this just enforces that redirection could be
controlled only from the main KUnit thread
>
>> + 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.
v2 is using READ_ONCE()/WRITE_ONCE() to update stub pointers and
atomic_t to track whether stub is disabled(0) or activated(1) or in_use(2+)
>
> 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.
v2 is trying to care of it and waits until any active calls finish
>
>> +}
>> +EXPORT_SYMBOL_GPL(__kunit_activate_fixed_stub);
>
>> --
>> 2.43.0
>>
