mptcp: sockopt: add coverage for TCP_CORK and TCP_NODELAY #75
Conversation
darkwrat
force-pushed
the
mptcp-nagle
branch
2 times, most recently
from
dc5a1c5 to
596a544
Compare
|
Sorry for the force-push, fixed wrong email in signoff and commit author. |
| +0 bind(3, ..., ...) = 0 | ||
| +0 listen(3, 1) = 0 | ||
|
|
||
| +0.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.
if you don't need to use multiple subflows, you can remove addr[caddr0] > addr[saddr0] (and spaces below), that will be easier to read :)
| +0.0 > . 1:1(0) ack 3 <nop, nop, TS val 4074418293 ecr 4074418292, dss dack8=3 dll=0 nocs> | ||
| +0.0 read(4, ..., 2) = 2 | ||
|
|
||
| // more inbound data and MP_PRIO to flip active -> backup |
There was a problem hiding this comment.
Will be removed, part of C&P from mptcp/mp_prio/mp_prio_server.pkt after I tried flipping active->backup myself and failed. :) Wanted to see setsockopt affecting multiple subflows.
| +0.0 read(4, ..., 10) = 10 | ||
|
|
||
| // check that TCP_CORK was cloned from listener | ||
| +0.0 getsockopt(4, SOL_TCP, TCP_CORK, [1], [4]) = 0 |
There was a problem hiding this comment.
if you move this getsockopt() call to the test with a single flow, do you think we still need this "multi" test?
I mean: that's good to check with multiple subflows but I'm not sure we are covering more code with it. Or do we?
There was a problem hiding this comment.
I really wanted for the subflow loop in setsockopt to spin a couple of times, just to double-check that I have correct locking and change the right sockets. I also was toying a bit with packetdrill to gain some understanding. Now I agree that this doesn't add any meaningful coverage, thank you for pointing this out. WIll remove the multi test.
There was a problem hiding this comment.
👍 Good idea to play a bit with Packetdrill. This is a nice tool but not always easy to use :)
| +0.05 write(4, ..., 40) = 40 | ||
|
|
||
| // clearing TCP_CORK should push pending bytes out | ||
| +0.01 setsockopt(4, SOL_TCP, TCP_CORK, [0], 4) = 0 |
There was a problem hiding this comment.
detail about the call side: may you add one more space after each time to have at least two spaces between the time and the next column?
While at it, the two <nop, below are not aligned with all the others :)
There was a problem hiding this comment.
Thank you for the review!
Could you tell me if the commit adressing all the changes should be force-pushed, or just it will be squashed on merge?
There was a problem hiding this comment.
Please force-push your new version to keep only "good commits". Even if it is not as good as with Gerrit, we can easily see the differences between two force-pushed versions on Github so that side is not an issue :)
Remove mptcp/sockopts/sockopt_cork_multi.pkt Move getsockopt check to mptcp/sockopts/sockopt_cork_nodelay.pkt Make whitespace more consistent Add comment about the original tcp script Link: https://lore.kernel.org/mptcp/20211123070708.2897469-1-max@internet.ru/T/ Link: multipath-tcp#75 Signed-off-by: Maxim Galaganov <max@internet.ru>
Add coverage for new socket options that allow changing behavior of Nagle's algorithm on MPTCP sockets. Link: https://lore.kernel.org/mptcp/20211123070708.2897469-1-max@internet.ru/T/ Link: multipath-tcp#75 Signed-off-by: Maxim Galaganov <max@internet.ru>
darkwrat
force-pushed
the
mptcp-nagle
branch
from
429cb79 to
942f5c8
Compare
Add coverage for new socket options that allow changing behavior of Nagle's algorithm on MPTCP sockets. Link: https://lore.kernel.org/mptcp/20211123070708.2897469-1-max@internet.ru/T/ Link: multipath-tcp#75 Signed-off-by: Maxim Galaganov <max@internet.ru>
Add coverage for new socket options that allow changing behavior of Nagle's algorithm on MPTCP sockets. Link: https://lore.kernel.org/mptcp/20211123070708.2897469-1-max@internet.ru/T/ Link: multipath-tcp#75 Signed-off-by: Maxim Galaganov <max@internet.ru>
darkwrat
force-pushed
the
mptcp-nagle
branch
from
5974867 to
752e066
Compare
|
I just did a very small modification related to the code style, nothing important ;-) I hope you don't mind! Everything OK on my side. We can merge this once the kernel side has been modified as well. |
Add coverage for new socket options that allow changing behavior of Nagle's algorithm on MPTCP sockets. Link: https://lore.kernel.org/mptcp/20211123070708.2897469-1-max@internet.ru/T/ Link: multipath-tcp#75 Signed-off-by: Maxim Galaganov <max@internet.ru>
|
(good catch for the setsockopt(), I missed this one, I just updated my version with your fix ;-) ) |
|
I'm applying the linked patches in mptcp_net-next repo, we can then merge this new test! Thank you for your work! |
Add coverage for new socket options that allow changing behavior of
Nagle's algorithm on MPTCP sockets.
Link: https://lore.kernel.org/mptcp/20211123070708.2897469-1-max@internet.ru/T/
Closes: multipath-tcp/mptcp_net-next#243
Signed-off-by: Maxim Galaganov max@internet.ru