Re: [dpdk-dev] [PATCH v2] net/af_packet: remove timestamp from packet status

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 9/13/2021 6:23 PM, Tudor Cornea wrote:
> We should eliminate the timestamp status from the packet
> status. This should only matter if timestamping is enabled
> on the socket, but we might hit a kernel bug, which is fixed
> in newer releases.
> 
> For interfaces of type 'veth', the sent skb is forwarded
> to the peer and back into the network stack which timestamps
> it on the RX path if timestamping is enabled globally
> (which happens if any socket enables timestamping).
> 
> When the skb is destructed, tpacket_destruct_skb() is called
> and it calls __packet_set_timestamp() which doesn't check
> the flags on the socket and returns the timestamp if it is
> set in the skb (and for veth it is, as mentioned above).
> 
> See the following kernel commit for reference [1]:
> 
> net: packetmmap: fix only tx timestamp on request
> 
> The packetmmap tx ring should only return timestamps if requested
> via setsockopt PACKET_TIMESTAMP, as documented. This allows
> compatibility with non-timestamp aware user-space code which checks
> tp_status == TP_STATUS_AVAILABLE; not expecting additional timestamp
> flags to be set in tp_status.
> 
> [1] https://www.spinics.net/lists/kernel/msg3959391.html
> 
> Signed-off-by: Mihai Pogonaru <pogonarumihai@gmail.com>
> Signed-off-by: Tudor Cornea <tudor.cornea@gmail.com>
> 
> ---
> v2:
> * Remove compile-time check for kernel version

OK, Stephen's comment makes sense.

> ---
>  drivers/net/af_packet/rte_eth_af_packet.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
> index b73b211..7ecea4e 100644
> --- a/drivers/net/af_packet/rte_eth_af_packet.c
> +++ b/drivers/net/af_packet/rte_eth_af_packet.c
> @@ -167,6 +167,22 @@ eth_af_packet_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  	return num_rx;
>  }
>  
> +static inline bool tx_ring_status_unavailable(uint32_t tp_status)
> +{

Minor syntax comment, can you have the 'static inline bool' part in separate
line. And a basic function comment can be good.

Thanks,
ferruh

> +	/*
> +	 * We eliminate the timestamp status from the packet status.
> +	 * This should only matter if timestamping is enabled on the socket,
> +	 * but there is a bug in the kernel which is fixed in newer releases.
> +	 *
> +	 * See the following kernel commit for reference:
> +	 *     commit 171c3b151118a2fe0fc1e2a9d1b5a1570cfe82d2
> +	 *     net: packetmmap: fix only tx timestamp on request
> +	 */
> +	tp_status &= ~(TP_STATUS_TS_SOFTWARE | TP_STATUS_TS_RAW_HARDWARE);
> +
> +	return tp_status != TP_STATUS_AVAILABLE;
> +}
> +
>  /*
>   * Callback to handle sending packets through a real NIC.
>   */
> @@ -212,8 +228,8 @@ eth_af_packet_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  		}
>  
>  		/* point at the next incoming frame */
> -		if ((ppd->tp_status != TP_STATUS_AVAILABLE) &&
> -		    (poll(&pfd, 1, -1) < 0))
> +		if (tx_ring_status_unavailable(ppd->tp_status) &&
> +		    poll(&pfd, 1, -1) < 0)
>  			break;
>  
>  		/* copy the tx frame data */
>