Re: [dpdk-dev] [PATCH 19.08 v2] net/pcap: enable infinitely rxing a pcap file

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

On 6/5/2019 1:46 PM, Ferriter, Cian wrote:
> Adding in my changelog at the top of this, since I forgot to add it in the original mail:
> 
> v2:
> * Rework the method of filling the ring to infinitely rx from
> 	* Avoids potential huge allocation of mbufs
> 	* Removes double allocation of mbufs used during queue setup
> * rename count_packets_in_pcaps to count_packets_in_pcap
> * initialize pcap_pkt_count in count_packets_in_pcap
> * use RTE_PMD_REGISTER_PARAM_STRING <0|1> rather than <int>
> * replace calls to rte_panic with proper error returning
> * count rx and tx stat bytes in pcap_rx_infinite and tx_drop
> * make internals->infinite_rx = infinite_rx assignment unconditional
> * add cleanup for infinite_rx in eth_dev_close and pmd_pcap_remove
> * add cleanup when multi seg mbufs are found
> * add some clarifications to the documentation update
> 
>> -----Original Message-----
>> From: Ferriter, Cian
>> Sent: 05 June 2019 12:56
>> To: Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
>> <ferruh.yigit@intel.com>; Mcnamara, John <john.mcnamara@intel.com>;
>> Kovacevic, Marko <marko.kovacevic@intel.com>
>> Cc: dev@dpdk.org; Ferriter, Cian <cian.ferriter@intel.com>
>> Subject: [PATCH 19.08 v2] net/pcap: enable infinitely rxing a pcap file
>>
>> It can be useful to use pcap files for some rudimental performance testing.
>> This patch enables this functionality in the pcap driver.
>>
>> At a high level, this works by creaing a ring of sufficient size to store the
>> packets in the pcap file passed to the application. When the rx function for
>> this mode is called, packets are dequeued from the ring for use by the
>> application and also enqueued back on to the ring to be "received" again.
>>
>> A tx_drop mode is also added since transmitting to a tx_pcap file isn't
>> desirable at a high traffic rate.
>>
>> Jumbo frames are not supported in this mode. When filling the ring at rx
>> queue setup time, the presence of multi segment mbufs is checked for.
>> The PMD will exit on detection of these multi segment mbufs.
>>
>> Signed-off-by: Cian Ferriter <cian.ferriter@intel.com>
>> ---
>>  doc/guides/nics/pcap_ring.rst   |  19 +++
>>  drivers/net/pcap/rte_eth_pcap.c | 268
>> ++++++++++++++++++++++++++++++--
>>  2 files changed, 277 insertions(+), 10 deletions(-)
>>
>> diff --git a/doc/guides/nics/pcap_ring.rst b/doc/guides/nics/pcap_ring.rst
>> index c1ef9196b..b272e6fe3 100644
>> --- a/doc/guides/nics/pcap_ring.rst
>> +++ b/doc/guides/nics/pcap_ring.rst
>> @@ -106,6 +106,25 @@ Runtime Config Options
>>
>>     --vdev 'net_pcap0,iface=eth0,phy_mac=1'
>>
>> +- Use the RX PCAP file to infinitely receive packets
>> +
>> + In case ``rx_pcap=`` configuration is set, user may want to use the
>> + selected PCAP file for rudimental performance testing. This can be done
>> with a ``devarg`` ``infinite_rx``, for example::
>> +
>> +   --vdev 'net_pcap0,rx_pcap=file_rx.pcap,infinite_rx=1,tx_drop=1'

Can be good to highlight that this flag is not per queue, but should be provided
once (explictly once since code checks it) per Rx.

>> +
>> + When this mode is used, it is recommended to use the ``tx_drop``
>> ``devarg``.
>> +
>> + This option is device wide, so all queues on a device will either have this
>> enabled or disabled.
>> +
>> +- Drop all packets on transmit
>> +
>> + The user may want to drop all packets on tx for a device. This can be done
>> with the ``tx_drop`` ``devarg``, for example::
>> +
>> +   --vdev 'net_pcap0,rx_pcap=file_rx.pcap,tx_drop=1'
>> +
>> + One tx drop queue is created for each rxq on that device.

Can we drop the ``tx_drop`` completely?

What happens when no 'tx_pcap' or 'tx_iface' provided at all, to imply the tx_drop?

<...>

>> @@ -1105,7 +1290,8 @@ static int
>>  eth_from_pcaps(struct rte_vdev_device *vdev,
>>  		struct pmd_devargs *rx_queues, const unsigned int
>> nb_rx_queues,
>>  		struct pmd_devargs *tx_queues, const unsigned int
>> nb_tx_queues,
>> -		int single_iface, unsigned int using_dumpers)
>> +		int single_iface, unsigned int using_dumpers,
>> +		unsigned int infinite_rx, unsigned int tx_drop)


The argument list is keep increasing. What happens is 'pmd_pcap_probe()'
processes the user input (devargs) and passes the processed output to this
function to create ethdev.
What do you think gathering all processed output to a struct and pass it the
this function, in a patch before this patch? Like:

struct pmd_devargs_all {
  struct pmd_devargs pcaps;
  struct pmd_devargs dumpers;
  int single_iface;
  unsigned intis_tx_pcap;
};

And add 'unsigned int infinite_rx;' into that struct in this patch.

<...>

>> @@ -1148,6 +1342,7 @@ pmd_pcap_probe(struct rte_vdev_device *dev)  {
>>  	const char *name;
>>  	unsigned int is_rx_pcap = 0, is_tx_pcap = 0;
>> +	unsigned int infinite_rx = 0, infinite_rx_arg_cnt = 0, tx_drop = 0;

Is  initial value required for 'infinite_rx_arg_cnt '?

>>  	struct rte_kvargs *kvlist;
>>  	struct pmd_devargs pcaps = {0};
>>  	struct pmd_devargs dumpers = {0};
>> @@ -1216,7 +1411,25 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
>>  	is_rx_pcap = rte_kvargs_count(kvlist, ETH_PCAP_RX_PCAP_ARG) ? 1
>> : 0;
>>  	pcaps.num_of_queue = 0;
>>
>> +	infinite_rx_arg_cnt = rte_kvargs_count(kvlist,
>> +			ETH_PCAP_INFINITE_RX_ARG);

Can move this under 'is_rx_pcap', since this value only make sense when Rx is pcap.

>> +
>>  	if (is_rx_pcap) {
>> +		/*
>> +		 * We check whether we want to infinitely rx the pcap file.
>> +		 */
>> +		if (infinite_rx_arg_cnt == 1) {
>> +			ret = rte_kvargs_process(kvlist,
>> +					ETH_PCAP_INFINITE_RX_ARG,
>> +					&get_infinite_rx_arg, &infinite_rx);
>> +			if (ret < 0)
>> +				goto free_kvlist;
>> +		} else if (infinite_rx_arg_cnt >= 1) {

I guess it should be ">" instead of ">="

<...>

>> @@ -1285,8 +1512,12 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
>>  		goto free_kvlist;
>>  	}
>>
>> +	PMD_LOG(INFO, "Configure pmd_pcap: infinite_rx is %s",
>> +			infinite_rx ? "enabled" : "disabled");

What do you think printing the message when feature is requested? Instead of
printing each time that it is disabled, which is default behaviour.

<...>

>> @@ -1318,6 +1550,20 @@ pmd_pcap_remove(struct rte_vdev_device *dev)
>>  			eth_dev->data->mac_addrs = NULL;
>>  	}
>>
>> +	/* Device wide flag, but cleanup must be performed per queue. */
>> +	if (internals->infinite_rx) {
>> +		for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
>> +			struct pcap_rx_queue *pcap_q = &internals-
>>> rx_queue[i];
>> +			struct rte_mbuf *pcap_buf;
>> +
>> +			while (!rte_ring_dequeue(pcap_q->pkts,
>> +					(void **)&pcap_buf))
>> +				rte_pktmbuf_free(pcap_buf);
>> +
>> +			rte_ring_free(pcap_q->pkts);
>> +		}
>> +	}

Can it possible to call 'eth_dev_close()' which seems dublicating the above code?