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

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

Hi Ferruh,

Thanks for the review.

I've send a v3 and responded to your comments below.

Thanks,
Cian

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: 12 June 2019 15:10
> To: Ferriter, Cian <cian.ferriter@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; Kovacevic, Marko
> <marko.kovacevic@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 19.08 v2] net/pcap: enable infinitely rxing a pcap file
> 
> 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.
> 

Added to the docs in the next version.

> >> +
> >> + 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?
> 

This sound like nice default behavior to have. I've updated the latest version to implement this and I've removed the tx_drop args parsing and related doc section.

> <...>
> 
> >> @@ -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.
> 

Good idea, I've implemented these changes in the next version.

> <...>
> 
> >> @@ -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 '?
> 

I don't think the initial value is required. I was just following the convention used with the 'is_rx_pcap' and 'is_tx_pcap' variables.
I think that the below comment takes care of this, since we can move 'infinite_rx_arg_cnt' under 'is_rx_pcap' and assign directly to the output of the 'rte_kvargs_count'.

> >>  	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.
> 

True, that makes sense. Fixed in the next version.

> >> +
> >>  	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 ">="
> 

Ooops, you are correct. The '=' part is pointless here. Fixed in the next version.

> <...>
> 
> >> @@ -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.
> 

I agree, there's no point in mentioning that infinite_rx has been disabled if the user hasn't requested it in the args.
Fixed in the new version.

> <...>
> 
> >> @@ -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?

This makes sense. Added this in the next version.