Message-ID: <681b9d2210879_1f6aad294bc@willemb.c.googlers.com.notmuch>
Date: Wed, 07 May 2025 13:49:22 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>, 
 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

Jason Xing wrote:
> 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]?

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
> > >
> >
> >

Powered by blists - more mailing lists