Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

net: tc: Add a skip for rx-queues #84600

Merged
merged 1 commit into from
Jan 31, 2025
Merged

net: tc: Add a skip for rx-queues #84600

merged 1 commit into from
Jan 31, 2025
+28 −10

Conversation

clamattia
Copy link
Contributor

@clamattia clamattia commented Jan 27, 2025 •
edited by kartben
Loading

Add a configuration option to skip the rx-queues for high priority packets
analogously to the tx-side.

Based on #84442, this PR proposes 8cc6d69 on top.
Implementation of: #84524.

@clamattia
Copy link
Contributor Author

  • Added a warning in the description of the option

I would like to propose additionally, that an alias NET_TC_TX_SKIP_FOR_HIGH_PRIO be introduced for NET_TC_SKIP_FOR_HIGH_PRIO and that NET_TC_SKIP_FOR_HIGH_PRIO is deprecated in favor of NET_TC_TX_SKIP_FOR_HIGH_PRIO to avoid confusing rx and tx side.

@clamattia clamattia requested a review from rlubos January 29, 2025 09:50
@rlubos
Copy link
Contributor

rlubos commented Jan 29, 2025

I would like to propose additionally, that an alias NET_TC_TX_SKIP_FOR_HIGH_PRIO be introduced for NET_TC_SKIP_FOR_HIGH_PRIO and that NET_TC_SKIP_FOR_HIGH_PRIO is deprecated in favor of NET_TC_TX_SKIP_FOR_HIGH_PRIO to avoid confusing rx and tx side.

Sounds quite reasonable to me.

@clamattia
Copy link
Contributor Author

I would like to propose additionally, that an alias NET_TC_TX_SKIP_FOR_HIGH_PRIO be introduced for NET_TC_SKIP_FOR_HIGH_PRIO and that NET_TC_SKIP_FOR_HIGH_PRIO is deprecated in favor of NET_TC_TX_SKIP_FOR_HIGH_PRIO to avoid confusing rx and tx side.

Sounds quite reasonable to me.

Need this be done in this PR? If so, could you give some guidance on how to do it?

@rlubos
Copy link
Contributor

rlubos commented Jan 29, 2025

I would like to propose additionally, that an alias NET_TC_TX_SKIP_FOR_HIGH_PRIO be introduced for NET_TC_SKIP_FOR_HIGH_PRIO and that NET_TC_SKIP_FOR_HIGH_PRIO is deprecated in favor of NET_TC_TX_SKIP_FOR_HIGH_PRIO to avoid confusing rx and tx side.

Sounds quite reasonable to me.

Need this be done in this PR? If so, could you give some guidance on how to do it?

Doesn't have to, but it could.

Regarding deprecation:

  • Add new option, as a copy of the old option. Set default NET_TC_SKIP_FOR_HIGH_PRIO so that apps using the old symbol will still work during the deprecation period.
  • Add [DEPRECATED] suffix in the old option description and select DEPRECATED, change help text to tell option is deprecated.
  • Replace any usage of the old option with a new one within the codebase.
  • Add 4.1 migration guide entry about the deprecation.

@clamattia
Copy link
Contributor Author

Updated based on latest changes from: #84442

@clamattia clamattia requested a review from jukkar January 30, 2025 12:17
jukkar
jukkar reviewed Jan 30, 2025
View reviewed changes
subsys/net/ip/net_core.c Outdated
@@ -458,28 +458,23 @@ static void net_rx(struct net_if *iface, struct net_pkt *pkt)

void net_process_rx_packet(struct net_pkt *pkt)
{
net_pkt_set_rx_stats_tick(pkt, k_cycle_get_32());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why removing this? It was originally placed here to calculate how long the packet processing takes for this tc. If you move it to net_queue_rx() then we are also calculating any extra processing in tc side which was not the point.
Also this commit was about adding statistics and this change is not related to that. If you think this line is incorrect now, please propose a change in another commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It happened in the caller in the if and else case. So I hoisted it there, now more stats updating happens in the same function. It has at least to do with the statistics. To me this seemed more logical. Will remove and make separate PR to not hold this one off any longer.

subsys/net/ip/net_if.c Outdated
@@ -367,8 +366,6 @@ void net_if_queue_tx(struct net_if *iface, struct net_pkt *pkt)
*/
if ((IS_ENABLED(CONFIG_NET_TC_SKIP_FOR_HIGH_PRIO) &&
prio >= NET_PRIORITY_CA) || NET_TC_TX_COUNT == 0) {
net_pkt_set_tx_stats_tick(pkt, k_cycle_get_32());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment to this one, why do we need to change this?

@clamattia
Copy link
Contributor Author

Fixed @jukkar suggestion over there: #84442
And rebased this on top.

@clamattia clamattia requested a review from jukkar January 30, 2025 15:23
jukkar
jukkar previously approved these changes Jan 30, 2025
View reviewed changes
@clamattia
net: tc: Add a skip for rx-queues
82abfe1 
Add a configuration option to skip the rx-queues for high priority packets
analogously to the tx-side.

Signed-off-by: Cla Mattia Galliard <[email protected]>
@clamattia
Copy link
Contributor Author

rebased onto main

@clamattia clamattia mentioned this pull request Jan 31, 2025
Open
@clamattia clamattia requested a review from jukkar January 31, 2025 07:49
@kartben kartben merged commit bb3ac84 into zephyrproject-rtos:main Jan 31, 2025
26 checks passed
@clamattia clamattia mentioned this pull request Jan 31, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rlubos rlubos rlubos approved these changes

@jukkar jukkar jukkar approved these changes

@pdgendt pdgendt Awaiting requested review from pdgendt

@ssharks ssharks Awaiting requested review from ssharks

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

Assignees

@jukkar jukkar

@rlubos rlubos

Labels
area: Networking
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@clamattia @rlubos @jukkar @kartben @zephyrbot