| Message ID | 71b88f509237bcce4139c152b3f624d7532047fd.1739097311.git.pav@iki.fi (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/SubjectPrefix | fail | "Bluetooth: " prefix is not specified in the subject |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | warning | CheckSparse WARNING net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):net/bluetooth/sco.c: note: in included file:./include/net/bluetooth/hci_core.h:147:35: warning: array of flexible structures |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
| tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
| tedd_an/TestRunner_iso-tester | success | TestRunner PASS |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mgmt-tester | success | TestRunner PASS |
| tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
| tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
| tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mesh-tester | success | TestRunner PASS |
| tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
| tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=931989 ---Test result--- Test Summary: CheckPatch PENDING 0.33 seconds GitLint PENDING 0.26 seconds SubjectPrefix FAIL 0.57 seconds BuildKernel PASS 23.88 seconds CheckAllWarning PASS 26.68 seconds CheckSparse WARNING 30.05 seconds BuildKernel32 PASS 24.15 seconds TestRunnerSetup PASS 423.29 seconds TestRunner_l2cap-tester PASS 20.23 seconds TestRunner_iso-tester PASS 27.56 seconds TestRunner_bnep-tester PASS 4.76 seconds TestRunner_mgmt-tester PASS 119.84 seconds TestRunner_rfcomm-tester PASS 7.48 seconds TestRunner_sco-tester PASS 9.31 seconds TestRunner_ioctl-tester PASS 8.91 seconds TestRunner_mesh-tester PASS 5.96 seconds TestRunner_smp-tester PASS 6.86 seconds TestRunner_userchan-tester PASS 4.97 seconds IncrementalBuild PENDING 0.95 seconds Details ############################## Test: CheckPatch - PENDING Desc: Run checkpatch.pl script Output: ############################## Test: GitLint - PENDING Desc: Run gitlint Output: ############################## Test: SubjectPrefix - FAIL Desc: Check subject contains "Bluetooth" prefix Output: "Bluetooth: " prefix is not specified in the subject ############################## Test: CheckSparse - WARNING Desc: Run sparse tool with linux kernel Output: net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):net/bluetooth/sco.c: note: in included file:./include/net/bluetooth/hci_core.h:147:35: warning: array of flexible structures ############################## Test: IncrementalBuild - PENDING Desc: Incremental build with the patches in the series Output: --- Regards, Linux Bluetooth
Pauli Virtanen wrote: > Add SOF_TIMESTAMPING_TX_COMPLETION, for requesting a software timestamp > when hardware reports a packet completed. > > Completion tstamp is useful for Bluetooth, where hardware tx timestamps > cannot be obtained 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. Getting the completion timestamp may indeed be useful more broadly. Alternatively, the HW timestamp is relatively imprecisely defined so you could even just use that. Ideally, a hw timestamp conforms to IEEE 1588v2 PHY: first symbol on the wire IIRC. But in many cases this is not the case. It is not feasible at line rate, or the timestamp is only taken when the completion is written over PCI, which may be subject to PCI backpressure and happen after transmission on the wire. As a result, the worst case hw tstamp must already be assumed not much earlier than a completion timestamp. That said, +1 on adding explicit well defined measurement point instead. > Signed-off-by: Pauli Virtanen <pav@iki.fi> > --- > Documentation/networking/timestamping.rst | 9 +++++++++ > include/linux/skbuff.h | 6 +++++- > include/uapi/linux/errqueue.h | 1 + > include/uapi/linux/net_tstamp.h | 6 ++++-- > net/ethtool/common.c | 1 + > net/socket.c | 3 +++ > 6 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst > index 61ef9da10e28..de2afed7a516 100644 > --- a/Documentation/networking/timestamping.rst > +++ b/Documentation/networking/timestamping.rst > @@ -140,6 +140,15 @@ 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 > + packets at once, and completion timestamps reflect the timing of the > + report and not actual tx time. The completion timestamps are > + currently implemented only for: Bluetooth L2CAP and ISO. This > + flag can be enabled via both socket options and control messages. > + Either we should support this uniformly, or it should be possible to query whether a driver supports this. Unfortunately all completion callbacks are driver specific. But drivers that support hwtstamps will call skb_tstamp_tx with nonzero hwtstamps. We could use that also to compute and queue a completion timestamp if requested. At least for existing NIC drivers. > 1.3.2 Timestamp Reporting > ^^^^^^^^^^^^^^^^^^^^^^^^^ > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index bb2b751d274a..3707c9075ae9 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -489,10 +489,14 @@ enum { > > /* generate software time stamp when entering packet scheduling */ > SKBTX_SCHED_TSTAMP = 1 << 6, > + > + /* generate software time stamp on packet tx completion */ > + SKBTX_COMPLETION_TSTAMP = 1 << 7, > }; > > #define SKBTX_ANY_SW_TSTAMP (SKBTX_SW_TSTAMP | \ > - SKBTX_SCHED_TSTAMP) > + SKBTX_SCHED_TSTAMP | \ > + SKBTX_COMPLETION_TSTAMP) These fields are used in the skb_shared_info tx_flags field. Which is a very scarce resource. This takes the last available bit. That is my only possible concern: the opportunity cost. > #define SKBTX_ANY_TSTAMP (SKBTX_HW_TSTAMP | \ > SKBTX_HW_TSTAMP_USE_CYCLES | \ > SKBTX_ANY_SW_TSTAMP) > diff --git a/include/uapi/linux/errqueue.h b/include/uapi/linux/errqueue.h > index 3c70e8ac14b8..1ea47309d772 100644 > --- a/include/uapi/linux/errqueue.h > +++ b/include/uapi/linux/errqueue.h > @@ -73,6 +73,7 @@ enum { > SCM_TSTAMP_SND, /* driver passed skb to NIC, or HW */ > SCM_TSTAMP_SCHED, /* data entered the packet scheduler */ > SCM_TSTAMP_ACK, /* data acknowledged by peer */ > + SCM_TSTAMP_COMPLETION, /* packet tx completion */ > }; > > #endif /* _UAPI_LINUX_ERRQUEUE_H */ > diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h > index 55b0ab51096c..383213de612a 100644 > --- a/include/uapi/linux/net_tstamp.h > +++ b/include/uapi/linux/net_tstamp.h > @@ -44,8 +44,9 @@ enum { > SOF_TIMESTAMPING_BIND_PHC = (1 << 15), > SOF_TIMESTAMPING_OPT_ID_TCP = (1 << 16), > SOF_TIMESTAMPING_OPT_RX_FILTER = (1 << 17), > + SOF_TIMESTAMPING_TX_COMPLETION = (1 << 18), > > - SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_RX_FILTER, > + SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_TX_COMPLETION, > SOF_TIMESTAMPING_MASK = (SOF_TIMESTAMPING_LAST - 1) | > SOF_TIMESTAMPING_LAST > }; > @@ -58,7 +59,8 @@ enum { > #define SOF_TIMESTAMPING_TX_RECORD_MASK (SOF_TIMESTAMPING_TX_HARDWARE | \ > SOF_TIMESTAMPING_TX_SOFTWARE | \ > SOF_TIMESTAMPING_TX_SCHED | \ > - SOF_TIMESTAMPING_TX_ACK) > + SOF_TIMESTAMPING_TX_ACK | \ > + SOF_TIMESTAMPING_TX_COMPLETION) > > /** > * struct so_timestamping - SO_TIMESTAMPING parameter > diff --git a/net/ethtool/common.c b/net/ethtool/common.c > index 2bd77c94f9f1..75e3b756012e 100644 > --- a/net/ethtool/common.c > +++ b/net/ethtool/common.c > @@ -431,6 +431,7 @@ const char sof_timestamping_names[][ETH_GSTRING_LEN] = { > [const_ilog2(SOF_TIMESTAMPING_BIND_PHC)] = "bind-phc", > [const_ilog2(SOF_TIMESTAMPING_OPT_ID_TCP)] = "option-id-tcp", > [const_ilog2(SOF_TIMESTAMPING_OPT_RX_FILTER)] = "option-rx-filter", > + [const_ilog2(SOF_TIMESTAMPING_TX_COMPLETION)] = "completion-transmit", just "tx-completion"?
Hi Pauli, On Sun, Feb 9, 2025 at 6:40 PM Pauli Virtanen <pav@iki.fi> wrote: > > Add SOF_TIMESTAMPING_TX_COMPLETION, for requesting a software timestamp > when hardware reports a packet completed. > > Completion tstamp is useful for Bluetooth, where hardware tx timestamps > cannot be obtained except for ISO packets, and the hardware has a queue Could you say more about why the hw timestamp cannot be obtained? > 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. It's worth mentioning the earlier discussion that took place last year in the commit log. It's hard to retrieve such an important discussion buried in the mailing list, which should be helpful for people to get to know/recall the exact background :) https://lore.kernel.org/all/6642c7f3427b5_20539c2949a@willemb.c.googlers.com.notmuch/. https://lore.kernel.org/all/cover.1710440392.git.pav@iki.fi/ And it's also good to attach the real use case from the following link for readers to know the exact case :) https://lore.kernel.org/all/7ade362f178297751e8a0846e0342d5086623edc.camel@iki.fi/ Quoting here: " sendmsg() from user generates skbs to net/bluetooth side queue | * wait in net/bluetooth side queue until HW has free packet slot | * send to driver (-> SCM_TSTAMP_SCHED*) | * driver (usu. ASAP) queues to transport e.g. USB | * transport tx complete, skb freed | * packet waits in hardware-side buffers (usu. the largest delay) | * packet completion report from HW (-> SCM_TSTAMP_SND*) | * for one packet type, HW timestamp for last tx packet can queried The packet completion report does not imply the packet was received. " > Signed-off-by: Pauli Virtanen <pav@iki.fi> > --- > Documentation/networking/timestamping.rst | 9 +++++++++ > include/linux/skbuff.h | 6 +++++- > include/uapi/linux/errqueue.h | 1 + > include/uapi/linux/net_tstamp.h | 6 ++++-- > net/ethtool/common.c | 1 + > net/socket.c | 3 +++ > 6 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst > index 61ef9da10e28..de2afed7a516 100644 > --- a/Documentation/networking/timestamping.rst > +++ b/Documentation/networking/timestamping.rst > @@ -140,6 +140,15 @@ 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 > + packets at once, and completion timestamps reflect the timing of the > + report and not actual tx time. The completion timestamps are > + currently implemented only for: Bluetooth L2CAP and ISO. This > + flag can be enabled via both socket options and control messages. > + I'd like to know if this flag can also be applied to NICs which have already implemented the hardware timestamp, like Intel i40e, no? Thanks, Jason > > 1.3.2 Timestamp Reporting > ^^^^^^^^^^^^^^^^^^^^^^^^^ > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index bb2b751d274a..3707c9075ae9 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -489,10 +489,14 @@ enum { > > /* generate software time stamp when entering packet scheduling */ > SKBTX_SCHED_TSTAMP = 1 << 6, > + > + /* generate software time stamp on packet tx completion */ > + SKBTX_COMPLETION_TSTAMP = 1 << 7, > }; > > #define SKBTX_ANY_SW_TSTAMP (SKBTX_SW_TSTAMP | \ > - SKBTX_SCHED_TSTAMP) > + SKBTX_SCHED_TSTAMP | \ > + SKBTX_COMPLETION_TSTAMP) > #define SKBTX_ANY_TSTAMP (SKBTX_HW_TSTAMP | \ > SKBTX_HW_TSTAMP_USE_CYCLES | \ > SKBTX_ANY_SW_TSTAMP) > diff --git a/include/uapi/linux/errqueue.h b/include/uapi/linux/errqueue.h > index 3c70e8ac14b8..1ea47309d772 100644 > --- a/include/uapi/linux/errqueue.h > +++ b/include/uapi/linux/errqueue.h > @@ -73,6 +73,7 @@ enum { > SCM_TSTAMP_SND, /* driver passed skb to NIC, or HW */ > SCM_TSTAMP_SCHED, /* data entered the packet scheduler */ > SCM_TSTAMP_ACK, /* data acknowledged by peer */ > + SCM_TSTAMP_COMPLETION, /* packet tx completion */ > }; > > #endif /* _UAPI_LINUX_ERRQUEUE_H */ > diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h > index 55b0ab51096c..383213de612a 100644 > --- a/include/uapi/linux/net_tstamp.h > +++ b/include/uapi/linux/net_tstamp.h > @@ -44,8 +44,9 @@ enum { > SOF_TIMESTAMPING_BIND_PHC = (1 << 15), > SOF_TIMESTAMPING_OPT_ID_TCP = (1 << 16), > SOF_TIMESTAMPING_OPT_RX_FILTER = (1 << 17), > + SOF_TIMESTAMPING_TX_COMPLETION = (1 << 18), > > - SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_RX_FILTER, > + SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_TX_COMPLETION, > SOF_TIMESTAMPING_MASK = (SOF_TIMESTAMPING_LAST - 1) | > SOF_TIMESTAMPING_LAST > }; > @@ -58,7 +59,8 @@ enum { > #define SOF_TIMESTAMPING_TX_RECORD_MASK (SOF_TIMESTAMPING_TX_HARDWARE | \ > SOF_TIMESTAMPING_TX_SOFTWARE | \ > SOF_TIMESTAMPING_TX_SCHED | \ > - SOF_TIMESTAMPING_TX_ACK) > + SOF_TIMESTAMPING_TX_ACK | \ > + SOF_TIMESTAMPING_TX_COMPLETION) > > /** > * struct so_timestamping - SO_TIMESTAMPING parameter > diff --git a/net/ethtool/common.c b/net/ethtool/common.c > index 2bd77c94f9f1..75e3b756012e 100644 > --- a/net/ethtool/common.c > +++ b/net/ethtool/common.c > @@ -431,6 +431,7 @@ const char sof_timestamping_names[][ETH_GSTRING_LEN] = { > [const_ilog2(SOF_TIMESTAMPING_BIND_PHC)] = "bind-phc", > [const_ilog2(SOF_TIMESTAMPING_OPT_ID_TCP)] = "option-id-tcp", > [const_ilog2(SOF_TIMESTAMPING_OPT_RX_FILTER)] = "option-rx-filter", > + [const_ilog2(SOF_TIMESTAMPING_TX_COMPLETION)] = "completion-transmit", > }; > static_assert(ARRAY_SIZE(sof_timestamping_names) == __SOF_TIMESTAMPING_CNT); > > diff --git a/net/socket.c b/net/socket.c > index 4afe31656a2b..22b7f6f50889 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -693,6 +693,9 @@ void __sock_tx_timestamp(__u32 tsflags, __u8 *tx_flags) > if (tsflags & SOF_TIMESTAMPING_TX_SCHED) > flags |= SKBTX_SCHED_TSTAMP; > > + if (tsflags & SOF_TIMESTAMPING_TX_COMPLETION) > + flags |= SKBTX_COMPLETION_TSTAMP; > + > *tx_flags = flags; > } > EXPORT_SYMBOL(__sock_tx_timestamp); > -- > 2.48.1 > >
Hi, su, 2025-02-09 kello 22:29 -0500, Willem de Bruijn kirjoitti: > Pauli Virtanen wrote: > > Add SOF_TIMESTAMPING_TX_COMPLETION, for requesting a software timestamp > > when hardware reports a packet completed. > > > > Completion tstamp is useful for Bluetooth, where hardware tx timestamps > > cannot be obtained 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. > > Getting the completion timestamp may indeed be useful more broadly. > > Alternatively, the HW timestamp is relatively imprecisely defined so > you could even just use that. Ideally, a hw timestamp conforms to IEEE > 1588v2 PHY: first symbol on the wire IIRC. But in many cases this is > not the case. It is not feasible at line rate, or the timestamp is > only taken when the completion is written over PCI, which may be > subject to PCI backpressure and happen after transmission on the wire. > As a result, the worst case hw tstamp must already be assumed not much > earlier than a completion timestamp. For BT ISO packets, in theory hw-provided TX timestamps exist, and we might want both (with separate flags for enabling them). I don't really know, last I looked Intel HW didn't support them, and it's not clear to which degree they are useful. > That said, +1 on adding explicit well defined measurement point > instead. > > > Signed-off-by: Pauli Virtanen <pav@iki.fi> > > --- > > Documentation/networking/timestamping.rst | 9 +++++++++ > > include/linux/skbuff.h | 6 +++++- > > include/uapi/linux/errqueue.h | 1 + > > include/uapi/linux/net_tstamp.h | 6 ++++-- > > net/ethtool/common.c | 1 + > > net/socket.c | 3 +++ > > 6 files changed, 23 insertions(+), 3 deletions(-) > > > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst > > index 61ef9da10e28..de2afed7a516 100644 > > --- a/Documentation/networking/timestamping.rst > > +++ b/Documentation/networking/timestamping.rst > > @@ -140,6 +140,15 @@ 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 > > + packets at once, and completion timestamps reflect the timing of the > > + report and not actual tx time. The completion timestamps are > > + currently implemented only for: Bluetooth L2CAP and ISO. This > > + flag can be enabled via both socket options and control messages. > > + > > Either we should support this uniformly, or it should be possible to > query whether a driver supports this. > > Unfortunately all completion callbacks are driver specific. > > But drivers that support hwtstamps will call skb_tstamp_tx with > nonzero hwtstamps. We could use that also to compute and queue > a completion timestamp if requested. At least for existing NIC > drivers. Ok. If possible, I'd like to avoid changing the behavior of the non- Bluetooth parts of net/ here, as I'm not familiar with those. I guess a simpler solution could be that sock_set_timestamping() checks the type of the socket, and gives EINVAL if the flag is set for non- Bluetooth sockets? One could then postpone having to invent how to check the driver support, and user would know non-supported status from setsockopt failing. > > 1.3.2 Timestamp Reporting > > ^^^^^^^^^^^^^^^^^^^^^^^^^ > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > index bb2b751d274a..3707c9075ae9 100644 > > --- a/include/linux/skbuff.h > > +++ b/include/linux/skbuff.h > > @@ -489,10 +489,14 @@ enum { > > > > /* generate software time stamp when entering packet scheduling */ > > SKBTX_SCHED_TSTAMP = 1 << 6, > > + > > + /* generate software time stamp on packet tx completion */ > > + SKBTX_COMPLETION_TSTAMP = 1 << 7, > > }; > > > > #define SKBTX_ANY_SW_TSTAMP (SKBTX_SW_TSTAMP | \ > > - SKBTX_SCHED_TSTAMP) > > + SKBTX_SCHED_TSTAMP | \ > > + SKBTX_COMPLETION_TSTAMP) > > These fields are used in the skb_shared_info tx_flags field. > Which is a very scarce resource. This takes the last available bit. > That is my only possible concern: the opportunity cost. If doing it per-protocol sounds ok, it could be put in bt_skb_cb instead. Since the completion timestamp didn't already exist, it maybe means it's probably not that important for other parts of net/ > > #define SKBTX_ANY_TSTAMP (SKBTX_HW_TSTAMP | \ > > SKBTX_HW_TSTAMP_USE_CYCLES | \ > > SKBTX_ANY_SW_TSTAMP) > > diff --git a/include/uapi/linux/errqueue.h b/include/uapi/linux/errqueue.h > > index 3c70e8ac14b8..1ea47309d772 100644 > > --- a/include/uapi/linux/errqueue.h > > +++ b/include/uapi/linux/errqueue.h > > @@ -73,6 +73,7 @@ enum { > > SCM_TSTAMP_SND, /* driver passed skb to NIC, or HW */ > > SCM_TSTAMP_SCHED, /* data entered the packet scheduler */ > > SCM_TSTAMP_ACK, /* data acknowledged by peer */ > > + SCM_TSTAMP_COMPLETION, /* packet tx completion */ > > }; > > > > #endif /* _UAPI_LINUX_ERRQUEUE_H */ > > diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h > > index 55b0ab51096c..383213de612a 100644 > > --- a/include/uapi/linux/net_tstamp.h > > +++ b/include/uapi/linux/net_tstamp.h > > @@ -44,8 +44,9 @@ enum { > > SOF_TIMESTAMPING_BIND_PHC = (1 << 15), > > SOF_TIMESTAMPING_OPT_ID_TCP = (1 << 16), > > SOF_TIMESTAMPING_OPT_RX_FILTER = (1 << 17), > > + SOF_TIMESTAMPING_TX_COMPLETION = (1 << 18), > > > > - SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_RX_FILTER, > > + SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_TX_COMPLETION, > > SOF_TIMESTAMPING_MASK = (SOF_TIMESTAMPING_LAST - 1) | > > SOF_TIMESTAMPING_LAST > > }; > > @@ -58,7 +59,8 @@ enum { > > #define SOF_TIMESTAMPING_TX_RECORD_MASK (SOF_TIMESTAMPING_TX_HARDWARE | \ > > SOF_TIMESTAMPING_TX_SOFTWARE | \ > > SOF_TIMESTAMPING_TX_SCHED | \ > > - SOF_TIMESTAMPING_TX_ACK) > > + SOF_TIMESTAMPING_TX_ACK | \ > > + SOF_TIMESTAMPING_TX_COMPLETION) > > > > /** > > * struct so_timestamping - SO_TIMESTAMPING parameter > > diff --git a/net/ethtool/common.c b/net/ethtool/common.c > > index 2bd77c94f9f1..75e3b756012e 100644 > > --- a/net/ethtool/common.c > > +++ b/net/ethtool/common.c > > @@ -431,6 +431,7 @@ const char sof_timestamping_names[][ETH_GSTRING_LEN] = { > > [const_ilog2(SOF_TIMESTAMPING_BIND_PHC)] = "bind-phc", > > [const_ilog2(SOF_TIMESTAMPING_OPT_ID_TCP)] = "option-id-tcp", > > [const_ilog2(SOF_TIMESTAMPING_OPT_RX_FILTER)] = "option-rx-filter", > > + [const_ilog2(SOF_TIMESTAMPING_TX_COMPLETION)] = "completion-transmit", > > just "tx-completion"? Ok.
Pauli Virtanen wrote: > Hi, > > su, 2025-02-09 kello 22:29 -0500, Willem de Bruijn kirjoitti: > > Pauli Virtanen wrote: > > > Add SOF_TIMESTAMPING_TX_COMPLETION, for requesting a software timestamp > > > when hardware reports a packet completed. > > > > > > Completion tstamp is useful for Bluetooth, where hardware tx timestamps > > > cannot be obtained 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. > > > > Getting the completion timestamp may indeed be useful more broadly. > > > > Alternatively, the HW timestamp is relatively imprecisely defined so > > you could even just use that. Ideally, a hw timestamp conforms to IEEE > > 1588v2 PHY: first symbol on the wire IIRC. But in many cases this is > > not the case. It is not feasible at line rate, or the timestamp is > > only taken when the completion is written over PCI, which may be > > subject to PCI backpressure and happen after transmission on the wire. > > As a result, the worst case hw tstamp must already be assumed not much > > earlier than a completion timestamp. > > For BT ISO packets, in theory hw-provided TX timestamps exist, and we > might want both (with separate flags for enabling them). I don't really > know, last I looked Intel HW didn't support them, and it's not clear to > which degree they are useful. That's reason enough to separate these measurement types. If we don't do it properly now, we won't be able to update drivers later once users depend on requesting hw timestamps when they mean to get completion timestamps. > > That said, +1 on adding explicit well defined measurement point > > instead. > > > > > Signed-off-by: Pauli Virtanen <pav@iki.fi> > > > --- > > > Documentation/networking/timestamping.rst | 9 +++++++++ > > > include/linux/skbuff.h | 6 +++++- > > > include/uapi/linux/errqueue.h | 1 + > > > include/uapi/linux/net_tstamp.h | 6 ++++-- > > > net/ethtool/common.c | 1 + > > > net/socket.c | 3 +++ > > > 6 files changed, 23 insertions(+), 3 deletions(-) > > > > > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst > > > index 61ef9da10e28..de2afed7a516 100644 > > > --- a/Documentation/networking/timestamping.rst > > > +++ b/Documentation/networking/timestamping.rst > > > @@ -140,6 +140,15 @@ 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 > > > + packets at once, and completion timestamps reflect the timing of the > > > + report and not actual tx time. The completion timestamps are > > > + currently implemented only for: Bluetooth L2CAP and ISO. This > > > + flag can be enabled via both socket options and control messages. > > > + > > > > Either we should support this uniformly, or it should be possible to > > query whether a driver supports this. > > > > Unfortunately all completion callbacks are driver specific. > > > > But drivers that support hwtstamps will call skb_tstamp_tx with > > nonzero hwtstamps. We could use that also to compute and queue > > a completion timestamp if requested. At least for existing NIC > > drivers. > > Ok. If possible, I'd like to avoid changing the behavior of the non- > Bluetooth parts of net/ here, as I'm not familiar with those. > > I guess a simpler solution could be that sock_set_timestamping() checks > the type of the socket, and gives EINVAL if the flag is set for non- > Bluetooth sockets? Actually, I'd prefer to have this completion timestamp ability for all drivers. And avoid creating subsystem private mechanisms. I suppose we can punt on the get_ts_info control API if need be. > One could then postpone having to invent how to check the driver > support, and user would know non-supported status from setsockopt > failing. > > > > 1.3.2 Timestamp Reporting > > > ^^^^^^^^^^^^^^^^^^^^^^^^^ > > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > > index bb2b751d274a..3707c9075ae9 100644 > > > --- a/include/linux/skbuff.h > > > +++ b/include/linux/skbuff.h > > > @@ -489,10 +489,14 @@ enum { > > > > > > /* generate software time stamp when entering packet scheduling */ > > > SKBTX_SCHED_TSTAMP = 1 << 6, > > > + > > > + /* generate software time stamp on packet tx completion */ > > > + SKBTX_COMPLETION_TSTAMP = 1 << 7, > > > }; > > > > > > #define SKBTX_ANY_SW_TSTAMP (SKBTX_SW_TSTAMP | \ > > > - SKBTX_SCHED_TSTAMP) > > > + SKBTX_SCHED_TSTAMP | \ > > > + SKBTX_COMPLETION_TSTAMP) > > > > These fields are used in the skb_shared_info tx_flags field. > > Which is a very scarce resource. This takes the last available bit. > > That is my only possible concern: the opportunity cost. > > If doing it per-protocol sounds ok, it could be put in bt_skb_cb > instead. > > Since the completion timestamp didn't already exist, it maybe means > it's probably not that important for other parts of net/ I can see its value especially for hardware that does not support hardware timestamps, or hw timestamps at line rate. This gives a reasonable estimation of transmission time and measure of device delay. It is device specific whether it will be an over- or under-estimation, depending on whether the completion is queued to the host after or before the data is written on the wire. But either way, it will include the delay in processing the tx queue, which on multi-queue NICs and with TSO may be substantial (even before considering HW rate limiting). > > > #define SKBTX_ANY_TSTAMP (SKBTX_HW_TSTAMP | \ > > > SKBTX_HW_TSTAMP_USE_CYCLES | \ > > > SKBTX_ANY_SW_TSTAMP) > > > diff --git a/include/uapi/linux/errqueue.h b/include/uapi/linux/errqueue.h > > > index 3c70e8ac14b8..1ea47309d772 100644 > > > --- a/include/uapi/linux/errqueue.h > > > +++ b/include/uapi/linux/errqueue.h > > > @@ -73,6 +73,7 @@ enum { > > > SCM_TSTAMP_SND, /* driver passed skb to NIC, or HW */ > > > SCM_TSTAMP_SCHED, /* data entered the packet scheduler */ > > > SCM_TSTAMP_ACK, /* data acknowledged by peer */ > > > + SCM_TSTAMP_COMPLETION, /* packet tx completion */ > > > }; > > > > > > #endif /* _UAPI_LINUX_ERRQUEUE_H */ > > > diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h > > > index 55b0ab51096c..383213de612a 100644 > > > --- a/include/uapi/linux/net_tstamp.h > > > +++ b/include/uapi/linux/net_tstamp.h > > > @@ -44,8 +44,9 @@ enum { > > > SOF_TIMESTAMPING_BIND_PHC = (1 << 15), > > > SOF_TIMESTAMPING_OPT_ID_TCP = (1 << 16), > > > SOF_TIMESTAMPING_OPT_RX_FILTER = (1 << 17), > > > + SOF_TIMESTAMPING_TX_COMPLETION = (1 << 18), > > > > > > - SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_RX_FILTER, > > > + SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_TX_COMPLETION, > > > SOF_TIMESTAMPING_MASK = (SOF_TIMESTAMPING_LAST - 1) | > > > SOF_TIMESTAMPING_LAST > > > }; > > > @@ -58,7 +59,8 @@ enum { > > > #define SOF_TIMESTAMPING_TX_RECORD_MASK (SOF_TIMESTAMPING_TX_HARDWARE | \ > > > SOF_TIMESTAMPING_TX_SOFTWARE | \ > > > SOF_TIMESTAMPING_TX_SCHED | \ > > > - SOF_TIMESTAMPING_TX_ACK) > > > + SOF_TIMESTAMPING_TX_ACK | \ > > > + SOF_TIMESTAMPING_TX_COMPLETION) > > > > > > /** > > > * struct so_timestamping - SO_TIMESTAMPING parameter > > > diff --git a/net/ethtool/common.c b/net/ethtool/common.c > > > index 2bd77c94f9f1..75e3b756012e 100644 > > > --- a/net/ethtool/common.c > > > +++ b/net/ethtool/common.c > > > @@ -431,6 +431,7 @@ const char sof_timestamping_names[][ETH_GSTRING_LEN] = { > > > [const_ilog2(SOF_TIMESTAMPING_BIND_PHC)] = "bind-phc", > > > [const_ilog2(SOF_TIMESTAMPING_OPT_ID_TCP)] = "option-id-tcp", > > > [const_ilog2(SOF_TIMESTAMPING_OPT_RX_FILTER)] = "option-rx-filter", > > > + [const_ilog2(SOF_TIMESTAMPING_TX_COMPLETION)] = "completion-transmit", > > > > just "tx-completion"? > > Ok. > > -- > Pauli Virtanen
Hi Willem, On Mon, Feb 10, 2025 at 1:43 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > Pauli Virtanen wrote: > > Hi, > > > > su, 2025-02-09 kello 22:29 -0500, Willem de Bruijn kirjoitti: > > > Pauli Virtanen wrote: > > > > Add SOF_TIMESTAMPING_TX_COMPLETION, for requesting a software timestamp > > > > when hardware reports a packet completed. > > > > > > > > Completion tstamp is useful for Bluetooth, where hardware tx timestamps > > > > cannot be obtained 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. > > > > > > Getting the completion timestamp may indeed be useful more broadly. > > > > > > Alternatively, the HW timestamp is relatively imprecisely defined so > > > you could even just use that. Ideally, a hw timestamp conforms to IEEE > > > 1588v2 PHY: first symbol on the wire IIRC. But in many cases this is > > > not the case. It is not feasible at line rate, or the timestamp is > > > only taken when the completion is written over PCI, which may be > > > subject to PCI backpressure and happen after transmission on the wire. > > > As a result, the worst case hw tstamp must already be assumed not much > > > earlier than a completion timestamp. > > > > For BT ISO packets, in theory hw-provided TX timestamps exist, and we > > might want both (with separate flags for enabling them). I don't really > > know, last I looked Intel HW didn't support them, and it's not clear to > > which degree they are useful. > > That's reason enough to separate these measurement types. > > If we don't do it properly now, we won't be able to update drivers > later once users depend on requesting hw timestamps when they mean to > get completion timestamps. > > > > That said, +1 on adding explicit well defined measurement point > > > instead. > > > > > > > Signed-off-by: Pauli Virtanen <pav@iki.fi> > > > > --- > > > > Documentation/networking/timestamping.rst | 9 +++++++++ > > > > include/linux/skbuff.h | 6 +++++- > > > > include/uapi/linux/errqueue.h | 1 + > > > > include/uapi/linux/net_tstamp.h | 6 ++++-- > > > > net/ethtool/common.c | 1 + > > > > net/socket.c | 3 +++ > > > > 6 files changed, 23 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst > > > > index 61ef9da10e28..de2afed7a516 100644 > > > > --- a/Documentation/networking/timestamping.rst > > > > +++ b/Documentation/networking/timestamping.rst > > > > @@ -140,6 +140,15 @@ 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 > > > > + packets at once, and completion timestamps reflect the timing of the > > > > + report and not actual tx time. The completion timestamps are > > > > + currently implemented only for: Bluetooth L2CAP and ISO. This > > > > + flag can be enabled via both socket options and control messages. > > > > + > > > > > > Either we should support this uniformly, or it should be possible to > > > query whether a driver supports this. > > > > > > Unfortunately all completion callbacks are driver specific. > > > > > > But drivers that support hwtstamps will call skb_tstamp_tx with > > > nonzero hwtstamps. We could use that also to compute and queue > > > a completion timestamp if requested. At least for existing NIC > > > drivers. > > > > Ok. If possible, I'd like to avoid changing the behavior of the non- > > Bluetooth parts of net/ here, as I'm not familiar with those. > > > > I guess a simpler solution could be that sock_set_timestamping() checks > > the type of the socket, and gives EINVAL if the flag is set for non- > > Bluetooth sockets? > > Actually, I'd prefer to have this completion timestamp ability for all > drivers. And avoid creating subsystem private mechanisms. > > I suppose we can punt on the get_ts_info control API if need be. I guess that it is reasonable if we don't have to do the work for drivers other than Bluetooth otherwise I'd say you are probably asking too much here, also doesn't this land on the TSN space if one needs to tightly control timings? I suspect if this sort of change was not necessary for TSN then perhaps it wouldn't be of much value to try to generalize this. > > One could then postpone having to invent how to check the driver > > support, and user would know non-supported status from setsockopt > > failing. > > > > > > 1.3.2 Timestamp Reporting > > > > ^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > > > index bb2b751d274a..3707c9075ae9 100644 > > > > --- a/include/linux/skbuff.h > > > > +++ b/include/linux/skbuff.h > > > > @@ -489,10 +489,14 @@ enum { > > > > > > > > /* generate software time stamp when entering packet scheduling */ > > > > SKBTX_SCHED_TSTAMP = 1 << 6, > > > > + > > > > + /* generate software time stamp on packet tx completion */ > > > > + SKBTX_COMPLETION_TSTAMP = 1 << 7, > > > > }; > > > > > > > > #define SKBTX_ANY_SW_TSTAMP (SKBTX_SW_TSTAMP | \ > > > > - SKBTX_SCHED_TSTAMP) > > > > + SKBTX_SCHED_TSTAMP | \ > > > > + SKBTX_COMPLETION_TSTAMP) > > > > > > These fields are used in the skb_shared_info tx_flags field. > > > Which is a very scarce resource. This takes the last available bit. > > > That is my only possible concern: the opportunity cost. > > > > If doing it per-protocol sounds ok, it could be put in bt_skb_cb > > instead. > > > > Since the completion timestamp didn't already exist, it maybe means > > it's probably not that important for other parts of net/ > > I can see its value especially for hardware that does not support > hardware timestamps, or hw timestamps at line rate. > > This gives a reasonable estimation of transmission time and > measure of device delay. > > It is device specific whether it will be an over- or under-estimation, > depending on whether the completion is queued to the host after or > before the data is written on the wire. But either way, it will > include the delay in processing the tx queue, which on multi-queue > NICs and with TSO may be substantial (even before considering HW > rate limiting). > > > > > #define SKBTX_ANY_TSTAMP (SKBTX_HW_TSTAMP | \ > > > > SKBTX_HW_TSTAMP_USE_CYCLES | \ > > > > SKBTX_ANY_SW_TSTAMP) > > > > diff --git a/include/uapi/linux/errqueue.h b/include/uapi/linux/errqueue.h > > > > index 3c70e8ac14b8..1ea47309d772 100644 > > > > --- a/include/uapi/linux/errqueue.h > > > > +++ b/include/uapi/linux/errqueue.h > > > > @@ -73,6 +73,7 @@ enum { > > > > SCM_TSTAMP_SND, /* driver passed skb to NIC, or HW */ > > > > SCM_TSTAMP_SCHED, /* data entered the packet scheduler */ > > > > SCM_TSTAMP_ACK, /* data acknowledged by peer */ > > > > + SCM_TSTAMP_COMPLETION, /* packet tx completion */ > > > > }; > > > > > > > > #endif /* _UAPI_LINUX_ERRQUEUE_H */ > > > > diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h > > > > index 55b0ab51096c..383213de612a 100644 > > > > --- a/include/uapi/linux/net_tstamp.h > > > > +++ b/include/uapi/linux/net_tstamp.h > > > > @@ -44,8 +44,9 @@ enum { > > > > SOF_TIMESTAMPING_BIND_PHC = (1 << 15), > > > > SOF_TIMESTAMPING_OPT_ID_TCP = (1 << 16), > > > > SOF_TIMESTAMPING_OPT_RX_FILTER = (1 << 17), > > > > + SOF_TIMESTAMPING_TX_COMPLETION = (1 << 18), > > > > > > > > - SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_RX_FILTER, > > > > + SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_TX_COMPLETION, > > > > SOF_TIMESTAMPING_MASK = (SOF_TIMESTAMPING_LAST - 1) | > > > > SOF_TIMESTAMPING_LAST > > > > }; > > > > @@ -58,7 +59,8 @@ enum { > > > > #define SOF_TIMESTAMPING_TX_RECORD_MASK (SOF_TIMESTAMPING_TX_HARDWARE | \ > > > > SOF_TIMESTAMPING_TX_SOFTWARE | \ > > > > SOF_TIMESTAMPING_TX_SCHED | \ > > > > - SOF_TIMESTAMPING_TX_ACK) > > > > + SOF_TIMESTAMPING_TX_ACK | \ > > > > + SOF_TIMESTAMPING_TX_COMPLETION) > > > > > > > > /** > > > > * struct so_timestamping - SO_TIMESTAMPING parameter > > > > diff --git a/net/ethtool/common.c b/net/ethtool/common.c > > > > index 2bd77c94f9f1..75e3b756012e 100644 > > > > --- a/net/ethtool/common.c > > > > +++ b/net/ethtool/common.c > > > > @@ -431,6 +431,7 @@ const char sof_timestamping_names[][ETH_GSTRING_LEN] = { > > > > [const_ilog2(SOF_TIMESTAMPING_BIND_PHC)] = "bind-phc", > > > > [const_ilog2(SOF_TIMESTAMPING_OPT_ID_TCP)] = "option-id-tcp", > > > > [const_ilog2(SOF_TIMESTAMPING_OPT_RX_FILTER)] = "option-rx-filter", > > > > + [const_ilog2(SOF_TIMESTAMPING_TX_COMPLETION)] = "completion-transmit", > > > > > > just "tx-completion"? > > > > Ok. > > > > -- > > Pauli Virtanen > >
Luiz Augusto von Dentz wrote: > Hi Willem, > > On Mon, Feb 10, 2025 at 1:43 PM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > Pauli Virtanen wrote: > > > Hi, > > > > > > su, 2025-02-09 kello 22:29 -0500, Willem de Bruijn kirjoitti: > > > > Pauli Virtanen wrote: > > > > > Add SOF_TIMESTAMPING_TX_COMPLETION, for requesting a software timestamp > > > > > when hardware reports a packet completed. > > > > > > > > > > Completion tstamp is useful for Bluetooth, where hardware tx timestamps > > > > > cannot be obtained 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. > > > > > > > > Getting the completion timestamp may indeed be useful more broadly. > > > > > > > > Alternatively, the HW timestamp is relatively imprecisely defined so > > > > you could even just use that. Ideally, a hw timestamp conforms to IEEE > > > > 1588v2 PHY: first symbol on the wire IIRC. But in many cases this is > > > > not the case. It is not feasible at line rate, or the timestamp is > > > > only taken when the completion is written over PCI, which may be > > > > subject to PCI backpressure and happen after transmission on the wire. > > > > As a result, the worst case hw tstamp must already be assumed not much > > > > earlier than a completion timestamp. > > > > > > For BT ISO packets, in theory hw-provided TX timestamps exist, and we > > > might want both (with separate flags for enabling them). I don't really > > > know, last I looked Intel HW didn't support them, and it's not clear to > > > which degree they are useful. > > > > That's reason enough to separate these measurement types. > > > > If we don't do it properly now, we won't be able to update drivers > > later once users depend on requesting hw timestamps when they mean to > > get completion timestamps. > > > > > > That said, +1 on adding explicit well defined measurement point > > > > instead. > > > > > > > > > Signed-off-by: Pauli Virtanen <pav@iki.fi> > > > > > --- > > > > > Documentation/networking/timestamping.rst | 9 +++++++++ > > > > > include/linux/skbuff.h | 6 +++++- > > > > > include/uapi/linux/errqueue.h | 1 + > > > > > include/uapi/linux/net_tstamp.h | 6 ++++-- > > > > > net/ethtool/common.c | 1 + > > > > > net/socket.c | 3 +++ > > > > > 6 files changed, 23 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst > > > > > index 61ef9da10e28..de2afed7a516 100644 > > > > > --- a/Documentation/networking/timestamping.rst > > > > > +++ b/Documentation/networking/timestamping.rst > > > > > @@ -140,6 +140,15 @@ 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 > > > > > + packets at once, and completion timestamps reflect the timing of the > > > > > + report and not actual tx time. The completion timestamps are > > > > > + currently implemented only for: Bluetooth L2CAP and ISO. This > > > > > + flag can be enabled via both socket options and control messages. > > > > > + > > > > > > > > Either we should support this uniformly, or it should be possible to > > > > query whether a driver supports this. > > > > > > > > Unfortunately all completion callbacks are driver specific. > > > > > > > > But drivers that support hwtstamps will call skb_tstamp_tx with > > > > nonzero hwtstamps. We could use that also to compute and queue > > > > a completion timestamp if requested. At least for existing NIC > > > > drivers. > > > > > > Ok. If possible, I'd like to avoid changing the behavior of the non- > > > Bluetooth parts of net/ here, as I'm not familiar with those. > > > > > > I guess a simpler solution could be that sock_set_timestamping() checks > > > the type of the socket, and gives EINVAL if the flag is set for non- > > > Bluetooth sockets? > > > > Actually, I'd prefer to have this completion timestamp ability for all > > drivers. And avoid creating subsystem private mechanisms. > > > > I suppose we can punt on the get_ts_info control API if need be. > > I guess that it is reasonable if we don't have to do the work for > drivers other than Bluetooth otherwise I'd say you are probably asking > too much here, I was mainly agreeing to Pauli's implementation in this series. Not asking to do any work irrelevant to BT. > also doesn't this land on the TSN space if one needs to > tightly control timings? I suspect if this sort of change was not > necessary for TSN then perhaps it wouldn't be of much value to try to > generalize this. I don't fully follow. But in general SO_TIMESTAMPING is not limited to TSN. > > > > One could then postpone having to invent how to check the driver > > > support, and user would know non-supported status from setsockopt > > > failing. > > > > > > > > 1.3.2 Timestamp Reporting > > > > > ^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > > > > index bb2b751d274a..3707c9075ae9 100644 > > > > > --- a/include/linux/skbuff.h > > > > > +++ b/include/linux/skbuff.h > > > > > @@ -489,10 +489,14 @@ enum { > > > > > > > > > > /* generate software time stamp when entering packet scheduling */ > > > > > SKBTX_SCHED_TSTAMP = 1 << 6, > > > > > + > > > > > + /* generate software time stamp on packet tx completion */ > > > > > + SKBTX_COMPLETION_TSTAMP = 1 << 7, > > > > > }; > > > > > > > > > > #define SKBTX_ANY_SW_TSTAMP (SKBTX_SW_TSTAMP | \ > > > > > - SKBTX_SCHED_TSTAMP) > > > > > + SKBTX_SCHED_TSTAMP | \ > > > > > + SKBTX_COMPLETION_TSTAMP) > > > > > > > > These fields are used in the skb_shared_info tx_flags field. > > > > Which is a very scarce resource. This takes the last available bit. > > > > That is my only possible concern: the opportunity cost. > > > > > > If doing it per-protocol sounds ok, it could be put in bt_skb_cb > > > instead. > > > > > > Since the completion timestamp didn't already exist, it maybe means > > > it's probably not that important for other parts of net/ > > > > I can see its value especially for hardware that does not support > > hardware timestamps, or hw timestamps at line rate. > > > > This gives a reasonable estimation of transmission time and > > measure of device delay. > > > > It is device specific whether it will be an over- or under-estimation, > > depending on whether the completion is queued to the host after or > > before the data is written on the wire. But either way, it will > > include the delay in processing the tx queue, which on multi-queue > > NICs and with TSO may be substantial (even before considering HW > > rate limiting). > > > > > > > #define SKBTX_ANY_TSTAMP (SKBTX_HW_TSTAMP | \ > > > > > SKBTX_HW_TSTAMP_USE_CYCLES | \ > > > > > SKBTX_ANY_SW_TSTAMP) > > > > > diff --git a/include/uapi/linux/errqueue.h b/include/uapi/linux/errqueue.h > > > > > index 3c70e8ac14b8..1ea47309d772 100644 > > > > > --- a/include/uapi/linux/errqueue.h > > > > > +++ b/include/uapi/linux/errqueue.h > > > > > @@ -73,6 +73,7 @@ enum { > > > > > SCM_TSTAMP_SND, /* driver passed skb to NIC, or HW */ > > > > > SCM_TSTAMP_SCHED, /* data entered the packet scheduler */ > > > > > SCM_TSTAMP_ACK, /* data acknowledged by peer */ > > > > > + SCM_TSTAMP_COMPLETION, /* packet tx completion */ > > > > > }; > > > > > > > > > > #endif /* _UAPI_LINUX_ERRQUEUE_H */ > > > > > diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h > > > > > index 55b0ab51096c..383213de612a 100644 > > > > > --- a/include/uapi/linux/net_tstamp.h > > > > > +++ b/include/uapi/linux/net_tstamp.h > > > > > @@ -44,8 +44,9 @@ enum { > > > > > SOF_TIMESTAMPING_BIND_PHC = (1 << 15), > > > > > SOF_TIMESTAMPING_OPT_ID_TCP = (1 << 16), > > > > > SOF_TIMESTAMPING_OPT_RX_FILTER = (1 << 17), > > > > > + SOF_TIMESTAMPING_TX_COMPLETION = (1 << 18), > > > > > > > > > > - SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_RX_FILTER, > > > > > + SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_TX_COMPLETION, > > > > > SOF_TIMESTAMPING_MASK = (SOF_TIMESTAMPING_LAST - 1) | > > > > > SOF_TIMESTAMPING_LAST > > > > > }; > > > > > @@ -58,7 +59,8 @@ enum { > > > > > #define SOF_TIMESTAMPING_TX_RECORD_MASK (SOF_TIMESTAMPING_TX_HARDWARE | \ > > > > > SOF_TIMESTAMPING_TX_SOFTWARE | \ > > > > > SOF_TIMESTAMPING_TX_SCHED | \ > > > > > - SOF_TIMESTAMPING_TX_ACK) > > > > > + SOF_TIMESTAMPING_TX_ACK | \ > > > > > + SOF_TIMESTAMPING_TX_COMPLETION) > > > > > > > > > > /** > > > > > * struct so_timestamping - SO_TIMESTAMPING parameter > > > > > diff --git a/net/ethtool/common.c b/net/ethtool/common.c > > > > > index 2bd77c94f9f1..75e3b756012e 100644 > > > > > --- a/net/ethtool/common.c > > > > > +++ b/net/ethtool/common.c > > > > > @@ -431,6 +431,7 @@ const char sof_timestamping_names[][ETH_GSTRING_LEN] = { > > > > > [const_ilog2(SOF_TIMESTAMPING_BIND_PHC)] = "bind-phc", > > > > > [const_ilog2(SOF_TIMESTAMPING_OPT_ID_TCP)] = "option-id-tcp", > > > > > [const_ilog2(SOF_TIMESTAMPING_OPT_RX_FILTER)] = "option-rx-filter", > > > > > + [const_ilog2(SOF_TIMESTAMPING_TX_COMPLETION)] = "completion-transmit", > > > > > > > > just "tx-completion"? > > > > > > Ok. > > > > > > -- > > > Pauli Virtanen > > > > > > > -- > Luiz Augusto von Dentz
Hi, su, 2025-02-09 kello 22:29 -0500, Willem de Bruijn kirjoitti: > Pauli Virtanen wrote: > > > 1.3.2 Timestamp Reporting > > ^^^^^^^^^^^^^^^^^^^^^^^^^ > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > index bb2b751d274a..3707c9075ae9 100644 > > --- a/include/linux/skbuff.h > > +++ b/include/linux/skbuff.h > > @@ -489,10 +489,14 @@ enum { > > > > /* generate software time stamp when entering packet scheduling */ > > SKBTX_SCHED_TSTAMP = 1 << 6, > > + > > + /* generate software time stamp on packet tx completion */ > > + SKBTX_COMPLETION_TSTAMP = 1 << 7, > > }; > > > > #define SKBTX_ANY_SW_TSTAMP (SKBTX_SW_TSTAMP | \ > > - SKBTX_SCHED_TSTAMP) > > + SKBTX_SCHED_TSTAMP | \ > > + SKBTX_COMPLETION_TSTAMP) > > These fields are used in the skb_shared_info tx_flags field. > Which is a very scarce resource. This takes the last available bit. > That is my only possible concern: the opportunity cost. One alternative here could be: Make SOF_TIMESTAMPING_TX_COMPLETION available only as a socket option, and make it emit COMPLETION tstamp exactly for those packets that also have SOF_TIMESTAMPING_TX_SOFTWARE enabled. User apps can then still timestamp only selected packets via CMSG, however the choice between SND and SND+COMPLETION is socket-wide. The API is then less uniform, but probably usable in practice, although some use cases may not be interested in the extra SND tstamps. IIUC, sizeof skb_shared_info cannot be easily changed and I'm not sure if there is room left there. So if the option of putting it into protocol-specific skb cb is also out, then I'm not sure what the plan here should be.
diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst index 61ef9da10e28..de2afed7a516 100644 --- a/Documentation/networking/timestamping.rst +++ b/Documentation/networking/timestamping.rst @@ -140,6 +140,15 @@ 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 + packets at once, and completion timestamps reflect the timing of the + report and not actual tx time. The completion timestamps are + currently implemented only for: Bluetooth L2CAP and ISO. This + flag can be enabled via both socket options and control messages. + 1.3.2 Timestamp Reporting ^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index bb2b751d274a..3707c9075ae9 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -489,10 +489,14 @@ enum { /* generate software time stamp when entering packet scheduling */ SKBTX_SCHED_TSTAMP = 1 << 6, + + /* generate software time stamp on packet tx completion */ + SKBTX_COMPLETION_TSTAMP = 1 << 7, }; #define SKBTX_ANY_SW_TSTAMP (SKBTX_SW_TSTAMP | \ - SKBTX_SCHED_TSTAMP) + SKBTX_SCHED_TSTAMP | \ + SKBTX_COMPLETION_TSTAMP) #define SKBTX_ANY_TSTAMP (SKBTX_HW_TSTAMP | \ SKBTX_HW_TSTAMP_USE_CYCLES | \ SKBTX_ANY_SW_TSTAMP) diff --git a/include/uapi/linux/errqueue.h b/include/uapi/linux/errqueue.h index 3c70e8ac14b8..1ea47309d772 100644 --- a/include/uapi/linux/errqueue.h +++ b/include/uapi/linux/errqueue.h @@ -73,6 +73,7 @@ enum { SCM_TSTAMP_SND, /* driver passed skb to NIC, or HW */ SCM_TSTAMP_SCHED, /* data entered the packet scheduler */ SCM_TSTAMP_ACK, /* data acknowledged by peer */ + SCM_TSTAMP_COMPLETION, /* packet tx completion */ }; #endif /* _UAPI_LINUX_ERRQUEUE_H */ diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h index 55b0ab51096c..383213de612a 100644 --- a/include/uapi/linux/net_tstamp.h +++ b/include/uapi/linux/net_tstamp.h @@ -44,8 +44,9 @@ enum { SOF_TIMESTAMPING_BIND_PHC = (1 << 15), SOF_TIMESTAMPING_OPT_ID_TCP = (1 << 16), SOF_TIMESTAMPING_OPT_RX_FILTER = (1 << 17), + SOF_TIMESTAMPING_TX_COMPLETION = (1 << 18), - SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_RX_FILTER, + SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_TX_COMPLETION, SOF_TIMESTAMPING_MASK = (SOF_TIMESTAMPING_LAST - 1) | SOF_TIMESTAMPING_LAST }; @@ -58,7 +59,8 @@ enum { #define SOF_TIMESTAMPING_TX_RECORD_MASK (SOF_TIMESTAMPING_TX_HARDWARE | \ SOF_TIMESTAMPING_TX_SOFTWARE | \ SOF_TIMESTAMPING_TX_SCHED | \ - SOF_TIMESTAMPING_TX_ACK) + SOF_TIMESTAMPING_TX_ACK | \ + SOF_TIMESTAMPING_TX_COMPLETION) /** * struct so_timestamping - SO_TIMESTAMPING parameter diff --git a/net/ethtool/common.c b/net/ethtool/common.c index 2bd77c94f9f1..75e3b756012e 100644 --- a/net/ethtool/common.c +++ b/net/ethtool/common.c @@ -431,6 +431,7 @@ const char sof_timestamping_names[][ETH_GSTRING_LEN] = { [const_ilog2(SOF_TIMESTAMPING_BIND_PHC)] = "bind-phc", [const_ilog2(SOF_TIMESTAMPING_OPT_ID_TCP)] = "option-id-tcp", [const_ilog2(SOF_TIMESTAMPING_OPT_RX_FILTER)] = "option-rx-filter", + [const_ilog2(SOF_TIMESTAMPING_TX_COMPLETION)] = "completion-transmit", }; static_assert(ARRAY_SIZE(sof_timestamping_names) == __SOF_TIMESTAMPING_CNT); diff --git a/net/socket.c b/net/socket.c index 4afe31656a2b..22b7f6f50889 100644 --- a/net/socket.c +++ b/net/socket.c @@ -693,6 +693,9 @@ void __sock_tx_timestamp(__u32 tsflags, __u8 *tx_flags) if (tsflags & SOF_TIMESTAMPING_TX_SCHED) flags |= SKBTX_SCHED_TSTAMP; + if (tsflags & SOF_TIMESTAMPING_TX_COMPLETION) + flags |= SKBTX_COMPLETION_TSTAMP; + *tx_flags = flags; } EXPORT_SYMBOL(__sock_tx_timestamp);
Add SOF_TIMESTAMPING_TX_COMPLETION, for requesting a software timestamp when hardware reports a packet completed. Completion tstamp is useful for Bluetooth, where hardware tx timestamps cannot be obtained 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> --- Documentation/networking/timestamping.rst | 9 +++++++++ include/linux/skbuff.h | 6 +++++- include/uapi/linux/errqueue.h | 1 + include/uapi/linux/net_tstamp.h | 6 ++++-- net/ethtool/common.c | 1 + net/socket.c | 3 +++ 6 files changed, 23 insertions(+), 3 deletions(-)