Pauli Virtanen wrote: > Hi all, > > Thanks for the comments, I guess the original series should have been > Cc'd more widely to get them earlier. > > ma, 2024-05-13 kello 22:09 -0400, Willem de Bruijn kirjoitti: > > Luiz Augusto von Dentz wrote: > > > Hi Willem, > > > > > > On Mon, May 13, 2024 at 9:32 PM Willem de Bruijn > > > <willemdebruijn.kernel@xxxxxxxxx> wrote: > > > > > > > > Jakub Kicinski wrote: > > > > > On Mon, 13 May 2024 18:09:31 -0400 Luiz Augusto von Dentz wrote: > > > > > > > There is one more warning in the Intel driver: > > > > > > > > > > > > > > drivers/bluetooth/btintel_pcie.c:673:33: warning: symbol 'causes_list' > > > > > > > was not declared. Should it be static? > > > > > > > > > > > > We have a fix for that but I was hoping to have it in before the merge > > > > > > window and then have the fix merged later. > > > > > > > > > > > > > It'd also be great to get an ACK from someone familiar with the socket > > > > > > > time stamping (Willem?) I'm not sure there's sufficient detail in the > > > > > > > commit message to explain the choices to: > > > > > > > - change the definition of SCHED / SEND to mean queued / completed, > > > > > > > while for Ethernet they mean queued to qdisc, queued to HW. > > > > > > > > > > > > hmm I thought this was hardware specific, it obviously won't work > > > > > > exactly as Ethernet since it is a completely different protocol stack, > > > > > > or are you suggesting we need other definitions for things like TX > > > > > > completed? > > > > > > > > > > I don't know anything about queuing in BT, in terms of timestamping > > > > > the SEND - SCHED difference is supposed to indicate the level of > > > > > host delay or host congestion. If the queuing in BT happens mostly in > > > > > the device HW queue then it may make sense to generate SCHED when > > > > > handing over to the driver. OTOH if the devices can coalesce or delay > > > > > completions the completion timeout may be less accurate than stamping > > > > > before submitting to HW... I'm looking for the analysis that the choices > > > > > were well thought thru. > > > > > > > > SCM_TSTAMP_SND is taken before an skb is passed to the device. > > > > This matches request SOF_TIMESTAMPING_TX_SOFTWARE. > > > > > > > > A timestamp returned on transmit completion is requested as > > > > SOF_TIMESTAMPING_TX_HARDWARE. We do not have a type for a software > > > > timestamp taken at tx completion cleaning. If anything, I would think > > > > it would be a passes as a hardware timestamp. > > > > > > In that case I think we probably misinterpret it, at least I though > > > that TX_HARDWARE would really be a hardware generated timestamp using > > > it own clock > > > > It normally is. It is just read from the tx descriptor on completion. > > > > We really don't have a good model for a software timestamp taken at > > completion processing. > > > > It may be worthwhile more broadly, especially for devices that do not > > support true hardware timestamps. > > > > Perhaps we should add an SCM_TSTAMP_TXCOMPLETION for this case. And a > > new SOF_TIMESTAMPING option to go with it. Similar to what we did for > > SCM_STAMP_SCHED. > > Ok, I was also under the impression TX_HARDWARE was only for actual HW > timestamps. TSTAMP_ACK appeared to not really match semantics either, > so TSTAMP_SND it then was. > > > The general timestamping flow here was: > > 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. > > From the above, I gather SCHED* should be SND, and SND* should be > TXCOMPLETION. Then I'm not sure when we should generate SCHED, if at > all, unless it's done more or less in sendmsg() when it generates the > skbs. Agreed. Missing SCHED if packets do not go through software queuing should be fine. Inversely, with multiple qdiscs processes see multiple SCHED events. > Possibly the SND timestamp could also be generated on driver side if > one wants to have it taken at transport tx completion. I don't > immediately know what is the use case would be though, as the packet > may still have to wait on HW side before it goes over the air. > > For the use case here, we want to know the total latency, so the > completion timestamp is the interesting one. In the audio use case, in > normal operation there is a free HW slot and packets do not wait in > net/bluetooth queues but end up in HW buffers ASAP (fast, maybe < 1 > ms), and then wait a much longer time (usu. 5-50 ms) in the HW buffers > before it reports completion. > > > > if you are saying that TX_HARDWARE is just marking the > > > TX completion of the packet at the host then we can definitely align > > > with the current exception, that said we do have a command to actually > > > read out the actual timestamp from the BT controller, that is usually > > > more precise since some of the connection do require usec precision > > > which is something that can get skew by the processing of HCI events > > > themselves, well I guess we use that if the controller supports it and > > > if it doesn't then we do based on the host timestamp when processing > > > the HCI event indicating the completion of the transmission. > > > > > > > Returning SCHED when queuing to a device and SND later on receiving > > > > completions seems like not following SO_TIMESTAMPING convention to me. > > > > But I don't fully know the HCI model. > > > > > > > > As for the "experimental" BT_POLL_ERRQUEUE. This is an addition to the > > > > ABI, right? So immutable. Is it fair to call that experimental? > > > > > > I guess you are referring to the fact that sockopt ID reserved to > > > BT_POLL_ERRQUEUE cannot be reused anymore even if we drop its usage in > > > the future, yes that is correct, but we can actually return > > > ENOPROTOOPT as it current does: > > > > > > if (!bt_poll_errqueue_enabled()) > > > return -ENOPROTOOPT > > > > I see. Once applications rely on a feature, it can be hard to actually > > deprecate. But in this case it may be possible. > > > > > Anyway I would be really happy to drop it so we don't have to worry > > > about it later. > > > > > > > It might be safer to only suppress the sk_error_report in > > > > sock_queue_err_skb. Or at least in bt_sock_poll to check the type of > > > > all outstanding errors and only suppress if all are timestamps. > > > > > > Or perhaps we could actually do that via poll/epoll directly? Not that > > > it would make it much simpler since the library tends to wrap the > > > usage of poll/epoll but POLLERR meaning both errors or errqueue events > > > is sort of the problem we are trying to figure out how to process them > > > separately. > > > > The process would still be awoken, of course. If bluetoothd can just > > be modified to ignore the reports, that would indeed be easiest from > > a kernel PoV. > > This can be done on bluetoothd side, the ugly part is just the wakeup > on every TX timestamp, which is every ~10ms in these use cases if every > packet is stamped. EPOLLET probably would indeed avoid busy looping on > the same timestamp though. > > In the first round of this patchset, this was handled on bluetoothd > side without kernel additions, with rate limiting the polling. If > POLL_ERRQUEUE sounds like a bad idea, maybe we can go back to that. We have prior art to avoid having timestamp completions on MSG_ERRQUEUE set sk_err and so block normal processing. Additional work on opting out of timestamp/zerocopy wake-ups sounds reasonable to me.
