smoltcp-rs / smoltcp

a smol tcp/ip stack
BSD Zero Clause License
3.73k stars 414 forks source link

[RFC] Packet dispatch - up to date #973

Open tomDev5 opened 1 month ago

tomDev5 commented 1 month ago

This is a rewrite of the feature originally proposed here, up to date with current smoltcp, but relying on the same mechanism

Currently only TCP, UDP and Raw sockets are implemented, so the dirty_sockets list is unused. I am in favor of implementing dispatch for all sockets, but I wanted to hear general thoughts about the design and about how I should split the feature into smaller, logical PRs.

Another problem I encountered is that when I return a SocketTracker<Socket> instead of Socket, I had to specify {} around each usage (as you can see in examples/client.rs. This is quite annoying which is why I didn't yet do it for all examples (if you have a prettier solution I would love to hear it before changing all examples 🫤.

Also, the current code won't work in no_std, due to small corrections needed, which I will of course do in the PRs.

The code is currently in main..tomDev5:feature/socket-dispatch

Any notes would be welcome!

tomDev5 commented 1 month ago

What I am thinking regarding PR separation, is a PR for the boilerplate (DispatchTable, SocketTracker, TrackedSocket, return TrackedSocket from SocketSet), and then following PRs with implementations for each socket type. The dirty sockets mechanism can be added at the end. The first PR would be fairly large, but each individual PR would make sense.

tomDev5 commented 3 weeks ago

@whitequark Hi, pinging because you worked on the old issue back then, Is this a feature you would accept?

Dirbaio commented 3 weeks ago

Smoltcp is primarily designed for embedded systems, running on microcontrollers with no OS, typically around 256kB of RAM. The typical applications don't create many sockets. For example you might have 1 DHCP + 1 DNS + 1 TCP if the device connects to some server on boot, or 1 DHCP + 1 DNS + a handful of TCP if you want the device to be some server you connect to and handle some concurrent clients.

O(n) packet processing is fine when you have max 5-10 sockets. Hashing and tracking "dirty" state won't improve perf there,, it only will if you have many more sockets.

There's some disadvantages:

The latter can be mitigated by adidng more cfg's, but that in turn increases the complexity more.

tomDev5 commented 3 weeks ago

I can see your point on complexity, even though the solution that was proposed (by batonius) is fairly simple in my opinion. The original rfc had some design complexities which can be improved upon, this is an almost one-to-one implementation I made to see it works.

About the code size, I agree this feature can (and should) be guarded with a feature flag, and be simplified to be significantly smaller in terms of LOC.

I also think that dispatch would be useful to a lot of users in different projects - maybe not in the embedded devices you mentioned, but smoltcp is the best network stack available in rust.

After all, this is a standard feature in network stacks.

Anyway, I am willing to work on it if you decide in favor, let me know.

whitequark commented 3 weeks ago

@tomDev5 My point of view here is essentially the same as @Dirbaio's, and I think he explained the challenges here well.