* [xdp-hints] [PATCH bpf-next v2 0/3] xsk: TX metadata txtime support
@ 2023-12-01 6:24 Song Yoong Siang
2023-12-01 6:24 ` [xdp-hints] [PATCH bpf-next v2 1/3] xsk: add ETF support to XDP Tx metadata Song Yoong Siang
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Song Yoong Siang @ 2023-12-01 6:24 UTC (permalink / raw)
To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jonathan Corbet, Bjorn Topel, Magnus Karlsson,
Maciej Fijalkowski, Jonathan Lemon, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Stanislav Fomichev, Lorenzo Bianconi, Tariq Toukan,
Willem de Bruijn, Maxime Coquelin, Andrii Nakryiko,
Mykola Lysenko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Hao Luo, Jiri Olsa, Shuah Khan, Alexandre Torgue,
Jose Abreu
Cc: netdev, linux-kernel, linux-doc, bpf, xdp-hints, linux-stm32,
linux-arm-kernel, linux-kselftest, Song Yoong Siang
This series expands XDP TX metadata framework to include ETF HW offload.
Changes since v1:
- rename Time-Based Scheduling (TBS) to Earliest TxTime First (ETF)
- rename launch-time to txtime
v1: https://patchwork.kernel.org/project/netdevbpf/cover/20231130162028.852006-1-yoong.siang.song@intel.com/
Song Yoong Siang (3):
xsk: add ETF support to XDP Tx metadata
net: stmmac: Add txtime support to XDP ZC
selftests/bpf: Add txtime to xdp_hw_metadata
Documentation/netlink/specs/netdev.yaml | 4 ++++
Documentation/networking/xsk-tx-metadata.rst | 5 +++++
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++++++++++++
include/net/xdp_sock.h | 9 +++++++++
include/net/xdp_sock_drv.h | 1 +
include/uapi/linux/if_xdp.h | 9 +++++++++
include/uapi/linux/netdev.h | 3 +++
net/core/netdev-genl.c | 2 ++
net/xdp/xsk.c | 3 +++
tools/include/uapi/linux/if_xdp.h | 9 +++++++++
tools/include/uapi/linux/netdev.h | 3 +++
tools/net/ynl/generated/netdev-user.c | 1 +
tools/testing/selftests/bpf/xdp_hw_metadata.c | 18 +++++++++++++++++-
14 files changed, 81 insertions(+), 1 deletion(-)
--
2.34.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [xdp-hints] [PATCH bpf-next v2 1/3] xsk: add ETF support to XDP Tx metadata
2023-12-01 6:24 [xdp-hints] [PATCH bpf-next v2 0/3] xsk: TX metadata txtime support Song Yoong Siang
@ 2023-12-01 6:24 ` Song Yoong Siang
2023-12-01 6:24 ` [xdp-hints] [PATCH bpf-next v2 2/3] net: stmmac: Add txtime support to XDP ZC Song Yoong Siang
` (2 subsequent siblings)
3 siblings, 0 replies; 17+ messages in thread
From: Song Yoong Siang @ 2023-12-01 6:24 UTC (permalink / raw)
To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jonathan Corbet, Bjorn Topel, Magnus Karlsson,
Maciej Fijalkowski, Jonathan Lemon, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Stanislav Fomichev, Lorenzo Bianconi, Tariq Toukan,
Willem de Bruijn, Maxime Coquelin, Andrii Nakryiko,
Mykola Lysenko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Hao Luo, Jiri Olsa, Shuah Khan, Alexandre Torgue,
Jose Abreu
Cc: netdev, linux-kernel, linux-doc, bpf, xdp-hints, linux-stm32,
linux-arm-kernel, linux-kselftest, Song Yoong Siang
This patch extends the XDP Tx metadata framework to include Earliest
TxTime First (ETF) HW offload support where the NIC will schedule the
packet for transmission at a pre-determined time called txtime. The value
of txtime is communicated from user space to Ethernet driver via txtime
field of struct xsk_tx_metadata.
Suggested-by: Stanislav Fomichev <sdf@google.com>
Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com>
---
Documentation/netlink/specs/netdev.yaml | 4 ++++
Documentation/networking/xsk-tx-metadata.rst | 5 +++++
include/net/xdp_sock.h | 9 +++++++++
include/net/xdp_sock_drv.h | 1 +
include/uapi/linux/if_xdp.h | 9 +++++++++
include/uapi/linux/netdev.h | 3 +++
net/core/netdev-genl.c | 2 ++
net/xdp/xsk.c | 3 +++
tools/include/uapi/linux/if_xdp.h | 9 +++++++++
tools/include/uapi/linux/netdev.h | 3 +++
tools/net/ynl/generated/netdev-user.c | 1 +
11 files changed, 49 insertions(+)
diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
index 00439bcbd2e3..339cdcddbfc0 100644
--- a/Documentation/netlink/specs/netdev.yaml
+++ b/Documentation/netlink/specs/netdev.yaml
@@ -66,6 +66,10 @@ definitions:
name: tx-checksum
doc:
L3 checksum HW offload is supported by the driver.
+ -
+ name: txtime
+ doc:
+ Earliest TxTime First (ETF) HW offload is supported by the driver.
attribute-sets:
-
diff --git a/Documentation/networking/xsk-tx-metadata.rst b/Documentation/networking/xsk-tx-metadata.rst
index 97ecfa480d00..654ff692062a 100644
--- a/Documentation/networking/xsk-tx-metadata.rst
+++ b/Documentation/networking/xsk-tx-metadata.rst
@@ -44,6 +44,10 @@ The flags field enables the particular offload:
checksum. ``csum_start`` specifies byte offset of where the checksumming
should start and ``csum_offset`` specifies byte offset where the
device should store the computed checksum.
+- ``XDP_TXMD_FLAGS_TXTIME``: requests Earliest TxTime First (ETF) HW
+ offload to launch the packet at a pre-determined time. ``txtime``
+ indicates the time which the NIC should schedule the packet for
+ transmission.
Besides the flags above, in order to trigger the offloads, the first
packet's ``struct xdp_desc`` descriptor should set ``XDP_TX_METADATA``
@@ -68,6 +72,7 @@ Refer to ``xsk-flags`` features bitmask in
- ``tx-timestamp``: device supports ``XDP_TXMD_FLAGS_TIMESTAMP``
- ``tx-checksum``: device supports ``XDP_TXMD_FLAGS_CHECKSUM``
+- ``txtime``: device supports ``XDP_TXMD_FLAGS_TXTIME``
See ``tools/net/ynl/samples/netdev.c`` on how to query this information.
diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
index 3cb4dc9bd70e..0651b5264b72 100644
--- a/include/net/xdp_sock.h
+++ b/include/net/xdp_sock.h
@@ -110,11 +110,16 @@ struct xdp_sock {
* indicates position where checksumming should start.
* csum_offset indicates position where checksum should be stored.
*
+ * void (*tmo_request_txtime)(u64 txtime, void *priv)
+ * Called when AF_XDP frame requested Earliest TxTime First (ETF) HW offload
+ * support. txtime indicates the time which the NIC should schedule the
+ * packet for transmission.
*/
struct xsk_tx_metadata_ops {
void (*tmo_request_timestamp)(void *priv);
u64 (*tmo_fill_timestamp)(void *priv);
void (*tmo_request_checksum)(u16 csum_start, u16 csum_offset, void *priv);
+ void (*tmo_request_txtime)(u64 txtime, void *priv);
};
#ifdef CONFIG_XDP_SOCKETS
@@ -170,6 +175,10 @@ static inline void xsk_tx_metadata_request(const struct xsk_tx_metadata *meta,
if (meta->flags & XDP_TXMD_FLAGS_CHECKSUM)
ops->tmo_request_checksum(meta->request.csum_start,
meta->request.csum_offset, priv);
+
+ if (ops->tmo_request_txtime)
+ if (meta->flags & XDP_TXMD_FLAGS_TXTIME)
+ ops->tmo_request_txtime(meta->request.txtime, priv);
}
/**
diff --git a/include/net/xdp_sock_drv.h b/include/net/xdp_sock_drv.h
index 81e02de3f453..e66d597e7079 100644
--- a/include/net/xdp_sock_drv.h
+++ b/include/net/xdp_sock_drv.h
@@ -168,6 +168,7 @@ static inline void *xsk_buff_raw_get_data(struct xsk_buff_pool *pool, u64 addr)
#define XDP_TXMD_FLAGS_VALID ( \
XDP_TXMD_FLAGS_TIMESTAMP | \
XDP_TXMD_FLAGS_CHECKSUM | \
+ XDP_TXMD_FLAGS_TXTIME | \
0)
static inline bool xsk_buff_valid_tx_metadata(struct xsk_tx_metadata *meta)
diff --git a/include/uapi/linux/if_xdp.h b/include/uapi/linux/if_xdp.h
index d31698410410..24d123bce7f3 100644
--- a/include/uapi/linux/if_xdp.h
+++ b/include/uapi/linux/if_xdp.h
@@ -123,6 +123,12 @@ struct xdp_options {
*/
#define XDP_TXMD_FLAGS_CHECKSUM (1 << 1)
+/* Request Earliest TxTime First (ETF) HW offload to launch the packet at a
+ * pre-determined time. The time which the NIC should schedule the packet for
+ * transmission is communicated via txtime field of struct xsk_tx_metadata.
+ */
+#define XDP_TXMD_FLAGS_TXTIME (1 << 2)
+
/* AF_XDP offloads request. 'request' union member is consumed by the driver
* when the packet is being transmitted. 'completion' union member is
* filled by the driver when the transmit completion arrives.
@@ -138,6 +144,9 @@ struct xsk_tx_metadata {
__u16 csum_start;
/* Offset from csum_start where checksum should be stored. */
__u16 csum_offset;
+
+ /* XDP_TXMD_FLAGS_TXTIME */
+ __u64 txtime;
} request;
struct {
diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
index 48d5477a668c..03b913757e1c 100644
--- a/include/uapi/linux/netdev.h
+++ b/include/uapi/linux/netdev.h
@@ -59,10 +59,13 @@ enum netdev_xdp_rx_metadata {
* by the driver.
* @NETDEV_XSK_FLAGS_TX_CHECKSUM: L3 checksum HW offload is supported by the
* driver.
+ * @NETDEV_XSK_FLAGS_TXTIME: Earliest TxTime First (ETF) HW offload is supported
+ * by the driver.
*/
enum netdev_xsk_flags {
NETDEV_XSK_FLAGS_TX_TIMESTAMP = 1,
NETDEV_XSK_FLAGS_TX_CHECKSUM = 2,
+ NETDEV_XSK_FLAGS_TXTIME = 3,
/* private: */
NETDEV_XSK_FLAGS_MASK = 3,
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 10f2124e9e23..aecc6e26f839 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -33,6 +33,8 @@ XDP_METADATA_KFUNC_xxx
xsk_features |= NETDEV_XSK_FLAGS_TX_TIMESTAMP;
if (netdev->xsk_tx_metadata_ops->tmo_request_checksum)
xsk_features |= NETDEV_XSK_FLAGS_TX_CHECKSUM;
+ if (netdev->xsk_tx_metadata_ops->tmo_request_txtime)
+ xsk_features |= NETDEV_XSK_FLAGS_TXTIME;
}
if (nla_put_u32(rsp, NETDEV_A_DEV_IFINDEX, netdev->ifindex) ||
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 281d49b4fca4..4d48fc6caf7c 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -751,6 +751,9 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
goto free_err;
}
}
+
+ if (meta->flags & XDP_TXMD_FLAGS_TXTIME)
+ skb->skb_mstamp_ns = meta->request.txtime;
}
}
diff --git a/tools/include/uapi/linux/if_xdp.h b/tools/include/uapi/linux/if_xdp.h
index 638c606dfa74..abcf79e8a879 100644
--- a/tools/include/uapi/linux/if_xdp.h
+++ b/tools/include/uapi/linux/if_xdp.h
@@ -123,6 +123,12 @@ struct xdp_options {
*/
#define XDP_TXMD_FLAGS_CHECKSUM (1 << 1)
+/* Request Earliest TxTime First (ETF) HW offload to launch the packet at a
+ * pre-determined time. The time which the NIC should schedule the packet for
+ * transmission is communicated via txtime field of struct xsk_tx_metadata.
+ */
+#define XDP_TXMD_FLAGS_TXTIME (1 << 2)
+
/* AF_XDP offloads request. 'request' union member is consumed by the driver
* when the packet is being transmitted. 'completion' union member is
* filled by the driver when the transmit completion arrives.
@@ -138,6 +144,9 @@ struct xsk_tx_metadata {
__u16 csum_start;
/* Offset from csum_start where checksum should be stored. */
__u16 csum_offset;
+
+ /* XDP_TXMD_FLAGS_TXTIME */
+ __u64 txtime;
} request;
struct {
diff --git a/tools/include/uapi/linux/netdev.h b/tools/include/uapi/linux/netdev.h
index 48d5477a668c..03b913757e1c 100644
--- a/tools/include/uapi/linux/netdev.h
+++ b/tools/include/uapi/linux/netdev.h
@@ -59,10 +59,13 @@ enum netdev_xdp_rx_metadata {
* by the driver.
* @NETDEV_XSK_FLAGS_TX_CHECKSUM: L3 checksum HW offload is supported by the
* driver.
+ * @NETDEV_XSK_FLAGS_TXTIME: Earliest TxTime First (ETF) HW offload is supported
+ * by the driver.
*/
enum netdev_xsk_flags {
NETDEV_XSK_FLAGS_TX_TIMESTAMP = 1,
NETDEV_XSK_FLAGS_TX_CHECKSUM = 2,
+ NETDEV_XSK_FLAGS_TXTIME = 3,
/* private: */
NETDEV_XSK_FLAGS_MASK = 3,
diff --git a/tools/net/ynl/generated/netdev-user.c b/tools/net/ynl/generated/netdev-user.c
index 6283d87dad37..02f1bd4cd97b 100644
--- a/tools/net/ynl/generated/netdev-user.c
+++ b/tools/net/ynl/generated/netdev-user.c
@@ -61,6 +61,7 @@ const char *netdev_xdp_rx_metadata_str(enum netdev_xdp_rx_metadata value)
static const char * const netdev_xsk_flags_strmap[] = {
[0] = "tx-timestamp",
[1] = "tx-checksum",
+ [2] = "txtime"
};
const char *netdev_xsk_flags_str(enum netdev_xsk_flags value)
--
2.34.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [xdp-hints] [PATCH bpf-next v2 2/3] net: stmmac: Add txtime support to XDP ZC
2023-12-01 6:24 [xdp-hints] [PATCH bpf-next v2 0/3] xsk: TX metadata txtime support Song Yoong Siang
2023-12-01 6:24 ` [xdp-hints] [PATCH bpf-next v2 1/3] xsk: add ETF support to XDP Tx metadata Song Yoong Siang
@ 2023-12-01 6:24 ` Song Yoong Siang
2023-12-01 15:02 ` [xdp-hints] " Jesper Dangaard Brouer
2023-12-01 6:24 ` [xdp-hints] [PATCH bpf-next v2 3/3] selftests/bpf: Add txtime to xdp_hw_metadata Song Yoong Siang
2023-12-01 10:46 ` [xdp-hints] Re: [PATCH bpf-next v2 0/3] xsk: TX metadata txtime support Jesper Dangaard Brouer
3 siblings, 1 reply; 17+ messages in thread
From: Song Yoong Siang @ 2023-12-01 6:24 UTC (permalink / raw)
To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jonathan Corbet, Bjorn Topel, Magnus Karlsson,
Maciej Fijalkowski, Jonathan Lemon, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Stanislav Fomichev, Lorenzo Bianconi, Tariq Toukan,
Willem de Bruijn, Maxime Coquelin, Andrii Nakryiko,
Mykola Lysenko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Hao Luo, Jiri Olsa, Shuah Khan, Alexandre Torgue,
Jose Abreu
Cc: netdev, linux-kernel, linux-doc, bpf, xdp-hints, linux-stm32,
linux-arm-kernel, linux-kselftest, Song Yoong Siang
This patch enables txtime support to XDP zero copy via XDP Tx
metadata framework.
Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++++++++++++
2 files changed, 15 insertions(+)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 686c94c2e8a7..e8538af6e207 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -105,6 +105,8 @@ struct stmmac_metadata_request {
struct stmmac_priv *priv;
struct dma_desc *tx_desc;
bool *set_ic;
+ struct dma_edesc *edesc;
+ int tbs;
};
struct stmmac_xsk_tx_complete {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index c2ac88aaffed..c7b9338be9e0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2465,9 +2465,20 @@ static u64 stmmac_xsk_fill_timestamp(void *_priv)
return 0;
}
+static void stmmac_xsk_request_txtime(u64 txtime, void *_priv)
+{
+ struct stmmac_metadata_request *meta_req = _priv;
+ struct timespec64 ts = ns_to_timespec64(txtime);
+
+ if (meta_req->tbs & STMMAC_TBS_EN)
+ stmmac_set_desc_tbs(meta_req->priv, meta_req->edesc, ts.tv_sec,
+ ts.tv_nsec);
+}
+
static const struct xsk_tx_metadata_ops stmmac_xsk_tx_metadata_ops = {
.tmo_request_timestamp = stmmac_xsk_request_timestamp,
.tmo_fill_timestamp = stmmac_xsk_fill_timestamp,
+ .tmo_request_txtime = stmmac_xsk_request_txtime,
};
static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
@@ -2545,6 +2556,8 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
meta_req.priv = priv;
meta_req.tx_desc = tx_desc;
meta_req.set_ic = &set_ic;
+ meta_req.tbs = tx_q->tbs;
+ meta_req.edesc = &tx_q->dma_entx[entry];
xsk_tx_metadata_request(meta, &stmmac_xsk_tx_metadata_ops,
&meta_req);
if (set_ic) {
--
2.34.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [xdp-hints] [PATCH bpf-next v2 3/3] selftests/bpf: Add txtime to xdp_hw_metadata
2023-12-01 6:24 [xdp-hints] [PATCH bpf-next v2 0/3] xsk: TX metadata txtime support Song Yoong Siang
2023-12-01 6:24 ` [xdp-hints] [PATCH bpf-next v2 1/3] xsk: add ETF support to XDP Tx metadata Song Yoong Siang
2023-12-01 6:24 ` [xdp-hints] [PATCH bpf-next v2 2/3] net: stmmac: Add txtime support to XDP ZC Song Yoong Siang
@ 2023-12-01 6:24 ` Song Yoong Siang
2023-12-01 10:46 ` [xdp-hints] Re: [PATCH bpf-next v2 0/3] xsk: TX metadata txtime support Jesper Dangaard Brouer
3 siblings, 0 replies; 17+ messages in thread
From: Song Yoong Siang @ 2023-12-01 6:24 UTC (permalink / raw)
To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jonathan Corbet, Bjorn Topel, Magnus Karlsson,
Maciej Fijalkowski, Jonathan Lemon, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Stanislav Fomichev, Lorenzo Bianconi, Tariq Toukan,
Willem de Bruijn, Maxime Coquelin, Andrii Nakryiko,
Mykola Lysenko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Hao Luo, Jiri Olsa, Shuah Khan, Alexandre Torgue,
Jose Abreu
Cc: netdev, linux-kernel, linux-doc, bpf, xdp-hints, linux-stm32,
linux-arm-kernel, linux-kselftest, Song Yoong Siang
This patch adds txtime support to xdp_hw_metadata. User can configure the
delta of HW txtime to HW RX-time by using "-l" argument. The default delta
is set to 1 second.
This patch is tested with stmmac on Intel Tiger Lake platform. Refer to
result below, the delta between pre-determined ETF txtime and actual HW
transmit complete time is around 24 us.
$ sudo ./xdp_hw_metadata eth0
...
xsk_ring_cons__peek: 1
0x55fcb80ce7a8: rx_desc[0]->addr=80100 addr=80100 comp_addr=80100 EoP
No rx_hash err=-95
HW RX-time: 1677764507059055964 (sec:1677764507.0591) delta to User RX-time sec:0.0002 (237.548 usec)
XDP RX-time: 1677764507059280741 (sec:1677764507.0593) delta to User RX-time sec:0.0000 (12.771 usec)
0x55fcb80ce7a8: ping-pong with csum=5619 (want 8626) csum_start=34 csum_offset=6
HW RX-time: 1677764507059055964 (sec:1677764507.0591) delta to HW Txtime sec:1.0000 (1000000.000 usec)
0x55fcb80ce7a8: complete tx idx=0 addr=18
HW Txtime: 1677764508059055964 (sec:1677764508.0591) delta to HW TX-complete-time sec:0.0000 (24.235 usec)
HW TX-complete-time: 1677764508059080199 (sec:1677764508.0591) delta to User TX-complete-time sec:0.0054 (5423.263 usec)
XDP RX-time: 1677764507059280741 (sec:1677764507.0593) delta to User TX-complete-time sec:1.0052 (1005222.721 usec)
HW RX-time: 1677764507059055964 (sec:1677764507.0591) delta to HW TX-complete-time sec:1.0000 (1000024.235 usec)
0x55fcb80ce7a8: complete rx idx=128 addr=80100
$ sudo ./xdp_hw_metadata eth0 -l 10000000
...
xsk_ring_cons__peek: 1
0x5626d54de7a8: rx_desc[0]->addr=80100 addr=80100 comp_addr=80100 EoP
No rx_hash err=-95
HW RX-time: 1677764655807717783 (sec:1677764655.8077) delta to User RX-time sec:0.0002 (240.571 usec)
XDP RX-time: 1677764655807942983 (sec:1677764655.8079) delta to User RX-time sec:0.0000 (15.371 usec)
0x5626d54de7a8: ping-pong with csum=5619 (want 8626) csum_start=34 csum_offset=6
HW RX-time: 1677764655807717783 (sec:1677764655.8077) delta to HW Txtime sec:0.0100 (10000.000 usec)
0x5626d54de7a8: complete tx idx=0 addr=18
HW Txtime: 1677764655817717783 (sec:1677764655.8177) delta to HW TX-complete-time sec:0.0000 (23.965 usec)
HW TX-complete-time: 1677764655817741748 (sec:1677764655.8177) delta to User TX-complete-time sec:0.0003 (291.792 usec)
XDP RX-time: 1677764655807942983 (sec:1677764655.8079) delta to User TX-complete-time sec:0.0101 (10090.557 usec)
HW RX-time: 1677764655807717783 (sec:1677764655.8077) delta to HW TX-complete-time sec:0.0100 (10023.965 usec)
0x5626d54de7a8: complete rx idx=128 addr=80100
Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com>
---
tools/testing/selftests/bpf/xdp_hw_metadata.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/xdp_hw_metadata.c b/tools/testing/selftests/bpf/xdp_hw_metadata.c
index 3291625ba4fb..e9c3e29dc538 100644
--- a/tools/testing/selftests/bpf/xdp_hw_metadata.c
+++ b/tools/testing/selftests/bpf/xdp_hw_metadata.c
@@ -13,6 +13,7 @@
* - UDP 9091 packets trigger TX reply
* - TX HW timestamp is requested and reported back upon completion
* - TX checksum is requested
+ * - HW txtime is set for transmission
*/
#include <test_progs.h>
@@ -61,6 +62,8 @@ int rxq;
bool skip_tx;
__u64 last_hw_rx_timestamp;
__u64 last_xdp_rx_timestamp;
+__u64 last_txtime;
+__u64 txtime_delta_to_hw_rx_timestamp = 1000000000; /* 1 second */
void test__fail(void) { /* for network_helpers.c */ }
@@ -274,6 +277,8 @@ static bool complete_tx(struct xsk *xsk, clockid_t clock_id)
if (meta->completion.tx_timestamp) {
__u64 ref_tstamp = gettime(clock_id);
+ print_tstamp_delta("HW Txtime", "HW TX-complete-time",
+ last_txtime, meta->completion.tx_timestamp);
print_tstamp_delta("HW TX-complete-time", "User TX-complete-time",
meta->completion.tx_timestamp, ref_tstamp);
print_tstamp_delta("XDP RX-time", "User TX-complete-time",
@@ -371,6 +376,13 @@ static void ping_pong(struct xsk *xsk, void *rx_packet, clockid_t clock_id)
xsk, ntohs(udph->check), ntohs(want_csum),
meta->request.csum_start, meta->request.csum_offset);
+ /* Set txtime for Earliest TxTime First (ETF) */
+ meta->flags |= XDP_TXMD_FLAGS_TXTIME;
+ meta->request.txtime = last_hw_rx_timestamp + txtime_delta_to_hw_rx_timestamp;
+ last_txtime = meta->request.txtime;
+ print_tstamp_delta("HW RX-time", "HW Txtime", last_hw_rx_timestamp,
+ meta->request.txtime);
+
memcpy(data, rx_packet, len); /* don't share umem chunk for simplicity */
tx_desc->options |= XDP_TX_METADATA;
tx_desc->len = len;
@@ -595,6 +607,7 @@ static void print_usage(void)
" -h Display this help and exit\n\n"
" -m Enable multi-buffer XDP for larger MTU\n"
" -r Don't generate AF_XDP reply (rx metadata only)\n"
+ " -l Delta of HW Txtime to HW RX-time in ns (default: 1s)\n"
"Generate test packets on the other machine with:\n"
" echo -n xdp | nc -u -q1 <dst_ip> 9091\n";
@@ -605,7 +618,7 @@ static void read_args(int argc, char *argv[])
{
int opt;
- while ((opt = getopt(argc, argv, "chmr")) != -1) {
+ while ((opt = getopt(argc, argv, "chmrl:")) != -1) {
switch (opt) {
case 'c':
bind_flags &= ~XDP_USE_NEED_WAKEUP;
@@ -621,6 +634,9 @@ static void read_args(int argc, char *argv[])
case 'r':
skip_tx = true;
break;
+ case 'l':
+ txtime_delta_to_hw_rx_timestamp = atoll(optarg);
+ break;
case '?':
if (isprint(optopt))
fprintf(stderr, "Unknown option: -%c\n", optopt);
--
2.34.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [xdp-hints] Re: [PATCH bpf-next v2 0/3] xsk: TX metadata txtime support
2023-12-01 6:24 [xdp-hints] [PATCH bpf-next v2 0/3] xsk: TX metadata txtime support Song Yoong Siang
` (2 preceding siblings ...)
2023-12-01 6:24 ` [xdp-hints] [PATCH bpf-next v2 3/3] selftests/bpf: Add txtime to xdp_hw_metadata Song Yoong Siang
@ 2023-12-01 10:46 ` Jesper Dangaard Brouer
2023-12-01 13:43 ` Song, Yoong Siang
3 siblings, 1 reply; 17+ messages in thread
From: Jesper Dangaard Brouer @ 2023-12-01 10:46 UTC (permalink / raw)
To: Song Yoong Siang, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Jonathan Corbet, Bjorn Topel, Magnus Karlsson,
Maciej Fijalkowski, Jonathan Lemon, Alexei Starovoitov,
Daniel Borkmann, John Fastabend, Stanislav Fomichev,
Lorenzo Bianconi, Tariq Toukan, Willem de Bruijn,
Maxime Coquelin, Andrii Nakryiko, Mykola Lysenko,
Martin KaFai Lau, Song Liu, Yonghong Song, KP Singh, Hao Luo,
Jiri Olsa, Shuah Khan, Alexandre Torgue, Jose Abreu
Cc: netdev, linux-kernel, linux-doc, bpf, xdp-hints, linux-stm32,
linux-arm-kernel, linux-kselftest
On 12/1/23 07:24, Song Yoong Siang wrote:
> This series expands XDP TX metadata framework to include ETF HW offload.
>
> Changes since v1:
> - rename Time-Based Scheduling (TBS) to Earliest TxTime First (ETF)
> - rename launch-time to txtime
>
I strongly disagree with this renaming (sorry to disagree with Willem).
The i210 and i225 chips call this LaunchTime in their programmers
datasheets, and even in the driver code[1].
Using this "txtime" name in the code is also confusing, because how can
people reading the code know the difference between:
- tmo_request_timestamp and tmo_request_txtime
[1]
https://github.com/xdp-project/xdp-project/blob/master/areas/tsn/code01_follow_qdisc_TSN_offload.org
> v1: https://patchwork.kernel.org/project/netdevbpf/cover/20231130162028.852006-1-yoong.siang.song@intel.com/
>
> Song Yoong Siang (3):
> xsk: add ETF support to XDP Tx metadata
> net: stmmac: Add txtime support to XDP ZC
> selftests/bpf: Add txtime to xdp_hw_metadata
>
> Documentation/netlink/specs/netdev.yaml | 4 ++++
> Documentation/networking/xsk-tx-metadata.rst | 5 +++++
> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++
> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++++++++++++
> include/net/xdp_sock.h | 9 +++++++++
> include/net/xdp_sock_drv.h | 1 +
> include/uapi/linux/if_xdp.h | 9 +++++++++
> include/uapi/linux/netdev.h | 3 +++
> net/core/netdev-genl.c | 2 ++
> net/xdp/xsk.c | 3 +++
> tools/include/uapi/linux/if_xdp.h | 9 +++++++++
> tools/include/uapi/linux/netdev.h | 3 +++
> tools/net/ynl/generated/netdev-user.c | 1 +
> tools/testing/selftests/bpf/xdp_hw_metadata.c | 18 +++++++++++++++++-
> 14 files changed, 81 insertions(+), 1 deletion(-)
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [xdp-hints] Re: [PATCH bpf-next v2 0/3] xsk: TX metadata txtime support
2023-12-01 10:46 ` [xdp-hints] Re: [PATCH bpf-next v2 0/3] xsk: TX metadata txtime support Jesper Dangaard Brouer
@ 2023-12-01 13:43 ` Song, Yoong Siang
2023-12-01 15:09 ` Willem de Bruijn
0 siblings, 1 reply; 17+ messages in thread
From: Song, Yoong Siang @ 2023-12-01 13:43 UTC (permalink / raw)
To: Jesper Dangaard Brouer, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Jonathan Corbet, Bjorn Topel,
Karlsson, Magnus, Fijalkowski, Maciej, Jonathan Lemon,
Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Stanislav Fomichev, Lorenzo Bianconi, Tariq Toukan,
Willem de Bruijn, Maxime Coquelin, Andrii Nakryiko,
Mykola Lysenko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Hao Luo, Jiri Olsa, Shuah Khan, Alexandre Torgue,
Jose Abreu
Cc: netdev, linux-kernel, linux-doc, bpf, xdp-hints, linux-stm32,
linux-arm-kernel, linux-kselftest
On Friday, December 1, 2023 6:46 PM, Jesper Dangaard Brouer <hawk@kernel.org> wrote:
>On 12/1/23 07:24, Song Yoong Siang wrote:
>> This series expands XDP TX metadata framework to include ETF HW offload.
>>
>> Changes since v1:
>> - rename Time-Based Scheduling (TBS) to Earliest TxTime First (ETF)
>> - rename launch-time to txtime
>>
>
>I strongly disagree with this renaming (sorry to disagree with Willem).
>
>The i210 and i225 chips call this LaunchTime in their programmers
>datasheets, and even in the driver code[1].
>
>Using this "txtime" name in the code is also confusing, because how can
>people reading the code know the difference between:
> - tmo_request_timestamp and tmo_request_txtime
>
Hi Jesper and Willem,
How about using "launch_time" for the flag/variable and
"Earliest TxTime First" for the description/comments?
Thanks & Regards
Siang
>
>[1]
>https://github.com/xdp-project/xdp-
>project/blob/master/areas/tsn/code01_follow_qdisc_TSN_offload.org
>
>> v1:
>https://patchwork.kernel.org/project/netdevbpf/cover/20231130162028.852006-1-
>yoong.siang.song@intel.com/
>>
>> Song Yoong Siang (3):
>> xsk: add ETF support to XDP Tx metadata
>> net: stmmac: Add txtime support to XDP ZC
>> selftests/bpf: Add txtime to xdp_hw_metadata
>>
>> Documentation/netlink/specs/netdev.yaml | 4 ++++
>> Documentation/networking/xsk-tx-metadata.rst | 5 +++++
>> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++
>> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++++++++++++
>> include/net/xdp_sock.h | 9 +++++++++
>> include/net/xdp_sock_drv.h | 1 +
>> include/uapi/linux/if_xdp.h | 9 +++++++++
>> include/uapi/linux/netdev.h | 3 +++
>> net/core/netdev-genl.c | 2 ++
>> net/xdp/xsk.c | 3 +++
>> tools/include/uapi/linux/if_xdp.h | 9 +++++++++
>> tools/include/uapi/linux/netdev.h | 3 +++
>> tools/net/ynl/generated/netdev-user.c | 1 +
>> tools/testing/selftests/bpf/xdp_hw_metadata.c | 18 +++++++++++++++++-
>> 14 files changed, 81 insertions(+), 1 deletion(-)
>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [xdp-hints] Re: [PATCH bpf-next v2 2/3] net: stmmac: Add txtime support to XDP ZC
2023-12-01 6:24 ` [xdp-hints] [PATCH bpf-next v2 2/3] net: stmmac: Add txtime support to XDP ZC Song Yoong Siang
@ 2023-12-01 15:02 ` Jesper Dangaard Brouer
2023-12-01 15:26 ` Willem de Bruijn
2023-12-03 10:11 ` Song, Yoong Siang
0 siblings, 2 replies; 17+ messages in thread
From: Jesper Dangaard Brouer @ 2023-12-01 15:02 UTC (permalink / raw)
To: Song Yoong Siang, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Jonathan Corbet, Bjorn Topel, Magnus Karlsson,
Maciej Fijalkowski, Jonathan Lemon, Alexei Starovoitov,
Daniel Borkmann, John Fastabend, Stanislav Fomichev,
Lorenzo Bianconi, Tariq Toukan, Willem de Bruijn,
Maxime Coquelin, Andrii Nakryiko, Mykola Lysenko,
Martin KaFai Lau, Song Liu, Yonghong Song, KP Singh, Hao Luo,
Jiri Olsa, Shuah Khan, Alexandre Torgue, Jose Abreu
Cc: netdev, linux-kernel, linux-doc, bpf, xdp-hints, linux-stm32,
linux-arm-kernel, linux-kselftest
On 12/1/23 07:24, Song Yoong Siang wrote:
> This patch enables txtime support to XDP zero copy via XDP Tx
> metadata framework.
>
> Signed-off-by: Song Yoong Siang<yoong.siang.song@intel.com>
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++++++++++++
> 2 files changed, 15 insertions(+)
I think we need to see other drivers using this new feature to evaluate
if API is sane.
I suggest implementing this for igc driver (chip i225) and also for igb
(i210 chip) that both support this kind of LaunchTime feature in HW.
The API and stmmac driver takes a u64 as time.
I'm wondering how this applies to i210 that[1] have 25-bit for
LaunchTime (with 32 nanosec granularity) limiting LaunchTime max 0.5
second into the future.
And i225 that [1] have 30-bit max 1 second into the future.
[1]
https://github.com/xdp-project/xdp-project/blob/master/areas/tsn/code01_follow_qdisc_TSN_offload.org
^ permalink raw reply [flat|nested] 17+ messages in thread
* [xdp-hints] Re: [PATCH bpf-next v2 0/3] xsk: TX metadata txtime support
2023-12-01 13:43 ` Song, Yoong Siang
@ 2023-12-01 15:09 ` Willem de Bruijn
2023-12-01 15:39 ` Jesper Dangaard Brouer
0 siblings, 1 reply; 17+ messages in thread
From: Willem de Bruijn @ 2023-12-01 15:09 UTC (permalink / raw)
To: Song, Yoong Siang, Jesper Dangaard Brouer, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Jonathan Corbet,
Bjorn Topel, Karlsson, Magnus, Fijalkowski, Maciej,
Jonathan Lemon, Alexei Starovoitov, Daniel Borkmann,
John Fastabend, Stanislav Fomichev, Lorenzo Bianconi,
Tariq Toukan, Willem de Bruijn, Maxime Coquelin, Andrii Nakryiko,
Mykola Lysenko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Hao Luo, Jiri Olsa, Shuah Khan, Alexandre Torgue,
Jose Abreu
Cc: netdev, linux-kernel, linux-doc, bpf, xdp-hints, linux-stm32,
linux-arm-kernel, linux-kselftest
Song, Yoong Siang wrote:
> On Friday, December 1, 2023 6:46 PM, Jesper Dangaard Brouer <hawk@kernel.org> wrote:
> >On 12/1/23 07:24, Song Yoong Siang wrote:
> >> This series expands XDP TX metadata framework to include ETF HW offload.
> >>
> >> Changes since v1:
> >> - rename Time-Based Scheduling (TBS) to Earliest TxTime First (ETF)
> >> - rename launch-time to txtime
> >>
> >
> >I strongly disagree with this renaming (sorry to disagree with Willem).
> >
> >The i210 and i225 chips call this LaunchTime in their programmers
> >datasheets, and even in the driver code[1].
> >
> >Using this "txtime" name in the code is also confusing, because how can
> >people reading the code know the difference between:
> > - tmo_request_timestamp and tmo_request_txtime
> >
>
> Hi Jesper and Willem,
>
> How about using "launch_time" for the flag/variable and
> "Earliest TxTime First" for the description/comments?
I don't particularly care which term we use, as long as we're
consistent. Especially, don't keep introducing new synonyms.
The fact that one happens to be one vendor's marketing term does not
make it preferable, IMHO. On the contrary.
SO_TXTIME is in the ABI, and EDT has been used publicly in kernel
patches and conference talks, e.g., Van Jacobson's Netdev 0x12
keynote. Those are vendor agnostic commonly used terms.
But as long as Launch Time is not an Intel only trademark, fine to
select that.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [xdp-hints] Re: [PATCH bpf-next v2 2/3] net: stmmac: Add txtime support to XDP ZC
2023-12-01 15:02 ` [xdp-hints] " Jesper Dangaard Brouer
@ 2023-12-01 15:26 ` Willem de Bruijn
2023-12-03 10:11 ` Song, Yoong Siang
1 sibling, 0 replies; 17+ messages in thread
From: Willem de Bruijn @ 2023-12-01 15:26 UTC (permalink / raw)
To: Jesper Dangaard Brouer, Song Yoong Siang, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Jonathan Corbet,
Bjorn Topel, Magnus Karlsson, Maciej Fijalkowski, Jonathan Lemon,
Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Stanislav Fomichev, Lorenzo Bianconi, Tariq Toukan,
Willem de Bruijn, Maxime Coquelin, Andrii Nakryiko,
Mykola Lysenko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Hao Luo, Jiri Olsa, Shuah Khan, Alexandre Torgue,
Jose Abreu
Cc: netdev, linux-kernel, linux-doc, bpf, xdp-hints, linux-stm32,
linux-arm-kernel, linux-kselftest
Jesper Dangaard Brouer wrote:
>
>
> On 12/1/23 07:24, Song Yoong Siang wrote:
> > This patch enables txtime support to XDP zero copy via XDP Tx
> > metadata framework.
> >
> > Signed-off-by: Song Yoong Siang<yoong.siang.song@intel.com>
> > ---
> > drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++
> > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++++++++++++
> > 2 files changed, 15 insertions(+)
>
> I think we need to see other drivers using this new feature to evaluate
> if API is sane.
>
> I suggest implementing this for igc driver (chip i225) and also for igb
> (i210 chip) that both support this kind of LaunchTime feature in HW.
>
> The API and stmmac driver takes a u64 as time.
> I'm wondering how this applies to i210 that[1] have 25-bit for
> LaunchTime (with 32 nanosec granularity) limiting LaunchTime max 0.5
> second into the future.
> And i225 that [1] have 30-bit max 1 second into the future.
>
>
> [1]
> https://github.com/xdp-project/xdp-project/blob/master/areas/tsn/code01_follow_qdisc_TSN_offload.org
Good point Jesper.
Can we also explicitly document what the type of the field is?
Nanoseconds against the NIC hardware clock, it sounds like.
We have some experience with this, too. Something needs to do the
conversion from host clock to NIC clock. It is not sufficent to just
assume that the host clock is synced against the NIC clock by PTP.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [xdp-hints] Re: [PATCH bpf-next v2 0/3] xsk: TX metadata txtime support
2023-12-01 15:09 ` Willem de Bruijn
@ 2023-12-01 15:39 ` Jesper Dangaard Brouer
2023-12-02 14:15 ` Willem de Bruijn
0 siblings, 1 reply; 17+ messages in thread
From: Jesper Dangaard Brouer @ 2023-12-01 15:39 UTC (permalink / raw)
To: Willem de Bruijn, Song, Yoong Siang, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Jonathan Corbet,
Bjorn Topel, Karlsson, Magnus, Fijalkowski, Maciej,
Jonathan Lemon, Alexei Starovoitov, Daniel Borkmann,
John Fastabend, Stanislav Fomichev, Lorenzo Bianconi,
Tariq Toukan, Willem de Bruijn, Maxime Coquelin, Andrii Nakryiko,
Mykola Lysenko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Hao Luo, Jiri Olsa, Shuah Khan, Alexandre Torgue,
Jose Abreu, Andre Fredette
Cc: netdev, linux-kernel, linux-doc, bpf, xdp-hints, linux-stm32,
linux-arm-kernel, linux-kselftest
On 12/1/23 16:09, Willem de Bruijn wrote:
> Song, Yoong Siang wrote:
>> On Friday, December 1, 2023 6:46 PM, Jesper Dangaard Brouer <hawk@kernel.org> wrote:
>>> On 12/1/23 07:24, Song Yoong Siang wrote:
>>>> This series expands XDP TX metadata framework to include ETF HW offload.
>>>>
>>>> Changes since v1:
>>>> - rename Time-Based Scheduling (TBS) to Earliest TxTime First (ETF)
>>>> - rename launch-time to txtime
>>>>
>>>
>>> I strongly disagree with this renaming (sorry to disagree with Willem).
>>>
>>> The i210 and i225 chips call this LaunchTime in their programmers
>>> datasheets, and even in the driver code[1].
>>>
>>> Using this "txtime" name in the code is also confusing, because how can
>>> people reading the code know the difference between:
>>> - tmo_request_timestamp and tmo_request_txtime
>>>
>>
>> Hi Jesper and Willem,
>>
>> How about using "launch_time" for the flag/variable and
>> "Earliest TxTime First" for the description/comments?
>
I don't follow why you are calling the feature:
- "Earliest TxTime First" (ETF).
- AFAIK this just reference an qdisc name (that most don't know exists)
> I don't particularly care which term we use, as long as we're
> consistent. Especially, don't keep introducing new synonyms.
>
> The fact that one happens to be one vendor's marketing term does not
> make it preferable, IMHO. On the contrary.
>
These kind of hardware features are defined as part of Time Sensitive
Networking (TSN).
I believe these TSN features are defined as part of IEEE 802.1Qbv (2015)
and according to Wikipedia[2] incorporated into IEEE 802.1Q.
[2] https://en.wikipedia.org/wiki/Time-Sensitive_Networking
> SO_TXTIME is in the ABI, and EDT has been used publicly in kernel
> patches and conference talks, e.g., Van Jacobson's Netdev 0x12
> keynote. Those are vendor agnostic commonly used terms.
>
I agree that EDT (Earliest Departure Time) have become a thing and term
in our community.
We could associate this feature with this.
I do fear what hardware behavior will be it if I e.g. ask it to send a
packet 2 sec in the future on i225 which max support 1 sec.
Will hardware send it at 1 sec?
Because then I'm violating the *Earliest* Departure Time.
> But as long as Launch Time is not an Intel only trademark, fine to
> select that.
The IEEE 802.1Qbv is sometimes called Time-Aware Shaper (TAS), but I
don't like to for us to name this after this. This features is simply
taking advantage of exposing one of the hardware building blocks
(controlling/setting packet "launch time") that can be used for
implementing a TAS.
I like the name "launch time" because it doesn't get easily confused
with other timestamps, and intuitively describes packet will be send at
a specific time (likely in future).
--Jesper
^ permalink raw reply [flat|nested] 17+ messages in thread
* [xdp-hints] Re: [PATCH bpf-next v2 0/3] xsk: TX metadata txtime support
2023-12-01 15:39 ` Jesper Dangaard Brouer
@ 2023-12-02 14:15 ` Willem de Bruijn
2023-12-03 9:16 ` Song, Yoong Siang
0 siblings, 1 reply; 17+ messages in thread
From: Willem de Bruijn @ 2023-12-02 14:15 UTC (permalink / raw)
To: Jesper Dangaard Brouer, Willem de Bruijn, Song, Yoong Siang,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jonathan Corbet, Bjorn Topel, Karlsson, Magnus, Fijalkowski,
Maciej, Jonathan Lemon, Alexei Starovoitov, Daniel Borkmann,
John Fastabend, Stanislav Fomichev, Lorenzo Bianconi,
Tariq Toukan, Willem de Bruijn, Maxime Coquelin, Andrii Nakryiko,
Mykola Lysenko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Hao Luo, Jiri Olsa, Shuah Khan, Alexandre Torgue,
Jose Abreu, Andre Fredette
Cc: netdev, linux-kernel, linux-doc, bpf, xdp-hints, linux-stm32,
linux-arm-kernel, linux-kselftest
Jesper Dangaard Brouer wrote:
>
>
> On 12/1/23 16:09, Willem de Bruijn wrote:
> > Song, Yoong Siang wrote:
> >> On Friday, December 1, 2023 6:46 PM, Jesper Dangaard Brouer <hawk@kernel.org> wrote:
> >>> On 12/1/23 07:24, Song Yoong Siang wrote:
> >>>> This series expands XDP TX metadata framework to include ETF HW offload.
> >>>>
> >>>> Changes since v1:
> >>>> - rename Time-Based Scheduling (TBS) to Earliest TxTime First (ETF)
> >>>> - rename launch-time to txtime
> >>>>
> >>>
> >>> I strongly disagree with this renaming (sorry to disagree with Willem).
> >>>
> >>> The i210 and i225 chips call this LaunchTime in their programmers
> >>> datasheets, and even in the driver code[1].
> >>>
> >>> Using this "txtime" name in the code is also confusing, because how can
> >>> people reading the code know the difference between:
> >>> - tmo_request_timestamp and tmo_request_txtime
> >>>
> >>
> >> Hi Jesper and Willem,
> >>
> >> How about using "launch_time" for the flag/variable and
> >> "Earliest TxTime First" for the description/comments?
> >
>
> I don't follow why you are calling the feature:
> - "Earliest TxTime First" (ETF).
> - AFAIK this just reference an qdisc name (that most don't know exists)
>
>
> > I don't particularly care which term we use, as long as we're
> > consistent. Especially, don't keep introducing new synonyms.
> >
> > The fact that one happens to be one vendor's marketing term does not
> > make it preferable, IMHO. On the contrary.
> >
>
> These kind of hardware features are defined as part of Time Sensitive
> Networking (TSN).
> I believe these TSN features are defined as part of IEEE 802.1Qbv (2015)
> and according to Wikipedia[2] incorporated into IEEE 802.1Q.
>
> [2] https://en.wikipedia.org/wiki/Time-Sensitive_Networking
>
>
> > SO_TXTIME is in the ABI, and EDT has been used publicly in kernel
> > patches and conference talks, e.g., Van Jacobson's Netdev 0x12
> > keynote. Those are vendor agnostic commonly used terms.
> >
>
> I agree that EDT (Earliest Departure Time) have become a thing and term
> in our community.
> We could associate this feature with this.
> I do fear what hardware behavior will be it if I e.g. ask it to send a
> packet 2 sec in the future on i225 which max support 1 sec.
> Will hardware send it at 1 sec?
> Because then I'm violating the *Earliest* Departure Time.
That should definitely not happen. At least not on a device that
implements EDT semantics.
This relates to Jakub's question in the previous thread on whether
this mechanism allows out-of-order transmission or maintains FIFO
behavior. That really is device specific.
Older devices only support this for low rate (PTP) and with a small
fixed number of outstanding requests. For pacing offload, devices need
to support up to linerate and out-of-order.
I don't think we want to enforce either in software, as the hardware
is already out there. But it would be good if drivers can somehow
label these capabilities. Including programmable horizon.
It is up to the qdisc to ensure that it does not pass packets to the
device beyond its horizon.
ETF and FQ already have a concept of horizon. And a way to queue
errors for packets out of bound (SO_EE_CODE_TXTIME_..).
>
> > But as long as Launch Time is not an Intel only trademark, fine to
> > select that.
>
> The IEEE 802.1Qbv is sometimes called Time-Aware Shaper (TAS), but I
> don't like to for us to name this after this. This features is simply
> taking advantage of exposing one of the hardware building blocks
> (controlling/setting packet "launch time") that can be used for
> implementing a TAS.
>
> I like the name "launch time" because it doesn't get easily confused
> with other timestamps, and intuitively describes packet will be send at
> a specific time (likely in future).
>
> --Jesper
Understood on your point that txtime and tx_timestamp are too similar.
As said, I don't care strongly. Launch time sounds fine to me. Others
can speak up if they disagree.
I take launch time as a less strict than EDT: it is a request to send
at a certain time, with no strict definition on uncertainty. While EDT
more strictly ensures that a packet is not sent before the timestamp.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [xdp-hints] Re: [PATCH bpf-next v2 0/3] xsk: TX metadata txtime support
2023-12-02 14:15 ` Willem de Bruijn
@ 2023-12-03 9:16 ` Song, Yoong Siang
0 siblings, 0 replies; 17+ messages in thread
From: Song, Yoong Siang @ 2023-12-03 9:16 UTC (permalink / raw)
To: Willem de Bruijn, Jesper Dangaard Brouer, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Jonathan Corbet,
Bjorn Topel, Karlsson, Magnus, Fijalkowski, Maciej,
Jonathan Lemon, Alexei Starovoitov, Daniel Borkmann,
John Fastabend, Stanislav Fomichev, Lorenzo Bianconi,
Tariq Toukan, Willem de Bruijn, Maxime Coquelin, Andrii Nakryiko,
Mykola Lysenko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Hao Luo, Jiri Olsa, Shuah Khan, Alexandre Torgue,
Jose Abreu, Andre Fredette
Cc: netdev, linux-kernel, linux-doc, bpf, xdp-hints, linux-stm32,
linux-arm-kernel, linux-kselftest
On Saturday, December 2, 2023 10:16 PM, Willem de Bruijn wrote:
>Jesper Dangaard Brouer wrote:
>>
>>
>> On 12/1/23 16:09, Willem de Bruijn wrote:
>> > Song, Yoong Siang wrote:
>> >> On Friday, December 1, 2023 6:46 PM, Jesper Dangaard Brouer
><hawk@kernel.org> wrote:
>> >>> On 12/1/23 07:24, Song Yoong Siang wrote:
>> >>>> This series expands XDP TX metadata framework to include ETF HW offload.
>> >>>>
>> >>>> Changes since v1:
>> >>>> - rename Time-Based Scheduling (TBS) to Earliest TxTime First (ETF)
>> >>>> - rename launch-time to txtime
>> >>>>
>> >>>
>> >>> I strongly disagree with this renaming (sorry to disagree with Willem).
>> >>>
>> >>> The i210 and i225 chips call this LaunchTime in their programmers
>> >>> datasheets, and even in the driver code[1].
>> >>>
>> >>> Using this "txtime" name in the code is also confusing, because how can
>> >>> people reading the code know the difference between:
>> >>> - tmo_request_timestamp and tmo_request_txtime
>> >>>
>> >>
>> >> Hi Jesper and Willem,
>> >>
>> >> How about using "launch_time" for the flag/variable and
>> >> "Earliest TxTime First" for the description/comments?
>> >
>>
>> I don't follow why you are calling the feature:
>> - "Earliest TxTime First" (ETF).
>> - AFAIK this just reference an qdisc name (that most don't know exists)
>>
>>
>> > I don't particularly care which term we use, as long as we're
>> > consistent. Especially, don't keep introducing new synonyms.
>> >
>> > The fact that one happens to be one vendor's marketing term does not
>> > make it preferable, IMHO. On the contrary.
>> >
>>
>> These kind of hardware features are defined as part of Time Sensitive
>> Networking (TSN).
>> I believe these TSN features are defined as part of IEEE 802.1Qbv (2015)
>> and according to Wikipedia[2] incorporated into IEEE 802.1Q.
>>
>> [2] https://en.wikipedia.org/wiki/Time-Sensitive_Networking
>>
>>
>> > SO_TXTIME is in the ABI, and EDT has been used publicly in kernel
>> > patches and conference talks, e.g., Van Jacobson's Netdev 0x12
>> > keynote. Those are vendor agnostic commonly used terms.
>> >
>>
>> I agree that EDT (Earliest Departure Time) have become a thing and term
>> in our community.
>> We could associate this feature with this.
>> I do fear what hardware behavior will be it if I e.g. ask it to send a
>> packet 2 sec in the future on i225 which max support 1 sec.
>> Will hardware send it at 1 sec?
>> Because then I'm violating the *Earliest* Departure Time.
>
>That should definitely not happen. At least not on a device that
>implements EDT semantics.
>
>This relates to Jakub's question in the previous thread on whether
>this mechanism allows out-of-order transmission or maintains FIFO
>behavior. That really is device specific.
>
>Older devices only support this for low rate (PTP) and with a small
>fixed number of outstanding requests. For pacing offload, devices need
>to support up to linerate and out-of-order.
>
>I don't think we want to enforce either in software, as the hardware
>is already out there. But it would be good if drivers can somehow
>label these capabilities. Including programmable horizon.
>
>It is up to the qdisc to ensure that it does not pass packets to the
>device beyond its horizon.
>
>ETF and FQ already have a concept of horizon. And a way to queue
>errors for packets out of bound (SO_EE_CODE_TXTIME_..).
>
>>
>> > But as long as Launch Time is not an Intel only trademark, fine to
>> > select that.
>>
>> The IEEE 802.1Qbv is sometimes called Time-Aware Shaper (TAS), but I
>> don't like to for us to name this after this. This features is simply
>> taking advantage of exposing one of the hardware building blocks
>> (controlling/setting packet "launch time") that can be used for
>> implementing a TAS.
>>
>> I like the name "launch time" because it doesn't get easily confused
>> with other timestamps, and intuitively describes packet will be send at
>> a specific time (likely in future).
>>
>> --Jesper
>
>Understood on your point that txtime and tx_timestamp are too similar.
>As said, I don't care strongly. Launch time sounds fine to me. Others
>can speak up if they disagree.
>
>I take launch time as a less strict than EDT: it is a request to send
>at a certain time, with no strict definition on uncertainty. While EDT
>more strictly ensures that a packet is not sent before the timestamp.
Thanks for the deep discussion and information. I agree with launch time too.
I will submit v3 with launch time so that others can review on the
new naming and provide their feedback.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [xdp-hints] Re: [PATCH bpf-next v2 2/3] net: stmmac: Add txtime support to XDP ZC
2023-12-01 15:02 ` [xdp-hints] " Jesper Dangaard Brouer
2023-12-01 15:26 ` Willem de Bruijn
@ 2023-12-03 10:11 ` Song, Yoong Siang
2023-12-04 14:57 ` Willem de Bruijn
1 sibling, 1 reply; 17+ messages in thread
From: Song, Yoong Siang @ 2023-12-03 10:11 UTC (permalink / raw)
To: Jesper Dangaard Brouer, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Jonathan Corbet, Bjorn Topel,
Karlsson, Magnus, Fijalkowski, Maciej, Jonathan Lemon,
Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Stanislav Fomichev, Lorenzo Bianconi, Tariq Toukan,
Willem de Bruijn, Maxime Coquelin, Andrii Nakryiko,
Mykola Lysenko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Hao Luo, Jiri Olsa, Shuah Khan, Alexandre Torgue,
Jose Abreu
Cc: netdev, linux-kernel, linux-doc, bpf, xdp-hints, linux-stm32,
linux-arm-kernel, linux-kselftest
On Friday, December 1, 2023 11:02 PM, Jesper Dangaard Brouer wrote:
>On 12/1/23 07:24, Song Yoong Siang wrote:
>> This patch enables txtime support to XDP zero copy via XDP Tx
>> metadata framework.
>>
>> Signed-off-by: Song Yoong Siang<yoong.siang.song@intel.com>
>> ---
>> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++
>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++++++++++++
>> 2 files changed, 15 insertions(+)
>
>I think we need to see other drivers using this new feature to evaluate
>if API is sane.
>
>I suggest implementing this for igc driver (chip i225) and also for igb
>(i210 chip) that both support this kind of LaunchTime feature in HW.
>
>The API and stmmac driver takes a u64 as time.
>I'm wondering how this applies to i210 that[1] have 25-bit for
>LaunchTime (with 32 nanosec granularity) limiting LaunchTime max 0.5
>second into the future.
>And i225 that [1] have 30-bit max 1 second into the future.
>
>
>[1]
>https://github.com/xdp-project/xdp-
>project/blob/master/areas/tsn/code01_follow_qdisc_TSN_offload.org
I am using u64 for launch time because existing EDT framework is using it.
Refer to struct sk_buff below. Both u64 and ktime_t can be used as launch time.
I choose u64 because ktime_t often requires additional type conversion and
we didn't expect negative value of time.
include/linux/skbuff.h-744- * @tstamp: Time we arrived/left
include/linux/skbuff.h:745- * @skb_mstamp_ns: (aka @tstamp) earliest departure time; start point
include/linux/skbuff.h-746- * for retransmit timer
--
include/linux/skbuff.h-880- union {
include/linux/skbuff.h-881- ktime_t tstamp;
include/linux/skbuff.h:882- u64 skb_mstamp_ns; /* earliest departure time */
include/linux/skbuff.h-883- };
tstamp/skb_mstamp_ns are used by various drivers for launch time support
on normal packet, so I think u64 should be "friendly" to all the drivers. For an
example, igc driver will take launch time from tstamp and recalculate it
accordingly (i225 expect user to program "delta time" instead of "time" into
HW register).
drivers/net/ethernet/intel/igc/igc_main.c-1602- txtime = skb->tstamp;
drivers/net/ethernet/intel/igc/igc_main.c-1603- skb->tstamp = ktime_set(0, 0);
drivers/net/ethernet/intel/igc/igc_main.c:1604- launch_time = igc_tx_launchtime(tx_ring, txtime, &first_flag, &insert_empty);
Do you think this is enough to say the API is sane?
^ permalink raw reply [flat|nested] 17+ messages in thread
* [xdp-hints] Re: [PATCH bpf-next v2 2/3] net: stmmac: Add txtime support to XDP ZC
2023-12-03 10:11 ` Song, Yoong Siang
@ 2023-12-04 14:57 ` Willem de Bruijn
2023-12-05 14:43 ` Song, Yoong Siang
0 siblings, 1 reply; 17+ messages in thread
From: Willem de Bruijn @ 2023-12-04 14:57 UTC (permalink / raw)
To: Song, Yoong Siang, Jesper Dangaard Brouer, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Jonathan Corbet,
Bjorn Topel, Karlsson, Magnus, Fijalkowski, Maciej,
Jonathan Lemon, Alexei Starovoitov, Daniel Borkmann,
John Fastabend, Stanislav Fomichev, Lorenzo Bianconi,
Tariq Toukan, Willem de Bruijn, Maxime Coquelin, Andrii Nakryiko,
Mykola Lysenko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Hao Luo, Jiri Olsa, Shuah Khan, Alexandre Torgue,
Jose Abreu
Cc: netdev, linux-kernel, linux-doc, bpf, xdp-hints, linux-stm32,
linux-arm-kernel, linux-kselftest
Song, Yoong Siang wrote:
> On Friday, December 1, 2023 11:02 PM, Jesper Dangaard Brouer wrote:
> >On 12/1/23 07:24, Song Yoong Siang wrote:
> >> This patch enables txtime support to XDP zero copy via XDP Tx
> >> metadata framework.
> >>
> >> Signed-off-by: Song Yoong Siang<yoong.siang.song@intel.com>
> >> ---
> >> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++
> >> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++++++++++++
> >> 2 files changed, 15 insertions(+)
> >
> >I think we need to see other drivers using this new feature to evaluate
> >if API is sane.
> >
> >I suggest implementing this for igc driver (chip i225) and also for igb
> >(i210 chip) that both support this kind of LaunchTime feature in HW.
> >
> >The API and stmmac driver takes a u64 as time.
> >I'm wondering how this applies to i210 that[1] have 25-bit for
> >LaunchTime (with 32 nanosec granularity) limiting LaunchTime max 0.5
> >second into the future.
> >And i225 that [1] have 30-bit max 1 second into the future.
> >
> >
> >[1]
> >https://github.com/xdp-project/xdp-
> >project/blob/master/areas/tsn/code01_follow_qdisc_TSN_offload.org
>
> I am using u64 for launch time because existing EDT framework is using it.
> Refer to struct sk_buff below. Both u64 and ktime_t can be used as launch time.
> I choose u64 because ktime_t often requires additional type conversion and
> we didn't expect negative value of time.
>
> include/linux/skbuff.h-744- * @tstamp: Time we arrived/left
> include/linux/skbuff.h:745- * @skb_mstamp_ns: (aka @tstamp) earliest departure time; start point
> include/linux/skbuff.h-746- * for retransmit timer
> --
> include/linux/skbuff.h-880- union {
> include/linux/skbuff.h-881- ktime_t tstamp;
> include/linux/skbuff.h:882- u64 skb_mstamp_ns; /* earliest departure time */
> include/linux/skbuff.h-883- };
>
> tstamp/skb_mstamp_ns are used by various drivers for launch time support
> on normal packet, so I think u64 should be "friendly" to all the drivers. For an
> example, igc driver will take launch time from tstamp and recalculate it
> accordingly (i225 expect user to program "delta time" instead of "time" into
> HW register).
>
> drivers/net/ethernet/intel/igc/igc_main.c-1602- txtime = skb->tstamp;
> drivers/net/ethernet/intel/igc/igc_main.c-1603- skb->tstamp = ktime_set(0, 0);
> drivers/net/ethernet/intel/igc/igc_main.c:1604- launch_time = igc_tx_launchtime(tx_ring, txtime, &first_flag, &insert_empty);
>
> Do you think this is enough to say the API is sane?
u64 nsec sounds sane to be. It must be made explicit with clock source
it is against.
Some applications could want to do the conversion from a clock source
to raw NIC cycle counter in userspace or BPF and program the raw
value. So it may be worthwhile to add an clock source argument -- even
if initially only CLOCK_MONOTONIC is supported.
See tools/testing/selftests/net/so_txtime.sh for how the FQ and ETF
qdiscs already disagree on the clock source that they use.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [xdp-hints] Re: [PATCH bpf-next v2 2/3] net: stmmac: Add txtime support to XDP ZC
2023-12-04 14:57 ` Willem de Bruijn
@ 2023-12-05 14:43 ` Song, Yoong Siang
2023-12-05 14:55 ` Willem de Bruijn
0 siblings, 1 reply; 17+ messages in thread
From: Song, Yoong Siang @ 2023-12-05 14:43 UTC (permalink / raw)
To: Willem de Bruijn, Jesper Dangaard Brouer, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Jonathan Corbet,
Bjorn Topel, Karlsson, Magnus, Fijalkowski, Maciej,
Jonathan Lemon, Alexei Starovoitov, Daniel Borkmann,
John Fastabend, Stanislav Fomichev, Lorenzo Bianconi,
Tariq Toukan, Willem de Bruijn, Maxime Coquelin, Andrii Nakryiko,
Mykola Lysenko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Hao Luo, Jiri Olsa, Shuah Khan, Alexandre Torgue,
Jose Abreu
Cc: netdev, linux-kernel, linux-doc, bpf, xdp-hints, linux-stm32,
linux-arm-kernel, linux-kselftest
On Monday, December 4, 2023 10:58 PM, Willem de Bruijn wrote:
>Song, Yoong Siang wrote:
>> On Friday, December 1, 2023 11:02 PM, Jesper Dangaard Brouer wrote:
>> >On 12/1/23 07:24, Song Yoong Siang wrote:
>> >> This patch enables txtime support to XDP zero copy via XDP Tx
>> >> metadata framework.
>> >>
>> >> Signed-off-by: Song Yoong Siang<yoong.siang.song@intel.com>
>> >> ---
>> >> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++
>> >> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++++++++++++
>> >> 2 files changed, 15 insertions(+)
>> >
>> >I think we need to see other drivers using this new feature to evaluate
>> >if API is sane.
>> >
>> >I suggest implementing this for igc driver (chip i225) and also for igb
>> >(i210 chip) that both support this kind of LaunchTime feature in HW.
>> >
>> >The API and stmmac driver takes a u64 as time.
>> >I'm wondering how this applies to i210 that[1] have 25-bit for
>> >LaunchTime (with 32 nanosec granularity) limiting LaunchTime max 0.5
>> >second into the future.
>> >And i225 that [1] have 30-bit max 1 second into the future.
>> >
>> >
>> >[1]
>> >https://github.com/xdp-project/xdp-
>> >project/blob/master/areas/tsn/code01_follow_qdisc_TSN_offload.org
>>
>> I am using u64 for launch time because existing EDT framework is using it.
>> Refer to struct sk_buff below. Both u64 and ktime_t can be used as launch time.
>> I choose u64 because ktime_t often requires additional type conversion and
>> we didn't expect negative value of time.
>>
>> include/linux/skbuff.h-744- * @tstamp: Time we arrived/left
>> include/linux/skbuff.h:745- * @skb_mstamp_ns: (aka @tstamp) earliest departure
>time; start point
>> include/linux/skbuff.h-746- * for retransmit timer
>> --
>> include/linux/skbuff.h-880- union {
>> include/linux/skbuff.h-881- ktime_t tstamp;
>> include/linux/skbuff.h:882- u64 skb_mstamp_ns; /* earliest departure
>time */
>> include/linux/skbuff.h-883- };
>>
>> tstamp/skb_mstamp_ns are used by various drivers for launch time support
>> on normal packet, so I think u64 should be "friendly" to all the drivers. For an
>> example, igc driver will take launch time from tstamp and recalculate it
>> accordingly (i225 expect user to program "delta time" instead of "time" into
>> HW register).
>>
>> drivers/net/ethernet/intel/igc/igc_main.c-1602- txtime = skb->tstamp;
>> drivers/net/ethernet/intel/igc/igc_main.c-1603- skb->tstamp = ktime_set(0, 0);
>> drivers/net/ethernet/intel/igc/igc_main.c:1604- launch_time =
>igc_tx_launchtime(tx_ring, txtime, &first_flag, &insert_empty);
>>
>> Do you think this is enough to say the API is sane?
>
>u64 nsec sounds sane to be. It must be made explicit with clock source
>it is against.
>
The u64 launch time should base on NIC PTP hardware clock (PHC).
I will add documentation saying which clock source it is against
>Some applications could want to do the conversion from a clock source
>to raw NIC cycle counter in userspace or BPF and program the raw
>value. So it may be worthwhile to add an clock source argument -- even
>if initially only CLOCK_MONOTONIC is supported.
Sorry, not so understand your suggestion on adding clock source argument.
Are you suggesting to add clock source for the selftest xdp_hw_metadata apps?
IMHO, no need to add clock source as the clock source for launch time
should always base on NIC PHC.
>
>See tools/testing/selftests/net/so_txtime.sh for how the FQ and ETF
>qdiscs already disagree on the clock source that they use.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [xdp-hints] Re: [PATCH bpf-next v2 2/3] net: stmmac: Add txtime support to XDP ZC
2023-12-05 14:43 ` Song, Yoong Siang
@ 2023-12-05 14:55 ` Willem de Bruijn
2023-12-05 15:28 ` Song, Yoong Siang
0 siblings, 1 reply; 17+ messages in thread
From: Willem de Bruijn @ 2023-12-05 14:55 UTC (permalink / raw)
To: Song, Yoong Siang, Willem de Bruijn, Jesper Dangaard Brouer,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jonathan Corbet, Bjorn Topel, Karlsson, Magnus, Fijalkowski,
Maciej, Jonathan Lemon, Alexei Starovoitov, Daniel Borkmann,
John Fastabend, Stanislav Fomichev, Lorenzo Bianconi,
Tariq Toukan, Willem de Bruijn, Maxime Coquelin, Andrii Nakryiko,
Mykola Lysenko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Hao Luo, Jiri Olsa, Shuah Khan, Alexandre Torgue,
Jose Abreu
Cc: netdev, linux-kernel, linux-doc, bpf, xdp-hints, linux-stm32,
linux-arm-kernel, linux-kselftest
Song, Yoong Siang wrote:
> On Monday, December 4, 2023 10:58 PM, Willem de Bruijn wrote:
> >Song, Yoong Siang wrote:
> >> On Friday, December 1, 2023 11:02 PM, Jesper Dangaard Brouer wrote:
> >> >On 12/1/23 07:24, Song Yoong Siang wrote:
> >> >> This patch enables txtime support to XDP zero copy via XDP Tx
> >> >> metadata framework.
> >> >>
> >> >> Signed-off-by: Song Yoong Siang<yoong.siang.song@intel.com>
> >> >> ---
> >> >> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++
> >> >> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++++++++++++
> >> >> 2 files changed, 15 insertions(+)
> >> >
> >> >I think we need to see other drivers using this new feature to evaluate
> >> >if API is sane.
> >> >
> >> >I suggest implementing this for igc driver (chip i225) and also for igb
> >> >(i210 chip) that both support this kind of LaunchTime feature in HW.
> >> >
> >> >The API and stmmac driver takes a u64 as time.
> >> >I'm wondering how this applies to i210 that[1] have 25-bit for
> >> >LaunchTime (with 32 nanosec granularity) limiting LaunchTime max 0.5
> >> >second into the future.
> >> >And i225 that [1] have 30-bit max 1 second into the future.
> >> >
> >> >
> >> >[1]
> >> >https://github.com/xdp-project/xdp-
> >> >project/blob/master/areas/tsn/code01_follow_qdisc_TSN_offload.org
> >>
> >> I am using u64 for launch time because existing EDT framework is using it.
> >> Refer to struct sk_buff below. Both u64 and ktime_t can be used as launch time.
> >> I choose u64 because ktime_t often requires additional type conversion and
> >> we didn't expect negative value of time.
> >>
> >> include/linux/skbuff.h-744- * @tstamp: Time we arrived/left
> >> include/linux/skbuff.h:745- * @skb_mstamp_ns: (aka @tstamp) earliest departure
> >time; start point
> >> include/linux/skbuff.h-746- * for retransmit timer
> >> --
> >> include/linux/skbuff.h-880- union {
> >> include/linux/skbuff.h-881- ktime_t tstamp;
> >> include/linux/skbuff.h:882- u64 skb_mstamp_ns; /* earliest departure
> >time */
> >> include/linux/skbuff.h-883- };
> >>
> >> tstamp/skb_mstamp_ns are used by various drivers for launch time support
> >> on normal packet, so I think u64 should be "friendly" to all the drivers. For an
> >> example, igc driver will take launch time from tstamp and recalculate it
> >> accordingly (i225 expect user to program "delta time" instead of "time" into
> >> HW register).
> >>
> >> drivers/net/ethernet/intel/igc/igc_main.c-1602- txtime = skb->tstamp;
> >> drivers/net/ethernet/intel/igc/igc_main.c-1603- skb->tstamp = ktime_set(0, 0);
> >> drivers/net/ethernet/intel/igc/igc_main.c:1604- launch_time =
> >igc_tx_launchtime(tx_ring, txtime, &first_flag, &insert_empty);
> >>
> >> Do you think this is enough to say the API is sane?
> >
> >u64 nsec sounds sane to be. It must be made explicit with clock source
> >it is against.
> >
>
> The u64 launch time should base on NIC PTP hardware clock (PHC).
> I will add documentation saying which clock source it is against
It's not that obvious to me that that is the right and only choice.
See below.
> >Some applications could want to do the conversion from a clock source
> >to raw NIC cycle counter in userspace or BPF and program the raw
> >value. So it may be worthwhile to add an clock source argument -- even
> >if initially only CLOCK_MONOTONIC is supported.
>
> Sorry, not so understand your suggestion on adding clock source argument.
> Are you suggesting to add clock source for the selftest xdp_hw_metadata apps?
> IMHO, no need to add clock source as the clock source for launch time
> should always base on NIC PHC.
This is not how FQ and ETF qdiscs pass timestamps to drivers today.
Those are in CLOCK_MONOTONIC or CLOCK_TAI. The driver is expected to
convert from that to its descriptor format, both to the reduced bit
width and the NIC PHC.
See also for instance how sch_etf has an explicit q->clock_id match,
and SO_TXTIME added an sk_clock_id for the same purpose: to agree on
which clock source is being used.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [xdp-hints] Re: [PATCH bpf-next v2 2/3] net: stmmac: Add txtime support to XDP ZC
2023-12-05 14:55 ` Willem de Bruijn
@ 2023-12-05 15:28 ` Song, Yoong Siang
0 siblings, 0 replies; 17+ messages in thread
From: Song, Yoong Siang @ 2023-12-05 15:28 UTC (permalink / raw)
To: Willem de Bruijn, Jesper Dangaard Brouer, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Jonathan Corbet,
Bjorn Topel, Karlsson, Magnus, Fijalkowski, Maciej,
Jonathan Lemon, Alexei Starovoitov, Daniel Borkmann,
John Fastabend, Stanislav Fomichev, Lorenzo Bianconi,
Tariq Toukan, Willem de Bruijn, Maxime Coquelin, Andrii Nakryiko,
Mykola Lysenko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Hao Luo, Jiri Olsa, Shuah Khan, Alexandre Torgue,
Jose Abreu
Cc: netdev, linux-kernel, linux-doc, bpf, xdp-hints, linux-stm32,
linux-arm-kernel, linux-kselftest
On Tuesday, December 5, 2023 10:55 PM, Willem de Bruijn wrote:
>Song, Yoong Siang wrote:
>> On Monday, December 4, 2023 10:58 PM, Willem de Bruijn wrote:
>> >Song, Yoong Siang wrote:
>> >> On Friday, December 1, 2023 11:02 PM, Jesper Dangaard Brouer wrote:
>> >> >On 12/1/23 07:24, Song Yoong Siang wrote:
>> >> >> This patch enables txtime support to XDP zero copy via XDP Tx
>> >> >> metadata framework.
>> >> >>
>> >> >> Signed-off-by: Song Yoong Siang<yoong.siang.song@intel.com>
>> >> >> ---
>> >> >> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++
>> >> >> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++++++++++++
>> >> >> 2 files changed, 15 insertions(+)
>> >> >
>> >> >I think we need to see other drivers using this new feature to evaluate
>> >> >if API is sane.
>> >> >
>> >> >I suggest implementing this for igc driver (chip i225) and also for igb
>> >> >(i210 chip) that both support this kind of LaunchTime feature in HW.
>> >> >
>> >> >The API and stmmac driver takes a u64 as time.
>> >> >I'm wondering how this applies to i210 that[1] have 25-bit for
>> >> >LaunchTime (with 32 nanosec granularity) limiting LaunchTime max 0.5
>> >> >second into the future.
>> >> >And i225 that [1] have 30-bit max 1 second into the future.
>> >> >
>> >> >
>> >> >[1]
>> >> >https://github.com/xdp-project/xdp-
>> >> >project/blob/master/areas/tsn/code01_follow_qdisc_TSN_offload.org
>> >>
>> >> I am using u64 for launch time because existing EDT framework is using it.
>> >> Refer to struct sk_buff below. Both u64 and ktime_t can be used as launch time.
>> >> I choose u64 because ktime_t often requires additional type conversion and
>> >> we didn't expect negative value of time.
>> >>
>> >> include/linux/skbuff.h-744- * @tstamp: Time we arrived/left
>> >> include/linux/skbuff.h:745- * @skb_mstamp_ns: (aka @tstamp) earliest
>departure
>> >time; start point
>> >> include/linux/skbuff.h-746- * for retransmit timer
>> >> --
>> >> include/linux/skbuff.h-880- union {
>> >> include/linux/skbuff.h-881- ktime_t tstamp;
>> >> include/linux/skbuff.h:882- u64 skb_mstamp_ns; /* earliest
>departure
>> >time */
>> >> include/linux/skbuff.h-883- };
>> >>
>> >> tstamp/skb_mstamp_ns are used by various drivers for launch time support
>> >> on normal packet, so I think u64 should be "friendly" to all the drivers. For an
>> >> example, igc driver will take launch time from tstamp and recalculate it
>> >> accordingly (i225 expect user to program "delta time" instead of "time" into
>> >> HW register).
>> >>
>> >> drivers/net/ethernet/intel/igc/igc_main.c-1602- txtime = skb->tstamp;
>> >> drivers/net/ethernet/intel/igc/igc_main.c-1603- skb->tstamp = ktime_set(0, 0);
>> >> drivers/net/ethernet/intel/igc/igc_main.c:1604- launch_time =
>> >igc_tx_launchtime(tx_ring, txtime, &first_flag, &insert_empty);
>> >>
>> >> Do you think this is enough to say the API is sane?
>> >
>> >u64 nsec sounds sane to be. It must be made explicit with clock source
>> >it is against.
>> >
>>
>> The u64 launch time should base on NIC PTP hardware clock (PHC).
>> I will add documentation saying which clock source it is against
>
>It's not that obvious to me that that is the right and only choice.
>See below.
>
>> >Some applications could want to do the conversion from a clock source
>> >to raw NIC cycle counter in userspace or BPF and program the raw
>> >value. So it may be worthwhile to add an clock source argument -- even
>> >if initially only CLOCK_MONOTONIC is supported.
>>
>> Sorry, not so understand your suggestion on adding clock source argument.
>> Are you suggesting to add clock source for the selftest xdp_hw_metadata apps?
>> IMHO, no need to add clock source as the clock source for launch time
>> should always base on NIC PHC.
>
>This is not how FQ and ETF qdiscs pass timestamps to drivers today.
>
>Those are in CLOCK_MONOTONIC or CLOCK_TAI. The driver is expected to
>convert from that to its descriptor format, both to the reduced bit
>width and the NIC PHC.
>
>See also for instance how sch_etf has an explicit q->clock_id match,
>and SO_TXTIME added an sk_clock_id for the same purpose: to agree on
>which clock source is being used.
I see. Thank for the explanation. I will try to add clock source arguments
In next version.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-12-05 15:28 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-01 6:24 [xdp-hints] [PATCH bpf-next v2 0/3] xsk: TX metadata txtime support Song Yoong Siang
2023-12-01 6:24 ` [xdp-hints] [PATCH bpf-next v2 1/3] xsk: add ETF support to XDP Tx metadata Song Yoong Siang
2023-12-01 6:24 ` [xdp-hints] [PATCH bpf-next v2 2/3] net: stmmac: Add txtime support to XDP ZC Song Yoong Siang
2023-12-01 15:02 ` [xdp-hints] " Jesper Dangaard Brouer
2023-12-01 15:26 ` Willem de Bruijn
2023-12-03 10:11 ` Song, Yoong Siang
2023-12-04 14:57 ` Willem de Bruijn
2023-12-05 14:43 ` Song, Yoong Siang
2023-12-05 14:55 ` Willem de Bruijn
2023-12-05 15:28 ` Song, Yoong Siang
2023-12-01 6:24 ` [xdp-hints] [PATCH bpf-next v2 3/3] selftests/bpf: Add txtime to xdp_hw_metadata Song Yoong Siang
2023-12-01 10:46 ` [xdp-hints] Re: [PATCH bpf-next v2 0/3] xsk: TX metadata txtime support Jesper Dangaard Brouer
2023-12-01 13:43 ` Song, Yoong Siang
2023-12-01 15:09 ` Willem de Bruijn
2023-12-01 15:39 ` Jesper Dangaard Brouer
2023-12-02 14:15 ` Willem de Bruijn
2023-12-03 9:16 ` Song, Yoong Siang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox