-
Notifications
You must be signed in to change notification settings - Fork 18
mptcp: add splice test #162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: mptcp-net-next
Are you sure you want to change the base?
Conversation
This patch adds splice test for MPTCP. Signed-off-by: Geliang Tang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very good idea to add a packetdrill test for that. Here are a few suggestions to make the test more robust and covering more code.
+0 listen(3, 1) = 0 | ||
|
||
// Connection should get accepted | ||
+0 < addr[caddr0] > addr[saddr0] S 0:0(0) win 65535 <mss 1460, sackOK, TS val 4074410674 ecr 0, nop, wscale 8, mpcapable v1 flags[flag_h] nokey> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you only have one subflow for the whole MPTCP connection, no need to specify addr[caddr0] > addr[saddr0]
|
||
+0 pipe([5, 6]) = 0 | ||
|
||
+0 < P. 1:101(100) ack 1 win 257 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here, you are injecting a "plain TCP" (without MPTCP options) packet. Is it on purpose?
+0.1 < . 1:1(0) ack 1 win 256 <nop, nop, TS val 4074410674 ecr 4074410674, mpcapable v1 flags[flag_h] key[ckey=2, skey]> | ||
+0 accept(3, ..., ...) = 4 | ||
|
||
+0 pipe([5, 6]) = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a comment explaining why it is needed please?
+0 pipe([5, 6]) = 0 | ||
|
||
+0 < P. 1:101(100) ack 1 win 257 | ||
+0 splice(4, NULL, 6, NULL, 99, 0) = 99 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a comment explaining what you are trying to do here and below.
|
||
+0 < P. 1:101(100) ack 1 win 257 | ||
+0 splice(4, NULL, 6, NULL, 99, 0) = 99 | ||
+0 splice(4, NULL, 6, NULL, 1, 0) = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here I see that you "move data" from FD 4 (MPTCP connection) to 6. But you never use 5. Maybe you should at least read from it to make sure 100B have been written?
Or, maybe better, can you "move data" from 5 to 4, and check that packets are generated on the wire as expected? (+0 > . 1:100(100) ack 101 <(...)>
)
|
||
+0 pipe([5, 6]) = 0 | ||
|
||
+0 < P. 1:101(100) ack 1 win 257 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after this, you should check the stack sends an ACK back with MPTCP options as expected.
+0 listen(3, 1) = 0 | ||
|
||
// Connection should get accepted | ||
+0 < addr[caddr0] > addr[saddr0] S 0:0(0) win 65535 <mss 1460, sackOK, TS val 4074410674 ecr 0, nop, wscale 8, mpcapable v1 flags[flag_h] nokey> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: the TCP timestamps might not be needed, it might be easier not to deal with them: simply drop them, align with nop
→ a bit similar to what was done in recent new .pkt
file, e.g. gtests/net/mptcp/mp_join/mp_join_server_dss_drop_after_mpj.pkt
This patch adds splice test for MPTCP.