On Thu, Feb 20, 2025 at 10:35 AM Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx> wrote: > > Jason Xing wrote: > > On Thu, Feb 20, 2025 at 2:15 AM Pauli Virtanen <pav@xxxxxx> 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@xxxxxx> > > > --- > > > > > > Notes: > > > v4: changed SOF_TIMESTAMPING_TX_COMPLETION to only emit COMPLETION > > > together with SND, to save a bit in skb_shared_info.tx_flags > > > > > > As it then cannot be set per-skb, reject setting it via CMSG. > > > > > > Documentation/networking/timestamping.rst | 9 +++++++++ > > > include/uapi/linux/errqueue.h | 1 + > > > include/uapi/linux/net_tstamp.h | 6 ++++-- > > > net/core/sock.c | 2 ++ > > > net/ethtool/common.c | 1 + > > > 5 files changed, 17 insertions(+), 2 deletions(-) > > > > > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst > > > index 61ef9da10e28..5034dfe326c0 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, for the packets that > > > + also have SOF_TIMESTAMPING_TX_SOFTWARE enabled. The completion > > > > Is it mandatory for other drivers that will try to use > > SOF_TIMESTAMPING_TX_COMPLETION in the future? I can see you coupled > > both of them in hci_conn_tx_queue in patch [2/5]. If so, it would be > > better if you add the limitation in sock_set_timestamping() so that > > the same rule can be applied to other drivers. > > > > But may I ask why you tried to couple them so tight in the version? > > Could you say more about this? It's optional, right? IIUC, you > > expected the driver to have both timestamps and then calculate the > > delta easily? > > This is a workaround around the limited number of bits available in > skb_shared_info.tx_flags. Oh, I'm surprised I missed the point even though I revisited the previous discussion. Pauli, please add the limitation when users setsockopt in sock_set_timestamping() :) > > Pauli could claim last available bit 7.. but then you would need to > find another bit for SKBTX_BPF ;) Right :D > > FWIW I think we could probably free up 1 or 2 bits if we look closely, > e.g., of SKBTX_HW_TSTAMP_USE_CYCLES or SKBTX_WIFI_STATUS. Good. Will you submit a patch series to do that, or...? > > But given that two uses for those bits are in review at the same time, > I doubt that this is the last such feature that is added. > > This workaround is clever. Only, we're leaking implementation > limitations into the API, making it API non-uniform and thus more > complex. Probably not a big deal because previously OPT_ID_TCP is also bound with OPT_ID? > > On the one hand, the feature should work just like all the existing > ones, and thus be configurable independently, and maintaining a > consistent API. But, this does require a tx_flags bit. And the > same for each subsequent such feature that gets proposed. > > On the other, if we can anticipate more such additional requests, > then perhaps it does make sense to use only one bit in the skb to > encode whether the skb is sampled (in this case SKBTX_SW_TSTAMP), > and use per-socket sk_tsflags to encode which types of timestamps > to generate for such sampled packets. Very good idea, I think. > > This is more or less how sampling in the new BPF mode works too. > > It is just not how SO_TIMESTAMPING works for existing bits. If needed, especially when a new bit is added, I think we can refactor this part in the future? Or can we adjust it in advance? Speaking of the design itself, it's really good :) Thanks, Jason > > > There's something to be said for either approach IMHO.
