| Message ID | 0dfb22ec3c9d9ed796ba8edc919a690ca2fb1fdd.1742324341.git.pav@iki.fi (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | de4f56cf6cfd5c54a78525d65419eaeba14edb0b |
| Headers | show |
| Series | net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | fail | error: patch failed: include/linux/skbuff.h:478 error: include/linux/skbuff.h: patch does not apply error: patch failed: net/core/skbuff.c:5523 error: net/core/skbuff.c: patch does not apply hint: Use 'git am --show-current-patch' to see the failed patch |
| 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 | fail | TestRunner_l2cap-tester: Total: 61, Passed: 58 (95.1%), Failed: 1, Not Run: 2 |
| 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 an automated email and please do not reply to this email. Dear Submitter, Thank you for submitting the patches to the linux bluetooth mailing list. While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository. ----- Output ----- error: patch failed: include/linux/skbuff.h:478 error: include/linux/skbuff.h: patch does not apply error: patch failed: net/core/skbuff.c:5523 error: net/core/skbuff.c: patch does not apply hint: Use 'git am --show-current-patch' to see the failed patch Please resolve the issue and submit the patches again. --- Regards, Linux Bluetooth
Hi Pauli, On Tue, Mar 18, 2025 at 3:15 PM <bluez.test.bot@gmail.com> wrote: > > This is an automated email and please do not reply to this email. > > Dear Submitter, > > Thank you for submitting the patches to the linux bluetooth mailing list. > While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository. > > ----- Output ----- > > error: patch failed: include/linux/skbuff.h:478 > error: include/linux/skbuff.h: patch does not apply > error: patch failed: net/core/skbuff.c:5523 > error: net/core/skbuff.c: patch does not apply > hint: Use 'git am --show-current-patch' to see the failed patch > > Please resolve the issue and submit the patches again. Did you base in something other than bluetooth-next? Do we need to rebase bluetooth-next to net-next perhaps?
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=945296 ---Test result--- Test Summary: CheckPatch PENDING 0.24 seconds GitLint PENDING 0.25 seconds SubjectPrefix FAIL 0.75 seconds BuildKernel PASS 24.54 seconds CheckAllWarning PASS 27.22 seconds CheckSparse WARNING 31.06 seconds BuildKernel32 PASS 24.33 seconds TestRunnerSetup PASS 432.00 seconds TestRunner_l2cap-tester FAIL 21.34 seconds TestRunner_iso-tester PASS 28.24 seconds TestRunner_bnep-tester PASS 4.78 seconds TestRunner_mgmt-tester PASS 121.31 seconds TestRunner_rfcomm-tester PASS 7.82 seconds TestRunner_sco-tester PASS 12.14 seconds TestRunner_ioctl-tester PASS 8.36 seconds TestRunner_mesh-tester PASS 5.88 seconds TestRunner_smp-tester PASS 7.18 seconds TestRunner_userchan-tester PASS 4.96 seconds IncrementalBuild PENDING 0.86 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: TestRunner_l2cap-tester - FAIL Desc: Run l2cap-tester with test-runner Output: Total: 61, Passed: 58 (95.1%), Failed: 1, Not Run: 2 Failed Test Cases L2CAP LE Client - Write 32k Success Failed 0.402 seconds ############################## Test: IncrementalBuild - PENDING Desc: Incremental build with the patches in the series Output: --- Regards, Linux Bluetooth
On Wed, Mar 19, 2025 at 3:08 AM 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, 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> Hi Pauli, This patch overall looks good to me but it depends on the small question in another patch that is relevant to how we use this new flag in reality. Let's discuss a bit there. Thanks, Jason > --- > > 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 > + packets at once, and completion timestamps reflect the timing of the > + report and not actual tx time. 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 cd8294cdc249..b974a277975a 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -478,8 +478,8 @@ enum { > /* device driver is going to provide hardware time stamp */ > SKBTX_IN_PROGRESS = 1 << 2, > > - /* reserved */ > - SKBTX_RESERVED = 1 << 3, > + /* generate software time stamp on packet tx completion */ > + SKBTX_COMPLETION_TSTAMP = 1 << 3, > > /* generate wifi status information (where possible) */ > SKBTX_WIFI_STATUS = 1 << 4, > @@ -498,7 +498,8 @@ enum { > > #define SKBTX_ANY_SW_TSTAMP (SKBTX_SW_TSTAMP | \ > SKBTX_SCHED_TSTAMP | \ > - SKBTX_BPF) > + SKBTX_BPF | \ > + SKBTX_COMPLETION_TSTAMP) > #define SKBTX_ANY_TSTAMP (SKBTX_HW_TSTAMP | \ > 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/core/skbuff.c b/net/core/skbuff.c > index ab8acb737b93..6cbf77bc61fc 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -5523,6 +5523,8 @@ static bool skb_tstamp_tx_report_so_timestamping(struct sk_buff *skb, > SKBTX_SW_TSTAMP); > case SCM_TSTAMP_ACK: > return TCP_SKB_CB(skb)->txstamp_ack & TSTAMP_ACK_SK; > + case SCM_TSTAMP_COMPLETION: > + return skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP; > } > > return false; > diff --git a/net/ethtool/common.c b/net/ethtool/common.c > index 7e3c16856c1a..0cb6da1f692a 100644 > --- a/net/ethtool/common.c > +++ b/net/ethtool/common.c > @@ -476,6 +476,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)] = "tx-completion", > }; > static_assert(ARRAY_SIZE(sof_timestamping_names) == __SOF_TIMESTAMPING_CNT); > > diff --git a/net/socket.c b/net/socket.c > index b64ecf2722e7..e3d879b53278 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -689,6 +689,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 > >
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, 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 Minor: double space above, grammar issue below "receives [packet] a". > + 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. This flag can be enabled via both > + socket options and control messages. > + Otherwise the patch LGTM.
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
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.
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>
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@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. From my side v5 should be good enough, if we want it now. 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
Hi, On Fri, Mar 21, 2025 at 12:12 AM Pauli Virtanen <pav@iki.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@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. > > 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@gmail.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 >
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 + packets at once, and completion timestamps reflect the timing of the + report and not actual tx time. 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 cd8294cdc249..b974a277975a 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -478,8 +478,8 @@ enum { /* device driver is going to provide hardware time stamp */ SKBTX_IN_PROGRESS = 1 << 2, - /* reserved */ - SKBTX_RESERVED = 1 << 3, + /* generate software time stamp on packet tx completion */ + SKBTX_COMPLETION_TSTAMP = 1 << 3, /* generate wifi status information (where possible) */ SKBTX_WIFI_STATUS = 1 << 4, @@ -498,7 +498,8 @@ enum { #define SKBTX_ANY_SW_TSTAMP (SKBTX_SW_TSTAMP | \ SKBTX_SCHED_TSTAMP | \ - SKBTX_BPF) + SKBTX_BPF | \ + SKBTX_COMPLETION_TSTAMP) #define SKBTX_ANY_TSTAMP (SKBTX_HW_TSTAMP | \ 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/core/skbuff.c b/net/core/skbuff.c index ab8acb737b93..6cbf77bc61fc 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -5523,6 +5523,8 @@ static bool skb_tstamp_tx_report_so_timestamping(struct sk_buff *skb, SKBTX_SW_TSTAMP); case SCM_TSTAMP_ACK: return TCP_SKB_CB(skb)->txstamp_ack & TSTAMP_ACK_SK; + case SCM_TSTAMP_COMPLETION: + return skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP; } return false; diff --git a/net/ethtool/common.c b/net/ethtool/common.c index 7e3c16856c1a..0cb6da1f692a 100644 --- a/net/ethtool/common.c +++ b/net/ethtool/common.c @@ -476,6 +476,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)] = "tx-completion", }; static_assert(ARRAY_SIZE(sof_timestamping_names) == __SOF_TIMESTAMPING_CNT); diff --git a/net/socket.c b/net/socket.c index b64ecf2722e7..e3d879b53278 100644 --- a/net/socket.c +++ b/net/socket.c @@ -689,6 +689,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, 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(-)