Song, Yoong Siang wrote:
On Saturday, March 2, 2024 1:55 AM, John Fastabend john.fastabend@gmail.com wrote:
Song Yoong Siang wrote:
This patch adds support to per-packet Tx hardware timestamp request to AF_XDP zero-copy packet via XDP Tx metadata framework. Please note that user needs to enable Tx HW timestamp capability via igc_ioctl() with SIOCSHWTSTAMP cmd before sending xsk Tx hardware timestamp request.
Same as implementation in RX timestamp XDP hints kfunc metadata, Timer 0 (adjustable clock) is used in xsk Tx hardware timestamp. i225/i226 have four sets of timestamping registers. Both *skb and *xsk_tx_buffer pointers are used to indicate whether the timestamping register is already occupied.
Furthermore, a boolean variable named xsk_pending_ts is used to hold the transmit completion until the tx hardware timestamp is ready. This is because, for i225/i226, the timestamp notification event comes some time after the transmit completion event. The driver will retrigger hardware irq to clean the packet after retrieve the tx hardware timestamp.
Besides, xsk_meta is added into struct igc_tx_timestamp_request as a hook to the metadata location of the transmit packet. When the Tx timestamp interrupt is fired, the interrupt handler will copy the value of Tx hwts into metadata location via xsk_tx_metadata_complete().
Co-developed-by: Lai Peter Jun Ann jun.ann.lai@intel.com Signed-off-by: Lai Peter Jun Ann jun.ann.lai@intel.com Signed-off-by: Song Yoong Siang yoong.siang.song@intel.com
[...]
+static void igc_xsk_request_timestamp(void *_priv) +{
- struct igc_metadata_request *meta_req = _priv;
- struct igc_ring *tx_ring = meta_req->tx_ring;
- struct igc_tx_timestamp_request *tstamp;
- u32 tx_flags = IGC_TX_FLAGS_TSTAMP;
- struct igc_adapter *adapter;
- unsigned long lock_flags;
- bool found = false;
- int i;
- if (test_bit(IGC_RING_FLAG_TX_HWTSTAMP, &tx_ring->flags)) {
adapter = netdev_priv(tx_ring->netdev);spin_lock_irqsave(&adapter->ptp_tx_lock, lock_flags);/* Search for available tstamp regs */for (i = 0; i < IGC_MAX_TX_TSTAMP_REGS; i++) {tstamp = &adapter->tx_tstamp[i];if (tstamp->skb)continue;found = true;break;Not how I would have written this loop construct seems a bit odd to default break but it works.
Hi John, First of all, thank you for reviewing the patch. I agree that we can make the loop better. How about I change the loop to below:
That is more natural to me, but whatever reads best for you is probably ok.
for (i = 0; i < IGC_MAX_TX_TSTAMP_REGS; i++) { tstamp = &adapter->tx_tstamp[i];
if (!tstamp->skb) { found = true; break; } }
}/* Return if no available tstamp regs */if (!found) {adapter->tx_hwtstamp_skipped++;spin_unlock_irqrestore(&adapter->ptp_tx_lock,lock_flags);return;}[...]
+static void igc_ptp_free_tx_buffer(struct igc_adapter *adapter,
struct igc_tx_timestamp_request *tstamp)+{
- if (tstamp->buffer_type == IGC_TX_BUFFER_TYPE_XSK) {
/* Release the transmit completion */tstamp->xsk_tx_buffer->xsk_pending_ts = false;tstamp->xsk_tx_buffer = NULL;tstamp->buffer_type = 0;/* Trigger txrx interrupt for transmit completion */igc_xsk_wakeup(adapter->netdev, tstamp->xsk_queue_index, 0);Just curious because I didn't find it. Fairly sure I just need to look more, but don't you want to still 'tstamp->skb = NULL' in this path somewhere? It looks like triggering the tx interrupt again with buffer_type == 0 wouldn't do the null.
I suspect I just missed it.
Since the timestamp register will only be used by either skb or xsk, So we make tstamp->xsk_tx_buffer and tstamp->skb as union, Then based on tstamp->buffer_type to decide whether tstamp->xsk_tx_buffer or tstamp->skb should be used.
My thought is, by setting tstamp->xsk_tx_buffer=NULL, tstamp->skb will become NULL as well, and vice versa.
Seems good to me. Maybe a comment though? Otherwise I suspect next person to read the code will have to spend the extra time to track it down as well.
Thanks & Regards Siang
Also feel free to carry my ack into the v2 if you make the couple small nitpick changes.
Acked-by: John Fastabend john.fastabend@gmail.com