On Fri, Feb 21, 2025 at 7:19 AM Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx> wrote: > > Willem de Bruijn wrote: > > Jason Xing wrote: > > > 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...? > > > > Reclaiming space is really up to whoever needs it. > > > > I'll take a quick look, just to see if there is an obvious path and > > we can postpone this whole conversation to next time we need a bit. > > SKBTX_HW_TSTAMP_USE_CYCLES is only true if SOF_TIMESTAMPING_BIND_PHC. > It cannot be set per cmsg (is not in SOF_TIMESTAMPING_TX_RECORD_MASK), > so no need to record it per skb. Those flags look like sub-features to me, not like the completion one. Occupying one bit in the skb is luxury for them. > > It only has two drivers using it, which can easily be updated: > > - if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_USE_CYCLES) > + if (skb->sk && > + READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_BIND_PHC) > tx_flags |= IGC_TX_FLAGS_TSTAMP_TIMER_1; > > They later call skb_tstamp_tx, which does nothing if !skb->sk. > Only cost is a higher cost of accessing the sk cacheline. > > SKBTX_WIFI_STATUS essentially follows the same argument. It can only > be set in the sockopt. It has a handful more callsites that would need > to be updated. sock_flag(sk, SOCK_WIFI_STATUS) will be tested without > the socket lock held. But this is already the case in the UDP lockless > fast path through ip_make_skb. > > SKBTX_HW_TSTAMP_NETDEV is only used on Rx. Could shadow another bit > that is used only on Tx. > > SKBTX_IN_PROGRESS is only used by the driver to suppress the software > tx timestamp from skb_tx_timestamp if a later hardware timestamp will > be generated. Predates SOF_TIMESTAMPING_OPT_TX_SWHW. Thanks for the detailed analysis. I just checked them out and agreed. > In short plenty of bits we can reclaim if we try. > > SKBTX_BPF was just merged, so we will have to reclaim one. It's worth knowing that we probably will work on top of the bpf-next net branch if so. Do you want to reclaim every possible bit in one go? One series can complete the work. // If there is anything, feel free to ask me to implement/co-work :) > The first one seems most straightforward. If there are more flags than the tx_flags can have in the future, I think we can turn to the second method you mentioned. Could we harvest one or more to have a better uniformed design before working on this completion feature, I'm wondering? Thanks, Jason
