Date: Thu, 27 Jul 2023 15:50:23 +0200
From: Jesper Dangaard Brouer <jbrouer@...hat.com>
To: Stanislav Fomichev <sdf@...gle.com>,
 Jesper Dangaard Brouer <jbrouer@...hat.com>
Cc: brouer@...hat.com, bpf@...r.kernel.org, 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: [xdp-hints] [RFC net-next v4 2/8] xsk: add TX timestamp and TX
 checksum offload support



On 26/07/2023 23.25, Stanislav Fomichev wrote:
> On 07/26, Jesper Dangaard Brouer wrote:
>>
>>
>> On 25/07/2023 01.59, Stanislav Fomichev wrote:
>>> 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)
>>> + *     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)
>>
>> (As you mentioned) this gets inlined in drivers for performance.
>>
>>> +{
>>> +	if (!meta)
>>> +		return;
>>> +
>>> +	if (ops->tmo_request_timestamp)
>>> +		if (meta->flags & XDP_TX_METADATA_TIMESTAMP)
>>> +			ops->tmo_request_timestamp(priv);
>>
>> We still have the overhead of function pointer call.
>> With RETPOLINE this is costly.
>>
>> Measured on my testlab CPU E5-1650 v4 @ 3.60GHz
>>   Type:function_call_cost:  3 cycles(tsc) 1.010 ns
>>   Type:func_ptr_call_cost: 30 cycles(tsc) 8.341 ns
>>
>> Given this get inlined in drivers, perhaps we can add some
>> INDIRECT_CALL_1 macros to avoid these indirect calls?
> 
> I'm assuming that the compiler is smart enough to resolve these const
> struct ops definitions as long as they are in the same file.
> 
> At least here is what I see for mlx5e_xmit_xdp_frame_mpwqe: those
> indirect jumps are resolved and the calls themselves are unrolled.
> TBF, I don't have retpolines enabled in the config, but I don't think
> it will bring indirect jumps back? Am I missing anything?
> 

I tried this with CONFIG_RETPOLINE and same results.
The compiler is smart and inlines the call to mlx5e_xsk_request_checksum().
This is great, no need for crazy INDIRECT_CALL_1 macros :-)

> 
> 0000000000001930 <mlx5e_xmit_xdp_frame_mpwqe>:
> ; mlx5e_xmit_xdp_frame_mpwqe():
> ; ./drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c:436
[...]
> ; ./drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c:381
> ; 	stats->mpwqe++;
>      1b4a: 49 ff 44 24 08               	incq	0x8(%r12)
> ; ././include/net/xdp_sock.h:107

How do you get objdump to add these file:line annotations?

I use:
  objdump -rS --visualize-jumps=color -Mintel | less -R

> ; 	if (!meta)
>      1b4f: 4d 85 ff                     	testq	%r15, %r15
>      1b52: 74 0e                        	je	0x1b62 <mlx5e_xmit_xdp_frame_mpwqe+0x232>
> ; ././include/net/xdp_sock.h:115
> ; 		if (meta->flags & XDP_TX_METADATA_CHECKSUM)
>      1b54: 41 f6 07 02                  	testb	$0x2, (%r15)
>      1b58: 74 08                        	je	0x1b62 <mlx5e_xmit_xdp_frame_mpwqe+0x232>
> ; ./drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c:483
> ; 		xsk_tx_metadata_request(meta, &mlx5e_xsk_tx_metadata_ops, &session->wqe->eth);
>      1b5a: 48 8b 43 50                  	movq	0x50(%rbx), %rax
> ; ./drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c:286
> ; 	eseg->cs_flags |= MLX5_ETH_WQE_L3_CSUM | MLX5_ETH_WQE_L4_CSUM;
>      1b5e: 80 48 14 c0                  	orb	$-0x40, 0x14(%rax)

Yes, here mlx5e_xsk_request_checksum() gets inlined, as
  MLX5_ETH_WQE_L3_CSUM | MLX5_ETH_WQE_L4_CSUM = 192
  192 = 0xC0 and signed byte value $-0x40, which we see in inst 'orb'.

I usually prefer Intel ASM code output via objdump -Mintel
and it decode ASM with unsigned value 0xc0 / 192:

  or     BYTE PTR ds:0x4,0xc0


> ; ./drivers/net/ethernet/mellanox/mlx5/core/en/xdp.h:203
> ; 		(struct mlx5_wqe_data_seg *)session->wqe + session->ds_count;
>      1b62: 48 8b 43 50                  	movq	0x50(%rbx), %rax
[...]

Thank you for checking up on this! :-)

--Jesper

Powered by blists - more mailing lists