net: tc: Limit the max amount of packets in the queue per traffic class #84442

clamattia · 2025-01-23T12:48:33Z

To avoid starvation of a traffic class by another, limit the maximum amount of packets, that can sit in a single traffic-class-fifo to a fraction of the maximum amount of available rx-packets.

clamattia · 2025-01-23T12:49:03Z

Proposal for: #84415

subsys/net/ip/net_tc.c

rlubos · 2025-01-23T13:59:10Z

subsys/net/ip/net_tc.c

@@ -254,6 +269,8 @@ static void tc_rx_handler(void *p1, void *p2, void *p3)
 			continue;
 		}

+		k_sem_give(fifo_slot);


include/zephyr/net/net_if.h

subsys/net/ip/net_tc.c

clamattia · 2025-01-23T16:25:31Z

Something that came to mind. The proposed approach is too conservative. It could be allowed for higher priority queues to hold all available packets. It could be only forbidden for lower priority queues.
Proposal as outlined by this pr so far:

CONFIG_NET_PKT_RX_COUNT=40
CONFIG_NET_TC_RX_COUNT=4
rx_q[0] max_slots : 10
rx_q[1] max_slots : 10
rx_q[2] max_slots : 10
rx_q[3] max_slots : 10

Less conservative, but also starvation avoiding:

CONFIG_NET_PKT_RX_COUNT=40
CONFIG_NET_TC_RX_COUNT=4
rx_q[0] max_slots : 10
rx_q[1] max_slots : 10
rx_q[2] max_slots : 10
rx_q[3] max_slots : 40

clamattia · 2025-01-23T16:37:31Z

Fixed copy paste mistake rx_classes vs tx_classes and removed the wrapper around k_fifo_put.

clamattia · 2025-01-24T07:33:38Z

Also avoid starvation of tx, if CONFIG_NET_TC_SKIP_FOR_HIGH_PRIO is set. It appears as another pseudo traffic class.
Removed the conditional addition of k_sem in favor of having it always due to added complexity of another case (can add it back if desired.
Added build assert for slots>0 to avoid misconfiguration.

clamattia · 2025-01-24T08:47:39Z

Added statistics in separate commit
Aligned the net_pkt_set_rx_stats_tick call (let me know if this is not correct)
Sent statistics only updated if not dropped
Remove unconditional increment and then decrement of tx_pending. Let me know, if this is not correct.
Added to statistics shell command. Let me know, if additional tab(s) is needed for nice layout.

Thanks in advance

clamattia · 2025-01-24T08:55:19Z

Fixed silly mistakes

clamattia · 2025-01-24T09:44:12Z

Rebased and fixed build failure in tests.

clamattia · 2025-01-24T11:11:47Z

Fixed minor oversight

jukkar

Some initial comments for this, will continue the review later.

jukkar · 2025-01-24T11:28:44Z

subsys/net/ip/net_core.c

+	net_pkt_set_rx_stats_tick(pkt, k_cycle_get_32());
+
 	if (NET_TC_RX_COUNT == 0) {
 		net_process_rx_packet(pkt);
 	} else {
-		net_tc_submit_to_rx_queue(tc, pkt);
+		if (net_tc_submit_to_rx_queue(tc, pkt) != NET_OK) {
+			goto drop;
+		}


This commit seems to be mixing statistics with other changes. Please only do the statistics update in this commit.

Fair, I have pushed the other stuff in the initial commit. Let me know if more splitting is needed.

jukkar · 2025-01-24T11:29:33Z

include/zephyr/net/net_stats.h

@@ -328,6 +328,8 @@ struct net_stats_tc {
 #endif


Tautology in commit net: net_tc: ... subject, it is enough to have net: tc: ... here.

jukkar · 2025-01-24T11:30:55Z

subsys/net/ip/net_tc.c

@@ -318,10 +364,14 @@ void net_tc_tx_init(void)

 		k_fifo_init(&tx_classes[i].fifo);

+		k_sem_init(&tx_classes[i].fifo_slot, NET_TC_TX_SLOTS, NET_TC_TX_SLOTS);


missing #if NET_TC_RX_COUNT > 1

I was of the opinion, that initing did not hurt.
Added

jukkar · 2025-01-24T11:31:07Z

subsys/net/ip/net_tc.c

@@ -376,10 +426,14 @@ void net_tc_rx_init(void)

 		k_fifo_init(&rx_classes[i].fifo);

+		k_sem_init(&rx_classes[i].fifo_slot, NET_TC_RX_SLOTS, NET_TC_RX_SLOTS);


jukkar · 2025-01-24T11:32:41Z

include/zephyr/net/net_if.h

@@ -614,6 +614,9 @@ struct net_traffic_class {
 	/** Fifo for handling this Tx or Rx packet */
 	struct k_fifo fifo;

+	/** Semaphore for tracking the available slots in the fifo */
+	struct k_sem fifo_slot;


Perhaps we need a separate Kconfig option to enable this. User should be able to select if this is to be used or not.

I would not do that. If someone enables NET_TC_XX_COUNT>1 they expect the different traffic classes not being able to starve each other.

Otherwise it is effectively as if NET_TC_XX_COUNT==1

To avoid starvation of a traffic class by another, limit the maximum amount of packets, that can sit in a single traffic-class-fifo to a fraction of the maximum amount of available packets. In the tx-case also reserve packets for direct sending, in the case, where CONFIG_NET_TC_SKIP_FOR_HIGH_PRIO is enabled. Signed-off-by: Cla Mattia Galliard <[email protected]>

clamattia · 2025-01-24T12:08:10Z

Fixed suggestions from @jukkar
Fixed a problem with tx_pending, please review, if I got this right

clamattia · 2025-01-24T12:22:47Z

Fixed net_pkt_get_len of possibly unrefed pkt.

Add statistics about packets dropped in net_tc. Signed-off-by: Cla Mattia Galliard <[email protected]>

clamattia · 2025-01-24T14:41:28Z

log len so it appears used, when statistics are disabled

clamattia mentioned this pull request Jan 23, 2025

Proposal: Improve rx_qs by putting a cap on the amount of packets they hold #84415

Open

zephyrbot added the area: Networking label Jan 23, 2025

zephyrbot requested review from jukkar, pdgendt, rlubos, ssharks and tbursztyka January 23, 2025 12:49

zephyrbot assigned rlubos and jukkar Jan 23, 2025

clamattia force-pushed the net_tc_limit_the_packet_count_per_queue branch 4 times, most recently from 12e8b94 to 20b7d7d Compare January 23, 2025 13:08

rlubos reviewed Jan 23, 2025

View reviewed changes

clamattia force-pushed the net_tc_limit_the_packet_count_per_queue branch from 20b7d7d to 5c78ff1 Compare January 23, 2025 14:43

pdgendt reviewed Jan 23, 2025

View reviewed changes

include/zephyr/net/net_if.h Show resolved Hide resolved

clamattia force-pushed the net_tc_limit_the_packet_count_per_queue branch from 5c78ff1 to 5ff607b Compare January 23, 2025 14:50

pdgendt reviewed Jan 23, 2025

View reviewed changes

subsys/net/ip/net_tc.c Outdated Show resolved Hide resolved

clamattia force-pushed the net_tc_limit_the_packet_count_per_queue branch from 5ff607b to 830ae9a Compare January 23, 2025 14:55

clamattia force-pushed the net_tc_limit_the_packet_count_per_queue branch from 830ae9a to 83c35af Compare January 23, 2025 16:36

clamattia force-pushed the net_tc_limit_the_packet_count_per_queue branch from 83c35af to 44bc833 Compare January 24, 2025 07:32

clamattia force-pushed the net_tc_limit_the_packet_count_per_queue branch 2 times, most recently from 5db4c26 to aa5c538 Compare January 24, 2025 07:41

clamattia requested review from rlubos and pdgendt January 24, 2025 08:48

clamattia force-pushed the net_tc_limit_the_packet_count_per_queue branch 3 times, most recently from b80466a to e67accc Compare January 24, 2025 08:54

clamattia force-pushed the net_tc_limit_the_packet_count_per_queue branch from e67accc to dc86bf0 Compare January 24, 2025 09:43

clamattia force-pushed the net_tc_limit_the_packet_count_per_queue branch from dc86bf0 to e9fc34d Compare January 24, 2025 11:11

jukkar reviewed Jan 24, 2025

View reviewed changes

clamattia mentioned this pull request Jan 24, 2025

High Ethernet network load starves user tasks #27662

Open

clamattia force-pushed the net_tc_limit_the_packet_count_per_queue branch from e9fc34d to e853c6b Compare January 24, 2025 12:05

clamattia requested a review from jukkar January 24, 2025 12:08

clamattia force-pushed the net_tc_limit_the_packet_count_per_queue branch from e853c6b to e8cff89 Compare January 24, 2025 12:22

clamattia mentioned this pull request Jan 24, 2025

Proposal: Ethernet centralize assignment of net-pkt priorities #84501

Open

net: tc: Add statistics about dropped packets

fe9a5b3

Add statistics about packets dropped in net_tc. Signed-off-by: Cla Mattia Galliard <[email protected]>

clamattia force-pushed the net_tc_limit_the_packet_count_per_queue branch from e8cff89 to fe9a5b3 Compare January 24, 2025 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: tc: Limit the max amount of packets in the queue per traffic class #84442

net: tc: Limit the max amount of packets in the queue per traffic class #84442

clamattia commented Jan 23, 2025

clamattia commented Jan 23, 2025

rlubos Jan 23, 2025

clamattia Jan 23, 2025

clamattia commented Jan 23, 2025 •

edited

Loading

clamattia commented Jan 23, 2025

clamattia commented Jan 24, 2025 •

edited

Loading

clamattia commented Jan 24, 2025

clamattia commented Jan 24, 2025

clamattia commented Jan 24, 2025

clamattia commented Jan 24, 2025

jukkar left a comment

jukkar Jan 24, 2025

clamattia Jan 24, 2025

jukkar Jan 24, 2025

clamattia Jan 24, 2025

jukkar Jan 24, 2025

clamattia Jan 24, 2025

jukkar Jan 24, 2025

clamattia Jan 24, 2025

jukkar Jan 24, 2025

clamattia Jan 24, 2025

clamattia Jan 24, 2025

clamattia commented Jan 24, 2025

clamattia commented Jan 24, 2025

clamattia commented Jan 24, 2025

		@@ -318,10 +364,14 @@ void net_tc_tx_init(void)

		k_fifo_init(&tx_classes[i].fifo);

		k_sem_init(&tx_classes[i].fifo_slot, NET_TC_TX_SLOTS, NET_TC_TX_SLOTS);

		@@ -376,10 +426,14 @@ void net_tc_rx_init(void)

		k_fifo_init(&rx_classes[i].fifo);

		k_sem_init(&rx_classes[i].fifo_slot, NET_TC_RX_SLOTS, NET_TC_RX_SLOTS);

net: tc: Limit the max amount of packets in the queue per traffic class #84442

Are you sure you want to change the base?

net: tc: Limit the max amount of packets in the queue per traffic class #84442

Conversation

clamattia commented Jan 23, 2025

clamattia commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clamattia commented Jan 23, 2025 • edited Loading

clamattia commented Jan 23, 2025

clamattia commented Jan 24, 2025 • edited Loading

clamattia commented Jan 24, 2025

clamattia commented Jan 24, 2025

clamattia commented Jan 24, 2025

clamattia commented Jan 24, 2025

jukkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clamattia commented Jan 24, 2025

clamattia commented Jan 24, 2025

clamattia commented Jan 24, 2025

clamattia commented Jan 23, 2025 •

edited

Loading

clamattia commented Jan 24, 2025 •

edited

Loading