Automatic GSO deactivation no longer works

[stormshield-fabs]

Context

#2565 changed GRO and GSO probing to use the transport socket instead of creating throwaways.

It introduced a logic error with regards to the automatic GSO deactivation, preventing quinn from working at all with network drivers that do not support segmentation offloading when this feature is enabled in the TransportConfig.

Previously, GSO was probed on the throwaway socket using set_socket_option, but the option was not activated on the transport socket. quinn instead relies on the possibility to set UDP_SEGMENT in the cmsg given to sendmsg (this is documented in man udp).

When quinn detects a possible GSO error, it resets max_gso_segments to 1, so that prepare_msg no longer puts UDP_SEGMENT, effectively disabling GSO for this specific batch.

Bug

With #2565, the option is set on the transport socket (if the kernel accepted it), so the special cmsg handling no longer works and quinn ends up asking for segmentation offload forever.

This can be reproduced by disabling UDP segmentation on the interface (ethtool -K virbr0 tx-udp-segmentation off) and running quinn/perf over that interface: the client is stuck and keeps trying to send the initial frame.

Solutions

I'm not sure which direction should be preferred for a fix. I think we could:

1. call set_socket_option to explicitly disable GSO when needed (but then we don't need the special cmsg handling)
2. call set_socket_option to return disable GSO after probing
3. revert to using a throwaway socket for GSO (I don't believe GRO is impacted)

[djc]

cc @mxinden @thomaseizinger

[Ralith]

Ahah, that was why we did a separate socket. We don't actually want to set the socket option at all. I guess we need to revert the GSO part of that change. GRO side should still be fine though.

[thomaseizinger]

Interesting! I had no idea but thank you for discovering this. I can prepare a patch earliest on Sunday to fix this unless somebody beats me to it!

[thomaseizinger]

1. call set_socket_option to explicitly disable GSO when needed (but then we don't need the special cmsg handling)

I am leaning towards this. Seems simpler to me. Also, we were previously binding a socket to the unspecified address to detect it but require the user to pass in a specific socket. So testing things just on that socket seems more correct to me.

[Ralith]

We need the cmsg anyway to set the segment size we're actually sending.

[thomaseizinger]

We need the cmsg anyway to set the segment size we're actually sending.

Should we even try to detect GSO during init then? What I still don't quite understand is: If enabling GSO works on the initial socket, how can it break later?

The EINVAL handling is also kind of flawed. I've seen that disable GSO on a socket that normally can handle it but for other reasons temporarily returns EINVAL. So if we can somehow remove that, I'd welcome that.

The problem this creates is that even though sending still works without GSO, performance gets crippled which creates other problems. I'd rather we fail entirely so that self-healing checks like repeated timeouts of keep-alives can trigger a reset / recreation of the socket.

[Ralith]

If enabling GSO works on the initial socket, how can it break later?

GSO support is dependent on the NIC driver. The driver that handles any given packet depends on how it's routed, which is difficult to predict in advance and can change at any time, but re-creating the socket won't help.

[thomaseizinger]

and can change at any time

Right, we experienced that as well. Do you have any experience as to what a good way / condition of re-enabling GSO is? It doesn't seem right to me that we disable it on EINVAL and then never try to re-enable it again and also provide no way of doing so externally (other than recreating the socket state).

[Ralith]

Maybe it would make the most sense to track this state at path granularity somehow? In an ideal world we'd also somehow listen for routing table changes and reset state, but in practice that should be rare enough that the effort isn't justified.

[flub]

Maybe it would make the most sense to track this state at path granularity somehow? In an ideal world we'd also somehow listen for routing table changes and reset state, but in practice that should be rare enough that the effort isn't justified.

In our experience routing table changes happen a lot these days. Phones and laptops constantly move between networks.

[thomaseizinger]

In our experience routing table changes happen a lot these days. Phones and laptops constantly move between networks.

I don't see that in the scope of quinn-udp though. The way we handle this in Firezone is that we recreate our sockets when we detect network roaming. For macOS, this is necessary to listen on the new interfaces, even if you previously bound your socket to 0.0.0.0.