Re: [dpdk-dev] [PATCH 1/2] net/pcap: multiple queues fix

[Ferruh Yigit <ferruh.yigit@intel.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.