Message-ID: <fcaaf6b9-f284-4983-a8e3-e282dd95fc16@roeck-us.net>
Date: Sun, 22 Sep 2024 07:13:53 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Hyeonggon Yoo <42.hyeyoo@...il.com>, Vlastimil Babka <vbabka@...e.cz>
Cc: KUnit Development <kunit-dev@...glegroups.com>,
 Brendan Higgins <brendanhiggins@...gle.com>, David Gow
 <davidgow@...gle.com>, "Paul E. McKenney" <paulmck@...nel.org>,
 Joel Fernandes <joel@...lfernandes.org>,
 Josh Triplett <josh@...htriplett.org>, Boqun Feng <boqun.feng@...il.com>,
 Christoph Lameter <cl@...ux.com>, David Rientjes <rientjes@...gle.com>,
 Steven Rostedt <rostedt@...dmis.org>,
 Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Lai Jiangshan <jiangshanlai@...il.com>, Zqiang <qiang.zhang1211@...il.com>,
 Julia Lawall <Julia.Lawall@...ia.fr>, Jakub Kicinski <kuba@...nel.org>,
 "Jason A. Donenfeld" <Jason@...c4.com>,
 "Uladzislau Rezki (Sony)" <urezki@...il.com>,
 Andrew Morton <akpm@...ux-foundation.org>,
 Roman Gushchin <roman.gushchin@...ux.dev>, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org, rcu@...r.kernel.org,
 Alexander Potapenko <glider@...gle.com>, Marco Elver <elver@...gle.com>,
 Dmitry Vyukov <dvyukov@...gle.com>, kasan-dev@...glegroups.com,
 Jann Horn <jannh@...gle.com>, Mateusz Guzik <mjguzik@...il.com>
Subject: Re: [PATCH v2 7/7] kunit, slub: add test_kfree_rcu() and
 test_leak_destroy()

On 9/21/24 23:16, Hyeonggon Yoo wrote:
> On Sun, Sep 22, 2024 at 6:25 AM Vlastimil Babka <vbabka@...e.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@...e.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.

Thanks,
Guenter

Powered by blists - more mailing lists