Re: [PATCH v2 net-next 07/14] selftests: xsk: split xskxceiver main() into setup, run and cleanup

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Concern: this patch moves get_hw_ring_size() into
  detect_ifobj_caps(), which init_iface() runs for both interfaces.
- [Low] In main() (tools/testing/selftests/net/lib/xsk/xskxceiver.c:439),
  the new ksft_print_header() call comes after setup code that can…

--

selftests: xsk: split xskxceiver main() into setup, run and cleanup

This commit splits xskxceiver main() into separate steps for interface
binding, capability detection, test execution and a single cleanup path.
HW ring detection moves into detect_ifobj_caps(), and run_pkt_test() now
returns its result. ksft_print_header() is now called before
ksft_set_plan() so that stdout is line buffered.

> diff --git a/tools/testing/selftests/net/lib/xsk/test_xsk.c b/tools/testing/selftests/net/lib/xsk/test_xsk.c
> index 22a81723d5dea..70a61fb511c8b 100644
> --- a/tools/testing/selftests/net/lib/xsk/test_xsk.c
> +++ b/tools/testing/selftests/net/lib/xsk/test_xsk.c

[ ... ]

> @@ -2332,6 +2335,13 @@ static int detect_ifobj_caps(struct ifobject *ifobj)
>  		}
>  	}
>  
> +	if (!get_hw_ring_size(ifobj->ifname, &ifobj->ring)) {
> +		ifobj_set_cap(ifobj, XSK_CAP_HW_RING);
> +		ifobj->caps.tx_max_pending = ifobj->ring.tx_max_pending;
> +		ifobj->set_ring.default_tx = ifobj->ring.tx_pending;
> +		ifobj->set_ring.default_rx = ifobj->ring.rx_pending;
> +	}
> +
>  	return 0;
>  }

[Severity: Low]
init_iface() runs detect_ifobj_caps() for both interfaces. Does the RX
ifobject now get XSK_CAP_HW_RING as well?

Before this change, main() only called get_hw_ring_size() for ifobj_tx.
Now cleanup_iface(ifobj_rx) also reaches hw_ring_size_reset() at every
exit:

    static void cleanup_iface(struct ifobject *ifobj)
    {
        if (ifobj_has_cap(ifobj, XSK_CAP_HW_RING))
            hw_ring_size_reset(ifobj);
        ...

That sends ETHTOOL_SRINGPARAM to the RX netdev even though the tests never
changed its rings. A driver that does not short-circuit an unchanged
request could reset its queues here.

The commit message doesn't say that ring handling now covers RX. Was this
intended?

The later patch "selftests: xsk: add a hardware mode to xskxceiver" makes
hw_ring_size_reset() return early when the rings are already at their
defaults. That would make this a no-op for RX by the end of the series.

> diff --git a/tools/testing/selftests/net/lib/xsk/xskxceiver.c b/tools/testing/selftests/net/lib/xsk/xskxceiver.c
> index 8b3e4d18e982b..afd6b242aa0df 100644
> --- a/tools/testing/selftests/net/lib/xsk/xskxceiver.c
> +++ b/tools/testing/selftests/net/lib/xsk/xskxceiver.c

[ ... ]

> @@ -373,93 +408,64 @@ int main(int argc, char **argv)
>  
>  	parse_command_line(ifobj_tx, ifobj_rx, argc, argv);
>  

[ ... ]

> +	/* Line-buffer stdout so verdicts reach a capturing launcher live. */
> +	ksft_print_header();
> +	ksft_set_plan(modes * nb_tests);

[Severity: Low]
Should ksft_print_header() be called earlier in main()?

kselftest.h says ksft_print_header() should be the first call. It does
this:

    setvbuf(stdout, NULL, _IOLBF, 0);

    if (!(getenv("KSFT_TAP_LEVEL")))
        printf("TAP version 13\n");

By the time it runs here, main() may already have written to stdout. One
example is when read_procfs_val() fails:

    cache_line_size = read_procfs_val(SMP_CACHE_BYTES_PATH);
    if (!cache_line_size) {
        ksft_print_msg("Can't get SMP_CACHE_BYTES from system, using default (64)\n");
        cache_line_size = 64;
    }

The MAX_SKB_FRAGS fallback is the same. Anything printed from
parse_command_line(), bind_iface() or detect_mode_caps() also comes
before the header.

In those cases the '#' lines appear before the "TAP version 13" line.
setvbuf() is also called after the stream has been used, and ISO C only
defines setvbuf() before any other operation on the stream.

The placement stays the same through the rest of the series. There,
ksft_print_header() runs after setup_peer(), which blocks until the TCP
peer connects, so any earlier output stays buffered until then.

Would it help to move ksft_print_header() to the start of main(),
possibly after the -l handling?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008114909.734364-1-maciej.fijalkowski%40intel.com