Re: [PATCH net-next v9 6/7] selftests: drv-net: in so_txtime tell apart sw from hw pacing

netdev-bot+sashiko@ wrote:

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] `-H` is documented in usage() as 'verify hardware offload
  (tx)' and the commit message claims it tells software apart from…

--

selftests: drv-net: in so_txtime tell apart sw from hw pacing

This adds a '-H' flag to so_txtime.c that enables SO_TIMESTAMPING with
SOF_TIMESTAMPING_TX_SOFTWARE, and flags an error if the software transmit
timestamp is more than cfg_variance_us after the send start time.  It also
extends the errqueue polling deadline by cfg_variance_us and sorts the
getopt string and usage text alphabetically.

quoted

diff --git a/tools/testing/selftests/drivers/net/so_txtime.c b/tools/testing/selftests/drivers/net/so_txtime.c
index f6f3253ed1748..9ebe9f91227c7 100644
--- a/tools/testing/selftests/drivers/net/so_txtime.c
+++ b/tools/testing/selftests/drivers/net/so_txtime.c

[ ... ]

quoted

@@ -191,12 +193,32 @@ static int do_recv_errqueue_txtime(struct sock_extended_err *err,
 	return 1;
 }
 
-static int do_recv_errqueue(int fdt)
+static int do_recv_errqueue_timestamping(struct scm_timestamping *tss)
+{
+	int64_t ts;
+
+	ts = tss->ts[0].tv_sec * 1000ULL * 1000 * 1000;
+	ts += tss->ts[0].tv_nsec;
+	ts -= glob_tstart_real;
+	ts /= 1000;
+
+	if (ts > cfg_variance_us) {
+		fprintf(stderr, "sw delay %" PRId64 "us exceeds bounds\n", ts);
+		if (!cfg_machine_slow)
+			errors++;
+	}
+
+	return 1;
+}

[Severity: Medium]

Can this check actually tell software pacing apart from hardware pacing for
every workload the next patch runs with '-H'?

Key here is "for every workload".

Yes, passing -H is superfluous for the non-paced tests.

No, that is not worth complicating the test to add a branch.
 

The only positive criterion here is that the software transmit timestamp
lands within cfg_variance_us (8000 us) of glob_tstart_real.  For a request
with no delay or a negative delay there is no pacing interval to observe at
all.  do_send_one() does not even attach SCM_TXTIME when delay_us is
negative:

	if (ts->delay_us >= 0) {
		...
		tdeliver = glob_tstart + ts->delay_us * 1000;
		...
		cm->cmsg_type = SCM_TXTIME;

So for the 'a,-1' and 'a,0' variants that the follow-up patch passes '-H'
to, ts comes out near 0 and the ts > cfg_variance_us test can never fire,
whether the transmit was hardware paced, software paced, or not paced at
all.  Would it make sense to skip or reject '-H' when the requested delay
is not positive, so those variants do not report a pass without having
verified anything?

For the 10 ms variants there are only 2 ms between the requested delay and