Re: [PATCH v4] pcap: support MTU set

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon,  6 Jun 2022 19:21:47 +0300
Ido Goshen <ido@cgstowernetworks.com> wrote:

> Support rte_eth_dev_set_mtu by pcap vdevs
> Enforce mtu on rx/tx
> 
> Bugzilla ID: 961

This is not really a bug, it is an enhancement specific to your
test setup. It should not be backported to stable.

Since it is change in behavior it might be better to add a vdev argument
for this rather than overloading meaning of MTU. Also, this does not behave
the same as virtio or hardware drivers.



> Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
> 
> ---
> v4:
> 1. Add release notes comment
> 2. Access pmd internals via queue struct
> 3. eth_mtu_set code convention fixes
> 
> v3:
> Preserve pcap behavior to support max size packets by default
> alternative to v2 in order to limit the code change to pcap only and
> avoid abi change.
> Enforce mtu only in case rte_eth_dev_set_mtu was explicitly called.
> 
> v2:
> Preserve pcap behavior to support max size packets by default.
> ---
>  doc/guides/rel_notes/release_22_07.rst |  3 ++
>  drivers/net/pcap/pcap_ethdev.c         | 38 ++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/release_22_07.rst b/doc/guides/rel_notes/release_22_07.rst
> index 0ed4f92820..717191d498 100644
> --- a/doc/guides/rel_notes/release_22_07.rst
> +++ b/doc/guides/rel_notes/release_22_07.rst
> @@ -95,6 +95,9 @@ New Features
>    * Added AH mode support in lookaside protocol (IPsec) for CN9K & CN10K.
>    * Added AES-GMAC support in lookaside protocol (IPsec) for CN9K & CN10K.
>  
> +* **Updated pcap driver.**
> +
> + * Added support for MTU via ``rte_eth_dev_set_mtu``
>  
>  Removed Items
>  -------------
> diff --git a/drivers/net/pcap/pcap_ethdev.c b/drivers/net/pcap/pcap_ethdev.c
> index ec29fd6bc5..db1958f20f 100644
> --- a/drivers/net/pcap/pcap_ethdev.c
> +++ b/drivers/net/pcap/pcap_ethdev.c
> @@ -74,6 +74,7 @@ struct pcap_rx_queue {
>  
>  	/* Contains pre-generated packets to be looped through */
>  	struct rte_ring *pkts;
> +	struct pmd_internals *internals;
>  };
>  
>  struct pcap_tx_queue {
> @@ -82,6 +83,7 @@ struct pcap_tx_queue {
>  	struct queue_stat tx_stat;
>  	char name[PATH_MAX];
>  	char type[ETH_PCAP_ARG_MAXLEN];
> +	struct pmd_internals *internals;
>  };
>  
>  struct pmd_internals {
> @@ -93,6 +95,7 @@ struct pmd_internals {
>  	int single_iface;
>  	int phy_mac;
>  	unsigned int infinite_rx;
> +	uint16_t mtu;
>  };

The mtu is already in dev->data->mtu, why copy it?

>  struct pmd_process_private {
> @@ -278,6 +281,7 @@ eth_pcap_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  	const u_char *packet;
>  	struct rte_mbuf *mbuf;
>  	struct pcap_rx_queue *pcap_q = queue;
> +	struct pmd_internals *internals = pcap_q->internals;
>  	uint16_t num_rx = 0;
>  	uint32_t rx_bytes = 0;
>  	pcap_t *pcap;
> @@ -303,6 +307,12 @@ eth_pcap_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  			break;
>  		}
>  
> +		if (unlikely(header.caplen > internals->mtu)) {
> +			pcap_q->rx_stat.err_pkts++;
> +			rte_pktmbuf_free(mbuf);
> +			break;
> +		}

This doesn't account for VLAN header.


>  		if (header.caplen <= rte_pktmbuf_tailroom(mbuf)) {
>  			/* pcap packet will fit in the mbuf, can copy it */
>  			rte_memcpy(rte_pktmbuf_mtod(mbuf, void *), packet,
> @@ -378,6 +388,7 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  	struct rte_mbuf *mbuf;
>  	struct pmd_process_private *pp;
>  	struct pcap_tx_queue *dumper_q = queue;
> +	struct pmd_internals *internals = dumper_q->internals;
>  	uint16_t num_tx = 0;
>  	uint32_t tx_bytes = 0;
>  	struct pcap_pkthdr header;
> @@ -396,6 +407,12 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  	for (i = 0; i < nb_pkts; i++) {
>  		mbuf = bufs[i];
>  		len = caplen = rte_pktmbuf_pkt_len(mbuf);
> +
> +		if (unlikely(len > internals->mtu)) {
> +			rte_pktmbuf_free(mbuf);
> +			continue;
> +		}

There needs to be a per queue counter any and all drops.
<

> +
>  		if (unlikely(!rte_pktmbuf_is_contiguous(mbuf) &&
>  				len > sizeof(temp_data))) {
>  			caplen = sizeof(temp_data);
> @@ -464,6 +481,7 @@ eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  	struct rte_mbuf *mbuf;
>  	struct pmd_process_private *pp;
>  	struct pcap_tx_queue *tx_queue = queue;
> +	struct pmd_internals *internals = tx_queue->internals;
>  	uint16_t num_tx = 0;
>  	uint32_t tx_bytes = 0;
>  	pcap_t *pcap;
> @@ -479,6 +497,12 @@ eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  	for (i = 0; i < nb_pkts; i++) {
>  		mbuf = bufs[i];
>  		len = rte_pktmbuf_pkt_len(mbuf);
> +
> +		if (unlikely(len > internals->mtu)) {
> +			rte_pktmbuf_free(mbuf);
> +			continue;
> +		}
> +
>  		if (unlikely(!rte_pktmbuf_is_contiguous(mbuf) &&
>  				len > sizeof(temp_data))) {
>  			PMD_LOG(ERR,
> @@ -807,6 +831,16 @@ 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;
> +
> +	PMD_LOG(INFO, "MTU set %s %u\n", dev->device->name, mtu);
> +	internals->mtu = mtu;
> +	return 0;
> +}

If you drop internals->mtu (redundant) then this just becomes stub (ie return 0)

> +
>  static inline void
>  infinite_rx_ring_free(struct rte_ring *pkts)
>  {
> @@ -878,6 +912,7 @@ eth_rx_queue_setup(struct rte_eth_dev *dev,
>  	pcap_q->mb_pool = mb_pool;
>  	pcap_q->port_id = dev->data->port_id;
>  	pcap_q->queue_id = rx_queue_id;
> +	pcap_q->internals = internals;
>  	dev->data->rx_queues[rx_queue_id] = pcap_q;
>  
>  	if (internals->infinite_rx) {
> @@ -952,6 +987,7 @@ eth_tx_queue_setup(struct rte_eth_dev *dev,
>  
>  	pcap_q->port_id = dev->data->port_id;
>  	pcap_q->queue_id = tx_queue_id;
> +	pcap_q->internals = internals;
>  	dev->data->tx_queues[tx_queue_id] = pcap_q;
>  
>  	return 0;
> @@ -1004,6 +1040,7 @@ static const struct eth_dev_ops ops = {
>  	.link_update = eth_link_update,
>  	.stats_get = eth_stats_get,
>  	.stats_reset = eth_stats_reset,
> +	.mtu_set = eth_mtu_set,
>  };
>  
>  static int
> @@ -1233,6 +1270,7 @@ pmd_init_internals(struct rte_vdev_device *vdev,
>  		.addr_bytes = { 0x02, 0x70, 0x63, 0x61, 0x70, iface_idx++ }
>  	};
>  	(*internals)->phy_mac = 0;
> +	(*internals)->mtu = RTE_ETH_PCAP_SNAPLEN;


Use dev->data->mtu not internal value.


>  	data = (*eth_dev)->data;
>  	data->nb_rx_queues = (uint16_t)nb_rx_queues;
>  	data->nb_tx_queues = (uint16_t)nb_tx_queues;