Re: [PATCH net-next v9 7/7] selftests: drv-net: extend so_txtime with hw offload

[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] The new `beyond_hw_horizon` variant hard-codes a 70ms txtime,
>   but `_dev_setup_pacing_offload()`…
> 
> --
> 
> selftests: drv-net: extend so_txtime with hw offload
> 
> Adds two hardware pacing offload variants of the so_txtime driver test, one
> on fq with an offload_horizon and one on pfifo_fast, plus a new
> beyond_hw_horizon testcase with a 70ms txtime. Also raises the receive
> timeout and switches the final empty-queue check to MSG_DONTWAIT.
> 
> > diff --git a/tools/testing/selftests/drivers/net/so_txtime.py b/tools/testing/selftests/drivers/net/so_txtime.py
> > index a097fae0b335a..66a87205e02db 100755
> > --- a/tools/testing/selftests/drivers/net/so_txtime.py
> > +++ b/tools/testing/selftests/drivers/net/so_txtime.py
> 
> [ ... ]
> 
> 
> [Severity: Medium]
> Is the beyond_hw_horizon expectation guaranteed on all devices that pass the
> capability gate in _dev_setup_pacing_offload()?
> 
> That helper only checks a lower bound on the device horizon:
> 
>     horizon = _HW_OFFLOAD_HORIZON_MS * 1000_000
>     if link['max-pacing-offload-horizon'] < horizon:
>         raise KsftSkipEx('pacing offload max horizon too small')
> 
> so a device advertising, say, 100ms or more is accepted. In
> test_so_txtime_pfifofast_mono_hw() the root qdisc is pfifo_fast, so there is
> no qdisc offload_horizon gate and the immediate-send decision is left to the
> driver's own horizon check, for example in idpf_tx_splitq_set_txtime():
> 
>     /* beyond offload horizon? set overflow bit only */
>     if (ts > now + horizon) {
>             tx_params->offload.desc_ts[2] =
>                     IDPF_TXD_FLOW_SCH_HORIZON_OVERFLOW_M;
> 
> On a device whose reported max-pacing-offload-horizon covers 70ms, the packet
> is paced in hardware, arrives on time, the receiver exits 0, and with
> expect_fail set by:
> 
>     expect_pass = args_tx != "a,70"
> 
> cmd.process() in tools/testing/selftests/net/lib/py/utils.py turns that into
> a failure:
> 
>     elif self.proc.returncode == 0 and expect_fail:
>         raise CmdExitZeroFailure("Command succeeded (expected fail)", self)
> 
> The timing_sensitive escape only clears expect_fail under
> KSFT_MACHINE_SLOW, so on a normal machine the failure is unconditional.

This is a good point.

With the replacement of configurable limit dev->pacing_offload_horizon
by boolean dev->pacing_offload, the horizon is now always the
max_pacing_offload_horizon.

The test fails, but perhaps for the wrong reason. Will take a look.
I have to take back my "all false positives", though this is fairly
small.