[dpdk-dev] [PATCH] app/flow-perf: fix condition of hairpin queues setup

[dpdk-dev] [PATCH] app/flow-perf: fix condition of hairpin queues setup

[Wisam Jaddo]

The hairpin queue is the one that start from normal rxq,
and will be less than nr_queues where nr_queues is the
sum of normal and hairpin

Fixes: bf3688f1e816 ("app/flow-perf: add insertion rate calculation")
Cc: wisamm@mellanox.com

Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
---
 app/test-flow-perf/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/test-flow-perf/main.c b/app/test-flow-perf/main.c
index e155e49c37..2a04bfb8d5 100644
--- a/app/test-flow-perf/main.c
+++ b/app/test-flow-perf/main.c
@@ -1013,7 +1013,7 @@ init_port(void)
 
 		if (hairpinq != 0) {
 			for (hairpin_q = RXQ_NUM, std_queue = 0;
-					std_queue < nr_queues;
+					hairpin_q < nr_queues;
 					hairpin_q++, std_queue++) {
 				hairpin_conf.peers[0].port = port_id;
 				hairpin_conf.peers[0].queue =
@@ -1028,7 +1028,7 @@ init_port(void)
 			}
 
 			for (hairpin_q = TXQ_NUM, std_queue = 0;
-					std_queue < nr_queues;
+					hairpin_q < nr_queues;
 					hairpin_q++, std_queue++) {
 				hairpin_conf.peers[0].port = port_id;
 				hairpin_conf.peers[0].queue =
-- 
2.17.1

Re: [dpdk-dev] [PATCH] app/flow-perf: fix condition of hairpin queues setup

[Asaf Penso]

Regards,
Asaf Penso

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Wisam Jaddo
> Sent: Tuesday, June 30, 2020 11:10 AM
> To: Thomas Monjalon <thomas@monjalon.net>; Jack Min
> <jackmin@mellanox.com>; david.marchand@redhat.com
> Cc: dev@dpdk.org; Wisam Monther <wisamm@mellanox.com>
> Subject: [dpdk-dev] [PATCH] app/flow-perf: fix condition of hairpin queues
> setup
> 
> The hairpin queue is the one that start from normal rxq,
> and will be less than nr_queues where nr_queues is the
> sum of normal and hairpin
> 
> Fixes: bf3688f1e816 ("app/flow-perf: add insertion rate calculation")
> Cc: wisamm@mellanox.com
> 
> Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>


Reviewed-by: Asaf Penso <asafp@mellanox.com>

> ---
>  app/test-flow-perf/main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-flow-perf/main.c b/app/test-flow-perf/main.c
> index e155e49c37..2a04bfb8d5 100644
> --- a/app/test-flow-perf/main.c
> +++ b/app/test-flow-perf/main.c
> @@ -1013,7 +1013,7 @@ init_port(void)
> 
>  		if (hairpinq != 0) {
>  			for (hairpin_q = RXQ_NUM, std_queue = 0;
> -					std_queue < nr_queues;
> +					hairpin_q < nr_queues;
>  					hairpin_q++, std_queue++) {
>  				hairpin_conf.peers[0].port = port_id;
>  				hairpin_conf.peers[0].queue =
> @@ -1028,7 +1028,7 @@ init_port(void)
>  			}
> 
>  			for (hairpin_q = TXQ_NUM, std_queue = 0;
> -					std_queue < nr_queues;
> +					hairpin_q < nr_queues;
>  					hairpin_q++, std_queue++) {
>  				hairpin_conf.peers[0].port = port_id;
>  				hairpin_conf.peers[0].queue =
> --
> 2.17.1

Re: [dpdk-dev] [PATCH] app/flow-perf: fix condition of hairpin queues setup

[Thomas Monjalon]

30/06/2020 10:10, Wisam Jaddo:
> The hairpin queue is the one that start from normal rxq,
> and will be less than nr_queues where nr_queues is the
> sum of normal and hairpin
> 
> Fixes: bf3688f1e816 ("app/flow-perf: add insertion rate calculation")
> Cc: wisamm@mellanox.com
> 
> Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>

You should take this opportunity to document the logic
for the allocation and peering of hairpin queues.

It would be good to add short code comments for the variables as well.
It confusing to have hairpinq and hairpin_q variables.

Currently, we cannot really understand whether this fix is good or not.

On hairpin topic, I suggest fixing this typo:
	hairping-rss -> hairpin-rss

[dpdk-dev] [PATCH v2] app/flow-perf: fix condition of hairpin queues setup

[Wisam Jaddo]

The hairpin queue is the one that start from normal rxq,
and will be less than nr_queues where nr_queues is the
sum of normal and hairpin

Fixes: bf3688f1e816 ("app/flow-perf: add insertion rate calculation")
Cc: wisamm@mellanox.com

Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
Reviewed-by: Asaf Penso <asafp@mellanox.com>

---
v2:
* Add documentation of hairpin peering and allocating logic.
* Add documentation for some variables.
---
 app/test-flow-perf/main.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/app/test-flow-perf/main.c b/app/test-flow-perf/main.c
index e155e49c37..8f12ee10f1 100644
--- a/app/test-flow-perf/main.c
+++ b/app/test-flow-perf/main.c
@@ -1012,8 +1012,26 @@ init_port(void)
 				rte_strerror(-ret), port_id);
 
 		if (hairpinq != 0) {
+			/* Each hairpin queue setup need a hairpin configuration
+			 * object, which determine the TX path for hairpin.
+			 *
+			 * The peering here represent the TX side, which mean the
+			 * peer.port represent TX port, and peer.queue represent
+			 * tx_queue.
+			 *
+			 * So if RXQ=4 and TXQ=4, and first hairpin_q is 4 after
+			 * [0, 1, 2, 3], then tx_queue is TXQ+i which is 4 as well.
+			 *
+			 * hairpinq: represent the number of hairpin queues needed
+			 * to be initialized.
+			 *
+			 * In 0 case means no hairpin queues needed which is the
+			 * default.
+			 *
+			 * hairpin_q: represent hairpin queue id to be initialized.
+			 */
 			for (hairpin_q = RXQ_NUM, std_queue = 0;
-					std_queue < nr_queues;
+					hairpin_q < nr_queues;
 					hairpin_q++, std_queue++) {
 				hairpin_conf.peers[0].port = port_id;
 				hairpin_conf.peers[0].queue =
@@ -1028,7 +1046,7 @@ init_port(void)
 			}
 
 			for (hairpin_q = TXQ_NUM, std_queue = 0;
-					std_queue < nr_queues;
+					hairpin_q < nr_queues;
 					hairpin_q++, std_queue++) {
 				hairpin_conf.peers[0].port = port_id;
 				hairpin_conf.peers[0].queue =
-- 
2.17.1

Re: [dpdk-dev] [PATCH] app/flow-perf: fix condition of hairpin queues setup

[Wisam Monther]

Hi,

>-----Original Message-----
>From: Thomas Monjalon <thomas@monjalon.net>
>Sent: Sunday, July 5, 2020 11:40 PM
>To: Wisam Monther <wisamm@mellanox.com>
>Cc: Jack Min <jackmin@mellanox.com>; david.marchand@redhat.com;
>dev@dpdk.org; Asaf Penso <asafp@mellanox.com>;
>arybchenko@solarflare.com
>Subject: Re: [dpdk-dev] [PATCH] app/flow-perf: fix condition of hairpin queues
>setup
>
>30/06/2020 10:10, Wisam Jaddo:
>> The hairpin queue is the one that start from normal rxq, and will be
>> less than nr_queues where nr_queues is the sum of normal and hairpin
>>
>> Fixes: bf3688f1e816 ("app/flow-perf: add insertion rate calculation")
>> Cc: wisamm@mellanox.com
>>
>> Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
>
>You should take this opportunity to document the logic for the allocation and
>peering of hairpin queues.
>
>It would be good to add short code comments for the variables as well.
>It confusing to have hairpinq and hairpin_q variables.
>
>Currently, we cannot really understand whether this fix is good or not.

I've sent V2 with the fix + those documentation.

>
>On hairpin topic, I suggest fixing this typo:
>	hairping-rss -> hairpin-rss

I've provided another patch to fix this typo:
http://patches.dpdk.org/patch/73162/

>
>

Re: [dpdk-dev] [PATCH v2] app/flow-perf: fix condition of hairpin queues setup

[Thomas Monjalon]

06/07/2020 09:53, Wisam Jaddo:
> The hairpin queue is the one that start from normal rxq,
> and will be less than nr_queues where nr_queues is the
> sum of normal and hairpin
> 
> Fixes: bf3688f1e816 ("app/flow-perf: add insertion rate calculation")
> Cc: wisamm@mellanox.com
> 
> Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
> Reviewed-by: Asaf Penso <asafp@mellanox.com>
> 
> ---
> v2:
> * Add documentation of hairpin peering and allocating logic.
> * Add documentation for some variables.
> ---
> --- a/app/test-flow-perf/main.c
> +++ b/app/test-flow-perf/main.c
> @@ -1012,8 +1012,26 @@ init_port(void)
>  				rte_strerror(-ret), port_id);
>  
>  		if (hairpinq != 0) {
> +			/* Each hairpin queue setup need a hairpin configuration
> +			 * object, which determine the TX path for hairpin.
> +			 *
> +			 * The peering here represent the TX side, which mean the
> +			 * peer.port represent TX port, and peer.queue represent
> +			 * tx_queue.
> +			 *
> +			 * So if RXQ=4 and TXQ=4, and first hairpin_q is 4 after
> +			 * [0, 1, 2, 3], then tx_queue is TXQ+i which is 4 as well.
> +			 *
> +			 * hairpinq: represent the number of hairpin queues needed
> +			 * to be initialized.
> +			 *
> +			 * In 0 case means no hairpin queues needed which is the
> +			 * default.
> +			 *
> +			 * hairpin_q: represent hairpin queue id to be initialized.
> +			 */

Variables doc should be on variable declaration.

[dpdk-dev] [PATCH v3] app/flow-perf: fix condition of hairpin queues setup

[Wisam Jaddo]

The hairpin queue is the one that start from normal rxq,
and will be less than nr_queues where nr_queues is the
sum of normal and hairpin

Fixes: bf3688f1e816 ("app/flow-perf: add insertion rate calculation")
Cc: wisamm@mellanox.com

Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
Reviewed-by: Asaf Penso <asafp@mellanox.com>

---
v3:
* Add variable documentation variable declaration.

v2:
* Add documentation of hairpin peering and allocating logic.
* Add documentation for some variables.
---
 app/test-flow-perf/main.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/app/test-flow-perf/main.c b/app/test-flow-perf/main.c
index e155e49c37..2200767e0d 100644
--- a/app/test-flow-perf/main.c
+++ b/app/test-flow-perf/main.c
@@ -59,6 +59,13 @@ static struct rte_mempool *mbuf_mp;
 static uint32_t nb_lcores;
 static uint32_t flows_count;
 static uint32_t iterations_number;
+/*
+ * hairpinq: represent the number of hairpin queues needed
+ * to be initialized.
+ *
+ * In 0 case means no hairpin queues needed which is the
+ * default.
+ */
 static uint32_t hairpinq;
 static uint32_t nb_lcores;
 
@@ -929,6 +936,7 @@ init_port(void)
 {
 	int ret;
 	uint16_t std_queue;
+	/* hairpin_q represent hairpin queue id to be initialized. */
 	uint16_t hairpin_q;
 	uint16_t port_id;
 	uint16_t nr_ports;
@@ -1012,8 +1020,19 @@ init_port(void)
 				rte_strerror(-ret), port_id);
 
 		if (hairpinq != 0) {
+			/*
+			 * Each hairpin queue setup need a hairpin configuration
+			 * object, which determine the TX path for hairpin.
+			 *
+			 * The peering here represent the TX side, which mean the
+			 * peer.port represent TX port, and peer.queue represent
+			 * tx_queue.
+			 *
+			 * So if RXQ=4 and TXQ=4, and first hairpin_q is 4 after
+			 * [0, 1, 2, 3], then tx_queue is TXQ+i which is 4 as well.
+			 */
 			for (hairpin_q = RXQ_NUM, std_queue = 0;
-					std_queue < nr_queues;
+					hairpin_q < nr_queues;
 					hairpin_q++, std_queue++) {
 				hairpin_conf.peers[0].port = port_id;
 				hairpin_conf.peers[0].queue =
@@ -1028,7 +1047,7 @@ init_port(void)
 			}
 
 			for (hairpin_q = TXQ_NUM, std_queue = 0;
-					std_queue < nr_queues;
+					hairpin_q < nr_queues;
 					hairpin_q++, std_queue++) {
 				hairpin_conf.peers[0].port = port_id;
 				hairpin_conf.peers[0].queue =
-- 
2.17.1

[dpdk-dev] [PATCH v4] app/flow-perf: fix condition of hairpin queues setup

[Wisam Jaddo]

The hairpin queue is the one that start from normal rxq,
and will be less than nr_queues where nr_queues is the
sum of normal and hairpin

Fixes: bf3688f1e816 ("app/flow-perf: add insertion rate calculation")
Cc: wisamm@mellanox.com

Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
Reviewed-by: Asaf Penso <asafp@mellanox.com>
---
v4:
* Rename hairpin variables to meaningful names.

v3:
* Add variable documentation variable declaration.

v2:
* Add documentation of hairpin peering and allocating logic.
* Add documentation for some variables.
---
 app/test-flow-perf/main.c | 40 ++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/app/test-flow-perf/main.c b/app/test-flow-perf/main.c
index e155e49c37..9fb8d1feaf 100644
--- a/app/test-flow-perf/main.c
+++ b/app/test-flow-perf/main.c
@@ -59,7 +59,7 @@ static struct rte_mempool *mbuf_mp;
 static uint32_t nb_lcores;
 static uint32_t flows_count;
 static uint32_t iterations_number;
-static uint32_t hairpinq;
+static uint32_t hairpin_queues_num; /* total hairpin q number - default: 0 */
 static uint32_t nb_lcores;
 
 #define MAX_PKT_BURST    32
@@ -323,7 +323,7 @@ args_parse(int argc, char **argv)
 	flow_items = 0;
 	flow_actions = 0;
 	flow_attrs = 0;
-	hairpinq = 0;
+	hairpin_queues_num = 0;
 	argvopt = argv;
 
 	printf(":: Flow -> ");
@@ -358,7 +358,7 @@ args_parse(int argc, char **argv)
 					"hairpin-rss") == 0) {
 				n = atoi(optarg);
 				if (n > 0)
-					hairpinq = n;
+					hairpin_queues_num = n;
 				else
 					rte_exit(EXIT_SUCCESS,
 						"Hairpin queues should be > 0\n");
@@ -370,7 +370,7 @@ args_parse(int argc, char **argv)
 					"hairpin-queue") == 0) {
 				n = atoi(optarg);
 				if (n > 0)
-					hairpinq = n;
+					hairpin_queues_num = n;
 				else
 					rte_exit(EXIT_SUCCESS,
 						"Hairpin queues should be > 0\n");
@@ -604,7 +604,8 @@ flows_handler(void)
 		for (i = 0; i < flows_count; i++) {
 			flow = generate_flow(port_id, flow_group,
 				flow_attrs, flow_items, flow_actions,
-				JUMP_ACTION_TABLE, i, hairpinq, &error);
+				JUMP_ACTION_TABLE, i,
+				hairpin_queues_num, &error);
 
 			if (force_quit)
 				i = flows_count;
@@ -929,7 +930,7 @@ init_port(void)
 {
 	int ret;
 	uint16_t std_queue;
-	uint16_t hairpin_q;
+	uint16_t hairpin_queue;
 	uint16_t port_id;
 	uint16_t nr_ports;
 	uint16_t nr_queues;
@@ -947,8 +948,8 @@ init_port(void)
 	struct rte_eth_dev_info dev_info;
 
 	nr_queues = RXQ_NUM;
-	if (hairpinq != 0)
-		nr_queues = RXQ_NUM + hairpinq;
+	if (hairpin_queues_num != 0)
+		nr_queues = RXQ_NUM + hairpin_queues_num;
 
 	nr_ports = rte_eth_dev_count_avail();
 	if (nr_ports == 0)
@@ -1011,15 +1012,20 @@ init_port(void)
 				":: promiscuous mode enable failed: err=%s, port=%u\n",
 				rte_strerror(-ret), port_id);
 
-		if (hairpinq != 0) {
-			for (hairpin_q = RXQ_NUM, std_queue = 0;
-					std_queue < nr_queues;
-					hairpin_q++, std_queue++) {
+		if (hairpin_queues_num != 0) {
+			/*
+			 * Configure peer which represents hairpin Tx.
+			 * Hairpin queue numbers start after standard queues
+			 * (RXQ_NUM and TXQ_NUM).
+			 */
+			for (hairpin_queue = RXQ_NUM, std_queue = 0;
+					hairpin_queue < nr_queues;
+					hairpin_queue++, std_queue++) {
 				hairpin_conf.peers[0].port = port_id;
 				hairpin_conf.peers[0].queue =
 					std_queue + TXQ_NUM;
 				ret = rte_eth_rx_hairpin_queue_setup(
-						port_id, hairpin_q,
+						port_id, hairpin_queue,
 						NR_RXD, &hairpin_conf);
 				if (ret != 0)
 					rte_exit(EXIT_FAILURE,
@@ -1027,14 +1033,14 @@ init_port(void)
 						ret, port_id);
 			}
 
-			for (hairpin_q = TXQ_NUM, std_queue = 0;
-					std_queue < nr_queues;
-					hairpin_q++, std_queue++) {
+			for (hairpin_queue = TXQ_NUM, std_queue = 0;
+					hairpin_queue < nr_queues;
+					hairpin_queue++, std_queue++) {
 				hairpin_conf.peers[0].port = port_id;
 				hairpin_conf.peers[0].queue =
 					std_queue + RXQ_NUM;
 				ret = rte_eth_tx_hairpin_queue_setup(
-						port_id, hairpin_q,
+						port_id, hairpin_queue,
 						NR_TXD, &hairpin_conf);
 				if (ret != 0)
 					rte_exit(EXIT_FAILURE,
-- 
2.17.1

Re: [dpdk-dev] [PATCH v4] app/flow-perf: fix condition of hairpin queues setup

[Thomas Monjalon]

16/07/2020 16:16, Wisam Jaddo:
> The hairpin queue is the one that start from normal rxq,
> and will be less than nr_queues where nr_queues is the
> sum of normal and hairpin
> 
> Fixes: bf3688f1e816 ("app/flow-perf: add insertion rate calculation")
> Cc: wisamm@mellanox.com
> 
> Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
> Reviewed-by: Asaf Penso <asafp@mellanox.com>

Applied, thanks