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 <pogonarumi...@gmail.com>
> Signed-off-by: Tudor Cornea <tudor.cor...@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 */
>
