examples/multi_process: fix RX packets distribution

PATCH Superseded Gregory Etelson 4 participants 15 messages Oct 26, 2021 – Nov 16, 2021

lore

Share & tools

lore

Gregory Etelson

author

2021-10-26 09:51 UTC #0 lore raw

PATCH 1/1 examples/multi_process: fix RX packets distribution

MP servers distributes RX packets between clients according to round-robin scheme.

Current implementation always started packets distribution from the first client. That procedure resulted in uniform distribution in cases when RX packets number was a multiple of clients number. However, if RX burst repeatedly returned single
packet, round-robin scheme would not work because all packets were assigned to the first client only.

The patch does not restart packets distribution from the first client.
Packets distribution always continues to the next client.

Fixes af75078fece3 ("first public release")

Signed-off-by Gregory Etelson <[email protected]>

Reviewed-by Dmitry Kozlyuk <[email protected]>

---
 examples/multi_process/client_server_mp/mp_server/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

examples/multi_process/client_server_mp/mp_server/main.c +1 -1

index b4761ebc7b..fb441cbbf0 100644
@@ -234,7 +234,7 @@ process_packets(uint32_t port_num __rte_unused,
 		struct rte_mbuf *pkts[], uint16_t rx_count)
 {
 	uint16_t i;
-	uint8_t client = 0;
+	static uint8_t client = 0;
 
 	for (i = 0; i < rx_count; i++) {
 		enqueue_rx_packet(client, pkts[i]);

Signature · 1 line

2.33.1

Burakov, Anatoly

2021-10-28 14:30 UTC #1 lore raw

Quoted 28 lines · Gregory Etelson

MP servers distributes RX packets between clients according to round-robin scheme.
Current implementation always started packets distribution from the first client. That procedure resulted in uniform distribution in cases when RX packets number was a multiple of clients number. However, if RX burst repeatedly returned single
packet, round-robin scheme would not work because all packets were assigned to the first client only.
The patch does not restart packets distribution from the first client.
Packets distribution always continues to the next client.
Fixes af75078fece3 ("first public release")
Signed-off-by Gregory Etelson <[email protected]>
Reviewed-by Dmitry Kozlyuk <[email protected]>
---
  examples/multi_process/client_server_mp/mp_server/main.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
examples/multi_process/client_server_mp/mp_server/main.c +0 -0
index b4761ebc7b..fb441cbbf0 100644

examples/multi_process/client_server_mp/mp_server/main.c process_packets(uint32_t port_num __rte_unused, · line 240

Quoted 8 lines

examples/multi_process/client_server_mp/mp_server/main.c +1 -1

@@ -234,7 +234,7 @@ process_packets(uint32_t port_num __rte_unused,
  		struct rte_mbuf *pkts[], uint16_t rx_count)
  {
  	uint16_t i;
-	uint8_t client = 0;
+	static uint8_t client = 0;
 
  	for (i = 0; i < rx_count; i++) {
  		enqueue_rx_packet(client, pkts[i]);

Wouldn't that make it global? I don't recall off the top of my head if the multiprocess app is intended to have multiple Rx threads, but if you did have two forwarding threads, they would effectively both use the same client value, stepping on top of each other. This should probably be per-thread?

Signature · 2 lines

Thanks,
Anatoly

Gregory Etelson

author

2021-10-28 15:14 UTC #2 lore raw

Hello Anatoly,

..snip..

Quoted 16 lines

b/examples/multi_process/client_server_mp/m
p_server/main.c
Review
Quoted 1 line
(quoted diff) +0 -0
@@ -234,7 +234,7 @@
process_packets(uint32_t port_num
__rte_unused,
Review line 234
Quoted 1 line
(quoted diff) +0 -0
              struct rte_mbuf *pkts[], uint16_t
rx_count)
examples/multi_process/client_server_mp/mp_server/main.c process_packets(uint32_t port_num __rte_unused, · line 240
Quoted 8 lines
examples/multi_process/client_server_mp/mp_server/main.c +1 -1
  {
      uint16_t i;
-     uint8_t client = 0;
+     static uint8_t client = 0;
 
      for (i = 0; i < rx_count; i++) {
              enqueue_rx_packet(client, pkts[i]);

Quoted 10 lines

Wouldn't that make it global? I don't recall off
the top of my head if
the multiprocess app is intended to have
multiple Rx threads, but if you
did have two forwarding threads, they would
effectively both use the
same client value, stepping on top of each
other. This should probably
be per-thread?

MP client-server example was not designed as a multi-threaded app. Server and clients run in a different process and the model allows one server process. Server allocates a dedicated ring to each client and distributes Rx packets between rings in round-robin sequence.
Each ring configured for single producer and single consumer.
Consider an example when server's rte_eth_rx_burst() returns a single packet on each call.
Without the patch, the server will ignore all clients with id > 0 and assign all Rx packets to rx_ring 0.
Changing process_packets() client variable to static allows unform round-robin packets distribution between rings.

Regards,
Gregory

Burakov, Anatoly

Acked-by

2021-10-28 15:35 UTC #3 lore raw

Quoted 43 lines · Gregory Etelson

Hello Anatoly,
..snip..
Quoted 16 lines
b/examples/multi_process/client_server_mp/m
p_server/main.c
Review
Quoted 1 line
(quoted diff) +0 -0
@@ -234,7 +234,7 @@
process_packets(uint32_t port_num
__rte_unused,
Review line 234
Quoted 1 line
(quoted diff) +0 -0
               struct rte_mbuf *pkts[], uint16_t
rx_count)
examples/multi_process/client_server_mp/mp_server/main.c process_packets(uint32_t port_num __rte_unused, · line 240
Quoted 8 lines
examples/multi_process/client_server_mp/mp_server/main.c +1 -1
   {
       uint16_t i;
-     uint8_t client = 0;
+     static uint8_t client = 0;
 
       for (i = 0; i < rx_count; i++) {
               enqueue_rx_packet(client, pkts[i]);
Quoted 10 lines
Wouldn't that make it global? I don't recall off
the top of my head if
the multiprocess app is intended to have
multiple Rx threads, but if you
did have two forwarding threads, they would
effectively both use the
same client value, stepping on top of each
other. This should probably
be per-thread?
MP client-server example was not designed as a multi-threaded app. Server and clients run in a different process and the model allows one server process. Server allocates a dedicated ring to each client and distributes Rx packets between rings in round-robin sequence.
Each ring configured for single producer and single consumer.
Consider an example when server's rte_eth_rx_burst() returns a single packet on each call.
Without the patch, the server will ignore all clients with id > 0 and assign all Rx packets to rx_ring 0.
Changing process_packets() client variable to static allows unform round-robin packets distribution between rings.

Quoted 3 lines

Regards,
Gregory

Right, i just checked the code, and the app indeed allows only one forwarding thread on the server.

Acked-by Anatoly Burakov <[email protected]>

Signature · 2 lines

Thanks,
Anatoly

Thomas Monjalon

2021-11-08 21:27 UTC #4 lore raw

28/10/2021 17:35, Burakov, Anatoly:

Quoted 5 lines · Gregory Etelson

Quoted 2 lines
Quoted 2 lines
examples/multi_process/client_server_mp/mp_server/main.c process_packets(uint32_t port_num __rte_unused, · line 237
Quoted 2 lines
examples/multi_process/client_server_mp/mp_server/main.c +1 -1
-     uint8_t client = 0;
+     static uint8_t client = 0;
Acked-by Anatoly Burakov <[email protected]>

checkpatch has a message for you:
ERROR:INITIALISED_STATIC: do not initialise statics to 0

Gregory Etelson

author

2021-11-09 06:42 UTC #5 lore raw

Hello Thomas,

Quoted 6 lines

28/10/2021 17:35, Burakov, Anatoly:
Quoted 4 lines · Gregory Etelson
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
examples/multi_process/client_server_mp/mp_server/main.c +1 -1
-     uint8_t client = 0;
+     static uint8_t client = 0;

Quoted 7 lines

Quoted 1 line
Acked-by Anatoly Burakov
<[email protected]>
checkpatch has a message for you:
ERROR:INITIALISED_STATIC: do not initialise
statics to 0

Turning the client variable to static ensured that the next time the function will be called it will proceed iterating clients instead of starting a loop from the beginning - that's the main idea of that patch. The variable must be initialized to 0 because the application model requires at least a single client with index 0.
ANSI C allows static variables initialization to any valid value. Do you know why the checkpatch utility denied such initialization ?

Regards,
Gregory

Thomas Monjalon

2021-11-09 07:30 UTC #6 lore raw

09/11/2021 07:42, Gregory Etelson:

Quoted 16 lines

Hello Thomas,
Quoted 6 lines
28/10/2021 17:35, Burakov, Anatoly:
Quoted 4 lines · Gregory Etelson
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
examples/multi_process/client_server_mp/mp_server/main.c +1 -1
-     uint8_t client = 0;
+     static uint8_t client = 0;
Quoted 7 lines
Quoted 1 line
Acked-by Anatoly Burakov
<[email protected]>
checkpatch has a message for you:
ERROR:INITIALISED_STATIC: do not initialise
statics to 0

Quoted 7 lines

Turning the client variable to static ensured that the next time the function will be called it will proceed iterating clients instead of starting a loop from the beginning - that's the main idea of that patch. The variable must be initialized to 0 because the application model requires at least a single client with index 0.
ANSI C allows static variables initialization to any valid value. Do you know why the checkpatch utility denied such initialization ?

ANSI C makes static variables iniatilized to 0 by default.

Gregory Etelson

author

2021-11-09 09:35 UTC #7 lore raw

Hello Thomas,

Quoted 22 lines

Quoted 2 lines
Quoted 2 lines
28/10/2021 17:35, Burakov, Anatoly:
Quoted 1 line
On 28-Oct-21 4:14 PM, Gregory Etelson
Quoted 4 lines · wrote:
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +1 -1
-     uint8_t client = 0;
+     static uint8_t client = 0;
Acked-by Anatoly Burakov
Quoted 8 lines
Quoted 6 lines
<[email protected]>
checkpatch has a message for you:
ERROR:INITIALISED_STATIC: do not initialise
statics to 0
Turning the client variable to static ensured
that the next time
Quoted 1 line
the function will be called it will proceed
iterating clients instead of
Quoted 1 line
starting a loop from the beginning - that's the
main idea of that patch.
Quoted 1 line
The variable must be initialized to 0 because
the application model

Quoted 9 lines

Quoted 2 lines
requires at least a single client with index 0.
ANSI C allows static variables initialization to
any valid value.
Quoted 1 line
Do you know why the checkpatch utility denied
such initialization ?
ANSI C makes static variables iniatilized to 0 by
default.

I'll post updated patch

Regards,
Gregory

Gregory Etelson

author

2021-11-09 09:59 UTC #8 lore raw

PATCH v2 1/1 examples/multi_proces: fix Rx packets distribution

In reply to Gregory Etelson

MP servers distributes Rx packets between clients according to round-robin scheme.

Current implementation always started packets distribution from the first client. That procedure resulted in uniform distribution in cases when Rx packets number was around clients number multiplication. However, if RX burst repeatedly returned single packet, round-robin scheme would not work because all packets were assigned to the first client only.

The patch does not restart packets distribution from the first client.
Packets distribution always continues to the next client.

Cc [email protected]

Fixes af75078fece3 ("first public release")

Signed-off-by Gregory Etelson <[email protected]>

Acked-by Anatoly Burakov <[email protected]>

---
v2: Remove explisit static variable initialization.

---
 examples/multi_process/client_server_mp/mp_server/main.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

examples/multi_process/client_server_mp/mp_server/main.c +6 -1

index b4761ebc7b..31e7e76706 100644
@@ -234,7 +234,12 @@ process_packets(uint32_t port_num __rte_unused,
 		struct rte_mbuf *pkts[], uint16_t rx_count)
 {
 	uint16_t i;
-	uint8_t client = 0;
+	/*
+	 * C99: All objects with static storage duration
+	 * shall be initialized (set to their initial values) before
+	 * program startup.
+	 */
+	static uint8_t client;
 
 	for (i = 0; i < rx_count; i++) {
 		enqueue_rx_packet(client, pkts[i]);

Signature · 1 line

2.33.1

Thomas Monjalon

2021-11-09 11:35 UTC #9 lore raw

Re: [dpdk-dev] [PATCH v2] examples/multi_proces: fix Rx packets distribution

09/11/2021 10:58, Gregory Etelson:

examples/multi_process/client_server_mp/mp_server/main.c process_packets(uint32_t port_num __rte_unused, · line 241

Quoted 6 lines

examples/multi_process/client_server_mp/mp_server/main.c +5 -1

-	uint8_t client = 0;
+	/*
+	 * C99: All objects with static storage duration
+	 * shall be initialized (set to their initial values) before
+	 * program startup.
+	 */

Why adding this comment?

Quoted 1 line

examples/multi_process/client_server_mp/mp_server/main.c +1 -0
+	static uint8_t client;

Gregory Etelson

author

2021-11-09 11:49 UTC #10 lore raw

Re: [dpdk-dev] [PATCH v2] examples/multi_proces: fix Rx packets distribution

Hello Thomas,

Quoted 4 lines

09/11/2021 10:58, Gregory Etelson:
examples/multi_process/client_server_mp/mp_server/main.c process_packets(uint32_t port_num __rte_unused,
Quoted 3 lines
examples/multi_process/client_server_mp/mp_server/main.c +2 -1
-     uint8_t client = 0;
+     /*
+      * C99: All objects with static storage

Quoted 11 lines

duration
Quoted 1 line
+ * shall be initialized (set to their initial
values) before
examples/multi_process/client_server_mp/mp_server/main.c process_packets(uint32_t port_num __rte_unused, · line 241
Quoted 2 lines
examples/multi_process/client_server_mp/mp_server/main.c +2 -0
+      * program startup.
+      */
Why adding this comment?
Quoted 1 line
+ static uint8_t client;

C99 optimization that was used here is not obvious.
The patch relies on client=0 initialization.
I added the comment to clarify why the client was not initialized.

Thomas Monjalon

2021-11-09 14:17 UTC #11 lore raw

Re: [dpdk-dev] [PATCH v2] examples/multi_proces: fix Rx packets distribution

09/11/2021 12:49, Gregory Etelson:

Quoted 14 lines

Hello Thomas,
Quoted 12 lines
09/11/2021 10:58, Gregory Etelson:
examples/multi_process/client_server_mp/mp_server/main.c process_packets(uint32_t port_num __rte_unused,
Quoted 3 lines
examples/multi_process/client_server_mp/mp_server/main.c +2 -1
-     uint8_t client = 0;
+     /*
+      * C99: All objects with static storage
duration
Quoted 1 line
+ * shall be initialized (set to their initial
values) before
examples/multi_process/client_server_mp/mp_server/main.c process_packets(uint32_t port_num __rte_unused, · line 241
Quoted 2 lines
examples/multi_process/client_server_mp/mp_server/main.c +2 -0
+      * program startup.
+      */
Why adding this comment?

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
+ static uint8_t client;
C99 optimization that was used here is not obvious.
The patch relies on client=0 initialization.
I added the comment to clarify why the client was not initialized.

I think it is the C standard, not only C99.
As far as I know, having static as 0 is obvious for a lot of people.

Gregory Etelson

author

2021-11-10 16:53 UTC #12 lore raw

PATCH v3 1/1 examples/multi_proces: fix Rx packets distribution

In reply to Gregory Etelson

MP servers distributes Rx packets between clients according to round-robin scheme.

The patch does not restart packets distribution from the first client.
Packets distribution always continues to the next client.

Cc [email protected]

Fixes af75078fece3 ("first public release")

Signed-off-by Gregory Etelson <[email protected]>

Acked-by Anatoly Burakov <[email protected]>

---
v2: Remove explisit static variable initialization.
v3: Remove comment.

---
 examples/multi_process/client_server_mp/mp_server/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

examples/multi_process/client_server_mp/mp_server/main.c +1 -1

index b4761ebc7b..f54bb8b75a 100644
@@ -234,7 +234,7 @@ process_packets(uint32_t port_num __rte_unused,
 		struct rte_mbuf *pkts[], uint16_t rx_count)
 {
 	uint16_t i;
-	uint8_t client = 0;
+	static uint8_t client;
 
 	for (i = 0; i < rx_count; i++) {
 		enqueue_rx_packet(client, pkts[i]);

Signature · 1 line

2.33.1

Gregory Etelson

author

2021-11-10 16:58 UTC #13 lore raw

PATCH v4 1/1 examples/multi_process: fix Rx packets distribution

In reply to Gregory Etelson

MP servers distributes Rx packets between clients according to round-robin scheme.

The patch does not restart packets distribution from the first client.
Packets distribution always continues to the next client.

Cc [email protected]

Fixes af75078fece3 ("first public release")

Signed-off-by Gregory Etelson <[email protected]>

Acked-by Anatoly Burakov <[email protected]>

---
v2: Remove explisit static variable initialization.
v3: Remove comment.
v4: Spell check.

---
 examples/multi_process/client_server_mp/mp_server/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

examples/multi_process/client_server_mp/mp_server/main.c +1 -1

index b4761ebc7b..f54bb8b75a 100644
@@ -234,7 +234,7 @@ process_packets(uint32_t port_num __rte_unused,
 		struct rte_mbuf *pkts[], uint16_t rx_count)
 {
 	uint16_t i;
-	uint8_t client = 0;
+	static uint8_t client;
 
 	for (i = 0; i < rx_count; i++) {
 		enqueue_rx_packet(client, pkts[i]);

Signature · 1 line

2.33.1

David Marchand

Applied

2021-11-16 15:08 UTC #14 lore raw

Re: [PATCH v4] examples/multi_process: fix Rx packets distribution

Quoted 15 lines · Gregory Etelson

MP servers distributes Rx packets between clients according to round-robin scheme.
Current implementation always started packets distribution from the first client. That procedure resulted in uniform distribution in cases when Rx packets number was around clients number multiplication. However, if RX burst repeatedly returned single packet, round-robin scheme would not work because all packets were assigned to the first client only.
The patch does not restart packets distribution from the first client.
Packets distribution always continues to the next client.

Quoted 5 lines

Fixes af75078fece3 ("first public release")
Cc [email protected]
Signed-off-by Gregory Etelson <[email protected]>
Acked-by Anatoly Burakov <[email protected]>

Applied, thanks.

Signature · 1 line

David Marchand