Re: [PATCH v3] pcap: support MTU set

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

On 5/30/2022 11:36 AM, Ido Goshen wrote:
> Support rte_eth_dev_set_mtu by pcap vdevs
> Enforce mtu on rx/tx
> 

Still not sure about enforcing MTU on pcap, but please find comments on 
mechanical issues

> Bugzilla ID: 961
> Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
> 
> ---
> 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.
> ---
>   drivers/net/pcap/pcap_ethdev.c | 44 +++++++++++++++++++++++++++++++---
>   1 file changed, 41 insertions(+), 3 deletions(-)
> 

Is documentation needs to be updated as well?

And what do you think to update release notes for this update?

> diff --git a/drivers/net/pcap/pcap_ethdev.c b/drivers/net/pcap/pcap_ethdev.c
> index ec29fd6bc5..2e7fff9579 100644
> --- a/drivers/net/pcap/pcap_ethdev.c
> +++ b/drivers/net/pcap/pcap_ethdev.c
> @@ -93,6 +93,7 @@ struct pmd_internals {
>   	int single_iface;
>   	int phy_mac;
>   	unsigned int infinite_rx;
> +	int is_mtu_set;
>   };
>   
>   struct pmd_process_private {
> @@ -278,11 +279,13 @@ 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 rte_eth_dev *dev = &rte_eth_devices[pcap_q->port_id];
> +	struct pmd_internals *internals = dev->data->dev_private;

'rte_eth_devices[]' needs to be used for 'process_private' but lets not 
tie it to access 'dev_private'.

You can add "struct pmd_internals *" to the "struct pcap_rx_queue" & 
"struct pcap_tx_queue" structs for the access. Please check null PMD for 
sample.

>   	uint16_t num_rx = 0;
>   	uint32_t rx_bytes = 0;
>   	pcap_t *pcap;
>   
> -	pp = rte_eth_devices[pcap_q->port_id].process_private;
> +	pp = dev->process_private;
>   	pcap = pp->rx_pcap[pcap_q->queue_id];
>   
>   	if (unlikely(pcap == NULL || nb_pkts == 0))
> @@ -303,6 +306,13 @@ eth_pcap_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>   			break;
>   		}
>   
> +		if (unlikely(header.caplen > dev->data->mtu) &&
> +				internals->is_mtu_set) {
> +			pcap_q->rx_stat.err_pkts++;
> +			rte_pktmbuf_free(mbuf);
> +			break;
> +		}
> +
>   		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,8 @@ 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 rte_eth_dev *dev = &rte_eth_devices[dumper_q->port_id];
> +	struct pmd_internals *internals = dev->data->dev_private;
>   	uint16_t num_tx = 0;
>   	uint32_t tx_bytes = 0;
>   	struct pcap_pkthdr header;
> @@ -385,7 +397,7 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>   	unsigned char temp_data[RTE_ETH_PCAP_SNAPLEN];
>   	size_t len, caplen;
>   
> -	pp = rte_eth_devices[dumper_q->port_id].process_private;
> +	pp = dev->process_private;
>   	dumper = pp->tx_dumper[dumper_q->queue_id];
>   
>   	if (dumper == NULL || nb_pkts == 0)
> @@ -396,6 +408,13 @@ 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 > dev->data->mtu) &&
> +				internals->is_mtu_set) {

It is possible to save only some part of the packet to the pcap file, 
please check snaplen patch [1], how MTU config should work with this 
feature?

[1]
https://patchwork.dpdk.org/project/dpdk/patch/20220313112638.3945-1-laitianli@tom.com/

> +			rte_pktmbuf_free(mbuf);
> +			continue;

Normally a PMD should not silently free a packet itself, it should 
return error and application will decide to free the packet or not.

> +		}
> +
>   		if (unlikely(!rte_pktmbuf_is_contiguous(mbuf) &&
>   				len > sizeof(temp_data))) {
>   			caplen = sizeof(temp_data);
> @@ -464,13 +483,15 @@ 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 rte_eth_dev *dev = &rte_eth_devices[tx_queue->port_id];
> +	struct pmd_internals *internals = dev->data->dev_private;
>   	uint16_t num_tx = 0;
>   	uint32_t tx_bytes = 0;
>   	pcap_t *pcap;
>   	unsigned char temp_data[RTE_ETH_PCAP_SNAPLEN];
>   	size_t len;
>   
> -	pp = rte_eth_devices[tx_queue->port_id].process_private;
> +	pp = dev->process_private;
>   	pcap = pp->tx_pcap[tx_queue->queue_id];
>   
>   	if (unlikely(nb_pkts == 0 || pcap == NULL))
> @@ -479,6 +500,13 @@ 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 > dev->data->mtu) &&
> +				internals->is_mtu_set) {
> +			rte_pktmbuf_free(mbuf);
> +			continue;

ditto

> +		}
> +
>   		if (unlikely(!rte_pktmbuf_is_contiguous(mbuf) &&
>   				len > sizeof(temp_data))) {
>   			PMD_LOG(ERR,
> @@ -807,6 +835,14 @@ eth_stats_reset(struct rte_eth_dev *dev)
>   	return 0;
>   }
>   
> +static int eth_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)

Please follow coding convention to have return type in a separate line:
   static int
   eth_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
   {

> +{
> +	PMD_LOG(INFO, "mtu set %s %u\n", dev->device->name, mtu);

Can you please move the log after variable declerations. And can be good 
to capitilise MTU in the log.

> +	struct pmd_internals *internals = dev->data->dev_private;
> +	internals->is_mtu_set = 1;
> +	return 0;
> +}
> +
>   static inline void
>   infinite_rx_ring_free(struct rte_ring *pkts)
>   {
> @@ -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)->is_mtu_set = 0;
>   	data = (*eth_dev)->data;
>   	data->nb_rx_queues = (uint16_t)nb_rx_queues;
>   	data->nb_tx_queues = (uint16_t)nb_tx_queues;