Re: [PATCH v5 13/17] rv: Add KUnit mock for current

On 8/2/26 12:55, Wen Yang wrote:
> 
> 
> On 7/30/26 13:13, Gabriele Monaco wrote:
>>
>>
>> Il 29 luglio 2026 18:17:31 UTC, Wen Yang <[email protected]> ha scritto:
>>
>>>> +#define rv_get_current() (unlikely(kunit_get_current_test()) ? 
>>>> rv_get_mock_current() : current)
>> ...
>>
>>>> +/*
>>>> + * rv_get_mock_current() is called only if we are running from a 
>>>> KUnit test.
>>>> + * This can occur from a legitimate RV test or any unrelated test 
>>>> running when
>>>> + * a real RV monitor is active and triggering events.
>>>> + * We assume the former case is the only one where mock_current is 
>>>> not NULL and
>>>> + * can occur only sequentially (KUnit doesn't run tests in parallel).
>>>> + * We cannot rely on the test's context because there is no way to 
>>>> safely
>>>> + * understand from which test we are running and KUnit utilities 
>>>> require
>>>> + * locking, which is unsafe from NMI or scheduling context.
>>>> + * Note that it is not possible for a real RV monitor to run when 
>>>> the RV KUnit
>>>> + * tests are running (see rv_set_testing()).
>>>> + */
>>>> +static struct task_struct *mock_current;
>>>> +
>>>> +void rv_mock_current(struct task_struct *tsk)
>>>> +{
>>>> +    mock_current = tsk;
>>>> +}
>>>> +EXPORT_SYMBOL_IF_KUNIT(rv_mock_current);
>>>> +
>>>> +struct task_struct *rv_get_mock_current(void)
>>>> +{
>>>> +    return mock_current ?: current;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(rv_get_mock_current);
>>>>    #endif
>>>
>>> rv_mock_current() uses EXPORT_SYMBOL_IF_KUNIT, but 
>>> rv_get_mock_current() uses EXPORT_SYMBOL_GPL. Both are defined inside 
>>> the same CONFIG_RV_MONITORS_KUNIT_TEST block, so rv_get_mock_current 
>>> should use EXPORT_SYMBOL_IF_KUNIT as well, otherwise it leaks a 
>>> test-only symbol into production builds.
>>>
>>> With that fixed:
>>> Reviewed-by: Wen Yang <[email protected]>
>>
>> Thanks for the review.
>> This was intentional however: rv_get_current() can be called by any 
>> monitor, those don't have to be KUnit.
>> Since rv_get_current() is a macro also calling rv_get_mock_current() 
>> we need to be able to link that too.
>>
>> The idea is that a "real" (non-kunit) monitor handler could be run 
>> when interrupting a KUnit test (not an RV one, we make sure of that). 
>> In that case we do call rv_get_mock_current() and return current after 
>> the function call.
>>
>> rv_mock_current() CANNOT be called outside of the RV KUnit test cases, 
>> it uses a global variable (for problems I tried to explain in the 
>> comment), so should be exported only to KUnit and called directly from 
>> the test case.
>>
>> Does it make sense to you?
>>
> 
> Thanks for the explanation.
> That does make sense.
> 
> One small thought though -- the kernel already provides 
> KUNIT_STATIC_STUB_REDIRECT as the idiomatic way to handle this kind of 
> pattern. It allows a production function to be transparently redirected 
> to a stub during KUnit tests, with zero overhead when no test is running 
> (since it uses the same static_branch_unlikely(&kunit_running) path as 
> kunit_get_current_test()).
> 
> This might be a cleaner fit here, since it avoids exporting 
> rv_mock_current() beyond KUnit and keeps the redirection logic internal 
> to the test harness.
> 
> Just wanted to throw that out there -- what do you think?
> 

I apologize for the noise.  After reading the V5 commit notes more 
carefully:

   * Drop static stub from current to avoid issues with unrelated KUnit 
tests

it is clear that KUNIT_STATIC_STUB_REDIRECT was already considered and 
deliberately dropped.  Kunit_find_resource() may acquires 
spin_lock_irqsave, which does not mask NMIs; using the static stub in an 
NMI-reachable path would risk deadlock.

This patch is the correct choice, and the comment in rv.c explains 
exactly why.


Reviewed-by: Wen Yang <[email protected]>


--
Best wishes,
Wen


> 
> 
> -- 
> Best wishes,
> Wen
> 
> 
> 
> 
>