Re: [PATCH v2 7/7] kunit, slub: add test_kfree_rcu() and test_leak_destroy()

[Vlastimil Babka <vbabka@suse.cz>]

On 9/25/24 14:56, Hyeonggon Yoo wrote:
> On Sun, Sep 22, 2024 at 11:13 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 9/21/24 23:16, Hyeonggon Yoo wrote:
>> > On Sun, Sep 22, 2024 at 6:25 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>> >>
>> >> On 9/21/24 23:08, Guenter Roeck wrote:
>> >>> On 9/21/24 13:40, Vlastimil Babka wrote:
>> >>>> +CC kunit folks
>> >>>>
>> >>>> On 9/20/24 15:35, Guenter Roeck wrote:
>> >>>>> Hi,
>> >>>>
>> >>>> Hi,
>> >>>>
>> >>>>> On Wed, Aug 07, 2024 at 12:31:20PM +0200, Vlastimil Babka wrote:
>> >>>>>> Add a test that will create cache, allocate one object, kfree_rcu() it
>> >>>>>> and attempt to destroy it. As long as the usage of kvfree_rcu_barrier()
>> >>>>>> in kmem_cache_destroy() works correctly, there should be no warnings in
>> >>>>>> dmesg and the test should pass.
>> >>>>>>
>> >>>>>> Additionally add a test_leak_destroy() test that leaks an object on
>> >>>>>> purpose and verifies that kmem_cache_destroy() catches it.
>> >>>>>>
>> >>>>>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>> >>>>>
>> >>>>> This test case, when run, triggers a warning traceback.
>> >>>>>
>> >>>>> kmem_cache_destroy TestSlub_kfree_rcu: Slab cache still has objects when called from test_leak_destroy+0x70/0x11c
>> >>>>> WARNING: CPU: 0 PID: 715 at mm/slab_common.c:511 kmem_cache_destroy+0x1dc/0x1e4
>> >>>>
>> >>>> Yes that should be suppressed like the other slub_kunit tests do. I have
>> >>>> assumed it's not that urgent because for example the KASAN kunit tests all
>> >>>> produce tons of warnings and thus assumed it's in some way acceptable for
>> >>>> kunit tests to do.
>> >>>>
>> >>>
>> >>> I have all tests which generate warning backtraces disabled. Trying to identify
>> >>> which warnings are noise and which warnings are on purpose doesn't scale,
>> >>> so it is all or nothing for me. I tried earlier to introduce a patch series
>> >>> which would enable selective backtrace suppression, but that died the death
>> >>> of architecture maintainers not caring and people demanding it to be perfect
>> >>> (meaning it only addressed WARNING: backtraces and not BUG: backtraces,
>> >>> and apparently that wasn't good enough).
>> >>
>> >> Ah, didn't know, too bad.
>> >>
>> >>> If the backtrace is intentional (and I think you are saying that it is),
>> >>> I'll simply disable the test. That may be a bit counter-productive, but
>> >>> there is really no alternative for me.
>> >>
>> >> It's intentional in the sense that the test intentionally triggers a
>> >> condition that normally produces a warning. Many if the slub kunit test do
>> >> that, but are able to suppress printing the warning when it happens in the
>> >> kunit context. I forgot to do that for the new test initially as the warning
>> >> there happens from a different path that those that already have the kunit
>> >> suppression, but we'll implement that suppression there too ASAP.
>> >
>> > We might also need to address the concern of the commit
>> > 7302e91f39a ("mm/slab_common: use WARN() if cache still has objects on
>> > destroy"),
>> > the concern that some users prefer WARN() over pr_err() to catch
>> > errors on testing systems
>> > which relies on WARN() format, and to respect panic_on_warn.
>> >
>> > So we might need to call WARN() instead of pr_err() if there are errors in
>> > slub error handling code in general, except when running kunit tests?
>> >
>>
>> If people _want_ to see WARNING backtraces generated on purpose, so be it.
>> For me it means that _real_ WARNING backtraces disappear in the noise.
>> Manually maintaining a list of expected warning backtraces is too maintenance
>> expensive for me, so I simply disable all kunit tests which generate
>> backtraces on purpose. That is just me, though. Other testbeds may have
>> more resources available and may be perfectly happy with the associated
>> maintenance cost.
>>
>> In this specific case, I now have disabled slub kunit tests, and, as
>> mentioned before, from my perspective there is no need to change the
>> code just to accommodate my needs. I'll do the same with all other new
>> unit tests which generate backtraces in the future, without bothering
>> anyone.
>>
>> Sorry for the noise.
> 
> I don't think this was a noise :) IMO some people want to see WARNING
> during testing to catch errors,
> but not for the slub_kunit test case. I think a proper approach here
> would be suppressing
> warnings while running slub_kunit test cases, but print WARNING when
> it is not running slub_kunit test cases.
> 
> That would require some work changing the slub error reporting logic
> to print WARNING on certain errors.
> Any opinions, Vlastimil?

Yes, we should suppress the existing warning on kmem_cache_destroy() in
kunit test context, and separately we can change pr_err() to WARN() as long
as they are still suppressed in kunit test context.

> Thanks,
> Hyeonggon