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

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

[ido goshen]

Fixes: 4c173302c307 ("pcap: add new driver")
Cc: stable@dpdk.org

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

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

Signed-off-by: ido goshen <ido@cgstowernetworks.com>
---
v2:
* fix num of queues check
---
 drivers/net/pcap/rte_eth_pcap.c | 86 +++++++++++++++++------------------------
 1 file changed, 36 insertions(+), 50 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 6bd4a7d..b4f81ac 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -665,19 +665,19 @@ struct pmd_devargs {
 static int
 open_rx_pcap(const char *key, const char *value, void *extra_args)
 {
-	unsigned int i;
 	const char *pcap_filename = value;
 	struct pmd_devargs *rx = extra_args;
 	pcap_t *pcap = NULL;
 
-	for (i = 0; i < rx->num_of_queue; i++) {
-		if (open_single_rx_pcap(pcap_filename, &pcap) < 0)
-			return -1;
+	if (rx->num_of_queue >= RTE_PMD_PCAP_MAX_QUEUES)
+		return -1;
+	if (open_single_rx_pcap(pcap_filename, &pcap) < 0)
+		return -1;
 
-		rx->queue[i].pcap = pcap;
-		rx->queue[i].name = pcap_filename;
-		rx->queue[i].type = key;
-	}
+	rx->queue[rx->num_of_queue].pcap = pcap;
+	rx->queue[rx->num_of_queue].name = pcap_filename;
+	rx->queue[rx->num_of_queue].type = key;
+	rx->num_of_queue++;
 
 	return 0;
 }
@@ -689,19 +689,19 @@ struct pmd_devargs {
 static int
 open_tx_pcap(const char *key, const char *value, void *extra_args)
 {
-	unsigned int i;
 	const char *pcap_filename = value;
 	struct pmd_devargs *dumpers = extra_args;
 	pcap_dumper_t *dumper;
 
-	for (i = 0; i < dumpers->num_of_queue; i++) {
-		if (open_single_tx_pcap(pcap_filename, &dumper) < 0)
-			return -1;
+	if (dumpers->num_of_queue >= RTE_PMD_PCAP_MAX_QUEUES)
+		return -1;
+	if (open_single_tx_pcap(pcap_filename, &dumper) < 0)
+		return -1;
 
-		dumpers->queue[i].dumper = dumper;
-		dumpers->queue[i].name = pcap_filename;
-		dumpers->queue[i].type = key;
-	}
+	dumpers->queue[dumpers->num_of_queue].dumper = dumper;
+	dumpers->queue[dumpers->num_of_queue].name = pcap_filename;
+	dumpers->queue[dumpers->num_of_queue].type = key;
+	dumpers->num_of_queue++;
 
 	return 0;
 }
@@ -732,18 +732,18 @@ struct pmd_devargs {
 static inline int
 open_rx_iface(const char *key, const char *value, void *extra_args)
 {
-	unsigned int i;
 	const char *iface = value;
 	struct pmd_devargs *rx = extra_args;
 	pcap_t *pcap = NULL;
 
-	for (i = 0; i < rx->num_of_queue; i++) {
-		if (open_single_iface(iface, &pcap) < 0)
-			return -1;
-		rx->queue[i].pcap = pcap;
-		rx->queue[i].name = iface;
-		rx->queue[i].type = key;
-	}
+	if (rx->num_of_queue >= RTE_PMD_PCAP_MAX_QUEUES)
+		return -1;
+	if (open_single_iface(iface, &pcap) < 0)
+		return -1;
+	rx->queue[rx->num_of_queue].pcap = pcap;
+	rx->queue[rx->num_of_queue].name = iface;
+	rx->queue[rx->num_of_queue].type = key;
+	rx->num_of_queue++;
 
 	return 0;
 }
@@ -754,18 +754,18 @@ struct pmd_devargs {
 static int
 open_tx_iface(const char *key, const char *value, void *extra_args)
 {
-	unsigned int i;
 	const char *iface = value;
 	struct pmd_devargs *tx = extra_args;
 	pcap_t *pcap;
 
-	for (i = 0; i < tx->num_of_queue; i++) {
-		if (open_single_iface(iface, &pcap) < 0)
-			return -1;
-		tx->queue[i].pcap = pcap;
-		tx->queue[i].name = iface;
-		tx->queue[i].type = key;
-	}
+	if (tx->num_of_queue >= RTE_PMD_PCAP_MAX_QUEUES)
+		return -1;
+	if (open_single_iface(iface, &pcap) < 0)
+		return -1;
+	tx->queue[tx->num_of_queue].pcap = pcap;
+	tx->queue[tx->num_of_queue].name = iface;
+	tx->queue[tx->num_of_queue].type = key;
+	tx->num_of_queue++;
 
 	return 0;
 }
@@ -958,15 +958,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,
@@ -982,15 +975,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;
+	dumpers.num_of_queue = 0;
 
 	if (is_tx_pcap)
 		ret = rte_kvargs_process(kvlist, ETH_PCAP_TX_PCAP_ARG,
-- 
1.9.1

[dpdk-dev] [PATCH v2 2/2] net/pcap: duplicate code consolidation

[ido goshen]

Signed-off-by: ido goshen <ido@cgstowernetworks.com>
---
 drivers/net/pcap/rte_eth_pcap.c | 77 +++++++++++++++++++++--------------------
 1 file changed, 40 insertions(+), 37 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index b4f81ac..b21930b 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -658,6 +658,22 @@ struct pmd_devargs {
 	.stats_reset = eth_stats_reset,
 };
 
+static int
+add_queue(struct pmd_devargs *pmd, const char *name, const char *type,
+		pcap_t *pcap, pcap_dumper_t *dumper)
+{
+	if (pmd->num_of_queue >= RTE_PMD_PCAP_MAX_QUEUES)
+		return -1;
+	if (pcap)
+		pmd->queue[pmd->num_of_queue].pcap = pcap;
+	if (dumper)
+		pmd->queue[pmd->num_of_queue].dumper = dumper;
+	pmd->queue[pmd->num_of_queue].name = name;
+	pmd->queue[pmd->num_of_queue].type = type;
+	pmd->num_of_queue++;
+	return 0;
+}
+
 /*
  * Function handler that opens the pcap file for reading a stores a
  * reference of it for use it later on.
@@ -669,15 +685,13 @@ struct pmd_devargs {
 	struct pmd_devargs *rx = extra_args;
 	pcap_t *pcap = NULL;
 
-	if (rx->num_of_queue >= RTE_PMD_PCAP_MAX_QUEUES)
-		return -1;
 	if (open_single_rx_pcap(pcap_filename, &pcap) < 0)
 		return -1;
 
-	rx->queue[rx->num_of_queue].pcap = pcap;
-	rx->queue[rx->num_of_queue].name = pcap_filename;
-	rx->queue[rx->num_of_queue].type = key;
-	rx->num_of_queue++;
+	if (add_queue(rx, pcap_filename, key, pcap, NULL) < 0) {
+		pcap_close(pcap);
+		return -1;
+	}
 
 	return 0;
 }
@@ -693,15 +707,13 @@ struct pmd_devargs {
 	struct pmd_devargs *dumpers = extra_args;
 	pcap_dumper_t *dumper;
 
-	if (dumpers->num_of_queue >= RTE_PMD_PCAP_MAX_QUEUES)
-		return -1;
 	if (open_single_tx_pcap(pcap_filename, &dumper) < 0)
 		return -1;
 
-	dumpers->queue[dumpers->num_of_queue].dumper = dumper;
-	dumpers->queue[dumpers->num_of_queue].name = pcap_filename;
-	dumpers->queue[dumpers->num_of_queue].type = key;
-	dumpers->num_of_queue++;
+	if (add_queue(dumpers, pcap_filename, key, NULL, dumper) < 0) {
+		pcap_dump_close(dumper);
+		return -1;
+	}
 
 	return 0;
 }
@@ -726,48 +738,39 @@ struct pmd_devargs {
 	return 0;
 }
 
-/*
- * Opens a NIC for reading packets from it
- */
 static inline int
-open_rx_iface(const char *key, const char *value, void *extra_args)
+open_iface(const char *key, const char *value, void *extra_args)
 {
 	const char *iface = value;
-	struct pmd_devargs *rx = extra_args;
+	struct pmd_devargs *pmd = extra_args;
 	pcap_t *pcap = NULL;
 
-	if (rx->num_of_queue >= RTE_PMD_PCAP_MAX_QUEUES)
-		return -1;
 	if (open_single_iface(iface, &pcap) < 0)
 		return -1;
-	rx->queue[rx->num_of_queue].pcap = pcap;
-	rx->queue[rx->num_of_queue].name = iface;
-	rx->queue[rx->num_of_queue].type = key;
-	rx->num_of_queue++;
+	if (add_queue(pmd, iface, key, pcap, NULL) < 0) {
+		pcap_close(pcap);
+		return -1;
+	}
 
 	return 0;
 }
 
 /*
+ * Opens a NIC for reading packets from it
+ */
+static inline int
+open_rx_iface(const char *key, const char *value, void *extra_args)
+{
+	return open_iface(key, value, extra_args);
+}
+
+/*
  * Opens a NIC for writing packets to it
  */
 static int
 open_tx_iface(const char *key, const char *value, void *extra_args)
 {
-	const char *iface = value;
-	struct pmd_devargs *tx = extra_args;
-	pcap_t *pcap;
-
-	if (tx->num_of_queue >= RTE_PMD_PCAP_MAX_QUEUES)
-		return -1;
-	if (open_single_iface(iface, &pcap) < 0)
-		return -1;
-	tx->queue[tx->num_of_queue].pcap = pcap;
-	tx->queue[tx->num_of_queue].name = iface;
-	tx->queue[tx->num_of_queue].type = key;
-	tx->num_of_queue++;
-
-	return 0;
+	return open_iface(key, value, extra_args);
 }
 
 static struct rte_vdev_driver pmd_pcap_drv;
-- 
1.9.1

Re: [dpdk-dev] [PATCH v2 2/2] net/pcap: duplicate code consolidation

[Ferruh Yigit]

On 6/19/2018 3:37 PM, ido goshen wrote:
> Signed-off-by: ido goshen <ido@cgstowernetworks.com>

<...>

>  /*
> + * Opens a NIC for reading packets from it
> + */
> +static inline int
> +open_rx_iface(const char *key, const char *value, void *extra_args)
> +{
> +	return open_iface(key, value, extra_args);
> +}
> +
> +/*
>   * Opens a NIC for writing packets to it
>   */
>  static int
>  open_tx_iface(const char *key, const char *value, void *extra_args)
>  {
> -	const char *iface = value;
> -	struct pmd_devargs *tx = extra_args;
> -	pcap_t *pcap;
> -
> -	if (tx->num_of_queue >= RTE_PMD_PCAP_MAX_QUEUES)
> -		return -1;
> -	if (open_single_iface(iface, &pcap) < 0)
> -		return -1;
> -	tx->queue[tx->num_of_queue].pcap = pcap;
> -	tx->queue[tx->num_of_queue].name = iface;
> -	tx->queue[tx->num_of_queue].type = key;
> -	tx->num_of_queue++;
> -
> -	return 0;
> +	return open_iface(key, value, extra_args);
>  }
>  
>  static struct rte_vdev_driver pmd_pcap_drv;
> 

Is there a reason to keep open_tx_iface() and open_rx_iface(), they both are
wrapper to open_iface().
Why not use open_iface() directly as callback function?

Re: [dpdk-dev] [PATCH v2 2/2] net/pcap: duplicate code consolidation

[Ido Goshen]

Although there's no functional need for them I considered keeping it for maintainability reasons:
1. Keep the call flow more aligned with pcap (file) that has separated open_rx_pcap()/open_tx_pcap()
2. If in future there'll rise a need for different functionality between rx and tx then it will be a good place to hook it in. 
e.g. if we'll want to force PCAP_D_IN/OUT for rx_iface/tx_iface as you suggested then it can go in those functions 
(I'd like to go back to that direction issue after this fix is done)

-----Original Message-----
From: Ferruh Yigit <ferruh.yigit@intel.com> 
Sent: Wednesday, June 20, 2018 8:41 PM
To: Ido Goshen <Ido@cgstowernetworks.com>
Cc: dev@dpdk.org
Subject: Re: [PATCH v2 2/2] net/pcap: duplicate code consolidation

On 6/19/2018 3:37 PM, ido goshen wrote:
> Signed-off-by: ido goshen <ido@cgstowernetworks.com>

<...>

>  /*
> + * Opens a NIC for reading packets from it  */ static inline int 
> +open_rx_iface(const char *key, const char *value, void *extra_args) {
> +	return open_iface(key, value, extra_args); }
> +
> +/*
>   * Opens a NIC for writing packets to it
>   */
>  static int
>  open_tx_iface(const char *key, const char *value, void *extra_args)  
> {
> -	const char *iface = value;
> -	struct pmd_devargs *tx = extra_args;
> -	pcap_t *pcap;
> -
> -	if (tx->num_of_queue >= RTE_PMD_PCAP_MAX_QUEUES)
> -		return -1;
> -	if (open_single_iface(iface, &pcap) < 0)
> -		return -1;
> -	tx->queue[tx->num_of_queue].pcap = pcap;
> -	tx->queue[tx->num_of_queue].name = iface;
> -	tx->queue[tx->num_of_queue].type = key;
> -	tx->num_of_queue++;
> -
> -	return 0;
> +	return open_iface(key, value, extra_args);
>  }
>  
>  static struct rte_vdev_driver pmd_pcap_drv;
> 

Is there a reason to keep open_tx_iface() and open_rx_iface(), they both are wrapper to open_iface().
Why not use open_iface() directly as callback function?

Re: [dpdk-dev] [PATCH v2 2/2] net/pcap: duplicate code consolidation

[Ferruh Yigit]

On 6/20/2018 7:06 PM, Ido Goshen wrote:
> Although there's no functional need for them I considered keeping it for maintainability reasons:
> 1. Keep the call flow more aligned with pcap (file) that has separated open_rx_pcap()/open_tx_pcap()
> 2. If in future there'll rise a need for different functionality between rx and tx then it will be a good place to hook it in. 
> e.g. if we'll want to force PCAP_D_IN/OUT for rx_iface/tx_iface as you suggested then it can go in those functions 
> (I'd like to go back to that direction issue after this fix is done)

OK. Sounds reasonable.

> 
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com> 
> Sent: Wednesday, June 20, 2018 8:41 PM
> To: Ido Goshen <Ido@cgstowernetworks.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v2 2/2] net/pcap: duplicate code consolidation
> 
> On 6/19/2018 3:37 PM, ido goshen wrote:
>> Signed-off-by: ido goshen <ido@cgstowernetworks.com>
> 
> <...>
> 
>>  /*
>> + * Opens a NIC for reading packets from it  */ static inline int 
>> +open_rx_iface(const char *key, const char *value, void *extra_args) {
>> +	return open_iface(key, value, extra_args); }
>> +
>> +/*
>>   * Opens a NIC for writing packets to it
>>   */
>>  static int
>>  open_tx_iface(const char *key, const char *value, void *extra_args)  
>> {
>> -	const char *iface = value;
>> -	struct pmd_devargs *tx = extra_args;
>> -	pcap_t *pcap;
>> -
>> -	if (tx->num_of_queue >= RTE_PMD_PCAP_MAX_QUEUES)
>> -		return -1;
>> -	if (open_single_iface(iface, &pcap) < 0)
>> -		return -1;
>> -	tx->queue[tx->num_of_queue].pcap = pcap;
>> -	tx->queue[tx->num_of_queue].name = iface;
>> -	tx->queue[tx->num_of_queue].type = key;
>> -	tx->num_of_queue++;
>> -
>> -	return 0;
>> +	return open_iface(key, value, extra_args);
>>  }
>>  
>>  static struct rte_vdev_driver pmd_pcap_drv;
>>
> 
> Is there a reason to keep open_tx_iface() and open_rx_iface(), they both are wrapper to open_iface().
> Why not use open_iface() directly as callback function?
> 

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

[Ferruh Yigit]

On 6/19/2018 3:37 PM, ido goshen wrote:
> Fixes: 4c173302c307 ("pcap: add new driver")
> Cc: stable@dpdk.org
> 
> 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
> 
> 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
> 
> Signed-off-by: ido goshen <ido@cgstowernetworks.com>

For series,
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

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

[Ferruh Yigit]

On 6/20/2018 7:22 PM, Ferruh Yigit wrote:
> On 6/19/2018 3:37 PM, ido goshen wrote:
>> Fixes: 4c173302c307 ("pcap: add new driver")
>> Cc: stable@dpdk.org
>>
>> 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
>>
>> 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
>>
>> Signed-off-by: ido goshen <ido@cgstowernetworks.com>
> 
> For series,
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

Series applied to dpdk-next-net/master, thanks.