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

[Gabriele Monaco <gmonaco@redhat.com>]

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.

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

sched_need_resched just requires interrupts disabled, so I disable preemption
and show that seeing it with only interrupts enabled should react (touching
preemption is actually not required, but just to get a different test case).

> If so, should we perhaps have something like RV_KUNIT_EXPECT_NO_REACTION()
> here? So that if the monitor is broken and the reactor is called, then
> the kunit test will fail exactly where the failure is.
> 
> Reading the patch further down below, we actually do have that
> macro! Shouldn't we use it here?

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

> > +#ifdef CONFIG_RV_MON_SCO
> > +extern void rv_test_sco(struct kunit *test);
> > +#else
> > +static inline void rv_test_sco(struct kunit *test)
> > +{
> > +	kunit_skip(test, "Monitor not enabled\n");
> > +}
> > +#endif
> 
> Instead of these, I would prefer we have something like
> 
> #define KUNIT_CASE_IF(test_name, enabled) \
> 	{ .run_case = enabled ? test_name : some_stub, ... }
> 
> and
> 
> static struct kunit_case rv_trigger_test_cases[] = {
> 	KUNIT_CASE_IF(rv_test_sco, CONFIG_RV_MON_SCO),
>         ...
> 	{}
> };
> 
> or something like that. But no big deal.

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

Thanks,
Gabriele