Fix a bug in _networkinterface_pop_next_packet_out#3240
Merged
stevenengler merged 1 commit intoshadow:mainfrom Nov 28, 2023
Merged
Fix a bug in _networkinterface_pop_next_packet_out#3240stevenengler merged 1 commit intoshadow:mainfrom
_networkinterface_pop_next_packet_out#3240stevenengler merged 1 commit intoshadow:mainfrom
Conversation
The queue may change in between when we pop the socket and when we re-add the socket. This can lead to duplicate entries in the queue if we don't check before re-adding.
559cc68 to
f03c113
Compare
robgjansen
approved these changes
Nov 27, 2023
stevenengler
added a commit
that referenced
this pull request
Nov 28, 2023
This cleans up the network interface and removes a linear hashmap search in the network interface's priority queue. The network interface used to need to deal with both rust and C sockets, so it stored tagged pointers for `CompatSocket` objects. Now that the TCP socket is the only remaining C socket, and it has a rust wrapper `LegacyTcpSocket`, C sockets aren't used in the network interface anymore. The network interface now only stores boxed `InetSocket` objects. This also allows us to remove tagged pointers from our C code. The hashmap in the priority queue used by the fifo qdisc now uses custom hash and eq implementations, which allows us to avoid a linear search through the hashmap which used the canonical handles to identify the same socket. The new hash function is more expensive since it uses a real hash function ([`DefaultHasher`](https://doc.rust-lang.org/std/collections/hash_map/struct.DefaultHasher.html) as used by the stdlib `HashMap`), whereas the original glib code doesn't seem to actually use a hash function: https://docs.gtk.org/glib/type_func.HashTable.new.html > If `hash_func` is NULL, `g_direct_hash()` is used. https://github.com/GNOME/glib/blob/40081f9b627904c2506b6d3837334e4ce05052da/glib/ghash.c#L2483-L2487 ```c guint g_direct_hash (gconstpointer v) { return GPOINTER_TO_UINT (v); } ``` This PR contains a lot of changes along the rust/C boundary, but everything seems to still work so hopefully it's correct. This may fix #3238. **Edit:** There doesn't seem to be much (if any) of a performance regression. The simulation results have remained unchanged. The [following](https://github.com/shadow/benchmark-results/blob/master/tor/2023-11-27-T03-09-35/README.md) are from a slightly older version of this PR, but I don't expect it would make any difference.    **Edit 2:** One commit was split off into #3240.
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
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.
The queue may change in between when we pop the socket and when we re-add the socket. This can lead to duplicate entries in the queue if we don't check before re-adding. I'm not exactly sure what the implication of this is. I think it can lead to some sockets being prioritized over others.
This was split off from #3239.
Edit: No change in performance or simulation behaviour in a 10% network benchmark.