Stanislav Fomichev wrote: > On 12/05, Willem de Bruijn wrote: > > Stanislav Fomichev wrote: > > > On Tue, Dec 5, 2023 at 7:34 AM Florian Bezdeka > > > <florian.bezdeka@xxxxxxxxxxx> wrote: > > > > > > > > On Tue, 2023-12-05 at 15:25 +0000, Song, Yoong Siang wrote: > > > > > On Monday, December 4, 2023 10:55 PM, Willem de Bruijn wrote: > > > > > > Jesper Dangaard Brouer wrote: > > > > > > > > > > > > > > > > > > > > > On 12/3/23 17:51, Song Yoong Siang wrote: > > > > > > > > This patch enables Launch Time (Time-Based Scheduling) support to XDP zero > > > > > > > > copy via XDP Tx metadata framework. > > > > > > > > > > > > > > > > Signed-off-by: Song Yoong Siang<yoong.siang.song@xxxxxxxxx> > > > > > > > > --- > > > > > > > > drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++ > > > > > > > > > > > > > > As requested before, I think we need to see another driver implementing > > > > > > > this. > > > > > > > > > > > > > > I propose driver igc and chip i225. > > > > > > > > > > Sure. I will include igc patches in next version. > > > > > > > > > > > > > > > > > > > The interesting thing for me is to see how the LaunchTime max 1 second > > > > > > > into the future[1] is handled code wise. One suggestion is to add a > > > > > > > section to Documentation/networking/xsk-tx-metadata.rst per driver that > > > > > > > mentions/documents these different hardware limitations. It is natural > > > > > > > that different types of hardware have limitations. This is a close-to > > > > > > > hardware-level abstraction/API, and IMHO as long as we document the > > > > > > > limitations we can expose this API without too many limitations for more > > > > > > > capable hardware. > > > > > > > > > > Sure. I will try to add hardware limitations in documentation. > > > > > > > > > > > > > > > > > I would assume that the kfunc will fail when a value is passed that > > > > > > cannot be programmed. > > > > > > > > > > > > > > > > In current design, the xsk_tx_metadata_request() dint got return value. > > > > > So user won't know if their request is fail. > > > > > It is complex to inform user which request is failing. > > > > > Therefore, IMHO, it is good that we let driver handle the error silently. > > > > > > > > > > > > > If the programmed value is invalid, the packet will be "dropped" / will > > > > never make it to the wire, right? > > > > Programmable behavior is to either drop or cap to some boundary > > value, such as the farthest programmable time in the future: the > > horizon. In fq: > > > > /* Check if packet timestamp is too far in the future. */ > > if (fq_packet_beyond_horizon(skb, q, now)) { > > if (q->horizon_drop) { > > q->stat_horizon_drops++; > > return qdisc_drop(skb, sch, to_free); > > } > > q->stat_horizon_caps++; > > skb->tstamp = now + q->horizon; > > } > > fq_skb_cb(skb)->time_to_send = skb->tstamp; > > > > Drop is the more obviously correct mode. > > > > Programming with a clock source that the driver does not support will > > then be a persistent failure. > > > > Preferably, this driver capability can be queried beforehand (rather > > than only through reading error counters afterwards). > > > > Perhaps it should not be a driver task to convert from possibly > > multiple clock sources to the device native clock. Right now, we do > > use per-device timecounters for this, implemented in the driver. > > > > As for which clocks are relevant. For PTP, I suppose the device PHC, > > converted to nsec. For pacing offload, TCP uses CLOCK_MONOTONIC. > > Do we need to expose some generic netdev netlink apis to query/adjust > nic clock sources (or maybe there is something existing already)? > Then the userspace can be responsible for syncing/converting the > timestamps to the internal nic clocks. +1 to trying to avoid doing > this in the drivers. Perhaps. I'm just a bit hesitant since that is UAPI and this is all quite hand-wavy still. Some of the conversion necessarily has to be in the driver. Only the driver knows the descriptor format, and limitations of that, such as the bit-width that can be encoded. If we cannot move anything out of the drivers (quite likely), then agreed that a netdev/ethtool netlink query approach is helpful. To be clear: I don't mean that that should be part of this series. This is not an XSK specific concern. > > > > That is clearly a situation that the user should be informed about. For > > > > RT systems this normally means that something is really wrong regarding > > > > timing / cycle overflow. Such systems have to react on that situation. > > > > > > In general, af_xdp is a bit lacking in this 'notify the user that they > > > somehow messed up' area :-( > > > For example, pushing a tx descriptor with a wrong addr/len in zc mode > > > will not give any visible signal back (besides driver potentially > > > spilling something into dmesg as it was in the mlx case). > > > We can probably start with having some counters for these events? > > > > This is because the AF_XDP completion queue descriptor format is only > > a u64 address? > > Yeah. XDP_COPY mode has the descriptor validation which is exported via > recvmsg errno, but zerocopy path seems to be too deep in the stack > to report something back. And there is no place, as you mention, > in the completion ring to report the status. > > > Could error conditions be reported on tx completion in the metadata, > > using xsk_tx_metadata_complete? > > That would be one way to do it, yes. But then the error reporting depends > on the metadata opt-in. Having a separate ring to export the errors, > or having a v2 tx-completions layout with extra 'status' field would also > work. > > But this seems like something that should be handled separately? Because > we'd have to teach all existing zc drivers to report those errors back > instead of dropping these descriptors.. Agreed on both points :) A v2 tx-completions that supports status could be useful. But again, this is out of scope of this specific launch time feature.