From: Ferruh Yigit <ferruh.yigit@intel.com>
To: ido goshen <ido@cgstowernetworks.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 1/2] net/pcap: multiple queues fix
Date: Mon, 18 Jun 2018 09:25:27 +0100 [thread overview]
Message-ID: <139aaeb9-d45b-c83d-7843-03c7385bb4e2@intel.com> (raw)
In-Reply-To: <1529163397-88330-1-git-send-email-ido@cgstowernetworks.com>
On 6/16/2018 4:36 PM, ido goshen wrote:
> Change open_rx/tx_pcap/iface functions to open only a single pcap/dumper
> and not loop num_of_queue times
> The num_of_queue loop is already acheived by the caller rte_kvargs_process
You are right, thanks for fixing this, a few comments below.
>
> Fixes:
> 1. Opens N requested pcaps/dumpers instead of N^2
> 2. Leak of pcap/dumper's which are being overwritten by
> the sequential calls to open_rx/tx_pcap/iface functions
> 3. Use the filename/iface args per queue and not just the last one
> that overwrites the previous names
Please add a "Fixes: xx" line, that is to trace initial commit the issue
introduced. More details in contribution guide.
Also please add "Cc: stable@dpdk.org" to be sure patch sent to stable tree too
and to help stable tree maintainers"
>
> Signed-off-by: ido goshen <ido@cgstowernetworks.com>
<...>
> @@ -958,15 +950,8 @@ struct pmd_devargs {
> * We check whether we want to open a RX stream from a real NIC or a
> * pcap file
> */
> - pcaps.num_of_queue = rte_kvargs_count(kvlist, ETH_PCAP_RX_PCAP_ARG);
> - if (pcaps.num_of_queue)
> - is_rx_pcap = 1;
> - else
> - pcaps.num_of_queue = rte_kvargs_count(kvlist,
> - ETH_PCAP_RX_IFACE_ARG);
> -
> - if (pcaps.num_of_queue > RTE_PMD_PCAP_MAX_QUEUES)
> - pcaps.num_of_queue = RTE_PMD_PCAP_MAX_QUEUES;
> + is_rx_pcap = rte_kvargs_count(kvlist, ETH_PCAP_RX_PCAP_ARG) ? 1 : 0;
> + pcaps.num_of_queue = 0;
>
> if (is_rx_pcap)
> ret = rte_kvargs_process(kvlist, ETH_PCAP_RX_PCAP_ARG,
> @@ -975,6 +960,10 @@ struct pmd_devargs {
> ret = rte_kvargs_process(kvlist, ETH_PCAP_RX_IFACE_ARG,
> &open_rx_iface, &pcaps);
>
> + if (pcaps.num_of_queue > RTE_PMD_PCAP_MAX_QUEUES)
> + pcaps.num_of_queue = RTE_PMD_PCAP_MAX_QUEUES;
Here is late for this check. You may be already access to rx->queue[],
tx->queue[] out of boundary at this point.
You should either check this value before rte_kvargs_process(), via
rte_kvargs_count(), OR you should add this check into callback functions.
> if (ret < 0)
> goto free_kvlist;
>
> @@ -982,15 +971,8 @@ struct pmd_devargs {
> * We check whether we want to open a TX stream to a real NIC or a
> * pcap file
> */
> - dumpers.num_of_queue = rte_kvargs_count(kvlist, ETH_PCAP_TX_PCAP_ARG);
> - if (dumpers.num_of_queue)
> - is_tx_pcap = 1;
> - else
> - dumpers.num_of_queue = rte_kvargs_count(kvlist,
> - ETH_PCAP_TX_IFACE_ARG);
> -
> - if (dumpers.num_of_queue > RTE_PMD_PCAP_MAX_QUEUES)
> - dumpers.num_of_queue = RTE_PMD_PCAP_MAX_QUEUES;
> + is_tx_pcap = rte_kvargs_count(kvlist, ETH_PCAP_TX_PCAP_ARG) ? 1 : 0;
Is "is_rx_pcap" or "is_tx_pcap" flags really required? Is there anything
preventing have a mixture of interface and pcap in multi queue case? With the
changes you are doing, I guess we can remove these checks and call following
sequentially:
rte_kvargs_process(kvlist, ETH_PCAP_RX_PCAP_ARG..)
rte_kvargs_process(kvlist, ETH_PCAP_RX_IFACE_ARG ..)
What do you think?
But please be sure the fix and refactor patches are separate, so that fix patch
can be backported to stable trees. But refactor patches won't be backported.
next prev parent reply other threads:[~2018-06-18 8:27 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-16 15:36 ido goshen
2018-06-16 15:36 ` [dpdk-dev] [PATCH 2/2] net/pcap: duplicate code consolidation ido goshen
2018-06-18 8:25 ` Ferruh Yigit [this message]
2018-06-19 9:45 ` [dpdk-dev] [PATCH 1/2] net/pcap: multiple queues fix Ido Goshen
2018-06-19 10:00 ` Ferruh Yigit
Reply instructions:
You may reply publicly 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=139aaeb9-d45b-c83d-7843-03c7385bb4e2@intel.com \
--to=ferruh.yigit@intel.com \
--cc=dev@dpdk.org \
--cc=ido@cgstowernetworks.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).