-
Notifications
You must be signed in to change notification settings - Fork 447
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
[RFC] Packet dispatch #19
Comments
This already works, doesn't it?
I have an idea that doesn't involve even more conditional compilation. Add two storage items:
|
Regarding ingress, using BTreeMap instead of a HashMap reduces the |
Yeah, I meant it for the packet dispatch mechanism, meaning a simple port -> socket map won't work.
Right, we don't need a vector for this, we can use ManagedSlice.
How tho? Right now a Socket doesn't hold a reference back to the SocketSet it's in to report it's dirty and adding it would require Weak<RefCell<>> or something like this. I have some ideas I've sketched above but I don't like any of them.
Right, so far it looks like #[cfg(any(feature = "std", feature = "collections"))]
#[derive(Debug)]
struct LookupTables {
raw: BTreeMap<(RawPayloadType, IpProtocol), BTreeSet<usize>>,
tcp: BTreeMap<u16, BTreeSet<usize>>,
udp: BTreeMap<u16, BTreeSet<usize>>,
dirty: Vec<usize>,
}
#[cfg(not(any(feature = "std", feature = "collections")))]
#[derive(Debug)]
struct LookupTables {}
#[derive(Debug)]
pub struct Set<'a, 'b: 'a, 'c: 'a + 'b> {
sockets: ManagedSlice<'a, Option<Item<'b, 'c>>>,
lookup_tables: LookupTables,
} |
OK, this looks more or less fine to me. A few suggestions:
Also, I encourage you to get early feedback by updating the PR in small pieces. |
I don't like the idea of using
This means we would have to do two searches, first with IpAddress set to the src_ip from the packet to search for an address-bound socket and then, if it fails (and it will more often than not), with IpAddress::Undefined to search for a not bound one. I'm not against it tho, it may be better from the API pov, since we would return an Option and not an iterator. Also, I think I'll make two PRs, since ingress and egress dispatching are mostly independent. |
Nope! We can use BTreeMap::range to perform just one search. |
Agreed. On further thinking |
I don't get it. Suppose you have three sockets, *:123, 192.168.1.1:123 and 192.168.1.3:123. They are stored in the BTreeMap in this order. How would you construct a range to find the right socket for each address in 192.168.1.1..192.168.1.4? |
Why do you need this? |
So here's my current attempt: master...batonius:packet_dispatch . I've spent some time on these lifetimes :) What I'm thinking of doing next is
I'm not sure I like |
You mean split |
Yes. Basically, we now need to parse l4 headers in I'm not sure about the accept/reject part tho, since without std/collections we're still gonna iterate over all the sockets so we can't just assume the packet is for the socket in these new methods. We can have separate versions for std/no_std, but I'm not sure if it's worth it since the accept/reject check is just a couple of u32/u16 comparisons. |
I'm not happy about this design. How about we factor out the IP handling machinery from EthernetInterface and then keep all of our L4 handling code there? |
I'm all for it, I've stated I propose I rename that I called |
Let's start with this. It's long overdue, will have a lot of benefits, and necessary for this anyway. |
Here's my attempt at refactoring
I haven't factored it out into a separate module as I was going to because these functions either have no state of their own or rely on If you're ok with it, I'll make a PR so I can move on with my packet_dispatch branch. |
Looks all good! I have a few nits but no real issues. |
In fact I went ahead and merged it. |
Ack on putting it into SocketSet. |
Moving forward, do you think you can cover EthernetInterface with tests? A basic coverage of every |
Sure, I'll look into it. |
Something I just remembered that might be very relevant to your work is that having several open sockets in LISTEN state with the same local endpoint is perfectly legal. That's how listen backlog is implemented (by a layer on top of smoltcp). |
Right, I somehow missed that point completely, it's not enough to dispatch tcp packets based on the dst endpoint, a server can have several clients connected to it, we need to consider src endpoint as well, and we don't know it until we established a connection. This means we need another layer of dispatching and a way for a socket to report the fact it has established a connection to a remote endpoint. |
Now I think if it, it should be easy enough to do by checking if socket's |
Yeah that works. |
I've updated master...batonius:packet_dispatch . I'm not sure about the API yet, but my current plan is to add a smart pointer-like type that would hold mutable references both to I'm also thinking of overriding |
Hmm, I'm not sure if I like this idea very much, we already have drop magic in Device and that's pretty bad already. But I can give it a look. |
I've got it working: master...batonius:packet_dispatch . I'm not sure about names, and I think there are some public API ready to be cut out, there are no tests, the formatting is wrong, but overall this is the design I propose we adopt. Examples appear to be working, but I can't test them in no_std mode. Smart-pointer approach works fine, I've even managed to implement it without RefCell, API changes are minimal, no_std mode is supported, adding egress dispatching should be easy. |
I've cleaned up the code, and I'm more or less happy with it. If you're ok with the approach I've taken, I'll create a PR so you can review the code and request changes. |
Added egress packet dispatching, the only thing I'm not sure about is the best way to determine if a TcpSocket is dirty. Right now I consider every TCP socket not in the CLOSED or LISTEN states as dirty, but limit the number of iterations over the dirty sockets list so we don't get an infinite loop. Overwise it appears to be working. |
Fixed Travis builds. It looks like the |
I'm in the middle of a fairly major refactoring of the TCP socket dispatch (it shouldn't impact your work much if at all), so I'll do a review once I finish that. |
Good. I'm gonna look into these tests for EthernetInterface, it's not as straightforward as I thought though. |
Any progress on reviewing this? I keep rebasing the branch just in case. |
Still working on that refactoring. |
I'm done with the refactoring, I think you will find that the new code is significantly better. |
Nice. Could you have a brief look at my branch to see if you're ok with the overall design before I start rebasing it? |
Your egress packet handling will have to be redone. One thing I'm working on right now is having So what should happen instead is every socket tells this interval for itself (it's either zero or infinity for UDP and raw sockets, and the retransmit timer derived vlaues for TCP sockets), and you sould use some sort of data structure to sort them and select the minimal one, maybe a BTreeMap also. Also, I would really prefer it if you broke up the branch into many small PRs. For example, you can start with the expandable ring buffer (this may well come useful in the future for TCP!), that's not dependent on anything and I can just merge it right now with minor adjustments instead of keeping reviewing it in the mega-PR. |
OK, I've very much warmed up to your smart pointer idea after thinking more about it. Essentially, existing interface (
After that I have some thoughts on what to do next, but am too tired to describe them in full. |
I've merged #38. Looking forward, can you start by adding the "smart pointers", but without any ingress/egress handling code? That seems like a good additive path forward. |
There is unfortunately a serious problem here. Namely, |
I couldn't wait for the RFC, so I rolled out my own. /// A handle specified for TCP
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Default, Hash)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
pub struct TcpHandle(IpEndpoint, IpEndpoint);
impl TcpHandle {
pub fn new(mut local: IpEndpoint, remote: IpEndpoint) -> Self {
local.addr = Address::Unspecified;
Self(local, remote)
}
pub fn local(self) -> IpEndpoint {
self.0
}
pub fn remote(self) -> IpEndpoint {
self.1
}
}
/// An extensible set of sockets.
///
/// The lifetime `'a` is used when storing a `Socket<'a>`.
#[derive(Debug)]
pub struct Set<'a> {
sockets: ManagedSlice<'a, Option<Item<'a>>>,
#[cfg(feature = "socket-tcp")]
tcp_sockets: HashMap<TcpHandle, TcpSocket<'a>>,
} |
@JakkuSakura do you mind sharing your entire version? I may want to do the same thing, even generalize it as a less complex solution than the one offered here |
@tomDev5 Hi, it's been many years, and my previous code included private IP. If you don't mind a paid consultation, I can offer further help |
Problem
Currently, sloltcp uses iteration to dispatch ingress packets to sockets and to poll sockets for egress packets. While acceptable on the platforms without
libstd
and memory allocation, this approach is very inefficient on systems where growable containers are available.Discussion
std
feature.Proposal
Socket::proccess
method.SocketSet
by (conditionally) adding threestd::HashMap
's to it, for raw, tcp and udp sockets, and aVec
to keep dirty sockets' handlers.std::HashMap<(RawSocketType, IpProtocol), Vec<Handle>>
, hash maps for udp and tcp should have a type ofstd::HashMap<u16, Vec<Handle>>
.Vec
is required to support the possibility of several sockets listening to the same port/protocol on different addresses.SocketSet::add
to use socket'sendpoint
andip_protocol
methods to construct an index to insert socket's handler into the corresponding lookup table.SocketSet::remove
to remove socket from a lookup table based on socket'sendpoint
.Set::IterMut
to support construction from a mutable reference toManagedSlice
and an iterator ofHandles
, producing an iterator of Socket.SocketSet::iter_mut
withiter_raw(&mut self, ip_repr: &IpRepr) -> IterMut
anditer_l4(&mut self, ip_repr: &IpRepr) -> IterMut
, which should look up sockets based on ip_repr and return an iterator over the sockets found. With thestd
feature disabled, the functions should return iterators over the entireSocketSet
.iter_dirty
toSocketSet
to produce an iterator of Socket by drainingdirty_sockets
vector, or an iterator over the entireSocketSet
ifstd
is disabled.mark_drity
toSocketSet
to mark a socket as dirty.iter_mut
inEthernetInterface::poll
withiter_raw
anditer_l4
and inEthernetInterface::emit
withiter_dirty
.Questions
SocketSet::get_mut
mark each requested socket as dirty (which kinda defeats the purpose), by having a specialized methodSocketSet::get_mut_for_write
to do so, or by devising some smart handler type which would mark a socket as dirty onsend
.Set::Item
is relevant here.EthernetInterface::poll
should be refactored into several methods to increase readability.The text was updated successfully, but these errors were encountered: