| Message ID | 20241206-test-vsock-leaks-v1-0-c31e8c875797@rbox.co (mailing list archive) |
|---|---|
| Headers | show |
| Series | vsock/test: Tests for memory leaks | expand |
Hi Michal, On Fri, Dec 06, 2024 at 07:34:50PM +0100, Michal Luczaj wrote: >Series adds tests for recently fixed memory leaks[1]: > >d7b0ff5a8667 ("virtio/vsock: Fix accept_queue memory leak") >fbf7085b3ad1 ("vsock: Fix sk_error_queue memory leak") >60cf6206a1f5 ("virtio/vsock: Improve MSG_ZEROCOPY error handling") Great! Thanks for these new tests! > >First patch is a non-functional preparatory cleanup. > >I initially considered triggering (and parsing) a kmemleak scan after each >test, but ultimately concluded that the slowdown and the required >privileges would be too much. Yeah, what about adding something in the README to suggest using kmemleak and how to check that everything is okay after a run? I'd suggest also to add something about that in each patch that introduce tests where we expects the user to check kmemleak, at least with a comment on top of the test functions, and maybe also in the commit description. WDYT? I left some comments and reported an issue with the last tests. Thanks, Stefano > >[1]: https://lore.kernel.org/netdev/20241107-vsock-mem-leaks-v2-0-4e21bfcfc818@rbox.co/ > >Signed-off-by: Michal Luczaj <mhal@rbox.co> >--- >Michal Luczaj (4): > vsock/test: Use NSEC_PER_SEC > vsock/test: Add test for accept_queue memory leak > vsock/test: Add test for sk_error_queue memory leak > vsock/test: Add test for MSG_ZEROCOPY completion memory leak > > tools/testing/vsock/vsock_test.c | 159 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 157 insertions(+), 2 deletions(-) >--- >base-commit: 51db5c8943001186be0b5b02456e7d03b3be1f12 >change-id: 20241203-test-vsock-leaks-38f9559f5636 > >Best regards, >-- >Michal Luczaj <mhal@rbox.co> >
On 12/10/24 17:25, Stefano Garzarella wrote: >> [...] >> I initially considered triggering (and parsing) a kmemleak scan after each >> test, but ultimately concluded that the slowdown and the required >> privileges would be too much. > > Yeah, what about adding something in the README to suggest using > kmemleak and how to check that everything is okay after a run? Something like this? diff --git a/tools/testing/vsock/README b/tools/testing/vsock/README index 84ee217ba8ee..0d6e73ecbf4d 100644 --- a/tools/testing/vsock/README +++ b/tools/testing/vsock/README @@ -36,6 +36,21 @@ Invoke test binaries in both directions as follows: --control-port=1234 \ --peer-cid=3 +Some tests are designed to produce kernel memory leaks. Leaks detection, +however, is deferred to Kernel Memory Leak Detector. It is recommended to enable +kmemleak (CONFIG_DEBUG_KMEMLEAK=y) and explicitly trigger a scan after each test +run, e.g. + + # echo clear > /sys/kernel/debug/kmemleak + # $TEST_BINARY ... + # echo "wait for any grace periods" && sleep 2 + # echo scan > /sys/kernel/debug/kmemleak + # echo "wait for kmemleak" && sleep 5 + # echo scan > /sys/kernel/debug/kmemleak + # cat /sys/kernel/debug/kmemleak + +For more information see Documentation/dev-tools/kmemleak.rst. + vsock_perf utility ------------------- 'vsock_perf' is a simple tool to measure vsock performance. It works in > I'd suggest also to add something about that in each patch that > introduce tests where we expects the user to check kmemleak, > at least with a comment on top of the test functions, and maybe > also in the commit description. Sure, will do. Thanks, Michal
On Fri, Dec 13, 2024 at 01:41:56AM +0100, Michal Luczaj wrote: >On 12/10/24 17:25, Stefano Garzarella wrote: >>> [...] >>> I initially considered triggering (and parsing) a kmemleak scan after each >>> test, but ultimately concluded that the slowdown and the required >>> privileges would be too much. >> >> Yeah, what about adding something in the README to suggest using >> kmemleak and how to check that everything is okay after a run? > >Something like this? > >diff --git a/tools/testing/vsock/README b/tools/testing/vsock/README >index 84ee217ba8ee..0d6e73ecbf4d 100644 >--- a/tools/testing/vsock/README >+++ b/tools/testing/vsock/README >@@ -36,6 +36,21 @@ Invoke test binaries in both directions as follows: > --control-port=1234 \ > --peer-cid=3 > >+Some tests are designed to produce kernel memory leaks. Leaks detection, >+however, is deferred to Kernel Memory Leak Detector. It is recommended to enable >+kmemleak (CONFIG_DEBUG_KMEMLEAK=y) and explicitly trigger a scan after each test >+run, e.g. >+ >+ # echo clear > /sys/kernel/debug/kmemleak >+ # $TEST_BINARY ... >+ # echo "wait for any grace periods" && sleep 2 >+ # echo scan > /sys/kernel/debug/kmemleak >+ # echo "wait for kmemleak" && sleep 5 >+ # echo scan > /sys/kernel/debug/kmemleak >+ # cat /sys/kernel/debug/kmemleak >+ >+For more information see Documentation/dev-tools/kmemleak.rst. >+ Yep, this would be great! > vsock_perf utility > ------------------- > 'vsock_perf' is a simple tool to measure vsock performance. It works in > >> I'd suggest also to add something about that in each patch that >> introduce tests where we expects the user to check kmemleak, >> at least with a comment on top of the test functions, and maybe >> also in the commit description. > >Sure, will do. Thanks, Stefano
Series adds tests for recently fixed memory leaks[1]: d7b0ff5a8667 ("virtio/vsock: Fix accept_queue memory leak") fbf7085b3ad1 ("vsock: Fix sk_error_queue memory leak") 60cf6206a1f5 ("virtio/vsock: Improve MSG_ZEROCOPY error handling") First patch is a non-functional preparatory cleanup. I initially considered triggering (and parsing) a kmemleak scan after each test, but ultimately concluded that the slowdown and the required privileges would be too much. [1]: https://lore.kernel.org/netdev/20241107-vsock-mem-leaks-v2-0-4e21bfcfc818@rbox.co/ Signed-off-by: Michal Luczaj <mhal@rbox.co> --- Michal Luczaj (4): vsock/test: Use NSEC_PER_SEC vsock/test: Add test for accept_queue memory leak vsock/test: Add test for sk_error_queue memory leak vsock/test: Add test for MSG_ZEROCOPY completion memory leak tools/testing/vsock/vsock_test.c | 159 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 157 insertions(+), 2 deletions(-) --- base-commit: 51db5c8943001186be0b5b02456e7d03b3be1f12 change-id: 20241203-test-vsock-leaks-38f9559f5636 Best regards,