Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 2 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] The new ndo-xmit checks in tools/testing/selftests/net/veth.sh
are only skipped when cli.py is missing.
- [Medium] The new `if [ -f "$YNL" ]` block (veth.sh lines 320-331) is
inserted between the 'with gro enabled on link down' setup and the…
--
selftests: net: veth: test peer ndo-xmit after GRO toggle while down
This adds chk_xdp_feature() to veth.sh. It reads xdp-features through the
ynl CLI and checks that toggling GRO on a veth device while it is down
updates the peer's ndo-xmit XDP feature once the device comes up. The new
checks are skipped when cli.py is not found.
> diff --git a/tools/testing/selftests/net/veth.sh
> b/tools/testing/selftests/net/veth.sh
> index 9709dd067c728..8b867e0675a7c 100755
> --- a/tools/testing/selftests/net/veth.sh
> +++ b/tools/testing/selftests/net/veth.sh
[ ... ]
> @@ -74,6 +76,37 @@ chk_tso_flag() {
> __chk_flag "$1" $2 $3 tcp-segmentation-offload
> }
>
> +chk_xdp_feature() {
[ ... ]
> + ifindex=`ip netns exec $BASE$target cat
> /sys/class/net/veth$target/ifindex`
> + out=`ip netns exec $BASE$target "$YNL" --family netdev --do dev-get \
> + --output-json --json "{\"ifindex\": $ifindex}" 2>&1`
> + st=$?
> +
> + printf "%-60s" "$msg"
> + if [ $st -ne 0 ]; then
> + echo " fail - ynl cli error: $out"
> + ret=1
> + return
> + fi
[Severity: Medium]
If cli.py is present but can't run, should that be a skip instead of a
failure?
The only checks on the environment just test whether the file exists:
[ ! -f "$YNL" ] && echo "ynl cli not found, ndo-xmit tests will be skipped"
...
if [ -f "$YNL" ]; then
The CLI's library always imports PyYAML, in
tools/net/ynl/pyynl/lib/nlspec.py:
import yaml as pyyaml
The CLI also has to find the netdev spec through specdir.find_spec(). That
raises FileNotFoundError if the spec directory is missing.
Suppose veth.sh runs from a kernel source tree on a host that has no
python3-yaml, no python3, or no spec directory. Each chk_xdp_feature() call
then exits non-zero, prints "fail - ynl cli error" and sets ret=1. Before
this patch, veth.sh needed only iproute2 and ethtool and passed on such a
host.
The commit message says the checks are skipped "if the CLI is not found".
The code does that. Could the script also do a trial run of the CLI before
it enables the ndo-xmit checks, and skip them if that run fails?
[ ... ]
> @@ -283,6 +317,18 @@ chk_gro_flag " - peer gro flag" $SRC off
> chk_tso_flag " - tso flag" $SRC on
> chk_tso_flag " - peer tso flag" $DST on
> ip -n $NS_DST link set dev veth$DST up
> +if [ -f "$YNL" ]; then
> + chk_xdp_feature " - peer ndo-xmit" $SRC 1 ndo-xmit
> + # make sure the peer flag is set before disabling gro while down
> + ip netns exec $NS_DST ethtool -K veth$DST gro off
> + ip netns exec $NS_DST ethtool -K veth$DST gro on
> + chk_xdp_feature " - peer ndo-xmit re-armed" $SRC 1 ndo-xmit
> + ip -n $NS_DST link set dev veth$DST down
> + ip netns exec $NS_DST ethtool -K veth$DST gro off
> + ip -n $NS_DST link set dev veth$DST up
> + chk_xdp_feature " - peer ndo-xmit cleared" $SRC 0 ndo-xmit
> + ip netns exec $NS_DST ethtool -K veth$DST gro on
> +fi
> ip netns exec $NS_SRC ethtool -K veth$SRC tx-udp-segmentation off
> ip netns exec $NS_DST ethtool -K veth$DST rx-udp-gro-forwarding on
> chk_gro " - aggregation with TSO off" 1
[Severity: Medium]
Does this block change which code path the chk_gro "aggregation with TSO
off" check below it tests?
Before this patch, the "with gro enabled on link down" section ran link
down, gro on, link up, and then chk_gro. veth_set_features() returns early
when the device is down:
drivers/net/veth.c:veth_set_features() {
...
if (!(changed & NETIF_F_GRO) || !(dev->flags & IFF_UP) ||
priv->_xdp_prog)
return 0;
...
}
So GRO NAPI was enabled only by veth_open():
drivers/net/veth.c:veth_open() {
...
} else if (veth_gro_requested(dev)) {
err = veth_napi_enable(dev);
...
}
When cli.py exists, the new block ends with link down, gro off, link up.
veth_open() enables no NAPI there. The block then runs ethtool -K veth$DST
gro on while the device is up, so NAPI is enabled in veth_set_features()
instead:
drivers/net/veth.c:veth_set_features() {
...
if (features & NETIF_F_GRO) {
err = veth_napi_enable(dev);
...
}
The final chk_gro now tests the "GRO enabled while up" path, which the
earlier "with gro on" section already covers. The GRO NAPI path in
veth_open() is no longer covered by any aggregation check. Which path is
tested also depends on whether cli.py exists.
The ndo-xmit checks don't fill this gap. veth_set_xdp_features() looks at
veth_gro_requested(), not at whether NAPI actually started.
Could the block end by restoring the down-state setup (link down, gro on,
link up) instead of turning GRO on while the device is up?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006173241.65945-1-tianyi%40cloudflare.com
