Skip to content

Conversation

@camshaft
Copy link
Contributor

@camshaft camshaft commented Jun 23, 2023

Description of changes:

Similar to #1765, this change adds a client.

The interesting thing about the client is, since it's the one to initiate the connection, it doesn't know any of the ethernet addresses, or even its own source IP in some cases. So what we do is open a UDP socket to transmit initial packets on. This allows us to offload address resolution onto the OS and not have to re-implement ARP. Once we hear back from the server, we know what all of the address fields are since the server responded with a completely filled-in ethernet/ip/udp packet. We then store the field on the path handle and send all future traffic over the AF_XDP socket instead.

Call-outs:

I've refactored the IO setup a bit to be in a central module, rather than copy/pasted in each qns command.

Testing:

The same testing applies as was done in #1765.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@camshaft camshaft force-pushed the camshaft/xdp-qns-client branch 2 times, most recently from 6d40fa5 to 6ec13ed Compare June 23, 2023 21:57
@camshaft camshaft marked this pull request as ready for review June 23, 2023 23:16
@camshaft camshaft force-pushed the camshaft/xdp-qns-client branch 2 times, most recently from d693f10 to 56322f9 Compare June 26, 2023 18:22
@camshaft camshaft requested a review from WesleyRosenblum June 27, 2023 14:58
// Initial packets don't need to be bigger than the minimum
let max_mtu = s2n_quic_core::path::MaxMtu::MIN;

// It's unlikely the initial packets will utilize, so just disable it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// It's unlikely the initial packets will utilize, so just disable it
// It's unlikely the initial packets will utilize GSO, so just disable it

Comment on lines 57 to 58
// compute the payload size for each message from the number of GSO segments we can
// fill
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment is a little confusing after the previous one about not utilizing gso

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that was copy/pasted from the tokio code 😄


if let Ok(udp_socket) = recv_udp_socket {
let udp_socket = tokio::net::UdpSocket::from_std(udp_socket)?;
// Set up a task to read from the bound UDP socket
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this exists just to verify everything is working properly? Would it make sense to do something more severe than logging if a packet is received?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's correct. I can shut down the process with an error message if you think that's better.

Another option is to merge the queues and still process them in the endpoint. But the problem with that is it'll be using normal UDP sockets and you won't really have any insight that that's happening...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have a sense of why this would happen or how likely it is. If you think it would only happen as soon as the process starts up then maybe its better to shut down with an error, otherwise I could see this spamming logs pretty heavily.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah if this starts logging the cause would likely be that the XDP program was unloaded, which we wouldn't recover from in the current code since we'd have to re-register all of the map values. So in this example code I think it's best to shut it down.

@camshaft camshaft force-pushed the camshaft/xdp-qns-client branch from 56322f9 to 33ecdee Compare June 27, 2023 19:39
@camshaft camshaft requested a review from WesleyRosenblum June 27, 2023 21:26
@camshaft camshaft merged commit e71e6e7 into main Jun 27, 2023
@camshaft camshaft deleted the camshaft/xdp-qns-client branch June 27, 2023 21:30
kagarmoe pushed a commit that referenced this pull request Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants