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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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.
> 
> > 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
> 
> [ ... ]
> 
> > @@ -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