| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 28 Jul 2023 13:56:03 +0200
From: Jesper Dangaard Brouer <jbrouer@...hat.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>,
Stanislav Fomichev <sdf@...gle.com>, bpf@...r.kernel.org
Cc: brouer@...hat.com, ast@...nel.org, daniel@...earbox.net,
andrii@...nel.org, martin.lau@...ux.dev, song@...nel.org, yhs@...com,
john.fastabend@...il.com, kpsingh@...nel.org, haoluo@...gle.com,
jolsa@...nel.org, kuba@...nel.org, toke@...nel.org, willemb@...gle.com,
dsahern@...nel.org, magnus.karlsson@...el.com, bjorn@...nel.org,
maciej.fijalkowski@...el.com, hawk@...nel.org, netdev@...r.kernel.org,
xdp-hints@...-project.net
Subject: Re: [RFC net-next v4 2/8] xsk: add TX timestamp and TX checksum
offload support
On 25/07/2023 22.58, Willem de Bruijn wrote:
> Stanislav Fomichev wrote:
>> This change actually defines the (initial) metadata layout
>> that should be used by AF_XDP userspace (xsk_tx_metadata).
>> The first field is flags which requests appropriate offloads,
>> followed by the offload-specific fields. The supported per-device
>> offloads are exported via netlink (new xsk-flags).
>>
>> The offloads themselves are still implemented in a bit of a
>> framework-y fashion that's left from my initial kfunc attempt.
>> I'm introducing new xsk_tx_metadata_ops which drivers are
>> supposed to implement. The drivers are also supposed
>> to call xsk_tx_metadata_request/xsk_tx_metadata_complete in
>> the right places. Since xsk_tx_metadata_{request,_complete}
>> are static inline, we don't incur any extra overhead doing
>> indirect calls.
>>
>> The benefit of this scheme is as follows:
>> - keeps all metadata layout parsing away from driver code
>> - makes it easy to grep and see which drivers implement what
>> - don't need any extra flags to maintain to keep track of that
>> offloads are implemented; if the callback is implemented - the offload
>> is supported (used by netlink reporting code)
>>
>> Two offloads are defined right now:
>> 1. XDP_TX_METADATA_CHECKSUM: skb-style csum_start+csum_offset
>> 2. XDP_TX_METADATA_TIMESTAMP: writes TX timestamp back into metadata
>> area upon completion (tx_timestamp field)
>>
>> The offloads are also implemented for copy mode:
>> 1. Extra XDP_TX_METADATA_CHECKSUM_SW to trigger skb_checksum_help; this
>> might be useful as a reference implementation and for testing
>> 2. XDP_TX_METADATA_TIMESTAMP writes SW timestamp from the skb
>> destructor (note I'm reusing hwtstamps to pass metadata pointer)
>>
>> The struct is forward-compatible and can be extended in the future
>> by appending more fields.
>>
>> Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
>> ---
>> Documentation/netlink/specs/netdev.yaml | 19 ++++++++
>> include/linux/netdevice.h | 27 +++++++++++
>> include/linux/skbuff.h | 5 ++-
>> include/net/xdp_sock.h | 60 +++++++++++++++++++++++++
>> include/net/xdp_sock_drv.h | 13 ++++++
>> include/uapi/linux/if_xdp.h | 35 +++++++++++++++
>> include/uapi/linux/netdev.h | 15 +++++++
>> net/core/netdev-genl.c | 12 ++++-
>> net/xdp/xsk.c | 38 ++++++++++++++++
>> net/xdp/xsk_queue.h | 2 +-
>> tools/include/uapi/linux/if_xdp.h | 50 ++++++++++++++++++---
>> 11 files changed, 268 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
>> index e41015310a6e..bf9c1cc32614 100644
>> --- a/Documentation/netlink/specs/netdev.yaml
>> +++ b/Documentation/netlink/specs/netdev.yaml
>> @@ -42,6 +42,19 @@ name: netdev
>> doc:
>> This feature informs if netdev implements non-linear XDP buffer
>> support in ndo_xdp_xmit callback.
>> + -
>> + type: flags
>> + name: xsk-flags
>> + render-max: true
>> + entries:
>> + -
>> + name: tx-timestamp
>> + doc:
>> + HW timestamping egress packets is supported by the driver.
>> + -
>> + name: tx-checksum
>> + doc:
>> + L3 checksum HW offload is supported by the driver.
>>
>> attribute-sets:
>> -
>> @@ -68,6 +81,12 @@ name: netdev
>> type: u32
>> checks:
>> min: 1
>> + -
>> + name: xsk-features
>> + doc: Bitmask of enabled AF_XDP features.
>> + type: u64
>> + enum: xsk-flags
>> + enum-as-flags: true
>>
>> operations:
>> list:
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 11652e464f5d..8b40c80557aa 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -1660,6 +1660,31 @@ struct xdp_metadata_ops {
>> enum xdp_rss_hash_type *rss_type);
>> };
>>
>> +/*
>> + * This structure defines the AF_XDP TX metadata hooks for network devices.
>> + * The following hooks can be defined; unless noted otherwise, they are
>> + * optional and can be filled with a null pointer.
>> + *
>> + * int (*tmo_request_timestamp)(void *priv)
>> + * This function is called when AF_XDP frame requested egress timestamp.
>> + *
>> + * int (*tmo_fill_timestamp)(void *priv)
>> + * This function is called when AF_XDP frame, that had requested
>> + * egress timestamp, received a completion. The hook needs to return
>> + * the actual HW timestamp.
>> + *
>> + * int (*tmo_request_timestamp)(u16 csum_start, u16 csum_offset, void *priv)
>
> typo: tmo_request_checksum
>
>> + * This function is called when AF_XDP frame requested HW checksum
>> + * offload. csum_start indicates position where checksumming should start.
>> + * csum_offset indicates position where checksum should be stored.
>> + *
>> + */
>> +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);
>> +};
>> +
>> /**
>> * enum netdev_priv_flags - &struct net_device priv_flags
>> *
>> @@ -1844,6 +1869,7 @@ enum netdev_ml_priv_type {
>> * @netdev_ops: Includes several pointers to callbacks,
>> * if one wants to override the ndo_*() functions
>> * @xdp_metadata_ops: Includes pointers to XDP metadata callbacks.
>> + * @xsk_tx_metadata_ops: Includes pointers to AF_XDP TX metadata callbacks.
>> * @ethtool_ops: Management operations
>> * @l3mdev_ops: Layer 3 master device operations
>> * @ndisc_ops: Includes callbacks for different IPv6 neighbour
>> @@ -2100,6 +2126,7 @@ struct net_device {
>> unsigned long long priv_flags;
>> const struct net_device_ops *netdev_ops;
>> const struct xdp_metadata_ops *xdp_metadata_ops;
>> + const struct xsk_tx_metadata_ops *xsk_tx_metadata_ops;
>> int ifindex;
>> unsigned short gflags;
>> unsigned short hard_header_len;
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index faaba050f843..5febc1a5131e 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -581,7 +581,10 @@ struct skb_shared_info {
>> /* Warning: this field is not always filled in (UFO)! */
>> unsigned short gso_segs;
>> struct sk_buff *frag_list;
>> - struct skb_shared_hwtstamps hwtstamps;
>> + union {
>> + struct skb_shared_hwtstamps hwtstamps;
>> + struct xsk_tx_metadata *xsk_meta;
>> + };
>> unsigned int gso_type;
>> u32 tskey;
>>
>> diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
>> index 467b9fb56827..288fa58c4665 100644
>> --- a/include/net/xdp_sock.h
>> +++ b/include/net/xdp_sock.h
>> @@ -90,6 +90,54 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp);
>> int __xsk_map_redirect(struct xdp_sock *xs, struct xdp_buff *xdp);
>> void __xsk_map_flush(void);
>>
>> +/**
>> + * xsk_tx_metadata_request - Evaluate AF_XDP TX metadata at submission
>> + * and call appropriate xsk_tx_metadata_ops operation.
>> + * @meta: pointer to AF_XDP metadata area
>> + * @ops: pointer to struct xsk_tx_metadata_ops
>> + * @priv: pointer to driver-private aread
>> + *
>> + * This function should be called by the networking device when
>> + * it prepares AF_XDP egress packet.
>> + */
>> +static inline void xsk_tx_metadata_request(const struct xsk_tx_metadata *meta,
>> + const struct xsk_tx_metadata_ops *ops,
>> + void *priv)
>> +{
>> + if (!meta)
>> + return;
>> +
>> + if (ops->tmo_request_timestamp)
>> + if (meta->flags & XDP_TX_METADATA_TIMESTAMP)
>> + ops->tmo_request_timestamp(priv);
>> +
>> + if (ops->tmo_request_checksum)
>> + if (meta->flags & XDP_TX_METADATA_CHECKSUM)
>> + ops->tmo_request_checksum(meta->csum_start, meta->csum_offset, priv);
>> +}
>
> Might be cheaper to test the flag in the hot cacheline before
> dereferencing ops?
>
I was thinking the same thing, but I was wrong, see below, as these ops
deref's are optimized out by the compiler.
> Also, just add these functions to net_device_ops directly,
> rather than dereferencing another pointer to xsk_tx_metadata_ops?
>
After the ASM/objdump discussion[1] in this thread, I think Stanislav's
code approach here is actually optimal. Because when the 'const' ops are
defined locally in the same file that use/does the inlining of
xsk_tx_metadata_request() then the compilers (both llvm and gcc) are
smart enough to inline and do dead-code elimination. E.g. I noticed
that generated ASM code eliminated `if (ops->tmo_request_timestamp)`
code path in mlx5 as its not implemented.
[1]
https://lore.kernel.org/all/cce9db50-8c9d-ea97-cb88-171fa46cc064@redhat.com/
--Jesper
Powered by blists - more mailing lists