Re: [dpdk-dev] [PATCH v2] net/pcap: fix infinite Rx with large files

["Ferriter, Cian" <cian.ferriter@intel.com>]

The new error message looks great.

As I've already given my ack, I'm happy for this to be applied.

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Thursday 4 February 2021 16:51
> To: Ferriter, Cian <cian.ferriter@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org; stable@dpdk.org
> Subject: [PATCH v2] net/pcap: fix infinite Rx with large files
> 
> Packet forwarding is not working when infinite Rx feature is used with
> large .pcap files that has high number of packets.
> 
> The problem is number of allocated mbufs are less than the infinite Rx
> ring size, and all mbufs consumed to fill the ring, so there is no mbuf
> left for forwarding.
> 
> Current logic can not detect that infinite Rx ring is not filled
> completely and no more mbufs left, and setup continues which leads
> silent fail on packet forwarding.
> 
> There isn't much can be done when there is not enough mbuf for the given
> .pcap file, so additional checks added to detect the case and fail
> explicitly with an error log.
> 
> Bugzilla ID: 595
> Fixes: a3f5252e5cbd ("net/pcap: enable infinitely Rx a pcap file")
> Cc: stable@dpdk.org
> 
> Reported-by: Cian Ferriter <cian.ferriter@intel.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Acked-by: Cian Ferriter <cian.ferriter@intel.com>
> ---
> v2:
> * Updated log message
> ---
>  drivers/net/pcap/rte_eth_pcap.c | 40 ++++++++++++++++++++-------------
>  1 file changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/pcap/rte_eth_pcap.c
> b/drivers/net/pcap/rte_eth_pcap.c
> index c7751b7ba742..90f5d75ea87f 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -735,6 +735,17 @@ eth_stats_reset(struct rte_eth_dev *dev)
>  	return 0;
>  }
> 
> +static inline void
> +infinite_rx_ring_free(struct rte_ring *pkts)
> +{
> +	struct rte_mbuf *bufs;
> +
> +	while (!rte_ring_dequeue(pkts, (void **)&bufs))
> +		rte_pktmbuf_free(bufs);
> +
> +	rte_ring_free(pkts);
> +}
> +
>  static int
>  eth_dev_close(struct rte_eth_dev *dev)
>  {
> @@ -753,7 +764,6 @@ eth_dev_close(struct rte_eth_dev *dev)
>  	if (internals->infinite_rx) {
>  		for (i = 0; i < dev->data->nb_rx_queues; i++) {
>  			struct pcap_rx_queue *pcap_q = &internals-
> >rx_queue[i];
> -			struct rte_mbuf *pcap_buf;
> 
>  			/*
>  			 * 'pcap_q->pkts' can be NULL if 'eth_dev_close()'
> @@ -762,11 +772,7 @@ eth_dev_close(struct rte_eth_dev *dev)
>  			if (pcap_q->pkts == NULL)
>  				continue;
> 
> -			while (!rte_ring_dequeue(pcap_q->pkts,
> -					(void **)&pcap_buf))
> -				rte_pktmbuf_free(pcap_buf);
> -
> -			rte_ring_free(pcap_q->pkts);
> +			infinite_rx_ring_free(pcap_q->pkts);
>  		}
>  	}
> 
> @@ -835,21 +841,25 @@ eth_rx_queue_setup(struct rte_eth_dev *dev,
>  		while (eth_pcap_rx(pcap_q, bufs, 1)) {
>  			/* Check for multiseg mbufs. */
>  			if (bufs[0]->nb_segs != 1) {
> -				rte_pktmbuf_free(*bufs);
> -
> -				while (!rte_ring_dequeue(pcap_q->pkts,
> -						(void **)bufs))
> -					rte_pktmbuf_free(*bufs);
> -
> -				rte_ring_free(pcap_q->pkts);
> -				PMD_LOG(ERR, "Multiseg mbufs are not
> supported in infinite_rx "
> -						"mode.");
> +				infinite_rx_ring_free(pcap_q->pkts);
> +				PMD_LOG(ERR,
> +					"Multiseg mbufs are not supported in
> infinite_rx mode.");
>  				return -EINVAL;
>  			}
> 
>  			rte_ring_enqueue_bulk(pcap_q->pkts,
>  					(void * const *)bufs, 1, NULL);
>  		}
> +
> +		if (rte_ring_count(pcap_q->pkts) < pcap_pkt_count) {
> +			infinite_rx_ring_free(pcap_q->pkts);
> +			PMD_LOG(ERR,
> +				"Not enough mbufs to accommodate packets
> in pcap file. "
> +				"At least %" PRIu64 " mbufs per queue is
> required.",
> +				pcap_pkt_count);
> +			return -EINVAL;
> +		}
> +
>  		/*
>  		 * Reset the stats for this queue since eth_pcap_rx calls
> above
>  		 * didn't result in the application receiving packets.
> --
> 2.29.2