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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAL+tcoAb4z81aBWukkMi9RU=3=6x7wrd0TGJOyjbOVqpGhVnSQ@mail.gmail.com>
Date: Fri, 21 Mar 2025 00:51:37 +0700
From: Jason Xing <kerneljasonxing@...il.com>
To: Pauli Virtanen <pav@....fi>
Cc: Luiz Augusto von Dentz <luiz.dentz@...il.com>, Paul Menzel <pmenzel@...gen.mpg.de>, 
	linux-bluetooth@...r.kernel.org, netdev@...r.kernel.org, davem@...emloft.net, 
	kuba@...nel.org, willemdebruijn.kernel@...il.com
Subject: Re: [PATCH v5 1/5] net-timestamp: COMPLETION timestamp on packet tx completion

Hi,

On Fri, Mar 21, 2025 at 12:12 AM Pauli Virtanen <pav@....fi> wrote:
>
> Hi,
>
> to, 2025-03-20 kello 10:43 -0400, Luiz Augusto von Dentz kirjoitti:
> > Hi Pauli, Willem, Jason,
> >
> > On Wed, Mar 19, 2025 at 11:48 AM Paul Menzel <pmenzel@...gen.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@....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.
>
> From my side v5 should be good enough, if we want it now.

Sorry for seeing this too late, I think I miss adding the reviewed-by
tags. Anyway,

Reviewed-by: Jason Xing <kerneljasonxing@...il.com>

Thanks for working on this!!

>
> The remaining things were:
>
> - Typo in documentation
>
> - Better tx_queue implementation: probably not highly important for
>   these use cases, as queues are likely just a few packets so there
>   will be only that amount of non-timestamped skbs cloned.
>
>   In future, we might consider emitting SCM_TSTAMP_ACK timestamps
>   eg. for L2CAP LE credit-based flow control, and there this would
>   matter more as you'd need to hang on to the packets for a longer
>   time.
>
> - I'd leave handling of unsupported sockcm fields as it is in
>   v5, as BT sockets have also previously just silently ignored
>   them so no change in behavior here.
>
> - BPF: separate patch series, also need the tests for that

Right, I agree :)

Thanks,
Jason

>
> --
> Pauli Virtanen
>