Re: [PATCH net-next v4 5/5] selftests: drv-net-hw: Add a test for symmetric RSS hash

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 20 Feb 2025 13:34:35 +0200 Gal Pressman wrote:
> +def _get_rand_port(remote):
> +    for _ in range(1000):
> +        port = rand_port()
> +        try:
> +            check_port_available_remote(port, remote)
> +            return port
> +        except:
> +            continue
> +
> +    raise Exception("Can't find any free unprivileged port")

TCP and UDP port spaces are separate, I think your checking if the
ports are available on TCP here, and then use them for UDP below.

We don't really care about the 100% success, I don't think we should 
be checking the ports. Pick two ports, send a A<>B packet, send a B<>A
packet, if either fails to connect or doesn't arrive just ignore.
As long as we can get ~10? successful pairs in 100? ties it's good.

> +def traffic(cfg, local_port, remote_port, ipver):
> +    af_inet = socket.AF_INET if ipver == "4" else socket.AF_INET6
> +    sock = socket.socket(af_inet, socket.SOCK_DGRAM)
> +    sock.bind(('', local_port))
> +    sock.connect((cfg.remote_addr_v[ipver], remote_port))
> +    tgt = f"{ipver}:[{cfg.addr_v[ipver]}]:{local_port},sourceport={remote_port}"
> +    cmd("echo a | socat - UDP" + tgt, host=cfg.remote)
> +    sock.recvmsg(100)

Could you use fd_read_timeout():
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/tools/testing/selftests/net/lib/py/utils.py#n20

In case the packet got lost?

> +    return sock.getsockopt(socket.SOL_SOCKET, socket.SO_INCOMING_CPU)
> +
> +
> +def test_rss_input_xfrm(cfg, ipver):
> +    """
> +    Test symmetric input_xfrm.
> +    If symmetric RSS hash is configured, send traffic twice, swapping the
> +    src/dst UDP ports, and verify that the same queue is receiving the traffic
> +    in both cases (IPs are constant).
> +    """
> +
> +    input_xfrm = cfg.ethnl.rss_get(
> +        {'header': {'dev-name': cfg.ifname}}).get('input_xfrm')
> +
> +    # Check for symmetric xor/or-xor
> +    if input_xfrm and (input_xfrm == 1 or input_xfrm == 2):
> +        cpus = set()
> +        for _ in range(8):
> +            port1 = _get_rand_port(cfg.remote)
> +            port2 = _get_rand_port(cfg.remote)
> +            cpu1 = traffic(cfg, port1, port2, ipver)
> +            cpu2 = traffic(cfg, port2, port1, ipver)
> +            cpus.update([cpu1, cpu2])
> +
> +            ksft_eq(
> +                cpu1, cpu2, comment=f"Received traffic on different cpus ({cpu1} != {cpu2}) with ports ({port1 = }, {port2 = }) while symmetric hash is configured")

the cpu1 cpu2 values will already be printed by the helper, no need 
to format them in

> +
> +        ksft_ge(len(cpus), 2, comment=f"Received traffic on less than two cpus")
> +    else:
> +        raise KsftSkipEx("Symmetric RSS hash not requested")

Flip the condition, raise the exception right after the if, then the
rest of the code doesn't have to be indented?

I'd also add a:

	if len(cpus) == 1:
		raise KsftSkipEx(f"Only one CPU seen traffic: {cpus}")
-- 
pw-bot: cr