On Wed, Aug 21, 2024 at 10:43 AM Michal Wajdeczko michal.wajdeczko@intel.com wrote:

Wrap uses of the static key 'kunit_running' into a helper macro to allow future checks to be placed in the code residing outside of the CONFIG_KUNIT. We will start using this in upcoming patch.

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com

Hello!

I am good with this. It is definitely a prettier way to access kunit_running.

Reviewed-by: Rae Moar rmoar@google.com

Thanks!

-Rae

---

Cc: Daniel Latypov dlatypov@google.com Cc: David Gow davidgow@google.com Cc: Lucas De Marchi lucas.demarchi@intel.com

---

include/kunit/test-bug.h | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h index 47aa8f21ccce..e8ea3bab7250 100644 --- a/include/kunit/test-bug.h +++ b/include/kunit/test-bug.h @@ -25,6 +25,13 @@ extern struct kunit_hooks_table { void *(*get_static_stub_address)(struct kunit *test, void *real_fn_addr); } kunit_hooks;

+/**

• • kunit_is_running() - True, if KUnit test is currently running.
• • If CONFIG_KUNIT is not enabled, it will compile down to a false.
• */

+#define kunit_is_running() static_branch_unlikely(&kunit_running)

/**

• kunit_get_current_test() - Return a pointer to the currently running

•                       KUnit test.
  

@@ -40,7 +47,7 @@ extern struct kunit_hooks_table { */ static inline struct kunit *kunit_get_current_test(void) {

•   if (!static_branch_unlikely(&kunit_running))
  

•   if (!kunit_is_running())
            return NULL;
  
    return current->kunit_test;
  

@@ -53,7 +60,7 @@ static inline struct kunit *kunit_get_current_test(void)

• If a KUnit test is running in the current task, mark that test as failed.

*/ #define kunit_fail_current_test(fmt, ...) do { \

•           if (static_branch_unlikely(&kunit_running)) {                   \
  

•           if (kunit_is_running()) {                                       \
                    /* Guaranteed to be non-NULL when kunit_running true*/  \
                    kunit_hooks.fail_current_test(__FILE__, __LINE__,       \
                                              fmt, ##__VA_ARGS__);          \
  

@@ -64,6 +71,7 @@ static inline struct kunit *kunit_get_current_test(void)

static inline struct kunit *kunit_get_current_test(void) { return NULL; }

+#define kunit_is_running() false #define kunit_fail_current_test(fmt, ...) do {} while (0)

#endif

2.43.0

-- You received this message because you are subscribed to the Google Groups "KUnit Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20240821144305.1958-2-michal.waj....

On Wed, 21 Aug 2024 at 22:43, Michal Wajdeczko michal.wajdeczko@intel.com wrote:

Wrap uses of the static key 'kunit_running' into a helper macro to allow future checks to be placed in the code residing outside of the CONFIG_KUNIT. We will start using this in upcoming patch.

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com

Cc: Daniel Latypov dlatypov@google.com Cc: David Gow davidgow@google.com Cc: Lucas De Marchi lucas.demarchi@intel.com

---

This is a big improvement, thanks!

Reviewed-by: David Gow davidgow@google.com

Cheers, -- David

include/kunit/test-bug.h | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h index 47aa8f21ccce..e8ea3bab7250 100644 --- a/include/kunit/test-bug.h +++ b/include/kunit/test-bug.h @@ -25,6 +25,13 @@ extern struct kunit_hooks_table { void *(*get_static_stub_address)(struct kunit *test, void *real_fn_addr); } kunit_hooks;

+/**

• • kunit_is_running() - True, if KUnit test is currently running.
• • If CONFIG_KUNIT is not enabled, it will compile down to a false.
• */

+#define kunit_is_running() static_branch_unlikely(&kunit_running)

/**

• kunit_get_current_test() - Return a pointer to the currently running

•                       KUnit test.
  

@@ -40,7 +47,7 @@ extern struct kunit_hooks_table { */ static inline struct kunit *kunit_get_current_test(void) {

•   if (!static_branch_unlikely(&kunit_running))
  

•   if (!kunit_is_running())
            return NULL;
  
    return current->kunit_test;
  

@@ -53,7 +60,7 @@ static inline struct kunit *kunit_get_current_test(void)

• If a KUnit test is running in the current task, mark that test as failed.

*/ #define kunit_fail_current_test(fmt, ...) do { \

•           if (static_branch_unlikely(&kunit_running)) {                   \
  

•           if (kunit_is_running()) {                                       \
                    /* Guaranteed to be non-NULL when kunit_running true*/  \
                    kunit_hooks.fail_current_test(__FILE__, __LINE__,       \
                                              fmt, ##__VA_ARGS__);          \
  

@@ -64,6 +71,7 @@ static inline struct kunit *kunit_get_current_test(void)

static inline struct kunit *kunit_get_current_test(void) { return NULL; }

+#define kunit_is_running() false #define kunit_fail_current_test(fmt, ...) do {} while (0)

#endif

2.43.0

On Wed, Aug 21, 2024 at 10:43 AM Michal Wajdeczko michal.wajdeczko@intel.com wrote:

The DECLARE_IF_KUNIT macro will introduces identifiers only if CONFIG_KUNIT is enabled. Otherwise if CONFIG_KUNIT is not enabled no identifiers from the param list will be defined.

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com

Hello!

I like this macro. I think it could definitely be useful in declaring static functions for KUnit testing in the header files. So I am happy to add it.

We should also add this to the documentation at some point. I've been wanting to revamp the visibility.h macros documentation anyways.

Reviewed-by: Rae Moar rmoar@google.com

Thanks!

-Rae

---

Cc: Rae Moar rmoar@google.com Cc: David Gow davidgow@google.com Cc: Lucas De Marchi lucas.demarchi@intel.com

---

include/kunit/visibility.h | 8 ++++++++ 1 file changed, 8 insertions(+)

diff --git a/include/kunit/visibility.h b/include/kunit/visibility.h index 0dfe35feeec6..1c23773f826c 100644 --- a/include/kunit/visibility.h +++ b/include/kunit/visibility.h @@ -11,6 +11,13 @@ #define _KUNIT_VISIBILITY_H

#if IS_ENABLED(CONFIG_KUNIT)

• /**

• * DECLARE_IF_KUNIT - A macro that introduces identifiers only if
  

• * CONFIG_KUNIT is enabled. Otherwise if CONFIG_KUNIT is not enabled
  

• * no identifiers will be defined.
  

• * @body: identifiers to be introduced conditionally
  

• */
  

• #define DECLARE_IF_KUNIT(body...) body /**
  • VISIBLE_IF_KUNIT - A macro that sets symbols to be static if
  • CONFIG_KUNIT is not enabled. Otherwise if CONFIG_KUNIT is enabled

@@ -26,6 +33,7 @@ #define EXPORT_SYMBOL_IF_KUNIT(symbol) EXPORT_SYMBOL_NS(symbol, \ EXPORTED_FOR_KUNIT_TESTING) #else

• #define DECLARE_IF_KUNIT(body...) #define VISIBLE_IF_KUNIT static #define EXPORT_SYMBOL_IF_KUNIT(symbol)

#endif

2.43.0

On 21.08.2024 23:32, Rae Moar wrote:

On Wed, Aug 21, 2024 at 10:43 AM Michal Wajdeczko michal.wajdeczko@intel.com wrote:

...

Hello!

This is looking good and seems to be working well. I just had some questions below.

Thanks! -Rae

...

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

A slight typo here: functionality

...

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@intel.com

Cc: David Gow davidgow@google.com Cc: Daniel Latypov dlatypov@google.com Cc: Lucas De Marchi lucas.demarchi@intel.com

---

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);
    

I think we should model activating the stub here in the example to make it a bit clearer.

oops, this DOC was taken almost as-is from my Xe series mentioned in the cover letter, where I didn't have any "activate_fixed_stub" code yet

will fix

...

• • }
    

• */

+#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",        \
  

•                           __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 { \

•   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;
  

+}

+/*

• • 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);
  

Should we check here if the replacement_func is null and if so deactivate the stub similar to the static stubbing?

I had KUNIT_ASSERT_NOT_NULL(test, replacement_func) but decided to drop that to also allow using 'activate_stub(NULL)' to deactivate the stub - but that was before I introduced a separate 'deactivate_stub()'

will bring that back

...

•   *stub = replacement_func;
  

I do really appreciate this simplicity. But I wonder if David has any thoughts on the security of direct replacement rather than using the resource API?

note that at redirection point we will not be able to use resource API since that could be run in a non-kunit context

...

•   KUNIT_ASSERT_EQ(test, 0, kunit_add_action_or_reset(test, nullify_stub_ptr, stub_ptr));
  

+}

+EXPORT_SYMBOL_GPL(__kunit_activate_fixed_stub);

2.43.0

-- You received this message because you are subscribed to the Google Groups "KUnit Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20240821144305.1958-4-michal.waj....

On 21.08.2024 23:38, Lucas De Marchi wrote:

On Wed, Aug 21, 2024 at 04:43:04PM GMT, Michal Wajdeczko 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@intel.com

Cc: David Gow davidgow@google.com Cc: Daniel Latypov dlatypov@google.com Cc: Lucas De Marchi lucas.demarchi@intel.com

---

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).

not sure about the name FIXED vs STATIC. What about

well, I must admit that I also wasn't sure about the name..

KUNIT_FIXED_STUB_REDIRECT() KUNIT_FIXED_STUB_REDIRECT_ALL_CONTEXTS()

since we have

KUNIT_STATIC_STUB_REDIRECT()

then maybe

KUNIT_STATIC_STUB_REDIRECT_ALL_CONTEXTS() KUNIT_STATIC_STUB_REDIRECT_GLOBAL()

?

...

• • Example:
• • .. code-block:: c
• *    static int (*func_stub)(int n);
• *    int real_func(int n)
• *    {
• *        KUNIT_FIXED_STUB_REDIRECT(func_stub, n);

what I don't like here is that KUNIT_STATIC_STUB_REDIRECT() uses the name of **this** function, whiel KUNIT_FIXED_STUB_REDIRECT() uses the function pointer. I don't have a better suggestion on how to handle it, but this looks error prone.

the KUNIT_STATIC_STUB_REDIRECT() is in a better position as it can do lookup a resource (which is a stub function pointer) based on the "this function" as it runs in a kunit context, while FIXED variant must access stub pointer directly

or do you mean that wrong stub name could provided? but note that you may also make the same mistake today:

int foo(int n) { KUNIT_STATIC_STUB_REDIRECT(bar, n); }

hmm, maybe we can try to do this:

KUNIT_STATIC_STUB_REDIRECT_GLOBAL(stubs, func, args...) @stubs - place where function pointers are kept @func - we can use it to check types and as member name

replacement = stubs.func; return replacement(args);

kunit_activate_static_stub_global(test, stubs, func, replacement) @stubs - place where function pointers are kept @func - we can use it to check types and as member name

stubs.func = replacement

...

• *        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)) {                        \

probably better as `if (unlikely(kunit_is_running() && replacement))`? Because we are likely to use one specific replacement in just 1 or few tests from an entire testsuite.

kunit_is_running() is already 'unlikely'

Lucas De Marchi

...

+            pr_info(KUNIT_SUBTEST_INDENT "# %s: calling stub %ps\n",    \ +                __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 {                \ +    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; +}

+/*

• • 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; +    KUNIT_ASSERT_EQ(test, 0, kunit_add_action_or_reset(test, nullify_stub_ptr, stub_ptr)); +} +EXPORT_SYMBOL_GPL(__kunit_activate_fixed_stub); --  2.43.0

On 22.08.2024 08:14, David Gow wrote:

On Wed, 21 Aug 2024 at 22:43, Michal Wajdeczko michal.wajdeczko@intel.com 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@intel.com

Cc: David Gow davidgow@google.com Cc: Daniel Latypov dlatypov@google.com Cc: Lucas De Marchi lucas.demarchi@intel.com

---

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

On Wed, Aug 21, 2024 at 10:43 AM Michal Wajdeczko michal.wajdeczko@intel.com wrote:

Add example how to use KUNIT_FIXED_STUB_REDIRECT and compare its usage with the KUNIT_STATIC_STUB_REDIRECT. Also show how the DECLARE_IF_KUNIT macro could be helpful in declaring test data in the non-test data structures.

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com

Hello!

I really like this test. It provides a great overview of this patch series. I just have a couple comments below.

Otherwise, Reviewed-by: Rae Moar rmoar@google.com

Thanks! -Rae

---

Cc: David Gow davidgow@google.com Cc: Daniel Latypov dlatypov@google.com Cc: Lucas De Marchi lucas.demarchi@intel.com

---

lib/kunit/kunit-example-test.c | 63 ++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+)

diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c index 3056d6bc705d..120e08d8899b 100644 --- a/lib/kunit/kunit-example-test.c +++ b/lib/kunit/kunit-example-test.c @@ -6,8 +6,10 @@

• Author: Brendan Higgins brendanhiggins@google.com

*/

+#include <linux/workqueue.h> #include <kunit/test.h> #include <kunit/static_stub.h> +#include <kunit/visibility.h>

/*

• This is the most fundamental element of KUnit, the test case. A test case

@@ -221,6 +223,66 @@ static void example_static_stub_using_fn_ptr_test(struct kunit *test) KUNIT_EXPECT_EQ(test, add_one(1), 2); }

+/* This could be a location of various fixed stub functions. */ +static struct {

•   DECLARE_IF_KUNIT(int (*add_two)(int i));
  

Is the use of DECLARE_IF_KUNIT useful here? KUnit should always be enabled if this file is being compiled/run. Is the idea to show an example here that could be used outside of kunit test files?

Additionally, would it make sense to call this add_two_stub instead to make it clear that this is not a definition of the add_two function? Or is it helpful for people to see this as an example of how to handle multiple stubs: struct of stubs with exact names? Let me know what you think.

+} stubs;

+/* This is a function we'll replace with stubs. */ +static int add_two(int i) +{

•   /* This will trigger the stub if active. */
  

•   KUNIT_STATIC_STUB_REDIRECT(add_two, i);
  

•   KUNIT_FIXED_STUB_REDIRECT(stubs.add_two, i);
  

•   return i + 2;
  

+}

+struct add_two_async_work {

•   struct work_struct work;
  

•   int param;
  

•   int result;
  

+};

+static void add_two_async_func(struct work_struct *work) +{

•   struct add_two_async_work *w = container_of(work, typeof(*w), work);
  

•   w->result = add_two(w->param);
  

+}

+static int add_two_async(int i) +{

•   struct add_two_async_work w = { .param = i };
  

•   INIT_WORK_ONSTACK(&w.work, add_two_async_func);
  

•   schedule_work(&w.work);
  

•   flush_work(&w.work);
  

•   destroy_work_on_stack(&w.work);
  

•   return w.result;
  

+}

+/*

• */

It looks like the method description is missing here.

+static void example_fixed_stub_test(struct kunit *test) +{

•   /* static stub redirection works only for KUnit thread */
  

•   kunit_activate_static_stub(test, add_two, subtract_one);
  

•   KUNIT_EXPECT_EQ(test, add_two(1), subtract_one(1));
  

•   KUNIT_EXPECT_NE_MSG(test, add_two_async(1), subtract_one(1),
  

•                       "stub shouldn't be active outside KUnit thread!");
  

•   kunit_deactivate_static_stub(test, add_two);
  

•   KUNIT_EXPECT_EQ(test, add_two(1), add_two(1));
  

•   /* fixed stub redirection works for KUnit and other threads */
  

•   kunit_activate_fixed_stub(test, stubs.add_two, subtract_one);
  

•   KUNIT_EXPECT_EQ(test, add_two(1), subtract_one(1));
  

•   KUNIT_EXPECT_EQ(test, add_two_async(1), subtract_one(1));
  

•   kunit_deactivate_fixed_stub(test, stubs.add_two);
  

•   KUNIT_EXPECT_EQ(test, add_two(1), add_two(1));
  

+}

static const struct example_param { int value; } example_params_array[] = { @@ -294,6 +356,7 @@ static struct kunit_case example_test_cases[] = { KUNIT_CASE(example_all_expect_macros_test), KUNIT_CASE(example_static_stub_test), KUNIT_CASE(example_static_stub_using_fn_ptr_test),

•   KUNIT_CASE(example_fixed_stub_test),
    KUNIT_CASE(example_priv_test),
    KUNIT_CASE_PARAM(example_params_test, example_gen_params),
    KUNIT_CASE_SLOW(example_slow_test),
  

-- 2.43.0

-- You received this message because you are subscribed to the Google Groups "KUnit Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20240821144305.1958-5-michal.waj....

On Wed, Aug 21, 2024 at 6:00 PM Michal Wajdeczko michal.wajdeczko@intel.com wrote:

On 21.08.2024 23:22, Rae Moar wrote:

...

On Wed, Aug 21, 2024 at 10:43 AM Michal Wajdeczko michal.wajdeczko@intel.com wrote:

...

Add example how to use KUNIT_FIXED_STUB_REDIRECT and compare its usage with the KUNIT_STATIC_STUB_REDIRECT. Also show how the DECLARE_IF_KUNIT macro could be helpful in declaring test data in the non-test data structures.

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com

Hello!

I really like this test. It provides a great overview of this patch series. I just have a couple comments below.

Otherwise, Reviewed-by: Rae Moar rmoar@google.com

Thanks! -Rae

...

---

Cc: David Gow davidgow@google.com Cc: Daniel Latypov dlatypov@google.com Cc: Lucas De Marchi lucas.demarchi@intel.com

---

lib/kunit/kunit-example-test.c | 63 ++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+)

diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c index 3056d6bc705d..120e08d8899b 100644 --- a/lib/kunit/kunit-example-test.c +++ b/lib/kunit/kunit-example-test.c @@ -6,8 +6,10 @@

• Author: Brendan Higgins brendanhiggins@google.com

*/

+#include <linux/workqueue.h> #include <kunit/test.h> #include <kunit/static_stub.h> +#include <kunit/visibility.h>

/*

• This is the most fundamental element of KUnit, the test case. A test case

@@ -221,6 +223,66 @@ static void example_static_stub_using_fn_ptr_test(struct kunit *test) KUNIT_EXPECT_EQ(test, add_one(1), 2); }

+/* This could be a location of various fixed stub functions. */ +static struct {

•   DECLARE_IF_KUNIT(int (*add_two)(int i));
  

Is the use of DECLARE_IF_KUNIT useful here? KUnit should always be enabled if this file is being compiled/run. Is the idea to show an example here that could be used outside of kunit test files?

yes, the idea was to show that 'stubs' declarations could be placed anywhere, without any cost if compiled without KUNIT (I was trying to mention that in commit message)

...

Additionally, would it make sense to call this add_two_stub instead to make it clear that this is not a definition of the add_two function? Or is it helpful for people to see this as an example of how to handle multiple stubs: struct of stubs with exact names? Let me know what you think.

the 'add_two' above is just a member name, and IMO we shouldn't repeat that this is about 'stub' since the whole struct is for 'stubs'

and yes, the idea was also to show that if applicable, other function stubs declarations could be either placed together

Happy with this as is then! Thanks for your response.

-Rae

...

...

+} stubs;

+/* This is a function we'll replace with stubs. */ +static int add_two(int i) +{

•   /* This will trigger the stub if active. */
  

•   KUNIT_STATIC_STUB_REDIRECT(add_two, i);
  

•   KUNIT_FIXED_STUB_REDIRECT(stubs.add_two, i);
  

•   return i + 2;
  

+}

+struct add_two_async_work {

•   struct work_struct work;
  

•   int param;
  

•   int result;
  

+};

+static void add_two_async_func(struct work_struct *work) +{

•   struct add_two_async_work *w = container_of(work, typeof(*w), work);
  

•   w->result = add_two(w->param);
  

+}

+static int add_two_async(int i) +{

•   struct add_two_async_work w = { .param = i };
  

•   INIT_WORK_ONSTACK(&w.work, add_two_async_func);
  

•   schedule_work(&w.work);
  

•   flush_work(&w.work);
  

•   destroy_work_on_stack(&w.work);
  

•   return w.result;
  

+}

+/*

• */

It looks like the method description is missing here.

ha, I missed to copy commit message here

...

...

+static void example_fixed_stub_test(struct kunit *test) +{

•   /* static stub redirection works only for KUnit thread */
  

•   kunit_activate_static_stub(test, add_two, subtract_one);
  

•   KUNIT_EXPECT_EQ(test, add_two(1), subtract_one(1));
  

•   KUNIT_EXPECT_NE_MSG(test, add_two_async(1), subtract_one(1),
  

•                       "stub shouldn't be active outside KUnit thread!");
  

•   kunit_deactivate_static_stub(test, add_two);
  

•   KUNIT_EXPECT_EQ(test, add_two(1), add_two(1));
  

•   /* fixed stub redirection works for KUnit and other threads */
  

•   kunit_activate_fixed_stub(test, stubs.add_two, subtract_one);
  

•   KUNIT_EXPECT_EQ(test, add_two(1), subtract_one(1));
  

•   KUNIT_EXPECT_EQ(test, add_two_async(1), subtract_one(1));
  

•   kunit_deactivate_fixed_stub(test, stubs.add_two);
  

•   KUNIT_EXPECT_EQ(test, add_two(1), add_two(1));
  

+}

static const struct example_param { int value; } example_params_array[] = { @@ -294,6 +356,7 @@ static struct kunit_case example_test_cases[] = { KUNIT_CASE(example_all_expect_macros_test), KUNIT_CASE(example_static_stub_test), KUNIT_CASE(example_static_stub_using_fn_ptr_test),

•   KUNIT_CASE(example_fixed_stub_test),
    KUNIT_CASE(example_priv_test),
    KUNIT_CASE_PARAM(example_params_test, example_gen_params),
    KUNIT_CASE_SLOW(example_slow_test),
  

-- 2.43.0

-- You received this message because you are subscribed to the Google Groups "KUnit Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20240821144305.1958-5-michal.waj....