| Message ID | 20240930171753.2572922-8-sdf@fomichev.me (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | selftests: ncdevmem: Add ncdevmem to ksft | expand |
On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote: > > ntuple off/on might be not enough to do it on all NICs. > Add a bunch of shell crap to explicitly remove the rules. > > Cc: Mina Almasry <almasrymina@google.com> > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me> > --- > tools/testing/selftests/net/ncdevmem.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c > index 47458a13eff5..48cbf057fde7 100644 > --- a/tools/testing/selftests/net/ncdevmem.c > +++ b/tools/testing/selftests/net/ncdevmem.c > @@ -207,13 +207,12 @@ void validate_buffer(void *line, size_t size) > > static int reset_flow_steering(void) > { > - int ret = 0; > - > - ret = run_command("sudo ethtool -K %s ntuple off >&2", ifname); > - if (ret) > - return ret; > - > - return run_command("sudo ethtool -K %s ntuple on >&2", ifname); > + run_command("sudo ethtool -K %s ntuple off >&2", ifname); > + run_command("sudo ethtool -K %s ntuple on >&2", ifname); > + run_command( > + "sudo ethtool -n %s | grep 'Filter:' | awk '{print $2}' | xargs -n1 ethtool -N %s delete >&2", > + ifname, ifname); > + return 0; Any reason to remove the checking of the return codes? Silent failures can waste time if the test fails and someone has to spend time finding out its the flow steering reset that failed (it may not be very obvious without the checking of the return code.
On 10/03, Mina Almasry wrote: > On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > ntuple off/on might be not enough to do it on all NICs. > > Add a bunch of shell crap to explicitly remove the rules. > > > > Cc: Mina Almasry <almasrymina@google.com> > > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me> > > --- > > tools/testing/selftests/net/ncdevmem.c | 13 ++++++------- > > 1 file changed, 6 insertions(+), 7 deletions(-) > > > > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c > > index 47458a13eff5..48cbf057fde7 100644 > > --- a/tools/testing/selftests/net/ncdevmem.c > > +++ b/tools/testing/selftests/net/ncdevmem.c > > @@ -207,13 +207,12 @@ void validate_buffer(void *line, size_t size) > > > > static int reset_flow_steering(void) > > { > > - int ret = 0; > > - > > - ret = run_command("sudo ethtool -K %s ntuple off >&2", ifname); > > - if (ret) > > - return ret; > > - > > - return run_command("sudo ethtool -K %s ntuple on >&2", ifname); > > + run_command("sudo ethtool -K %s ntuple off >&2", ifname); > > + run_command("sudo ethtool -K %s ntuple on >&2", ifname); > > + run_command( > > + "sudo ethtool -n %s | grep 'Filter:' | awk '{print $2}' | xargs -n1 ethtool -N %s delete >&2", > > + ifname, ifname); > > + return 0; > > Any reason to remove the checking of the return codes? Silent failures > can waste time if the test fails and someone has to spend time finding > out its the flow steering reset that failed (it may not be very > obvious without the checking of the return code. IIRC, for me the 'ntuple off' part fails because the NIC doesn't let me turn it of. And the new "ethtool .. | grep 'Filter: ' | ..." also fails when there are no existing filters. I will add a comment to clarify..
On Thu, Oct 3, 2024 at 9:42 AM Stanislav Fomichev <stfomichev@gmail.com> wrote: > > On 10/03, Mina Almasry wrote: > > On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > > > ntuple off/on might be not enough to do it on all NICs. > > > Add a bunch of shell crap to explicitly remove the rules. > > > > > > Cc: Mina Almasry <almasrymina@google.com> > > > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me> > > > --- > > > tools/testing/selftests/net/ncdevmem.c | 13 ++++++------- > > > 1 file changed, 6 insertions(+), 7 deletions(-) > > > > > > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c > > > index 47458a13eff5..48cbf057fde7 100644 > > > --- a/tools/testing/selftests/net/ncdevmem.c > > > +++ b/tools/testing/selftests/net/ncdevmem.c > > > @@ -207,13 +207,12 @@ void validate_buffer(void *line, size_t size) > > > > > > static int reset_flow_steering(void) > > > { > > > - int ret = 0; > > > - > > > - ret = run_command("sudo ethtool -K %s ntuple off >&2", ifname); > > > - if (ret) > > > - return ret; > > > - > > > - return run_command("sudo ethtool -K %s ntuple on >&2", ifname); > > > + run_command("sudo ethtool -K %s ntuple off >&2", ifname); > > > + run_command("sudo ethtool -K %s ntuple on >&2", ifname); > > > + run_command( > > > + "sudo ethtool -n %s | grep 'Filter:' | awk '{print $2}' | xargs -n1 ethtool -N %s delete >&2", > > > + ifname, ifname); > > > + return 0; > > > > Any reason to remove the checking of the return codes? Silent failures > > can waste time if the test fails and someone has to spend time finding > > out its the flow steering reset that failed (it may not be very > > obvious without the checking of the return code. > > IIRC, for me the 'ntuple off' part fails because the NIC doesn't let me > turn it of. And the new "ethtool .. | grep 'Filter: ' | ..." also fails > when there are no existing filters. > > I will add a comment to clarify.. Ah, understood. Seems this area is fraught with subtleties. If you have time, maybe to counter these subtleties we can do a get of ntuple filters and confirm they're 0 somehow at the end of the function. That would offset not checking the return code. But, I don't think it's extremely likely to run into errors here? So, this is probably good and can easily be improved later if we run into issues: Reviewed-by: Mina Almasry <almasrymina@google.com>
On 10/03, Mina Almasry wrote: > On Thu, Oct 3, 2024 at 9:42 AM Stanislav Fomichev <stfomichev@gmail.com> wrote: > > > > On 10/03, Mina Almasry wrote: > > > On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > > > > > ntuple off/on might be not enough to do it on all NICs. > > > > Add a bunch of shell crap to explicitly remove the rules. > > > > > > > > Cc: Mina Almasry <almasrymina@google.com> > > > > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me> > > > > --- > > > > tools/testing/selftests/net/ncdevmem.c | 13 ++++++------- > > > > 1 file changed, 6 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c > > > > index 47458a13eff5..48cbf057fde7 100644 > > > > --- a/tools/testing/selftests/net/ncdevmem.c > > > > +++ b/tools/testing/selftests/net/ncdevmem.c > > > > @@ -207,13 +207,12 @@ void validate_buffer(void *line, size_t size) > > > > > > > > static int reset_flow_steering(void) > > > > { > > > > - int ret = 0; > > > > - > > > > - ret = run_command("sudo ethtool -K %s ntuple off >&2", ifname); > > > > - if (ret) > > > > - return ret; > > > > - > > > > - return run_command("sudo ethtool -K %s ntuple on >&2", ifname); > > > > + run_command("sudo ethtool -K %s ntuple off >&2", ifname); > > > > + run_command("sudo ethtool -K %s ntuple on >&2", ifname); > > > > + run_command( > > > > + "sudo ethtool -n %s | grep 'Filter:' | awk '{print $2}' | xargs -n1 ethtool -N %s delete >&2", > > > > + ifname, ifname); > > > > + return 0; > > > > > > Any reason to remove the checking of the return codes? Silent failures > > > can waste time if the test fails and someone has to spend time finding > > > out its the flow steering reset that failed (it may not be very > > > obvious without the checking of the return code. > > > > IIRC, for me the 'ntuple off' part fails because the NIC doesn't let me > > turn it of. And the new "ethtool .. | grep 'Filter: ' | ..." also fails > > when there are no existing filters. > > > > I will add a comment to clarify.. > > Ah, understood. Seems this area is fraught with subtleties. > > If you have time, maybe to counter these subtleties we can do a get of > ntuple filters and confirm they're 0 somehow at the end of the > function. That would offset not checking the return code. > > But, I don't think it's extremely likely to run into errors here? So, > this is probably good and can easily be improved later if we run into > issues: > > Reviewed-by: Mina Almasry <almasrymina@google.com> Ack, I'll keep it as is with a comment. Ideally we should do proper ethtool netlink/ioctl instead of shelling out, but I don' think ntuple API is exposed to netlink and I'm too lazy to dive into how the old ioctl-based ntuple API works :-D
diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c index 47458a13eff5..48cbf057fde7 100644 --- a/tools/testing/selftests/net/ncdevmem.c +++ b/tools/testing/selftests/net/ncdevmem.c @@ -207,13 +207,12 @@ void validate_buffer(void *line, size_t size) static int reset_flow_steering(void) { - int ret = 0; - - ret = run_command("sudo ethtool -K %s ntuple off >&2", ifname); - if (ret) - return ret; - - return run_command("sudo ethtool -K %s ntuple on >&2", ifname); + run_command("sudo ethtool -K %s ntuple off >&2", ifname); + run_command("sudo ethtool -K %s ntuple on >&2", ifname); + run_command( + "sudo ethtool -n %s | grep 'Filter:' | awk '{print $2}' | xargs -n1 ethtool -N %s delete >&2", + ifname, ifname); + return 0; } static int configure_headersplit(bool on)
ntuple off/on might be not enough to do it on all NICs. Add a bunch of shell crap to explicitly remove the rules. Cc: Mina Almasry <almasrymina@google.com> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me> --- tools/testing/selftests/net/ncdevmem.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)