* [PATCH net-next] net: thunder: make tx software timestamp independent @ 2025-05-07 3:08 Jason Xing 2025-05-07 13:23 ` Willem de Bruijn 0 siblings, 1 reply; 5+ messages in thread From: Jason Xing @ 2025-05-07 3:08 UTC (permalink / raw) To: davem, edumazet, kuba, pabeni, horms, sgoutham, andrew+netdev, willemb Cc: linux-arm-kernel, netdev, Jason Xing From: Jason Xing <kernelxing@tencent.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@tencent.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 */ - if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) { - skb_tx_timestamp(skb); + if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) return; - } /* Tx timestamping not supported along with TSO, so ignore request */ if (skb_shinfo(skb)->gso_size) -- 2.43.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: thunder: make tx software timestamp independent 2025-05-07 3:08 [PATCH net-next] net: thunder: make tx software timestamp independent Jason Xing @ 2025-05-07 13:23 ` Willem de Bruijn 2025-05-07 14:53 ` Jason Xing 0 siblings, 1 reply; 5+ messages in thread From: Willem de Bruijn @ 2025-05-07 13:23 UTC (permalink / raw) To: Jason Xing, davem, edumazet, kuba, pabeni, horms, sgoutham, andrew+netdev, willemb Cc: linux-arm-kernel, netdev, Jason Xing Jason Xing wrote: > From: Jason Xing <kernelxing@tencent.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@tencent.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. > - 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. Unless SOF_TIMESTAMPING_OPT_TX_SWHW is set, by default a driver will only return software if no hardware timestamp is also requested. 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. 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. 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: thunder: make tx software timestamp independent 2025-05-07 13:23 ` Willem de Bruijn @ 2025-05-07 14:53 ` Jason Xing 2025-05-07 17:49 ` Willem de Bruijn 0 siblings, 1 reply; 5+ messages in thread From: Jason Xing @ 2025-05-07 14:53 UTC (permalink / raw) To: Willem de Bruijn Cc: davem, edumazet, kuba, pabeni, horms, sgoutham, andrew+netdev, willemb, linux-arm-kernel, netdev, Jason Xing Hi Willem, On Wed, May 7, 2025 at 9:23 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > Jason Xing wrote: > > From: Jason Xing <kernelxing@tencent.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@tencent.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 > > > > ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: thunder: make tx software timestamp independent 2025-05-07 14:53 ` Jason Xing @ 2025-05-07 17:49 ` Willem de Bruijn 2025-05-07 23:14 ` Jason Xing 0 siblings, 1 reply; 5+ messages in thread From: Willem de Bruijn @ 2025-05-07 17:49 UTC (permalink / raw) To: Jason Xing, Willem de Bruijn Cc: davem, edumazet, kuba, pabeni, horms, sgoutham, andrew+netdev, willemb, linux-arm-kernel, netdev, Jason Xing Jason Xing wrote: > Hi Willem, > > On Wed, May 7, 2025 at 9:23 PM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > Jason Xing wrote: > > > From: Jason Xing <kernelxing@tencent.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@tencent.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]? Best is just to do what other drivers do with the software timestamp: take it as late as possible in ndo_start_xmit, meaning just before ringing the doorbell. For this driver, that is in two places, as said. > 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 > > > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: thunder: make tx software timestamp independent 2025-05-07 17:49 ` Willem de Bruijn @ 2025-05-07 23:14 ` Jason Xing 0 siblings, 0 replies; 5+ messages in thread From: Jason Xing @ 2025-05-07 23:14 UTC (permalink / raw) To: Willem de Bruijn Cc: davem, edumazet, kuba, pabeni, horms, sgoutham, andrew+netdev, willemb, linux-arm-kernel, netdev, Jason Xing On Thu, May 8, 2025 at 1:49 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > Jason Xing wrote: > > Hi Willem, > > > > On Wed, May 7, 2025 at 9:23 PM Willem de Bruijn > > <willemdebruijn.kernel@gmail.com> wrote: > > > > > > Jason Xing wrote: > > > > From: Jason Xing <kernelxing@tencent.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@tencent.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]? > > Best is just to do what other drivers do with the software > timestamp: take it as late as possible in ndo_start_xmit, > meaning just before ringing the doorbell. > > For this driver, that is in two places, as said. Oh, now I see your other core suggestion. Thanks! Probably I will split it into a small series to make each patch only do one thing. Thanks, Jason > > > 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 > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-05-07 23:18 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-05-07 3:08 [PATCH net-next] net: thunder: make tx software timestamp independent Jason Xing 2025-05-07 13:23 ` Willem de Bruijn 2025-05-07 14:53 ` Jason Xing 2025-05-07 17:49 ` Willem de Bruijn 2025-05-07 23:14 ` Jason Xing
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).