diff mbox series

app/flow-perf: fix condition of hairpin queues setup

Message ID 20200630081028.21339-1-wisamm@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series app/flow-perf: fix condition of hairpin queues setup |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Performance-Testing fail build patch failure
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK
ci/iol-broadcom-Performance fail Performance Testing issues
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing fail Testing issues

Commit Message

Wisam Jaddo June 30, 2020, 8:10 a.m. UTC 
  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(-)

Comments

Asaf Penso July 5, 2020, 3:39 p.m. UTC | #1 
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
Thomas Monjalon July 5, 2020, 8:39 p.m. UTC | #2 
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
Wisam Jaddo July 6, 2020, 8 a.m. UTC | #3 
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/

>
>
diff mbox series

Patch

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 =