DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
From: "Ferriter, Cian" <cian.ferriter@intel.com>
To: "Yigit, Ferruh" <ferruh.yigit@intel.com>,
	"Richardson, Bruce" <bruce.richardson@intel.com>,
	"Mcnamara, John" <john.mcnamara@intel.com>,
	"Kovacevic, Marko" <marko.kovacevic@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 19.08 v2] net/pcap: enable infinitely rxing a pcap file
Date: Fri, 14 Jun 2019 14:48:12 +0000
Message-ID: <579B86492DFB364BBA627A48FB30C90E75D6838F@IRSMSX102.ger.corp.intel.com> (raw)
In-Reply-To: <7ac9657f-e67b-ffc0-1aa3-91693f497aeb@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.

      reply index

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-05 11:56 Cian Ferriter
2019-06-05 12:46 ` Ferriter, Cian
2019-06-12 14:10   ` Ferruh Yigit
2019-06-14 14:48     ` Ferriter, Cian [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=579B86492DFB364BBA627A48FB30C90E75D6838F@IRSMSX102.ger.corp.intel.com \
    --to=cian.ferriter@intel.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=john.mcnamara@intel.com \
    --cc=marko.kovacevic@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org dpdk-dev@archiver.kernel.org
	public-inbox-index dpdk-dev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox