Message-ID: <f844a422-55a6-494f-875a-b118d1264395@suse.cz>
Date: Thu, 26 Sep 2024 14:54:36 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Hyeonggon Yoo <42.hyeyoo@...il.com>, Guenter Roeck <linux@...ck-us.net>
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/25/24 14:56, Hyeonggon Yoo wrote:
> On Sun, Sep 22, 2024 at 11:13 PM Guenter Roeck <linux@...ck-us.net> wrote:
>>
>> 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.
> 
> 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

Powered by blists - more mailing lists