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. > > > > > 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? That is also unfortunate. Ideally we had never needed OPT_ID_TCP. > > > > 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. After giving it some thought, I prefer the first option to maintain the same API. Let's see if we can harvest a bit and use that for this new completion point. > > > > > 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 :) We cannot change existing behavior. > Thanks, > Jason > > > > > > > There's something to be said for either approach IMHO.
