Re: [PATCH net-next v10 22/22] io_uring/zcrx: add selftest

[Stanislav Fomichev <stfomichev@gmail.com>]

On 01/13, Pavel Begunkov wrote:
> On 1/9/25 17:50, David Wei wrote:
> > On 2025-01-09 09:33, Stanislav Fomichev wrote:
> > > On 01/08, David Wei wrote:
> > > > Add a selftest for io_uring zero copy Rx. This test cannot run locally
> > > > and requires a remote host to be configured in net.config. The remote
> > > > host must have hardware support for zero copy Rx as listed in the
> > > > documentation page. The test will restore the NIC config back to before
> > > > the test and is idempotent.
> > > > 
> [...]
> > > > +
> > > > +		if (addr &&
> > > > +		    inet_pton(AF_INET6, addr, &(addr6->sin6_addr)) != 1)
> > > > +			error(1, 0, "ipv6 parse error: %s", addr);
> > > > +		break;
> > > 
> > > nit: let's drop explicit af_inet support and use dual-stack af_inet6 sockets
> > > explicitly? Take a look at parse_address in
> > > tools/testing/selftests/drivers/net/hw/ncdevmem.c on how to
> > > transparently fallback to v4 (maybe even move that parsing function into
> > > some new networking_helpers.c lib similar to bpf subtree?).
> > > 
> > > (context: pure v4 environments are unlikely to exist at this point;
> > > anything that supports v6 can use v4-mapped-v6 addresses)
> > > 
> > > > +	default:
> > > > +		error(1, 0, "illegal domain");
> > > > +	}
> > > > +
> > > > +	if (cfg_payload_len > max_payload_len)
> > > > +		error(1, 0, "-l: payload exceeds max (%d)", max_payload_len);
> > > > +}
> > > > +
> ...
> > > [..]
> > > 
> > > > +def _get_rx_ring_entries(cfg):
> > > > +    eth_cmd = "ethtool -g {} | awk '/RX:/ {{count++}} count == 2 {{print $2; exit}}'"
> > > > +    res = cmd(eth_cmd.format(cfg.ifname), host=cfg.remote)
> > > > +    return int(res.stdout)
> > > > +
> > > > +
> > > > +def _get_combined_channels(cfg):
> > > > +    eth_cmd = "ethtool -l {} | awk '/Combined:/ {{count++}} count == 2 {{print $2; exit}}'"
> > > > +    res = cmd(eth_cmd.format(cfg.ifname), host=cfg.remote)
> > > > +    return int(res.stdout)
> > > > +
> > > > +
> > > > +def _set_flow_rule(cfg, chan):
> > > > +    eth_cmd = "ethtool -N {} flow-type tcp6 dst-port 9999 action {} | awk '{{print $NF}}'"
> > > > +    res = cmd(eth_cmd.format(cfg.ifname, chan), host=cfg.remote)
> > > > +    return int(res.stdout)
> > > 
> > > Most of these (except installing flow steering rule) can be done over
> > > ethtool ynl family. Should we try to move them over to YNL calls instead
> > > of shelling out to ethtool binary? There are some examples in rss_ctx.py
> > > on how to work with ethtool spec.
> > > 
> > > Same for setting/resetting number of queues below.
> > 
> > I wanted to use YNL, but these commands are run on the remote host and
> > it's currently challenging to use YNL there.
> > 
> > > 
> > > For the rest, there is also a ethtool() wrapper so you don't have to
> > > do cmd("ethtool ...") and can do ethtool("...").
> > 
> > SG, I will update to use ethtool() helper.
> 
> If there will be no more issues / concerns, I'd assume we can merge
> this series and follow up on top, as it rather sounds like an
> improvement but not a real problem. Stan, does it work for you?

Sure, can be fixed separately.