Repository navigation
Homa over UDP - #113
Homa over UDP#113axkum10 wants to merge 6 commits into
Conversation
johnousterhout
left a comment
johnousterhout
left a comment
There was a problem hiding this comment.
This is an excellent first pass. Here is an initial round of comments; there are several that I have marked "Let's discuss", which means I'd like to talk about them in person (either in our Wednesday meeting or in a separate Zoom call, if need be). No need to discuss the others unless you want to.
A couple of overall comments:
-
This PR assumes that UDP support will be in addition to "native" Homa-over-UDP and TCP hijacking. I have been assuming that UDP tunneling will be the only way that Homa packets are transmitted, so we can eliminate all of the existing native and TCP hijacking code. Is there some reason why we need all three of these? Focusing entirely on UDP tunneling should make the code much simpler. I marked a few places where I think code can be removed, but there are many more that I didn't mark. Let's discuss.
-
Assuming that UDP tunneling is going to be the only mechanism, the UDP code should not be inside
#ifndef __STRIP__blocks.
| * used for the pair of kernel tunnel sockets that implement UDP hijacking | ||
| * (see homa_hijack.c). Chosen from the dynamic/private port range. | ||
| */ | ||
| #define HOMA_UDP_HIJACK_PORT 54321 |
There was a problem hiding this comment.
This declaration is part of the public protocol spec, so it should go in homa.h (probably right next to the definition of IPPROTO_HOMA.
| * Homa packets to actually be transmitted as TCP packets (and thereby | ||
| * take advantage of TSO and other features). | ||
| */ | ||
| struct homa_common_hdr { |
There was a problem hiding this comment.
My assumption has been that this structure will change pretty significantly: all of the fields that mirror TCP should be removed, leaving only those that are necessary for Homa. I wonder if it would also make sense to include the UDP header in this struct? Let's discuss.
|
|
||
| if (mtu < min_mtu) { | ||
| rcu_read_unlock(); | ||
| return -EMSGSIZE; |
There was a problem hiding this comment.
Return EHOSTUNREACH here? Let's discuss.
| rpc->msgout.max_seg_data = mtu - rpc->hsk->ip_header_length - | ||
| sizeof(struct udphdr) - | ||
| sizeof(struct homa_data_hdr); | ||
| rpc->msgout.max_gso_segs = 1; |
There was a problem hiding this comment.
This seems to indicate that there is no segmentation offload with UDP tunneling? Let's discuss.
| rcu_read_unlock(); | ||
| if (mtu < hsk->ip_header_length + sizeof(struct udphdr) + | ||
| padded_length) | ||
| return -EMSGSIZE; |
| tunnel_cfg.encap_type = 1; | ||
| tunnel_cfg.encap_rcv = homa_hijack_udp_encap_rcv; | ||
| tunnel_cfg.encap_err_lookup = homa_hijack_udp_encap_err_lookup; | ||
| tunnel_cfg.encap_err_rcv = homa_hijack_udp_encap_err_rcv; |
There was a problem hiding this comment.
Don't the gro_receive and gro_complete fields need to be set as well? Let's discuss.
| * @work: The &homa_net.udp_release_work embedded in the target | ||
| * &struct homa_net. | ||
| */ | ||
| static void homa_hijack_udp_release_work_fn(struct work_struct *work) |
There was a problem hiding this comment.
Can this mechanism go away if the UDP mechanism exists for the entire lifetime of the network namespace? Let's discuss.
| @@ -0,0 +1,66 @@ | |||
| # UDP Tunnel Integration Tests | |||
|
|
|||
| The integration scripts exercise a loaded Homa module in temporary network | |||
There was a problem hiding this comment.
I haven't examined this directory in detail, but I'm a bit nervous about it because (a) it's a big slug of code to maintain and (b) historically I have found it hard to get much value out of integration tests (hard to write, run, and maintain for the number of bugs they find).
Maybe when we talk you can demo these for me so I get a sense of how they work?
Let's discuss.
| EXPECT_EQ(0, srpc->silent_ticks); | ||
| EXPECT_STREQ("", unit_log_get()); | ||
| } | ||
| #ifndef __STRIP__ /* See strip.py */ |
There was a problem hiding this comment.
It appears to me that this test is in the wrong place in the file. Tests should be isomorphic to the code, so the order of tests for a particular function matches the order of the functionality being tested within that function. Let's discuss.
| for (i = 0; i < GRO_HASH_BUCKETS; i++) { | ||
| INIT_LIST_HEAD(&self->napi.gro.hash[i].list); | ||
| self->napi.gro.hash[i].count = 0; | ||
| INIT_LIST_HEAD(&self->napi.gro_hash[i].list); |
There was a problem hiding this comment.
It looks like maybe you are compiling against a Linux version older than 6.17.8? Let's discuss.
2 participants
Summary
Adds optional Homa-over-UDP transport for networks that do not support Homa’s native IP protocol, while preserving the application API and native transport.
Main Flow
Tests and Utilities
Review and Validation
Please focus on namespace/socket lifetime, concurrent enable/disable, checksum/offload handling, PMTU/ICMP, pacing, and native Homa regressions.
All 27 local helper tests and the full utility build passed. Earlier Linux 6.12 module builds and unit suites passed before upstream integration. Post-merge kernel unit builds are blocked locally by __cond_acquires handling with Linux 5.14 headers;
Validation Pending: