-
Notifications
You must be signed in to change notification settings - Fork 444
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
Ipv4 fragmentation refactored #236
Conversation
Added tests and the Ethernet header size to MTU, instead of #237 |
@pothos It turns out that counting in the ethernet header is strictly speaking needed only for UDP packets. With MTU of 1500, linux sends ethernet frames that are 1514 bytes long. For TCP packets, they are only 1500 bytes long (including the header). You can try this with a wireshark and see for yourself. ICMP (ping) works too - try for example Sending large amount of TCP data is OK, try |
@pothos @whitequark any feedback here? |
@dlrobertson is there anything else I should change in this PR? |
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.
Thanks for working on this! I'm pumped about getting IP fragmentation support into smoltcp
.
Just an initial read through. I'll need to read through a few more times.
src/iface/ethernet.rs
Outdated
@@ -675,14 +695,97 @@ impl<'b, 'c, 'e> InterfaceInner<'b, 'c, 'e> { | |||
} | |||
} | |||
|
|||
/// Process an IPv4 fragment |
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.
whitespace
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.
Fixed
Cargo.toml
Outdated
@@ -34,6 +34,7 @@ verbose = [] | |||
"phy-raw_socket" = ["std", "libc"] | |||
"phy-tap_interface" = ["std", "libc"] | |||
"proto-ipv4" = [] | |||
"fragmentation-ipv4" = ["proto-ipv4"] |
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.
🚲 🏠 : ipv4-fragmentation
? Will it ever make sense to have a single generic ip-fragmentation
feature? Is there a reason to keep two separate features for ipv4 and ipv6?
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 am not familiar with Ipv6 so I cannot comment on how similar to IPv4 it is. But it can be refactored in the future, if someone with more knowledge about Ipv6 can pitch in. I would keep it separate for now, better some fragmentation support than none:-)
} | ||
|
||
/// Get a packet with given key | ||
pub fn get_packet(&mut self, id: u16, src_addr: Ipv4Address, dst_addr: Ipv4Address, time: Instant) -> Option<&mut Packet<'a>> { |
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.
The is O(3n)
right? Is there a way we could make this faster? What if we used a ManagedMap
instead of a ManagedSlice
?
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.
There is a thread in the old PR where I tried to explain why ManagedMap
doesn't work. In short, to insert a new packet to ManagedMap
you need to allocate memory for it somewhere. And either you create a new Vec<u8>
which is easy but requires alloc
. Or you have a static buffers somewhere, and that effectively makes it a ManagedSlice
.
I am not sure how much fragmented packets you normally encounter (TCP doesn't use fragmented packets typically), in our tests the performance penalty was negligible (with large UDP packets).
But - to use fragmentation in no-std
builds, you still need to solve problem with From
conversion for ManagedSlice
here: https://github.com/m-labs/rust-managed/blob/master/src/slice.rs#L74
If you don't have alloc/std then rust doesn't know how to create a FragmentSet
from static buffers. It would be cool to be able to do fragmentation without alloc
, but I am not sure what is the best way around this.
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.
Edit: I realized even the loopback.rs
example you have to run with at least alloc
because of the From
trait implementation. This definitely limits smoltcp's usage on bare metal systems:-(
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.
And either you create a new Vec which is easy but requires alloc. Or you have a static buffers somewhere, and that effectively makes it a ManagedSlice.
Yeah it is basically the same, but you get massivly improved lookup times due to the use of a btree. Couldn't you just use something like the following.
const FRAGMENT_BUFFER_SIZE: usize = // ...
static FRAGMENT_BUFFER: [Option<Ident, FragmentPacket> ; FRAGMENT_BUFFER_SIZE] = [None; FRAGMENT_BUFFER_SIZE];
let fragment_set = FragmentSet::new(FRAGMENT_BUFFER);
I am not sure how much fragmented packets you normally encounter (TCP doesn't use fragmented packets typically), in our tests the performance penalty was negligible (with large UDP packets).
Lots of embedded applicaitons make heavy use of fragmented packets. See 6LoWPAN (RFC 4944 and RFC 6282). As smoltcp
matures this is going to become more and more important. IMO the next two big hurdles are this and breaking up the EthernetInterface
.
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.
Thanks @dlrobertson, these are my thoughts exactly. I would really like to merge this but I'm afraid using ManagedMap
is a requirement.
☔ The latest upstream changes (presumably 94d7338) made this pull request unmergeable. Please resolve the merge conflicts. |
@whitequark @dlrobertson I switched FragmentedSet to ManagedMap. The unresolved things are:
what do you think is the best way to tackle these problems? |
I don't think this is true. See the implementation in rust-managed.
I think the cleanup implementation for the neighbor cache turned out to be a clean implementation. Maybe something like that would work here? I think some sort of count threshold makes more sense. Especially since IIRC the |
I am having a problem using const FRAGMENT_BUFFER_SIZE: usize = 6;
static FRAGMENT_BUFFER: [Option<(FragmentedKey, FragmentedPacket)> ; FRAGMENT_BUFFER_SIZE] = [None; FRAGMENT_BUFFER_SIZE];
let fragment_set = FragmentSet::new(FRAGMENT_BUFFER); I get this error:
I am compiling without |
/// Ethernet header length to be added to the MTU, since the header is not counted into MTU | ||
/// 6 bytes src addr, 6 bytes dst addr, 2 bytes EthType, | ||
/// the optionbal 4 bytes of IEEE 802.1Q Header are not accounted for | ||
pub const ETHERNET_HAEDER_MAX_LEN: usize = 14; |
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 it would be better to use 18 here as correct maximal length because otherwise you could just use EthernetFrame::<&[u8]>::header_len()
which is 14.
When using 18, this has to be changed as well:
diff --git a/src/iface/ethernet.rs b/src/iface/ethernet.rs
index e44abb4..e3fd029 100644
--- a/src/iface/ethernet.rs
+++ b/src/iface/ethernet.rs
@@ -388,7 +388,7 @@ impl<'b, 'c, 'e, DeviceT> Interface<'b, 'c, 'e, DeviceT>
fn socket_egress(&mut self, sockets: &mut SocketSet, timestamp: Instant) -> Result<bool> {
let mut caps = self.device.capabilities();
- caps.max_transmission_unit -= EthernetFrame::<&[u8]>::header_len();
+ caps.max_transmission_unit -= ETHERNET_HAEDER_MAX_LEN;
let mut emitted_any = false;
for mut socket in sockets.iter_mut() {
@@ -1122,7 +1122,7 @@ impl<'b, 'c, 'e> InterfaceInner<'b, 'c, 'e> {
// I'm really not happy about this "solution" but I don't know what else to do.
if let Some(max_burst_size) = caps.max_burst_size {
let mut max_segment_size = caps.max_transmission_unit;
- max_segment_size -= EthernetFrame::<&[u8]>::header_len();
+ max_segment_size -= ETHERNET_HAEDER_MAX_LEN;
max_segment_size -= ip_repr.buffer_len();
max_segment_size -= tcp_repr.header_len();
☔ The latest upstream changes (presumably 58a5473) made this pull request unmergeable. Please resolve the merge conflicts. |
@podhrmic This issue is caused because rust requires elements to implement copy when using the However in this case, since static FRAGMENT_BUFFER: [Option<(FragmentedKey, FragmentedPacket)> FRAGMENT_BUFFER_SIZE] = Default::default(); |
Is there any movement on this? It looks so close to complete it would be a shame to not see it merged. |
Unfortunately I don't have time to finish this, so somebody else would have to step in. I think a little bit more work is needed, but I had the algorithm working in a fork of smoltcp last year. |
591: Fragmentation mechanism r=Dirbaio a=thvdveld This contains the fragmentation mechanism used in #580 (which is heavily based on #236 taking into account the remarks that were made there). I tried to decouple it from the 6LoWPAN implementation such that it can be used for other protocols as well. I opened a separate PR for this such that #580 will only contain the mechanisms that are part of 6LoWPAN only. Co-authored-by: Thibaut Vandervelden <thvdveld@vub.be>
580: 6LoWPAN fragmentation r=Dirbaio a=thvdveld This adds the fragmentation part of 6LoWPAN from [RFC4944 5.3](https://datatracker.ietf.org/doc/html/rfc4944#section-5.3). This is still a work in progress: - [x] Add wire format of the fragmentation header of 6LoWPAN. - [x] Update documentation in wire. - [x] Use packet assembler (based on the IPv4 assembling #236). - [x] Use assembling in the interface for incoming packets. - [x] Assemble packets. - [x] Correct checksum (still need to find out why my checksums are incorrect). - [x] Check the interval between incoming fragments. - [x] Fragment outgoing packets (still not sure how to do this part). - [x] Add overlap detection in `Assembler`. - [x] Handle stale fragments. - [x] Do the correct thing when encountering fragment overlaps. - [x] Finish address resolution for 6LoWPAN. - [x] Try not to use a temporary buffer when trying to fragment outgoing packets. ~~**NOTE**: This requires more changes. For example: the emit function for UDP requires that the whole buffer is passed to it such that even the payload fits in it. However, IEEE802154 only has buffers of 127 bytes (125 bytes without the CRC).~~ Co-authored-by: Thibaut Vandervelden <thvdveld@vub.be> Co-authored-by: AntoonBeres <35069455+AntoonBeres@users.noreply.github.com>
Closing in favor of #624 |
Based on #190 except rebased on the recent master.
Notes based on the previous reviews: