[PATCH net-next v1 4/9] net: devmem: ksft: remove ksft_disruptive

Mina Almasry posted 9 patches 7 months ago
There is a newer version of this series
[PATCH net-next v1 4/9] net: devmem: ksft: remove ksft_disruptive
Posted by Mina Almasry 7 months ago
As far as I can tell the ksft_disruptive here is unnecessary. These
tests are largerly independent, and when one test fails, it's nice to
know the results from all the other test cases.

Signed-off-by: Mina Almasry <almasrymina@google.com>

---
 tools/testing/selftests/drivers/net/hw/devmem.py | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/hw/devmem.py b/tools/testing/selftests/drivers/net/hw/devmem.py
index 7fc686cf47a2..f5d7809400ea 100755
--- a/tools/testing/selftests/drivers/net/hw/devmem.py
+++ b/tools/testing/selftests/drivers/net/hw/devmem.py
@@ -6,7 +6,6 @@ from lib.py import ksft_run, ksft_exit
 from lib.py import ksft_eq, KsftSkipEx
 from lib.py import NetDrvEpEnv
 from lib.py import bkg, cmd, rand_port, wait_port_listen
-from lib.py import ksft_disruptive
 
 
 def require_devmem(cfg):
@@ -19,7 +18,6 @@ def require_devmem(cfg):
         raise KsftSkipEx("Test requires devmem support")
 
 
-@ksft_disruptive
 def check_rx(cfg) -> None:
     cfg.require_ipver("6")
     require_devmem(cfg)
@@ -34,7 +32,6 @@ def check_rx(cfg) -> None:
     ksft_eq(socat.stdout.strip(), "hello\nworld")
 
 
-@ksft_disruptive
 def check_tx(cfg) -> None:
     cfg.require_ipver("6")
     require_devmem(cfg)
-- 
2.49.0.1101.gccaa498523-goog
Re: [PATCH net-next v1 4/9] net: devmem: ksft: remove ksft_disruptive
Posted by Stanislav Fomichev 7 months ago
On 05/19, Mina Almasry wrote:
> As far as I can tell the ksft_disruptive here is unnecessary. These
> tests are largerly independent, and when one test fails, it's nice to
> know the results from all the other test cases.

We currently don't do anything special for disruptive tests. I'm assuming
anything that changes nic configuration is disruptive and was thinking of
an option to run all disruptive tests at the end of the run. But so far we
haven't had any problem with mixing disruptive and non-disruptive tests,
so it's all moot. I'd prefer to keep everything as is for now (or remove
this whole disruptive category).
Re: [PATCH net-next v1 4/9] net: devmem: ksft: remove ksft_disruptive
Posted by Mina Almasry 7 months ago
On Mon, May 19, 2025 at 8:25 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 05/19, Mina Almasry wrote:
> > As far as I can tell the ksft_disruptive here is unnecessary. These
> > tests are largerly independent, and when one test fails, it's nice to
> > know the results from all the other test cases.
>
> We currently don't do anything special for disruptive tests. I'm assuming
> anything that changes nic configuration is disruptive and was thinking of
> an option to run all disruptive tests at the end of the run. But so far we
> haven't had any problem with mixing disruptive and non-disruptive tests,
> so it's all moot. I'd prefer to keep everything as is for now (or remove
> this whole disruptive category).

I've noticed that if all the tests are marked disruptive, and one test
fails, the others don't run at all, which seems unnecessary. I'd like
to see if the rx test passed if the tx one failed and vice versa for
example. Removing the disruptive tag seems to resolve that.

dmabuf bind is automatically unbound when ncdevmem exits, so i don't
think these tests leave the nic in a bad state or anything that
warrants blocking running the other tests?

-- 
Thanks,
Mina
Re: [PATCH net-next v1 4/9] net: devmem: ksft: remove ksft_disruptive
Posted by Stanislav Fomichev 7 months ago
On 05/19, Mina Almasry wrote:
> On Mon, May 19, 2025 at 8:25 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
> >
> > On 05/19, Mina Almasry wrote:
> > > As far as I can tell the ksft_disruptive here is unnecessary. These
> > > tests are largerly independent, and when one test fails, it's nice to
> > > know the results from all the other test cases.
> >
> > We currently don't do anything special for disruptive tests. I'm assuming
> > anything that changes nic configuration is disruptive and was thinking of
> > an option to run all disruptive tests at the end of the run. But so far we
> > haven't had any problem with mixing disruptive and non-disruptive tests,
> > so it's all moot. I'd prefer to keep everything as is for now (or remove
> > this whole disruptive category).
> 
> I've noticed that if all the tests are marked disruptive, and one test
> fails, the others don't run at all, which seems unnecessary. I'd like
> to see if the rx test passed if the tx one failed and vice versa for
> example. Removing the disruptive tag seems to resolve that.

I don't think that's the expected behavior. Disruptive should not
have any effect on other tests if any one fails. Any idea why it happens?
Re: [PATCH net-next v1 4/9] net: devmem: ksft: remove ksft_disruptive
Posted by Jakub Kicinski 7 months ago
On Mon, 19 May 2025 13:18:24 -0700 Stanislav Fomichev wrote:
> On 05/19, Mina Almasry wrote:
> > On Mon, May 19, 2025 at 8:25 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:  
> > >
> > > On 05/19, Mina Almasry wrote:  
> > > > As far as I can tell the ksft_disruptive here is unnecessary. These
> > > > tests are largerly independent, and when one test fails, it's nice to
> > > > know the results from all the other test cases.  
> > >
> > > We currently don't do anything special for disruptive tests. I'm assuming
> > > anything that changes nic configuration is disruptive and was thinking of
> > > an option to run all disruptive tests at the end of the run. But so far we
> > > haven't had any problem with mixing disruptive and non-disruptive tests,
> > > so it's all moot. I'd prefer to keep everything as is for now (or remove
> > > this whole disruptive category).  
> > 
> > I've noticed that if all the tests are marked disruptive, and one test
> > fails, the others don't run at all, which seems unnecessary. I'd like
> > to see if the rx test passed if the tx one failed and vice versa for
> > example. Removing the disruptive tag seems to resolve that.  
> 
> I don't think that's the expected behavior. Disruptive should not
> have any effect on other tests if any one fails. Any idea why it happens?

Right, this sounds odd and needs investigating.

FWIW, in my mind disruptive tests were supposed to be the tests which
can cut a single NIC machine off the network. IOW the use case is that
the NIC under test is the same NIC over which we're SSH'ing into the
machine to run the test. So we shouldn't do things like take the link
down or flush the IP addresses because it may kill our SSH connection.

Obviously every test may be somewhat disruptive, but most shouldn't
break ssh?