| Message ID | 158871160668.7537.2576154513696580062.stgit@john-Precision-5820-Tower |
|---|---|
| Headers | show |
| Series | bpf: selftests, test_sockmap improvements | expand |
John Fastabend wrote: > Update test_sockmap to add ktls tests and in the process make output > easier to understand and reduce overall runtime significantly. Before > this series test_sockmap did a poor job of tracking sent bytes causing > the recv thread to wait for a timeout even though all expected bytes > had been received. Doing this many times causes significant delays. > Further, we did many redundant tests because the send/recv test we used > was not specific to the parameters we were testing. For example testing > a failure case that always fails many times with different send sizes > is mostly useless. If the test condition catches 10B in the kernel code > testing 100B, 1kB, 4kB, and so on is just noise. > > The main motivation for this is to add ktls tests, the last patch. Until > now I have been running these locally but we haven't had them checked in > to selftests. And finally I'm hoping to get these pushed into the libbpf > test infrastructure so we can get more testing. For that to work we need > ability to white and blacklist tests based on kernel features so we add > that here as well. > I forgot to note this series needs to be merged after the series here, https://patchwork.ozlabs.org/project/netdev/list/?series=174562 Otherwise we may hit the error from that series in selftests. It should be no problem to let the series sit on the list for a few days while the above series gets into bpf-next but if you would prefer I can resubmit later. Thanks, John
On Tue, 05 May 2020 13:49:36 -0700 John Fastabend <john.fastabend@gmail.com> wrote: > Update test_sockmap to add ktls tests and in the process make output > easier to understand and reduce overall runtime significantly. Before > this series test_sockmap did a poor job of tracking sent bytes causing > the recv thread to wait for a timeout even though all expected bytes > had been received. Doing this many times causes significant delays. > Further, we did many redundant tests because the send/recv test we used > was not specific to the parameters we were testing. For example testing > a failure case that always fails many times with different send sizes > is mostly useless. If the test condition catches 10B in the kernel code > testing 100B, 1kB, 4kB, and so on is just noise. > > The main motivation for this is to add ktls tests, the last patch. Until > now I have been running these locally but we haven't had them checked in > to selftests. And finally I'm hoping to get these pushed into the libbpf > test infrastructure so we can get more testing. For that to work we need > ability to white and blacklist tests based on kernel features so we add > that here as well. > > The new output looks like this broken into test groups with subtest > counters, > > $ time sudo ./test_sockmap > # 1/ 6 sockmap:txmsg test passthrough:OK > # 2/ 6 sockmap:txmsg test redirect:OK > ... > #22/ 1 sockhash:txmsg test push/pop data:OK > Pass: 22 Fail: 0 > > real 0m9.790s > user 0m0.093s > sys 0m7.318s > > The old output printed individual subtest and was rather noisy > > $ time sudo ./test_sockmap > [TEST 0]: (1, 1, 1, sendmsg, pass,): PASS > ... > [TEST 823]: (16, 1, 100, sendpage, ... ,pop (1599,1609),): PASS > Summary: 824 PASSED 0 FAILED > > real 0m56.761s > user 0m0.455s > sys 0m31.757s > > So we are able to reduce time from ~56s to ~10s. To recover older more > verbose output simply run with --verbose option. To whitelist and > blacklist tests use the new --whitelist and --blacklist flags added. For > example to run cork sockhash tests but only ones that don't have a receive > hang (used to test negative cases) we could do, > > $ ./test_sockmap --whitelist="cork" --blacklist="sockmap,hang" > > --- These are very nice improvements so thanks for putting time into it. I run these whenever I touch sockmap, and they do currently take long to run, especially on 1 vCPU (which sometimes catches interesting bugs). I've also ran before into the CLI quirks you've ironed out, like having to pass path to cgroup to get verbose output. Feel free to add my: Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
Jakub Sitnicki wrote: > On Tue, 05 May 2020 13:49:36 -0700 > John Fastabend <john.fastabend@gmail.com> wrote: > > > Update test_sockmap to add ktls tests and in the process make output > > easier to understand and reduce overall runtime significantly. Before > > this series test_sockmap did a poor job of tracking sent bytes causing > > the recv thread to wait for a timeout even though all expected bytes > > had been received. Doing this many times causes significant delays. > > Further, we did many redundant tests because the send/recv test we used > > was not specific to the parameters we were testing. For example testing > > a failure case that always fails many times with different send sizes > > is mostly useless. If the test condition catches 10B in the kernel code > > testing 100B, 1kB, 4kB, and so on is just noise. > > > > The main motivation for this is to add ktls tests, the last patch. Until > > now I have been running these locally but we haven't had them checked in > > to selftests. And finally I'm hoping to get these pushed into the libbpf > > test infrastructure so we can get more testing. For that to work we need > > ability to white and blacklist tests based on kernel features so we add > > that here as well. > > > > The new output looks like this broken into test groups with subtest > > counters, > > > > $ time sudo ./test_sockmap > > # 1/ 6 sockmap:txmsg test passthrough:OK > > # 2/ 6 sockmap:txmsg test redirect:OK > > ... > > #22/ 1 sockhash:txmsg test push/pop data:OK > > Pass: 22 Fail: 0 > > > > real 0m9.790s > > user 0m0.093s > > sys 0m7.318s > > > > The old output printed individual subtest and was rather noisy > > > > $ time sudo ./test_sockmap > > [TEST 0]: (1, 1, 1, sendmsg, pass,): PASS > > ... > > [TEST 823]: (16, 1, 100, sendpage, ... ,pop (1599,1609),): PASS > > Summary: 824 PASSED 0 FAILED > > > > real 0m56.761s > > user 0m0.455s > > sys 0m31.757s > > > > So we are able to reduce time from ~56s to ~10s. To recover older more > > verbose output simply run with --verbose option. To whitelist and > > blacklist tests use the new --whitelist and --blacklist flags added. For > > example to run cork sockhash tests but only ones that don't have a receive > > hang (used to test negative cases) we could do, > > > > $ ./test_sockmap --whitelist="cork" --blacklist="sockmap,hang" > > > > --- > > These are very nice improvements so thanks for putting time into it. > > I run these whenever I touch sockmap, and they do currently take long to > run, especially on 1 vCPU (which sometimes catches interesting bugs). > > I've also ran before into the CLI quirks you've ironed out, like having > to pass path to cgroup to get verbose output. > > Feel free to add my: > > Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com> Thanks will do. I'll push a v2 with the other comments addressed after bpf gets synced into bpf-next.
On Tue, May 5, 2020 at 1:50 PM John Fastabend <john.fastabend@gmail.com> wrote: > > Update test_sockmap to add ktls tests and in the process make output > easier to understand and reduce overall runtime significantly. Before > this series test_sockmap did a poor job of tracking sent bytes causing > the recv thread to wait for a timeout even though all expected bytes > had been received. Doing this many times causes significant delays. > Further, we did many redundant tests because the send/recv test we used > was not specific to the parameters we were testing. For example testing > a failure case that always fails many times with different send sizes > is mostly useless. If the test condition catches 10B in the kernel code > testing 100B, 1kB, 4kB, and so on is just noise. > > The main motivation for this is to add ktls tests, the last patch. Until > now I have been running these locally but we haven't had them checked in > to selftests. And finally I'm hoping to get these pushed into the libbpf > test infrastructure so we can get more testing. For that to work we need > ability to white and blacklist tests based on kernel features so we add > that here as well. > > The new output looks like this broken into test groups with subtest > counters, > > $ time sudo ./test_sockmap > # 1/ 6 sockmap:txmsg test passthrough:OK > # 2/ 6 sockmap:txmsg test redirect:OK > ... > #22/ 1 sockhash:txmsg test push/pop data:OK > Pass: 22 Fail: 0 > > real 0m9.790s > user 0m0.093s > sys 0m7.318s > > The old output printed individual subtest and was rather noisy > > $ time sudo ./test_sockmap > [TEST 0]: (1, 1, 1, sendmsg, pass,): PASS > ... > [TEST 823]: (16, 1, 100, sendpage, ... ,pop (1599,1609),): PASS > Summary: 824 PASSED 0 FAILED > > real 0m56.761s > user 0m0.455s > sys 0m31.757s > > So we are able to reduce time from ~56s to ~10s. To recover older more > verbose output simply run with --verbose option. To whitelist and > blacklist tests use the new --whitelist and --blacklist flags added. For > example to run cork sockhash tests but only ones that don't have a receive > hang (used to test negative cases) we could do, > > $ ./test_sockmap --whitelist="cork" --blacklist="sockmap,hang" > > --- A lot of this seems to be re-implementing good chunks of what we already have in test_progs. Would it make more sense to either extract test runner pieces from test_progs into something that can be easily re-used for creating other test runners or just fold all these test into test_progs framework? None of this code is fun to write and maintain, so I'd rather have less copies of it :) > > John Fastabend (10): > bpf: selftests, move sockmap bpf prog header into progs > bpf: selftests, remove prints from sockmap tests > bpf: selftests, sockmap test prog run without setting cgroup > bpf: selftests, print error in test_sockmap error cases > bpf: selftests, improve test_sockmap total bytes counter > bpf: selftests, break down test_sockmap into subtests > bpf: selftests, provide verbose option for selftests execution > bpf: selftests, add whitelist option to test_sockmap > bpf: selftests, add blacklist to test_sockmap > bpf: selftests, add ktls tests to test_sockmap > > > .../selftests/bpf/progs/test_sockmap_kern.h | 299 +++++++ > tools/testing/selftests/bpf/test_sockmap.c | 911 ++++++++++---------- > tools/testing/selftests/bpf/test_sockmap_kern.h | 451 ---------- > 3 files changed, 769 insertions(+), 892 deletions(-) > create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_kern.h > delete mode 100644 tools/testing/selftests/bpf/test_sockmap_kern.h > > -- > Signature
Andrii Nakryiko wrote: > On Tue, May 5, 2020 at 1:50 PM John Fastabend <john.fastabend@gmail.com> wrote: > > > > Update test_sockmap to add ktls tests and in the process make output > > easier to understand and reduce overall runtime significantly. Before > > this series test_sockmap did a poor job of tracking sent bytes causing > > the recv thread to wait for a timeout even though all expected bytes > > had been received. Doing this many times causes significant delays. > > Further, we did many redundant tests because the send/recv test we used > > was not specific to the parameters we were testing. For example testing > > a failure case that always fails many times with different send sizes > > is mostly useless. If the test condition catches 10B in the kernel code > > testing 100B, 1kB, 4kB, and so on is just noise. > > > > The main motivation for this is to add ktls tests, the last patch. Until > > now I have been running these locally but we haven't had them checked in > > to selftests. And finally I'm hoping to get these pushed into the libbpf > > test infrastructure so we can get more testing. For that to work we need > > ability to white and blacklist tests based on kernel features so we add > > that here as well. > > > > The new output looks like this broken into test groups with subtest > > counters, > > > > $ time sudo ./test_sockmap > > # 1/ 6 sockmap:txmsg test passthrough:OK > > # 2/ 6 sockmap:txmsg test redirect:OK > > ... > > #22/ 1 sockhash:txmsg test push/pop data:OK > > Pass: 22 Fail: 0 > > > > real 0m9.790s > > user 0m0.093s > > sys 0m7.318s > > > > The old output printed individual subtest and was rather noisy > > > > $ time sudo ./test_sockmap > > [TEST 0]: (1, 1, 1, sendmsg, pass,): PASS > > ... > > [TEST 823]: (16, 1, 100, sendpage, ... ,pop (1599,1609),): PASS > > Summary: 824 PASSED 0 FAILED > > > > real 0m56.761s > > user 0m0.455s > > sys 0m31.757s > > > > So we are able to reduce time from ~56s to ~10s. To recover older more > > verbose output simply run with --verbose option. To whitelist and > > blacklist tests use the new --whitelist and --blacklist flags added. For > > example to run cork sockhash tests but only ones that don't have a receive > > hang (used to test negative cases) we could do, > > > > $ ./test_sockmap --whitelist="cork" --blacklist="sockmap,hang" > > > > --- > > A lot of this seems to be re-implementing good chunks of what we > already have in test_progs. Would it make more sense to either extract > test runner pieces from test_progs into something that can be easily > re-used for creating other test runners or just fold all these test > into test_progs framework? None of this code is fun to write and > maintain, so I'd rather have less copies of it :) I like having its own test runner for test_sockmap. At leat I like having the CLI around to run arbitrary tests while doing devloping of BPF programs and on the kernel side. We could fold all the test progs into progs framework but because I want to keep test_sockmap CLI around it didn't make much sense. I would still need most the code for the tool. So I think the best thing is to share as much code as possible. I am working on a series behind this to share more code on the socket attach, listend, send/recv side but seeing this series was getting large and adds the ktls support which I want in selftests asap I pushed it. Once test_sockmap starts using the shared socket senders, receivers, etc. I hope lots of code will dissapper. The next easy candidate is the cgroup helpers. test_progs has test__join_cgroup() test_sockmap has equivelent. Its possible we could have used the same prog_test_def{} struct but it seemed a bit overkill to me for this the _test{} struct and helpers is ~50 lines here. Getting the socket handling and cgroup handling into helpers seems like a bigger win. Maybe the blacklist/whitelist could be reused with some refactoring and avoid one-off codei for that as well. Bottom line for me is this series improves things a lot on the test_sockmap side with a reasonable patch series size. I agree with your sentiment though and would propose doing those things in follow up series likely in this order: socket handlers, cgroup handlers, tester structure. Thanks! > > > > > John Fastabend (10): > > bpf: selftests, move sockmap bpf prog header into progs > > bpf: selftests, remove prints from sockmap tests > > bpf: selftests, sockmap test prog run without setting cgroup > > bpf: selftests, print error in test_sockmap error cases > > bpf: selftests, improve test_sockmap total bytes counter > > bpf: selftests, break down test_sockmap into subtests > > bpf: selftests, provide verbose option for selftests execution > > bpf: selftests, add whitelist option to test_sockmap > > bpf: selftests, add blacklist to test_sockmap > > bpf: selftests, add ktls tests to test_sockmap > > > > > > .../selftests/bpf/progs/test_sockmap_kern.h | 299 +++++++ > > tools/testing/selftests/bpf/test_sockmap.c | 911 ++++++++++---------- > > tools/testing/selftests/bpf/test_sockmap_kern.h | 451 ---------- > > 3 files changed, 769 insertions(+), 892 deletions(-) > > create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_kern.h > > delete mode 100644 tools/testing/selftests/bpf/test_sockmap_kern.h > > > > -- > > Signature
On Sat, May 9, 2020 at 7:55 AM John Fastabend <john.fastabend@gmail.com> wrote: > > Andrii Nakryiko wrote: > > On Tue, May 5, 2020 at 1:50 PM John Fastabend <john.fastabend@gmail.com> wrote: > > > > > > Update test_sockmap to add ktls tests and in the process make output > > > easier to understand and reduce overall runtime significantly. Before > > > this series test_sockmap did a poor job of tracking sent bytes causing > > > the recv thread to wait for a timeout even though all expected bytes > > > had been received. Doing this many times causes significant delays. > > > Further, we did many redundant tests because the send/recv test we used > > > was not specific to the parameters we were testing. For example testing > > > a failure case that always fails many times with different send sizes > > > is mostly useless. If the test condition catches 10B in the kernel code > > > testing 100B, 1kB, 4kB, and so on is just noise. > > > > > > The main motivation for this is to add ktls tests, the last patch. Until > > > now I have been running these locally but we haven't had them checked in > > > to selftests. And finally I'm hoping to get these pushed into the libbpf > > > test infrastructure so we can get more testing. For that to work we need > > > ability to white and blacklist tests based on kernel features so we add > > > that here as well. > > > > > > The new output looks like this broken into test groups with subtest > > > counters, > > > > > > $ time sudo ./test_sockmap > > > # 1/ 6 sockmap:txmsg test passthrough:OK > > > # 2/ 6 sockmap:txmsg test redirect:OK > > > ... > > > #22/ 1 sockhash:txmsg test push/pop data:OK > > > Pass: 22 Fail: 0 > > > > > > real 0m9.790s > > > user 0m0.093s > > > sys 0m7.318s > > > > > > The old output printed individual subtest and was rather noisy > > > > > > $ time sudo ./test_sockmap > > > [TEST 0]: (1, 1, 1, sendmsg, pass,): PASS > > > ... > > > [TEST 823]: (16, 1, 100, sendpage, ... ,pop (1599,1609),): PASS > > > Summary: 824 PASSED 0 FAILED > > > > > > real 0m56.761s > > > user 0m0.455s > > > sys 0m31.757s > > > > > > So we are able to reduce time from ~56s to ~10s. To recover older more > > > verbose output simply run with --verbose option. To whitelist and > > > blacklist tests use the new --whitelist and --blacklist flags added. For > > > example to run cork sockhash tests but only ones that don't have a receive > > > hang (used to test negative cases) we could do, > > > > > > $ ./test_sockmap --whitelist="cork" --blacklist="sockmap,hang" > > > > > > --- > > > > A lot of this seems to be re-implementing good chunks of what we > > already have in test_progs. Would it make more sense to either extract > > test runner pieces from test_progs into something that can be easily > > re-used for creating other test runners or just fold all these test > > into test_progs framework? None of this code is fun to write and > > maintain, so I'd rather have less copies of it :) > > I like having its own test runner for test_sockmap. At leat I like > having the CLI around to run arbitrary tests while doing devloping > of BPF programs and on the kernel side. Keeping them in separate binary is fine with me, but just wanted to make sure you are aware of -t and -n options to test_progs? -t test-substring[/subtest-substring] allows to select test(s), and, optionally, subtests(s) by substring of their names. -n allows to do selection by test/subtest numbers. This allows a very nice way to debug/develop singular test or a small subset of tests. Just FYI, in case you missed this feature. > > We could fold all the test progs into progs framework but because > I want to keep test_sockmap CLI around it didn't make much sense. > I would still need most the code for the tool. > > So I think the best thing is to share as much code as possible. > I am working on a series behind this to share more code on the > socket attach, listend, send/recv side but seeing this series was > getting large and adds the ktls support which I want in selftests > asap I pushed it. Once test_sockmap starts using the shared socket > senders, receivers, etc. I hope lots of code will dissapper. > > The next easy candidate is the cgroup helpers. test_progs has > test__join_cgroup() test_sockmap has equivelent. > > Its possible we could have used the same prog_test_def{} struct > but it seemed a bit overkill to me for this the _test{} struct > and helpers is ~50 lines here. Getting the socket handling and > cgroup handling into helpers seems like a bigger win. Test_progs is doing a bit more than just that. It's about 600 lines of code just in test_progs.c, which does generic test running/reporting/logging interception, etc. Plus some more in test_progs.h. So I think sharing that "test runner base" could save more code and allow more flexible test runner experience. > > Maybe the blacklist/whitelist could be reused with some refactoring > and avoid one-off codei for that as well. > > Bottom line for me is this series improves things a lot on > the test_sockmap side with a reasonable patch series size. I > agree with your sentiment though and would propose doing those > things in follow up series likely in this order: socket handlers, > cgroup handlers, tester structure. Yep, makes sense. I just wanted to make sure that this is on the table. Thanks! > > Thanks! > > > > > > > > > > John Fastabend (10): > > > bpf: selftests, move sockmap bpf prog header into progs > > > bpf: selftests, remove prints from sockmap tests > > > bpf: selftests, sockmap test prog run without setting cgroup > > > bpf: selftests, print error in test_sockmap error cases > > > bpf: selftests, improve test_sockmap total bytes counter > > > bpf: selftests, break down test_sockmap into subtests > > > bpf: selftests, provide verbose option for selftests execution > > > bpf: selftests, add whitelist option to test_sockmap > > > bpf: selftests, add blacklist to test_sockmap > > > bpf: selftests, add ktls tests to test_sockmap > > > > > > > > > .../selftests/bpf/progs/test_sockmap_kern.h | 299 +++++++ > > > tools/testing/selftests/bpf/test_sockmap.c | 911 ++++++++++---------- > > > tools/testing/selftests/bpf/test_sockmap_kern.h | 451 ---------- > > > 3 files changed, 769 insertions(+), 892 deletions(-) > > > create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_kern.h > > > delete mode 100644 tools/testing/selftests/bpf/test_sockmap_kern.h > > > > > > -- > > > Signature > >
Update test_sockmap to add ktls tests and in the process make output easier to understand and reduce overall runtime significantly. Before this series test_sockmap did a poor job of tracking sent bytes causing the recv thread to wait for a timeout even though all expected bytes had been received. Doing this many times causes significant delays. Further, we did many redundant tests because the send/recv test we used was not specific to the parameters we were testing. For example testing a failure case that always fails many times with different send sizes is mostly useless. If the test condition catches 10B in the kernel code testing 100B, 1kB, 4kB, and so on is just noise. The main motivation for this is to add ktls tests, the last patch. Until now I have been running these locally but we haven't had them checked in to selftests. And finally I'm hoping to get these pushed into the libbpf test infrastructure so we can get more testing. For that to work we need ability to white and blacklist tests based on kernel features so we add that here as well. The new output looks like this broken into test groups with subtest counters, $ time sudo ./test_sockmap # 1/ 6 sockmap:txmsg test passthrough:OK # 2/ 6 sockmap:txmsg test redirect:OK ... #22/ 1 sockhash:txmsg test push/pop data:OK Pass: 22 Fail: 0 real 0m9.790s user 0m0.093s sys 0m7.318s The old output printed individual subtest and was rather noisy $ time sudo ./test_sockmap [TEST 0]: (1, 1, 1, sendmsg, pass,): PASS ... [TEST 823]: (16, 1, 100, sendpage, ... ,pop (1599,1609),): PASS Summary: 824 PASSED 0 FAILED real 0m56.761s user 0m0.455s sys 0m31.757s So we are able to reduce time from ~56s to ~10s. To recover older more verbose output simply run with --verbose option. To whitelist and blacklist tests use the new --whitelist and --blacklist flags added. For example to run cork sockhash tests but only ones that don't have a receive hang (used to test negative cases) we could do, $ ./test_sockmap --whitelist="cork" --blacklist="sockmap,hang" --- John Fastabend (10): bpf: selftests, move sockmap bpf prog header into progs bpf: selftests, remove prints from sockmap tests bpf: selftests, sockmap test prog run without setting cgroup bpf: selftests, print error in test_sockmap error cases bpf: selftests, improve test_sockmap total bytes counter bpf: selftests, break down test_sockmap into subtests bpf: selftests, provide verbose option for selftests execution bpf: selftests, add whitelist option to test_sockmap bpf: selftests, add blacklist to test_sockmap bpf: selftests, add ktls tests to test_sockmap .../selftests/bpf/progs/test_sockmap_kern.h | 299 +++++++ tools/testing/selftests/bpf/test_sockmap.c | 911 ++++++++++---------- tools/testing/selftests/bpf/test_sockmap_kern.h | 451 ---------- 3 files changed, 769 insertions(+), 892 deletions(-) create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_kern.h delete mode 100644 tools/testing/selftests/bpf/test_sockmap_kern.h -- Signature