[PATCH net-next] net: thunder: make tx software timestamp independent

[Jason Xing]

On Thu, May 8, 2025 at 1:49 AM Willem de Bruijn
<willemdebruijn.kernel at gmail.com> wrote:
>
> Jason Xing wrote:
> > Hi Willem,
> >
> > On Wed, May 7, 2025 at 9:23 PM Willem de Bruijn
> > <willemdebruijn.kernel at gmail.com> wrote:
> > >
> > > Jason Xing wrote:
> > > > From: Jason Xing <kernelxing at 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 at 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
> > > >
> > >
> > >
>
>