From: Ido Goshen <Ido@cgstowernetworks.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 1/2] net/pcap: multiple queues fix
Date: Tue, 19 Jun 2018 09:45:08 +0000 [thread overview]
Message-ID: <AM5PR0901MB14278823FCDB64CA82A8AB6AD6700@AM5PR0901MB1427.eurprd09.prod.outlook.com> (raw)
In-Reply-To: <139aaeb9-d45b-c83d-7843-03c7385bb4e2@intel.com>
See Inline prefixed with [ido]
-----Original Message-----
From: Ferruh Yigit <ferruh.yigit@intel.com>
Sent: Monday, June 18, 2018 11:25 AM
To: Ido Goshen <Ido@cgstowernetworks.com>
Cc: dev@dpdk.org
Subject: Re: [PATCH 1/2] net/pcap: multiple queues fix
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"
[ido] as far as I can trace back this is from day one (4c17330 pcap: add new driver), Would "Fixes: 4c17330" be ok?
>
> 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.
[ido] good catch - will fix that
> 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?
[ido] nice idea - will test if they can co-exist
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-19 9:45 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 ` [dpdk-dev] [PATCH 1/2] net/pcap: multiple queues fix Ferruh Yigit
2018-06-19 9:45 ` Ido Goshen [this message]
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=AM5PR0901MB14278823FCDB64CA82A8AB6AD6700@AM5PR0901MB1427.eurprd09.prod.outlook.com \
--to=ido@cgstowernetworks.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@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
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).