Network traffic classfication support #6076
Conversation
jukkar
added
area: Networking
RFC
jukkar
requested review from
andrewboie,
andyross,
dbkinder,
nashif,
SebastianBoe and
youvedeep
as code owners
| @@ -332,8 +352,10 @@ struct net_if_dev { | |||
| /** The hardware link address */ | |||
| struct net_linkaddr link_addr; | |||
|
|
|||
| /** Queue for outgoing packets from apps */ | |||
| struct k_fifo tx_queue; | |||
There was a problem hiding this comment.
you don't want to get rid of that, no. It should be either/or.
On 1 tx queue (the common use-case currently) k_work_queue is going to bring an overhead as soon as you'll have 2+ ifaces. I actually centralized all in 1 unique tx thread last year exactly because of that: we were having 1 thread per-iface and that was really greedy on memory constrained system for 2+ ifaces being present. It's a regression on that aspect.
There was a problem hiding this comment.
you don't want to get rid of that, no. It should be either/or.
I disagree here, we should not have any ifdef's regarding this.
On 1 tx queue (the common use-case currently) k_work_queue is going to bring an overhead as soon as you'll have 2+ ifaces. I actually centralized all in 1 unique tx thread last year exactly because of that: we were having 1 thread per-iface and that was really greedy on memory constrained system for 2+ ifaces being present. It's a regression on that aspect.
There should be no regression in this respect by this PR. There will be only one work queue / network device so same thing as before. Note that there can be multiple network interfaces / network device, this was introduced by VLAN in #5688. Please re-review the code, imho this works quite nicely even if we have only 1 traffic class.
There was a problem hiding this comment.
Check net_if.c: net_if_tx_thread is unique. It runs for all net_if arounds (so all net devices).
k_work_queue here is going to break that (for instance if you have one net_if for bt ipsp, and one for 15.4.).
Not sure how to tackle this however. Having ifdefs all around is not nice indeed.
There was a problem hiding this comment.
separating the traffic struct place holder as you did for ip configuration could be one solution.
samples/net/traffic_class/README.rst
Outdated
| Overview | ||
| ******** | ||
|
|
||
| The TC sample application for Zephyr will setup two virtual LAN networks and |
samples/net/traffic_class/README.rst
Outdated
| ******** | ||
|
|
||
| The TC sample application for Zephyr will setup two virtual LAN networks and | ||
| starts to send prioritised UDP packets to peer. The use of VLAN network is |
There was a problem hiding this comment.
prioritized (American spelling)
It might be clearer to say:
send prioritized UDP packets from one VLAN network to the other.
samples/net/traffic_class/README.rst
Outdated
| Building and Running | ||
| ******************** | ||
|
|
||
| A good way to run this sample is to run this TC application inside QEMU |
There was a problem hiding this comment.
How about:
A good way to run this TC application is with QEMU
samples/net/traffic_class/README.rst
Outdated
| A good way to run this sample is to run this TC application inside QEMU | ||
| as described in :ref:`networking_with_qemu`. | ||
| Currently only one VLAN network (tag) is supported when Zephyr is run inside | ||
| QEMU. If using FRDM-K64F board, then multiple VLAN networks can be configured. |
There was a problem hiding this comment.
If you're using a FRDM-K64F board
samples/net/traffic_class/README.rst
Outdated
| routes to Zephyr. | ||
|
|
||
| If everything is configured correctly, you will be able to successfully execute | ||
| following commands on the Linux host. |
samples/net/vlan/README.rst
Outdated
| Building and Running | ||
| ******************** | ||
|
|
||
| A good way to run this sample is to run this VLAN application inside QEMU |
There was a problem hiding this comment.
Same comment:
A good way to run this VLAN application is with QEMU
samples/net/vlan/README.rst
Outdated
| routes to Zephyr. | ||
|
|
||
| If everything is configured correctly, you will be able to successfully execute | ||
| following commands on the Linux host. |
|
Taking a look "net: core: Add array support to stack info helpers" commit. The description is:
Nice. But why would one want to create "stack of arrays" using NET_STACK_ARRAY_DEFINE() macro? What usecase does it address? Especially that it's defined as: I.e. just renames existing functionality, duplicating macros. The whole terminology is rather ambiguous too. Reading "stack of arrays", I can imagine anything from an abstract stack (i.e. LIFO) data structure which allows to push similarly abstract array objects on it, to a specialized IP stack which deals with networking arrays (no idea what the latter would be, but if we have storage arrays, we've got to have networking arrays too, right?). I could even imagine there's a typo and it's "array of stacks", but given the overall confusion, I wouldn't be sure. |
|
Can this be marked with explicit "Milestone" setting please, to allow for better resource allocation for the 1.11 release preparation as happening now? Thanks. |
This is definitely not for 1.11 but I do not think you really need to care about what release this PR is targeted at. Anas will assign it a milestone according to release plan. |
No exactly. If you look more carefully the code, the macro is adding some extra stuff if net-shell is enabled. Then "net stacks" command will print more information about the created work queue stacks. |
Right, and my suggestion was to add that information to the commit message. Even "it's done similarly to other functionality wrapped by net subsystem (e.g. <example1>, <example2>)" would give good insight that at least it's consistent with existing state of affairs, and "Then "net stacks" command will print more information" is even more useful. (Otherwise yeah, I love these Monday's morning reviews (like when I posted that), it allows to emulate newcomer's perceptions more or less faithfully ;-) ). |
Well, I don't know, #5721 wasn't marked as 1.11, contained quite big refactors and new features, and was merged after the feature freeze date, according to https://github.com/zephyrproject-rtos/zephyr/wiki/Program-Management (there was no announcement on the mailing list, but apparently that would mean that the dates in the doc prevail). So, one may wonder about the plans re: this PR. I'm sure that Anas does the right thing, but as we have technical features to improve the visibility, would be nice to use them. |
jukkar
force-pushed
the
traffic-class
branch
from
b01b177 to
8a4ec91
Compare
|
Updated according to comments. |
Codecov Report
@@ Coverage Diff @@
## master #6076 +/- ##
==========================================
+ Coverage 54.64% 54.89% +0.25%
==========================================
Files 447 449 +2
Lines 42587 43317 +730
Branches 8040 8153 +113
==========================================
+ Hits 23272 23780 +508
- Misses 16084 16239 +155
- Partials 3231 3298 +67
Continue to review full report at Codecov.
|
jukkar
force-pushed
the
traffic-class
branch
from
8a4ec91 to
1669de1
Compare
|
Rebased because of changes in master. |
jukkar
force-pushed
the
traffic-class
branch
from
1669de1 to
ca042f1
Compare
|
Fixing merge conflicts and sanitycheck errors. |
jukkar
force-pushed
the
traffic-class
branch
from
ca042f1 to
bb33b9a
Compare
jukkar
force-pushed
the
traffic-class
branch
from
bb33b9a to
3009552
Compare
|
Fixing again sanity issues. |
|
sanity fixes |
|
more sanity fixes |
|
rebase against master |
|
Rebase against master. Fixed various sanity issues and couple of patches removed from this PR as they were no longer needed. |
include/net/net_if.h
Outdated
|
|
||
| /** T1 (Renewal) timer */ | ||
| struct k_delayed_work t1_timer; | ||
| } __net_if_align; |
There was a problem hiding this comment.
is it still needed though the struct is way much smaller now?
There was a problem hiding this comment.
It seems we can leave this alignment away, at least the unit test runs fine without the huge alignment.
| * | ||
| * @return 0 if ok, <0 if error | ||
| */ | ||
| int net_context_set_option(struct net_context *context, |
There was a problem hiding this comment.
So that's basically like set_sockopt(), ok.
There was a problem hiding this comment.
Yes, I made it look similar so it is easy to map to setsockopt() if needed.
include/linker/common-ram.ld
Outdated
| @@ -170,14 +170,6 @@ | |||
| __net_if_dev_end = .; | |||
There was a problem hiding this comment.
Seems fine to me.
However, to de-clutter net_core and net_if and avoid scattering TC stuff: maybe centralizing most of TC functions into net_tc.c would be nice
| @@ -163,7 +164,7 @@ static bool net_if_tx(struct net_if *iface, struct net_pkt *pkt) | |||
|
|
|||
| net_pkt_unref(pkt); | |||
| } else { | |||
| net_stats_update_bytes_sent(pkt_len); | |||
| net_stats_update_bytes_sent(pkt->total_pkt_len); | |||
There was a problem hiding this comment.
I am missing something here but where is pkt->total_pkt_len set?
There was a problem hiding this comment.
It is set in net_if_queue_tx() in line 201
There was a problem hiding this comment.
in next patch right, not this one?
There was a problem hiding this comment.
in next patch right, not this one?
Yes, I merged the two statistics patches to one.
include/net/net_stats.h
Outdated
| @@ -228,6 +228,27 @@ struct net_stats_ipv6_mld { | |||
| net_stats_t drop; | |||
| }; | |||
|
|
|||
| #if !defined(NET_TC_COUNT) | |||
There was a problem hiding this comment.
Shouldn't this be always set to, at least, 1?
There was a problem hiding this comment.
It is set to 1 in net_core.h, but I had some issues with this earlier. I will check if this check can be removed from net_stats.h
subsys/net/ip/net_core.c
Outdated
| size_t pkt_len = net_pkt_get_len(pkt); | ||
| size_t pkt_len; | ||
| #if defined(CONFIG_NET_STATISTICS) | ||
| pkt_len = pkt->total_pkt_len; |
There was a problem hiding this comment.
Ok, I guess you'll need to merge previous patch into this one. Then it's clearer.
|
New version according to comments. Also moved traffic class functionality to net_tc.c although there is not much stuff there but it is probably more easier to review now. |
Move IP address settings from net_if to separate structs. This is needed for VLAN support. Signed-off-by: Jukka Rissanen <[email protected]>
Instead of always allocating both IPv6 and IPv4 address information to every network interface, allow more fine grained address configuration. So it is possible to have IPv6 or IPv4 only network interfaces. This commit introduces two new config options: CONFIG_NET_IF_MAX_IPV4_COUNT and CONFIG_NET_IF_MAX_IPV6_COUNT which tell how many IP address information structs are allocated statically. At runtime when network interface is setup, it is then possible to attach this IP address info struct to a specific network interface. This can save considerable amount of memory as the IP address information struct can be quite large (depends on how many IP addresses user configures in the system). Note that the value of CONFIG_NET_IF_MAX_IPV4_COUNT and CONFIG_NET_IF_MAX_IPV6_COUNT should reflect the estimated number of network interfaces in the system. So if if CONFIG_NET_IF_MAX_IPV6_COUNT is set to 1 and there are two network interfaces that need IPv6 addresses, then the system will not be able to setup IPv6 addresses to the second network interface in this case. This scenario might be just fine if the second network interface is IPv4 only. The net_if.c will print a warning during startup if mismatch about the counts and the actual number of network interface is detected. Signed-off-by: Jukka Rissanen <[email protected]>
Add context option support and implement PRIORITY option that can be used to classify the network traffic to different trafic classes according to said priority value. Signed-off-by: Jukka Rissanen <[email protected]>
Allow caller to create array of thread stacks using NET_STACK_ARRAY_DEFINE() macro. This allows more debug information to be printed by "net stacks" command. Signed-off-by: Jukka Rissanen <[email protected]>
With this commit it is possible to add priority to sent or received network packets. So user is able to send or receive higher priority packets faster than lower level packets. The traffic class support is activated by CONFIG_NET_TC_COUNT option. The TC support uses work queues to separate the traffic. The priority of the work queue thread specifies the ordering of the network traffic. Each work queue thread handles traffic to one specific work queue. Note that you should not enable traffic classes unless you really need them by your application. Each TC thread needs stack so this feature requires more memory. It is possible to disable transmit traffic class support and keep the receive traffic class support, or vice versa. If both RX and TX traffic classes are enabled, then both will use the same number of queues defined by CONFIG_NET_TC_COUNT option. Fixes zephyrproject-rtos#6588 Signed-off-by: Jukka Rissanen <[email protected]>
Test that we can set the priority of the network packet and that high priority packets are sent before low priority ones. Signed-off-by: Jukka Rissanen <[email protected]>
Add statistics for number of packets and bytes to each traffic class. Print this information in net-shell. Also make sure that we do not calculate total packet length many times. So calculate network packet total length once and then use that value instead of calculating it many times in a row. Signed-off-by: Jukka Rissanen <[email protected]>
The "net stacks" command was printing TX or RX thread values so that it was not possible to use the values in debugging easily. Add traffic class value when printing the info so that it is easy to see what TX or RX queue is doing. Signed-off-by: Jukka Rissanen <[email protected]>
This demostrates how to classify the network traffic when sending data. The application will create similar functionality as echo-client so user can use echo-server running in remote host to test this sample application. Signed-off-by: Jukka Rissanen <[email protected]>
This avoids this crashing in qemu_x86 ***** Stack Check Fail! ***** Current thread ID = 0x004015e0 Faulting segment:address = 0x0008:0x00003593 eax: 0x0041e930, ebx: 0x00000000, ecx: 0x004170bc, edx: 0x0000dff8 esi: 0xffffffff, edi: 0x0041e930, ebp: 0x00417010, esp: 0x00416ffc eflags: 0x216 Fatal fault in essential thread! Spinning... Terminate emulator due to fatal kernel error Signed-off-by: Jukka Rissanen <[email protected]>
There can be lot of traffic class threads and each will have their own stacks. This can trigger issue when traversing the stacks list in "net stacks" shell command. To overcome this issue, we need to align each net_stack_info struct by 32 bytes. This is the same issue that happened with net_if earlier. Signed-off-by: Jukka Rissanen <[email protected]>
|
@nashif could you force-apply this one as sanity is complaining non-issue and I cannot merge this? |
This adds simple traffic classification support in TX and RX. This means that when packet is sent or received, it can be placed to different TX or RX queue depending on priority of the packet. This PR tries to keep things simple at this stage so no fancy queueing discipline implementation.