-
Notifications
You must be signed in to change notification settings - Fork 8.1k
net: pkt_filter: Add priority filters to enable quality of service #93341
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.
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
base: main
Are you sure you want to change the base?
net: pkt_filter: Add priority filters to enable quality of service #93341
Conversation
26d64cc
to
a4849f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial feedback (on the topmost 2 commits).
c5f9c6c
to
9bd6ae9
Compare
@rlubos + update_priority_l2(pkt);
+ if (!being_processed_by_correct_thread(pkt)) {
+ net_queue_rx(net_pkt_iface(pkt), pkt);
+ return NET_OK;
+ } For example after extracting the l2-header but before: I believe, that a loop and a if-else-chain or a switch would be more readable. Presumably the added computation cost is negligible? void process_data(struct net_pkt *pkt)
{
// switch would only work if raw packet sockets not needed for loop back etc.
switch (pkt->step) {
case STEP_1:
do_step_1();
return NET_CONTINUE;
case STEP_2:
do_step_2();
return NET_CONTINUE;
case STEP_3:
do_step_3();
return NET_CONTINUE;
// ...
}
}
void processing_data(struct net_pkt *pkt)
{
enum net_verdict verdict = NET_CONTINUE;
do {
verdict = process_data(pkt);
if (verdict != NET_CONTINUE) {
break;
}
update_prio(pkt, pkt->step);
if (!on_correct_thread(pkt)) {
net_queue_rx(pkt);
verdict = NET_OK;
}
} while (verdict == NET_CONTINUE);
// ... unref
} If we do not do this, it will be very hard to follow at which point the processing can be interrupted and then recontinued later. This may be an easy source of bugs. |
9bd6ae9
to
d88b7ac
Compare
d88b7ac
to
80f5552
Compare
Note: contains all commits from #93246 and adds two commit |
80f5552
to
a69163e
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think packet rescheduling logic turned out pretty straightforward and looks fine, however I think we need to think on how would the priority updating look like in practice.
a69163e
to
43cf73c
Compare
f6a7bcb
to
9f98615
Compare
9f98615
to
e301b1b
Compare
461d453
to
74a0377
Compare
include/zephyr/net/net_pkt_filter.h
Outdated
struct npf_rule { | ||
sys_snode_t node; /**< Slist rule list node */ | ||
enum net_verdict result; /**< result if all tests pass */ | ||
uint8_t priority; /**< priority in case of NET_CONTINUE */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put the filter changes to a separate commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done: 0f0ed9a
if (!pkt->frags) { | ||
NET_DBG("Corrupted packet (frags %p)", pkt->frags); | ||
net_stats_update_processing_error(net_pkt_iface(pkt)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated change, please remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
net_stats_update_processing_error( | ||
net_pkt_iface(pkt)); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, and actually it is be preferred that there is an empty line after closing block (so the original code is ok)
subsys/net/ip/net_core.c
Outdated
|
||
return ret; | ||
} | ||
if (!being_processed_by_correct_thread(pkt)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like here, it would be preferred if there is empty line before the added if
statement
subsys/net/ip/net_core.c
Outdated
return NET_DROP; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this extra empty line
|
||
SYS_SLIST_FOR_EACH_CONTAINER(rule_head, rule, node) { | ||
if (apply_tests(rule, pkt) == true) { | ||
if (rule->result == NET_CONTINUE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned earlier, the filter changes should be in separate commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done: 0f0ed9a
#elif defined(CONFIG_NET_TC_THREAD_COOPERATIVE) | ||
#define BASE_PRIO_TX (CONFIG_NET_TC_NUM_PRIORITIES - 1) | ||
#else | ||
#define BASE_PRIO_TX (CONFIG_NET_TC_TX_COUNT - 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not understand the purpose of the commit net: tc: WIP/TEMP spread threads by more then 1 priority level
. Anyway, the commit is WIP so I am assuming the idea comes in subsequent revisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I detailed the commit now.
Basically, without the commit the spacing between the tc-thread priorities must be one. The commit allows to modify it, so that the application writer may interleave with other threads. In the sample for example to alternate between rx and tx thread for nice result, that i now added to the description.
{ | ||
int res; | ||
struct net_pkt *pkt = NULL; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra empty line can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
samples/net/QoS/ethernet/src/main.c
Outdated
copy_mac_to(dest); | ||
|
||
pkt = net_pkt_rx_alloc_with_buffer(iface, MTU, AF_UNSPEC, 0, K_NO_WAIT); | ||
if (!pkt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please write these checks like this if (pkt == NULL) {
because of MISRA rule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
To enable quality-of-service (QoS) applications, allow filters to modify the priority of an incoming packet. This allows that processing can be done on tc-queue-threads with different priorities. Signed-off-by: Cla Mattia Galliard <[email protected]>
Allow to spread tc threads by more then one priority level and cleanup the computation of the priority. Signed-off-by: Cla Mattia Galliard <[email protected]>
74a0377
to
f068f37
Compare
|
Add a sample, that shows the power of quality of service in Zephyr. Signed-off-by: Cla Mattia Galliard <[email protected]>
f068f37
to
48a6dea
Compare
|
|
Quality of Service for Ethernet
To enable quality-of-service (QoS) applications, add net_pkt_filters with
result
NET_CONTINUE
, that allow to change the priority of an incoming packet dynamically. Then the processing of a network packet will happen on the respective traffic-class-queue. This enables a gradual degrading of availabilit of a service based on its priority (user application configurable).Results (Sample in this PR)
With QoS
c (x) := command service for priority x (high means higher priority)
e (x) := echo service for priority x (high means higher priority)
Without QoS
c (x) := command service for priority x (high means higher priority)
e (x) := echo service for priority x (high means higher priority)

Future Work
net_conn_input