gnrc/netif: 6lo: use 16 bit for max_frag_size#13979
Merged
miri64 merged 2 commits intoRIOT-OS:masterfrom May 6, 2020
Merged
Conversation
benpicco
added
Type: enhancement
benpicco
added
the
CI: ready for build
benpicco
requested review from
PeterKietzmann,
cgundogan and
haukepetersen
as code owners
Contributor
Author
|
The change required to make use of the full PDU is equally simple, so I piggybacked it here too. I had problems sending larger packets though, but that seems more like a driver / timing issue with RTTs > 1s getting unreliable. edit: who would have thought, but sending pings faster than the RTT does indeed lead to packet loss. Works with 2s between pings. |
Using 8 bit for `max_frag_size` limits that value to 255.
802.15.4g specifies a PDU of 2047 bytes which exceeds that limit.
Using a 16 bit value here allows to use the full L2 PDU.
Before:
12:02:16,300 # ping6 fe80::2068:3123:59f5:d238%8 -s 400
12:02:16,302 # sending 244 bytes
12:02:16,387 # sending 218 bytes
12:02:16,624 # 408 bytes from fe80::2068:3123:59f5:d238%8: icmp_seq=0 ttl=64 rssi=-49 dBm time=316.307 ms
12:02:17,302 # sending 244 bytes
12:02:17,387 # sending 218 bytes
12:02:17,624 # 408 bytes from fe80::2068:3123:59f5:d238%8: icmp_seq=1 ttl=64 rssi=-49 dBm time=316.307 ms
12:02:18,302 # sending 244 bytes
12:02:18,387 # sending 218 bytes
12:02:18,624 # 408 bytes from fe80::2068:3123:59f5:d238%8: icmp_seq=2 ttl=64 rssi=-50 dBm time=316.306 ms
12:02:18,625 #
12:02:18,629 # --- fe80::2068:3123:59f5:d238 PING statistics ---
With this patch:
12:09:44,276 # ping6 fe80::2068:3123:59f5:d238%8 -s 400
12:09:44,278 # sending 432 bytes
12:09:44,574 # 408 bytes from fe80::2068:3123:59f5:d238%8: icmp_seq=0 ttl=64 rssi=-52 dBm time=289.888 ms
12:09:45,279 # sending 432 bytes
12:09:45,574 # 408 bytes from fe80::2068:3123:59f5:d238%8: icmp_seq=1 ttl=64 rssi=-52 dBm time=289.885 ms
12:09:46,069 # sending 43 bytes
12:09:46,279 # sending 432 bytes
12:09:46,499 # sending 43 bytes
12:09:46,574 # 408 bytes from fe80::2068:3123:59f5:d238%8: icmp_seq=2 ttl=64 rssi=-47 dBm time=289.886 ms
12:09:46,574 #
12:09:46,578 # --- fe80::2068:3123:59f5:d238 PING statistics ---
If the L2-PDU is greater than IPV6_MIN_MTU, use that as the MTU instead.
benpicco
force-pushed
the
net/gnrc/netif/6lo-frag_size
branch
from
01102d5 to
18651eb
Compare
Contributor
Author
|
If I use a higher date rate ( Here, the data doesn't even each the driver. I'm a bit puzzled where those 40 bytes of headers are coming from here. |
Contributor
Author
|
For legacy O-QPSK the largest non-fragmented payload is 99 bytes, only 3 bytes less than the L2-PDU. Which results in a 123 byte frame (only 4 bytes shy of the max frame size of 127 bytes) |
miri64
reviewed
Apr 29, 2020
| assert(res == sizeof(tmp)); | ||
| #ifdef MODULE_GNRC_SIXLOWPAN | ||
| netif->ipv6.mtu = IPV6_MIN_MTU; | ||
| netif->ipv6.mtu = MAX(IPV6_MIN_MTU, tmp); |
Member
There was a problem hiding this comment.
This is not a good idea, as this will lead to fragmentation in the wider internet. Better cap it at 1500.
Member
There was a problem hiding this comment.
Or better: we should implement Path MTU discovery ;-P
Contributor
Author
There was a problem hiding this comment.
I'll see what I can do for Path MTU discovery, but that should be unrelated.
Btw.: SLIP already does MTU:65535 HL:64 RTR
Member
There was a problem hiding this comment.
Yeah. My bad. This comment was more loud thinking and inner alarm bells ringing ;-).
This was referenced Apr 29, 2020
Contributor
Author
|
btw I think this is still useful even without Path MTU discovery 😉 |
Member
|
Yes, of course. Just hadn't time to review the past few days. :-) |
miri64
approved these changes
May 6, 2020
Member
miri64
left a comment
miri64
left a comment
There was a problem hiding this comment.
I have no way of testing this, as I don't have the hardware. However, the test looks sane and does not increase the RAM usage of the gnrc_netif module.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contribution description
Using 8 bit for
max_frag_sizelimits that value to 255.802.15.4g (
at86rf215,cc26x2_cc13x2_rf) specifies a PDU of 2047 bytes which exceeds that limit.Using a 16 bit value here allows to use the full L2 PDU.
Before:
With this patch:
Testing procedure
Apply this patch to print the size of the TX frames:
Enable a 802.15.4g PHY mode
Unfortunately the MTU is still smaller than the L2-PDU so packets are still limited to 1280 bytes.
Issues/PRs references
#13912