Hi, On Fri, Mar 21, 2025 at 12:12 AM Pauli Virtanen <pav@xxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxx> > > > > --- > > > > > > > > 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@xxxxxxxxx> 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 >
