Conversation
modified: homa_hijack.h modified: homa_impl.h modified: homa_outgoing.c modified: homa_plumbing.c modified: homa_qdisc.c modified: homa_wire.h modified: util/cp_node.cc modified: util/homa_test.cc modified: util/server.cc
There was a problem hiding this comment.
Some overall comments:
-
The UDP hijacking triggers don't seem safe to me. In order to determine whether an incoming UDP packet actually belongs to Homa,
homa_skb_udp_hijackedis testing bits that fall in the data portion of the packet. This means that a real UDP packet could contain data that causes Homa to steal the packet incorrectly. -
The UDP header fields (length and checksum) conflict with TCP header fields (and Homa's offset in data packets), so I think you would need to define a different Homa packet format for UDP hijacking; I'm a bit nervous that this will create a lot of additional complexity.
-
How likely do you think it is that UDP hijacking will be needed for Mastercard's Homa deployment? My understanding from previous experiments was that Homa packets can successfully be transmitted across Mastercard's datacenter; have I misremembered this? And, if native Homa packets can be used, is there any advantage to UDP hijacking?
-
The cp_node support for UDP is problematic because UDP doesn't provide reliable delivery. Won't a single lost packet cause tests to hang? One possibility would be to implement timeout and retry for UDP, but overall this feels like a big slug of additional code and I'm not sure that it adds enough value to justify the support burden it will create. People naturally want to compare Homa to TCP, so it makes sense to have TCP support in cp_node, but will there be similar interest for UDP (no-one has asked me for Homa-UDP comparisons yet)? If you don't have a significant purpose in mind for the code, I'm inclined to leave it out. The code in homa_test.cc and server.cc is smaller, and I can imagine it might work well enough for some simple performance tests, so I don't object to that.
-
All code in the top-level Homa directory needs to have unit tests. Can you add new tests for the code you have added? They are in the
testsubdirectory. I just created a README.md file for that directory, but I'm not currently in a position where I can push it; I have cut and pasted the contents below. Let me know if you have questions or run into problems.
Contents of test/README.md:
This directory contains unit tests for the Linux kernel implementation
of Homa.
-
To run the tests, invoke "make test" in this directory. This will
compile a binary namedunitand run it. You can rununitunder
gdbfor debugging. -
The tests have a structure that is isomorphic to the code (this makes it
easier to see that coverage is complete and determine whether new tests
need to be added when making code changes):- There is one test file for each
.cfile in the parent directory;
for a code filehoma_foo.c, the test file will beunit_homa_foo.c. - Within the test file, there is a group of tests for each function
in the code file, in the same order as the functions in the code
file. - The tests for a given function should validate the features of the code
in that function (e.g. conditionals, loops, etc.). Each test should
focus on a single feature or a small number of related features.
The order of tests for a function should match the order of the
features within the code. - If a header file contains nontrivial inline functions, unit tests
for it should appear at the end of the test file used for the
corresponding.cfile.
- There is one test file for each
-
The name for a test in the
TEST_Fline should have the form
func__feature, where func is the name of the function and
feature describes the thing being tested (multiple words separated
by underscores). -
It's not unusual to start the tests for a given function with a
basic sanity check with name func__basics. This may exercise many
features of the function; the remaining tests only need to validate
things not checked by the__basictest. -
In addition to the unit tests, this file contains several additional
files that provide support for unit tests:mock.c: stubs out numerous Linux kernel functions and features so
that tests can be run outside the kernel. This file also contains a
few helper functions such asmock_skb_alloc: check for function
names that start withmock_.unit.c: C functions that help in writing tests, such as
unit_client_rpcandunit_server_rpcfor creating RPC structures
to use in tests.ccutils.c: functions written in C++, but with C interfaces, to help in
writing tests. For example, this includes functions to manage hash tables
using C++std::unordered_mapobjects and functions to manage a test
log using C++ strings.
*kselftest_harness.h: this defines the testing infrastructure; it is
a modified version of the kernel file
tools/testing/selftests/kselftest_harness.h.
| * homa_hijack.h for an overview of TCP hijacking. | ||
| /* This file implements TCP and UDP hijacking for Homa. See comments at the | ||
| * top of homa_hijack.h for an overview of TCP hijacking. UDP hijacking works | ||
| * similarly but uses UDP as the IP protocol, which avoids issues with |
There was a problem hiding this comment.
Instead of adding this sentence here, rework the comment at the beginning of homa_hijack.h so that it describes both TCP and UDP hijacking in a coordinated way.
| * @skb: The newly arrived packet. | ||
| */ | ||
| struct sk_buff *homa_udp_hijack_gro_receive(struct list_head *held_list, | ||
| struct sk_buff *skb) |
There was a problem hiding this comment.
Linux style guidelines require the 's' in 'struct' to align under the 's' in 'struct' of the preceding line.
| transport_len = skb->len - skb_transport_offset(skb); | ||
|
|
||
| /* Set UDP length at bytes 4-5 (overlaps high 16 bits of sequence). */ | ||
| *((__be16 *)((u8 *)h + 4)) = htons(transport_len); |
There was a problem hiding this comment.
This is too cryptic; please use C structure definitions to manage the UDP header fields (e.g. all the info needed for UDP hijacking should be clearly visible, symbolically, in homa_wire.h).
| struct sk_buff * | ||
| homa_udp_hijack_gro_receive(struct list_head *held_list, | ||
| struct sk_buff *skb); | ||
| void homa_udp_hijack_init(void); |
There was a problem hiding this comment.
Functions should be listed in alphabetical order.
| if (homa_sock_udp_hijacked(rpc->hsk)) | ||
| homa_udp_hijack_set_hdr(skb, rpc->peer, false); | ||
| else | ||
| homa_hijack_set_hdr(skb, rpc->peer, false); |
There was a problem hiding this comment.
There are a lot of places where code now has to do separate checks for TCP and UDP hijacking. Is there a way to reorganize things so that only a single check is needed?
| .maxlen = sizeof(int), | ||
| .mode = 0644, | ||
| .proc_handler = homa_dointvec | ||
| }, |
There was a problem hiding this comment.
Having a separate parameter 'hijack_udp' feels awkward to me: what happens if someone sets both 'hijack_tcp' and 'hijack_udp'? I think it would be better to change things to have a single parameter 'hijack' with three values: none, TCP, or UDP.
|
BTW, I just got access to an AWS machine running RHEL 8.9, and I see that the unit tests are pretty badly broken on that version. I'm in the process of fixing them, so I would suggest that you don't worry about unit tests until I have a change to get them working properly on the rhel8 branch. |
|
The unit tests now run for me in the rhel8 branch; I just pushed the changes to GitHub. |
2 participants
Hello John,
The code change is for supporting hijack UDP. UDP will be slower than TCP, due to absence of TSO.
Where UDP can be used:
1. When network blocks unknown protocol
2. When network blocks TCP (due to tcp state machine or firewall preventing TCP flag "SR" to pass through).
Thanks