Re: [RFC PATCH 10/12] rv: Add KUnit tests for some DA/HA monitors

[Nam Cao <namcao@linutronix.de>]

Gabriele Monaco <gmonaco@redhat.com> writes:
> On Mon, 2026-05-04 at 10:39 +0200, Nam Cao wrote:
>> Gabriele Monaco <gmonaco@redhat.com> writes:
>> > +obj-$(CONFIG_RV_MONITORS_KUNIT_TEST) += rv_monitors_test.o
>> > +# Stubbing rv_react triggers the error
>> > +CFLAGS_rv_monitors_test.o += -Wno-suggest-attribute=format
>> 
>> I try removing this flag, but my compiler does not produce any
>> warning. Which warning did you see?
>
> That's quite odd, I don't remember the exact GCC version that was showing the
> warning, my current one does not enable it by default, you can however enable it
> with:
>
>   CFLAGS_rv_monitors_test.o += -Wsuggest-attribute=format
>
> Then you get:
>
> In file included from kernel/trace/rv/rv_monitors_test.c:11:
> kernel/trace/rv/rv_monitors_test.c: In function ‘rv_trigger_test_init’:
> kernel/trace/rv/rv_monitors_test.c:68:42: error: argument 2 of ‘__kunit_activate_static_stub’ might be a candidate for a format attribute [-Werror=suggest-attribute=format]
>    68 |         kunit_activate_static_stub(test, rv_react, stub_rv_react);
>       |                                          ^~~~~~~~
> ./include/kunit/static_stub.h:97:44: note: in definition of macro ‘kunit_activate_static_stub’
>    97 |         __kunit_activate_static_stub(test, real_fn_addr, replacement_addr);     \
>       |                                            ^~~~~~~~~~~~
> kernel/trace/rv/rv_monitors_test.c:68:52: error: argument 3 of ‘__kunit_activate_static_stub’ might be a candidate for a format attribute [-Werror=suggest-attribute=format]
>    68 |         kunit_activate_static_stub(test, rv_react, stub_rv_react);
>       |                                                    ^~~~~~~~~~~~~
> ./include/kunit/static_stub.h:97:58: note: in definition of macro ‘kunit_activate_static_stub’
>    97 |         __kunit_activate_static_stub(test, real_fn_addr, replacement_addr);     \
>       |                                                          ^~~~~~~~~~~~~~~~
>
>> If it is not a hassle, I would prefer to address the warning in C code.
>> Grep tells me the rest of the kernel does not use this, how do other
>> subsystems not suffer from this warning?
>
> When the compiler was caring about it, I tried all I could think of to avoid the
> warning, but I didn't manage, some other places do disable this warning (just
> not in the makefile but with pragmas):
>
> $ git grep "suggest-attribute=format"
> drivers/infiniband/hw/hfi1/trace_dbg.h:#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.h:#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
> drivers/net/wireless/broadcom/brcm80211/brcmsmac/brcms_trace_brcmsmac_msg.h:#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
> drivers/net/wireless/intel/iwlwifi/iwl-devtrace.c:#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
> include/trace/events/qla.h:#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
> kernel/panic.c:#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
> lib/vsprintf.c:__diag_ignore(GCC, all, "-Wsuggest-attribute=format",
>
>
> Here, the error comes from macro expansions in KUnit and I'm not sure there's
> any practical way to solve it, that's why I resorted to disabling the warning
> altogether.
> I'm not sure whether a pragma would be cleaner though.

Thanks for sharing the details. I can see now that disabling the warning
is the way to go.

>> > +void rv_test_opid(struct kunit *test)
>> > +{
>> > +	struct rv_kunit_ctx *ctx = test->priv;
>> > +
>> > +	da_prepare_test(test, &rv_this);
>> > +
>> > +	/*
>> > +	 * The handlers are called with an additional level of preemption,
>> > +	 * ensure we start from 0 but apply it here to avoid warnings.
>> > +	 */
>> > +	KUNIT_ASSERT_TRUE(test, preemptible());
>> > +	guard(preempt)();
>> > +
>> > +	/* Wakeup with preemption and interrupts enabled */
>> > +	handle_sched_waking(NULL, NULL);
>> > +	RV_KUNIT_EXPECT_REACTION(test, ctx);
>> > +
>> > +	/* Need resched with interrupts enabled */
>> > +	scoped_guard(preempt)
>> > +		handle_sched_need_resched(NULL, NULL, 0, TIF_NEED_RESCHED);
>> 
>> From my understanding, this last one is testing that need_resched with
>> interrupt enabled does not invoke the reactor? And if the monitor is
>> broken and the reactor is invoked, we would have no test failure here,
>> but instead we have test failure the next time
>> RV_KUNIT_EXPECT_REACTION() is called. And if this is the last test, we
>> would not see a failure?
>
> Not really, I just forgot an RV_KUNIT_EXPECT_REACTION().

I added that missing RV_KUNIT_EXPECT_REACTION(), but I still see a test
failure:

[    1.070721]     # module: rv_monitors_test
[    1.073512]     1..7
[    1.077641] scsi 1:0:0:0: CD-ROM            QEMU     QEMU DVD-ROM     2.5+ PQ: 0 ANSI: 5
[    1.078494]     ok 1 rv_test_sco
[    1.083256]     ok 2 rv_test_sssw
[    1.085783]     ok 3 rv_test_sts # SKIP Monitor not enabled
[    1.092365]     ok 4 rv_test_opid
[    1.093462]     # rv_test_nomiss: EXPECTATION FAILED at kernel/trace/rv/monitors/nomiss/nomiss.c:306
[    1.093462]     Expected ctx->reactions == ++ctx->expected, but
[    1.093462]         ctx->reactions == 2 (0x2)
[    1.093462]         ++ctx->expected == 1 (0x1)
[    1.095699]     not ok 5 rv_test_nomiss
[    1.109418]     ok 6 rv_test_pagefault # SKIP Monitor not enabled
[    1.115146]     ok 7 rv_test_sleep # SKIP Monitor not enabled
[    1.118050] # rv_trigger: pass:3 fail:1 skip:3 total:7
[    1.118053] # Totals: pass:3 fail:1 skip:3 total:7
[    1.120622] not ok 1 rv_trigger

Any idea why?

> So I'm actually thinking of defining yet another macro that fundamentally does
>
> RV_KUNIT_EXPECT_NO_REACTION()
> handle_event()
> RV_KUNIT_EXPECT_REACTION()
>
> which would make sure the reaction happens exactly there, plus I'd add an
> RV_KUNIT_EXPECT_NO_REACTION() in the cleanup sequence to ensure no unexpected
> reaction occurred (or nobody forgot to expect a reaction like I did above).

Sounds nice, go for it.

> Yeah that should be neater, but weren't you the one not liking macros? ;)

It's not black and white, I like whatever makes the code clean and easy
to read. Sometimes macros are nice, other times not so much. I have
spent hours reading the tracepoints' macros and they are still black
magic to me (but to be fair, macros are probably the best we can do for
that case). I hope we can rewrite those in Rust's generic one day.

Nam