Re: [PATCH v2] pcap: support MTU set

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

On 7/4/2023 10:02 PM, Stephen Hemminger wrote:
> Support rte_eth_dev_set_mtu for pcap driver when the
> pcap device is convigured to point to a network interface.
> 
> This is rebased an consolidated from earlier version.
> Added support for FreeBSD.
> 

As far as I understand motivation is to make pcap PMD behave close the
physical NIC and able to test the application MTU feature.
If so, Ido's v4 was simpler, which doesn't distinguish if pcap backed by
physical interface or .pcap file.
What was wrong with that approach?

> Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v2 - fix FreeBSD part
> 
>  drivers/net/pcap/pcap_ethdev.c        | 49 ++++++++++++++++++++++++++-
>  drivers/net/pcap/pcap_osdep.h         |  1 +
>  drivers/net/pcap/pcap_osdep_freebsd.c | 23 +++++++++++++
>  drivers/net/pcap/pcap_osdep_linux.c   | 21 ++++++++++++
>  drivers/net/pcap/pcap_osdep_windows.c |  7 ++++
>  5 files changed, 100 insertions(+), 1 deletion(-)
> 

Better to update driver documentation and release notes too.

> diff --git a/drivers/net/pcap/pcap_ethdev.c b/drivers/net/pcap/pcap_ethdev.c
> index bfec0850456f..672f42d30d8e 100644
> --- a/drivers/net/pcap/pcap_ethdev.c
> +++ b/drivers/net/pcap/pcap_ethdev.c
> @@ -5,6 +5,7 @@
>   */
>  
>  #include <stdlib.h>
> +#include <stdbool.h>
>  #include <time.h>
>  
>  #include <pcap.h>
> @@ -495,8 +496,13 @@ eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  		 */
>  		ret = pcap_sendpacket(pcap,
>  			rte_pktmbuf_read(mbuf, 0, len, temp_data), len);
> -		if (unlikely(ret != 0))
> +		if (unlikely(ret != 0)) {
> +			/* Oversize packet dropped due to MTU */
> +			if (errno == EMSGSIZE)
> +				continue;

This will leak mbuf, since loop continues application will assume driver
sent the packet and freed mbuf. Ido's version does it right.

>  			break;
> +		}
> +
>  		num_tx++;
>  		tx_bytes += len;
>  		rte_pktmbuf_free(mbuf);
> @@ -808,6 +814,46 @@ eth_stats_reset(struct rte_eth_dev *dev)
>  	return 0;
>  }
>  
> +static int
> +eth_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
> +{
> +	struct pmd_internals *internals = dev->data->dev_private;
> +	unsigned int i;
> +	bool is_supported = false;
> +	int ret;
> +
> +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +		struct pcap_rx_queue *queue = &internals->rx_queue[i];
> +
> +		if ((strcmp(queue->type, ETH_PCAP_IFACE_ARG) == 0) ||
> +		    (strcmp(queue->type, ETH_PCAP_RX_IFACE_ARG) == 0) ||
> +		    (strcmp(queue->type, ETH_PCAP_RX_IFACE_IN_ARG) == 0)) {
> +			ret = osdep_iface_mtu_set(queue->name, mtu);
> +			if (ret < 0)
> +				return ret;
> +			is_supported = true;
> +		}

If there are multiple devices (each pcap queue represents a device), and
one in the middle one fails to set MTU, there will be mixed MTU values
in devices.

> +	}
> +
> +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> +		struct pcap_tx_queue *queue = &internals->tx_queue[i];
> +
> +		if ((strcmp(queue->type, ETH_PCAP_IFACE_ARG) == 0) ||

Rx devices and Tx devices can be same device, causing duplicated MTU set
for same device, but it is hard to detect if they are same devices.
At least it is easier to detect when queue type is 'ETH_PCAP_IFACE_ARG',
what about set MTU once for this queue type?