Re: [dpdk-dev] [PATCH] net/af_packet: fix ignoring full ring on tx

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

On 8/20/2021 2:39 PM, Tudor Cornea wrote:
> The poll call can return POLLERR which is ignored, or it can return
> POLLOUT, even if there are no free frames in the mmap-ed area.
> 
> We can account for both of these cases by re-checking if the next
> frame is empty before writing into it.
> 
> We also now eliminate the timestamp status from the frame status.
> 

Hi Tudor,

Would you mind separate timestamp status fix to its own patch? I think better to
fix 'ignoring full Tx ring' first, to not make it dependent to timestamp patch.

> Signed-off-by: Mihai Pogonaru <pogonarumihai@gmail.com>
> Signed-off-by: Tudor Cornea <tudor.cornea@gmail.com>
> ---
>  drivers/net/af_packet/rte_eth_af_packet.c | 47 +++++++++++++++++++++++++++++--
>  1 file changed, 45 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..3845df5 100644
> --- a/drivers/net/af_packet/rte_eth_af_packet.c
> +++ b/drivers/net/af_packet/rte_eth_af_packet.c
> @@ -167,6 +167,12 @@ eth_af_packet_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  	return num_rx;
>  }
>  
> +static inline __u32 tx_ring_status_remove_ts(volatile __u32 *tp_status)
> +{
> +	return *tp_status &
> +		~(TP_STATUS_TS_SOFTWARE | TP_STATUS_TS_RAW_HARDWARE);

I can see 'TP_STATUS_TS_SYS_HARDWARE' is deprecated, and I assume in the kernel
versions the bug exists, this flag is not set, but can you please confirm?

> +}
> +
>  /*
>   * Callback to handle sending packets through a real NIC.
>   */
> @@ -211,9 +217,41 @@ eth_af_packet_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  			}
>  		}
>  
> +		/*
> +		 * We must 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.
> +
> +		 * 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).
> +		 */
> +

Can you give some more details on this bug, any link etc..

And does it only seen with veth, if so I wonder if we can ignore it, not sure
how common to use af_packet PMD over veth interface, do you have this usecase?

And if only specific kernel versions impacted from this bug, what do you think
about adding kernel version check to reduce the scope of the fix in af_packet PMD.

>  		/* point at the next incoming frame */
> -		if ((ppd->tp_status != TP_STATUS_AVAILABLE) &&
> -		    (poll(&pfd, 1, -1) < 0))
> +		if ((tx_ring_status_remove_ts(&ppd->tp_status)
> +			!= TP_STATUS_AVAILABLE) && (poll(&pfd, 1, -1) < 0))
> +			break;
> +
> +		/*
> +		 * Poll can return POLLERR if the interface is down or POLLOUT,
> +		 * even if there are no extra buffers available.
> +		 * This happens, because packet_poll() calls datagram_poll()
> +		 * which checks the space left in the socket buffer and in the
> +		 * case of packet_mmap the default socket buffer length
> +		 * doesn't match the requested size for the tx_ring so there
> +		 * is always space left in socket buffer, which doesn't seem
> +		 * to be correlated to the requested size for the tx_ring
> +		 * in packet_mmap.
> +		 */
> +		if (tx_ring_status_remove_ts(&ppd->tp_status)
> +			!= TP_STATUS_AVAILABLE)
>  			break;

In this case should we break or poll again?

If 'POLLERR' is received when interface is down, it makes sense to break. Do you
know if is there any other case that 'POLLERR' is returned?

And for 'POLLOUT', when exactly this event sent? If 'POLLOUT' received, can we
poll again to wait Tx ring available to send more packets?

>  
>  		/* copy the tx frame data */
> @@ -242,6 +280,11 @@ eth_af_packet_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  		rte_pktmbuf_free(mbuf);
>  	}
>  
> +	/*
> +	 * We might have to ignore a few more errnos here since the packets
> +	 * remain in the mmap-ed queue and will be sent later, presumably.
> +	 */
> +

Can you please describe above comment more? What errors ignored?
And won't all packets sent will below sendto() call?

>  	/* kick-off transmits */
>  	if (sendto(pkt_q->sockfd, NULL, 0, MSG_DONTWAIT, NULL, 0) == -1 &&
>  			errno != ENOBUFS && errno != EAGAIN) {
>