| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAL+tcoC606GBfwo3BY_7vn3jPQJdC=78h9q-110hC3DCYRg7jQ@mail.gmail.com>
Date: Wed, 7 May 2025 22:53:31 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, horms@...nel.org, sgoutham@...vell.com,
andrew+netdev@...n.ch, willemb@...gle.com,
linux-arm-kernel@...ts.infradead.org, netdev@...r.kernel.org,
Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next] net: thunder: make tx software timestamp independent
Hi Willem,
On Wed, May 7, 2025 at 9:23 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Jason Xing wrote:
> > From: Jason Xing <kernelxing@...cent.com>
> >
> > skb_tx_timestamp() is used for tx software timestamp enabled by
> > SOF_TIMESTAMPING_TX_SOFTWARE while SKBTX_HW_TSTAMP is controlled by
> > SOF_TIMESTAMPING_TX_HARDWARE. As it clearly shows they are different
> > timestamps in two dimensions, this patch makes the software one
> > standalone.
> >
> > Signed-off-by: Jason Xing <kernelxing@...cent.com>
> > ---
> > drivers/net/ethernet/cavium/thunder/nicvf_queues.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
> > index 06397cc8bb36..d368f381b6de 100644
> > --- a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
> > +++ b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
> > @@ -1389,11 +1389,11 @@ nicvf_sq_add_hdr_subdesc(struct nicvf *nic, struct snd_queue *sq, int qentry,
> > this_cpu_inc(nic->pnicvf->drv_stats->tx_tso);
> > }
> >
> > + skb_tx_timestamp(skb);
> > +
> > /* Check if timestamp is requested */
>
> Nit: check if hw timestamp is requested.
Thanks for the review. Will change it.
>
> > - if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
> > - skb_tx_timestamp(skb);
> > + if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
> > return;
> > - }
>
> The SO_TIMESTAMPING behavior around both software and hardware
> timestamps is a bit odd.
Just a little bit. The reason why I looked into this driver is because
I was reviewing this recent patch[1]. Then I found that the thunder
driver uses the HW flag to test if we can generate a software
timestamp which is also a little bit odd. Software timestamp function
is controlled by the SW flag or SWHW flag instead of the pure HW flag.
[1]: https://lore.kernel.org/all/20250506215508.3611977-1-stfomichev@gmail.com/
>
> Unless SOF_TIMESTAMPING_OPT_TX_SWHW is set, by default a driver will
> only return software if no hardware timestamp is also requested.
Sure thing. SOF_TIMESTAMPING_OPT_TX_SWHW can be used in this case as
well as patch [1].
>
> Through the following in __skb_tstamp_tx
>
> if (!hwtstamps && !(tsflags & SOF_TIMESTAMPING_OPT_TX_SWHW) &&
> skb_shinfo(orig_skb)->tx_flags & SKBTX_IN_PROGRESS)
> return;
>
> There really is no good reason to have this dependency. But it is
> historical and all drivers should implement the same behavior.
As you said, this morning when I was reviewing patch[1], I noticed
that thunder code is not that consistent with others.
>
> This automatically happens if the software timestamp request
> skb_tx_timestamp is called after the hardware timestamp request
> is configured, i.e., after SKBTX_IN_PROGRESS is set. That usually
> happens because the software timestamp is requests as close to kicking
> the doorbell as possible.
Right. In most cases, they implemented in such an order:
if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)
skb_shinfo(skb)->tx_flags |=
SKBTX_IN_PROGRESS;
skb_tx_timestamp(skb);
Should I adjust this patch to have the same behavior in the next
revision like below[2]? Then we can get the conclusion:1) if only the
HW or SW flag is set, nothing changes and only corresponding timestamp
will be generated, 2) if HW and SW are set without the HWSW flag, it
will check the HW first. In non TSO mode, If the non outstanding skb
misses the HW timestamp, then the software timestamp will be
generated, 3) if HW and SW and HWSW are set with the HWSW flag, two
types of timestamp can be generated. To put it in a simpler way, after
[2] patch, thunder driver works like other drivers. Or else, without
[2], the HWSW flag doesn't even work.
[2]
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
index 06397cc8bb36..4be562ead392 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
@@ -1389,28 +1389,24 @@ nicvf_sq_add_hdr_subdesc(struct nicvf *nic,
struct snd_queue *sq, int qentry,
this_cpu_inc(nic->pnicvf->drv_stats->tx_tso);
}
- /* Check if timestamp is requested */
- if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
- skb_tx_timestamp(skb);
- return;
- }
- /* Tx timestamping not supported along with TSO, so ignore request */
- if (skb_shinfo(skb)->gso_size)
- return;
-
- /* HW supports only a single outstanding packet to timestamp */
- if (!atomic_add_unless(&nic->pnicvf->tx_ptp_skbs, 1, 1))
- return;
-
- /* Mark the SKB for later reference */
- skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+ /* Check if hw timestamp is requested */
+ if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
+ /* Tx timestamping not supported along with TSO, so ignore
request */
+ !skb_shinfo(skb)->gso_size &&
+ /* HW supports only a single outstanding packet to timestamp */
+ atomic_add_unless(&nic->pnicvf->tx_ptp_skbs, 1, 1)) {
+ /* Mark the SKB for later reference */
+ skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+
+ /* Finally enable timestamp generation
+ * Since 'post_cqe' is also set, two CQEs will be posted
+ * for this packet i.e CQE_TYPE_SEND and CQE_TYPE_SEND_PTP.
+ */
+ hdr->tstmp = 1;
+ }
- /* Finally enable timestamp generation
- * Since 'post_cqe' is also set, two CQEs will be posted
- * for this packet i.e CQE_TYPE_SEND and CQE_TYPE_SEND_PTP.
- */
- hdr->tstmp = 1;
+ skb_tx_timestamp(skb);
}
/* SQ GATHER subdescriptor
Thanks,
Jason
>
> In this driver, that would be not in nicvf_sq_add_hdr_subdesc, but
> just before calling nicvf_sq_doorbell. Unfortunately, there are two
> callers, TSO and non-TSO.
>
> >
> > /* Tx timestamping not supported along with TSO, so ignore request */
> > if (skb_shinfo(skb)->gso_size)
> > --
> > 2.43.5
> >
>
>
Powered by blists - more mailing lists