Re: [PATCH v5 1/5] net-timestamp: COMPLETION timestamp on packet tx completion

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Luiz Augusto von Dentz wrote:
> Hi Pauli, Willem, Jason,
> 
> On Wed, Mar 19, 2025 at 11:48 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> >
> > Dear Pauli,
> >
> >
> > Thank you for your patch. Two minor comments, should you resend.
> >
> > You could make the summary/title a statement:
> >
> > Add COMPLETION timestamp on packet tx completion
> >
> > Am 18.03.25 um 20:06 schrieb Pauli Virtanen:
> > > Add SOF_TIMESTAMPING_TX_COMPLETION, for requesting a software timestamp
> > > when hardware reports a packet completed.
> > >
> > > Completion tstamp is useful for Bluetooth, as hardware timestamps do not
> > > exist in the HCI specification except for ISO packets, and the hardware
> > > has a queue where packets may wait.  In this case the software SND
> > > timestamp only reflects the kernel-side part of the total latency
> > > (usually small) and queue length (usually 0 unless HW buffers
> > > congested), whereas the completion report time is more informative of
> > > the true latency.
> > >
> > > It may also be useful in other cases where HW TX timestamps cannot be
> > > obtained and user wants to estimate an upper bound to when the TX
> > > probably happened.
> > >
> > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > ---
> > >
> > > Notes:
> > >      v5:
> > >      - back to decoupled COMPLETION & SND, like in v3
> > >      - BPF reporting not implemented here
> > >
> > >   Documentation/networking/timestamping.rst | 8 ++++++++
> > >   include/linux/skbuff.h                    | 7 ++++---
> > >   include/uapi/linux/errqueue.h             | 1 +
> > >   include/uapi/linux/net_tstamp.h           | 6 ++++--
> > >   net/core/skbuff.c                         | 2 ++
> > >   net/ethtool/common.c                      | 1 +
> > >   net/socket.c                              | 3 +++
> > >   7 files changed, 23 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> > > index 61ef9da10e28..b8fef8101176 100644
> > > --- a/Documentation/networking/timestamping.rst
> > > +++ b/Documentation/networking/timestamping.rst
> > > @@ -140,6 +140,14 @@ SOF_TIMESTAMPING_TX_ACK:
> > >     cumulative acknowledgment. The mechanism ignores SACK and FACK.
> > >     This flag can be enabled via both socket options and control messages.
> > >
> > > +SOF_TIMESTAMPING_TX_COMPLETION:
> > > +  Request tx timestamps on packet tx completion.  The completion
> > > +  timestamp is generated by the kernel when it receives packet a
> > > +  completion report from the hardware. Hardware may report multiple
> >
> > … receives packate a completion … sounds strange to me, but I am a
> > non-native speaker.
> >
> > […]
> >
> >
> > Kind regards,
> >
> > Paul
> 
> Is v5 considered good enough to be merged into bluetooth-next and can
> this be send to in this merge window or you think it is best to leave
> for the next? In my opinion it could go in so we use the RC period to
> stabilize it.

I'm fine with merging as is.

Most of my comments were design points for consideration. If Pauli
prefers as is, no objections from me.

For the series:

Reviewed-by: Willem de Bruijn <willemb@google.com>