Skip to content

Fix a bug in _networkinterface_pop_next_packet_out#3240

Merged
stevenengler merged 1 commit intoshadow:mainfrom
stevenengler:netiface-inet-bug
Nov 28, 2023
Merged

Fix a bug in _networkinterface_pop_next_packet_out#3240
stevenengler merged 1 commit intoshadow:mainfrom
stevenengler:netiface-inet-bug

Conversation

@stevenengler
Copy link
Copy Markdown
Contributor

@stevenengler stevenengler commented Nov 27, 2023

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.

run_time
transfer_time_5242880 exit

@stevenengler stevenengler self-assigned this Nov 27, 2023
@github-actions github-actions bot added the Component: Main Composing the core Shadow executable label Nov 27, 2023
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.
@stevenengler stevenengler merged commit 3ef1bb3 into shadow:main Nov 28, 2023
@stevenengler stevenengler deleted the netiface-inet-bug branch November 28, 2023 00:28
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.


![run_time](https://github.com/shadow/shadow/assets/3708797/a65e571c-e0fe-42fa-997e-3279b480e52e)
![transfer_time_5242880
exit](https://github.com/shadow/shadow/assets/3708797/11f8661c-7750-4b63-8cdf-6db718b3eb2d)

![ram_simtime](https://github.com/shadow/shadow/assets/3708797/8e605dda-6a9b-4738-a5a4-19774f056af6)

**Edit 2:** One commit was split off into #3240.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Main Composing the core Shadow executable

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants