Skip to content
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 - up to date #973

Open
tomDev5 opened this issue Aug 10, 2024 · 5 comments
Open

[RFC] Packet dispatch - up to date #973

tomDev5 opened this issue Aug 10, 2024 · 5 comments

Comments

@tomDev5
Copy link
Contributor

tomDev5 commented Aug 10, 2024

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
Copy link
Contributor Author

tomDev5 commented Aug 10, 2024

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
Copy link
Contributor Author

tomDev5 commented Aug 19, 2024

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

@Dirbaio
Copy link
Member

Dirbaio commented Aug 19, 2024

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:

  • Complexity. The amount of code your diff adds is substantial.
  • Code size. In microcontrollers memory is scarce. Adding more code makes the binary occupy more space, for little benefit because you have few sockets.

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

@tomDev5
Copy link
Contributor Author

tomDev5 commented Aug 20, 2024

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants