netdev - Re: [PATCH net-next] selftests: net: add netpoll basic functionality test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aFq1z0BS6RCUCNwa@gmail.com>
Date: Tue, 24 Jun 2025 15:27:27 +0100
From: Breno Leitao <leitao@...ian.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Shuah Khan <shuah@...nel.org>, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, linux-kselftest@...r.kernel.org,
	Willem de Bruijn <willemdebruijn.kernel@...il.com>,
	gustavold@...il.com
Subject: Re: [PATCH net-next] selftests: net: add netpoll basic functionality
 test

On Mon, Jun 23, 2025 at 06:30:06PM -0700, Jakub Kicinski wrote:
> Could you turn this into a docstring?

<snip>

Sure, I will fix all of them except C0301, which is a error message
string and I prefer not to truncate.

> > +def set_single_rx_tx_queue(interface_name: str) -> None:
> > +    """Set the number of RX and TX queues to 1 using ethtool"""
> > +    try:
> > +        # This don't need to be reverted, since interfaces will be deleted after test
> > +        ethtool(f"-G {interface_name} rx 1 tx 1")
> 
> Would be nice to be able to run this test on real HW too.
> Can you add appropriate defer() calls to undo the configuration changes?

Ack!

> > +    try:
> > +        for key, value in config_data.items():
> > +            if DEBUG:
> > +                ksft_pr(f"Setting {key} to {value}")
> > +            with open(
> > +                f"{NETCONSOLE_CONFIGFS_PATH}/{target_name}/{key}",
> 
> Could be personal preference but I think that using temp variable to
> store the argument looks better than breaking out the function call
> over 5 lines..

I was not able to get what you mean here, sorry.

We have config_data, which is a dictionary that stores the netconsole
keys (as in configfs) and their value, which will be set in the code below.

What would this temp variable look like, and how it would look like?

> > +def test_netpoll(cfg: NetDrvEpEnv, netdevnl: NetdevFamily) -> None:
> > +    """
> > +    Test netpoll by sending traffic to the interface and then sending
> > +    netconsole messages to trigger a poll
> > +    """
> > +
> > +    target_name = generate_random_netcons_name()
> > +    ifname = cfg.dev["ifname"]
> > +    traffic = None
> > +
> > +    try:
> > +        set_single_rx_tx_queue(ifname)
> > +        traffic = GenerateTraffic(cfg)
> > +        check_traffic_flowing(cfg, netdevnl)
> 
> Any reason to perform this check? GenerateTraffic() already waits for
> traffic to ramp up. Do we need to adjust the logic there, or make some
> methods public?

Not really. I can just remove this code, in fact, given
GenerateTraffic() already waits for the code. Or, I can add under DEBUG.

As we discussed in the RFC thread, I will add support for bpftrace in
the v2.

Thanks for the review,
--breno