Page MenuHomeFreeBSD

Add simple tests for SO_REUSEPORT_LB sockets.
ClosedPublic

Authored by markj on Sep 10 2018, 6:19 PM.

Details

Summary

These tests create a LB group on the loopback address, connect to the
group address a number of times, and verify that the connections were
roughly balanced among the listening sockets in the group.

This provides some basic regression testing and makes it marginally
easier to add additional SO_REUSEPORT_LB tests in the future.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

tests/sys/netinet/reuseport_lb.c
107 ↗(On Diff #47886)

What's the meaning of 8?

109 ↗(On Diff #47886)

waitpid doesn't need to be in the loop.

123 ↗(On Diff #47886)

Are the write and read really necessary?

175 ↗(On Diff #47886)

Does the use of port 0 force the test to run as root?

184 ↗(On Diff #47886)

Any reason not to use ATF_REQUIRE_EQ_MSG?

262 ↗(On Diff #47886)

magic numbers should be #defined.

markj added inline comments.
tests/sys/netinet/reuseport_lb.c
107 ↗(On Diff #47886)

Because connections are balanced among sockets in the group, we expect each listening socket to accept nconns / sdcnt connections. 8 is just an arbitrary factor to allow for some error. I will add a comment.

109 ↗(On Diff #47886)

What if the child exits prematurely after creating a few connections?

123 ↗(On Diff #47886)

I guess not. The child is using a blocking socket, so connect() will block until the parent accept()s. Will remove.

175 ↗(On Diff #47886)

No, it's the wildcard port. (Is there a constant for this? I can't find one.)

184 ↗(On Diff #47886)

No, but I find it less clear.

markj marked 2 inline comments as done.
  • Address some review feedback.

In fact, you don't even need the second process for this test. It's possible to do this entirely with one single-threaded process. See the "accept_success" test in tests/sys/audit/network.c for an example. Basically it goes like this:

for (i=0; i<sdcnt; i++)
    listen(server_fd[i]);
while (true) {
    client_fd = socket();
    client_fd.connect(server_addr);  //blocking, but returns immediately
    for ((i=0; i<sdcnt; i++) {
        if (accept(server_fd[i]) == 0) {    // non-blocking
            acceptcnt[i]++;
            break;
        }
    }
}

What do you think of the single-threaded approach?

What do you think of the single-threaded approach?

It's much better. :) I'll give it a try.

  • Simplify according to asomers' suggestion.
This revision is now accepted and ready to land.Sep 11 2018, 3:07 AM
  • Simplify further: we don't need non-blocking connect().
  • Fix comment, fix loop which verifies that acceptcnt is balanced.
This revision now requires review to proceed.Sep 11 2018, 3:43 AM
tests/sys/netinet/reuseport_lb.c
92 ↗(On Diff #47903)

I think you should assert here that i < nsds. Otherwise, it's possible that some connections don't get accepted on any sockets and you wouldn't notice.

markj marked an inline comment as done.
  • Assert i < nsds after the accept() loop.

Do the tests still pass for you? After your latest change, I get the "connection wasn't accepted" error every time.

Do the tests still pass for you? After your latest change, I get the "connection wasn't accepted" error every time.

Yes, I just noticed that. (I rebuilt and ran the tests on two different machines by accident.) It seems like the polling loop is needed after all, which is somewhat surprising to me given that we're using a blocking connect().

Do the tests still pass for you? After your latest change, I get the "connection wasn't accepted" error every time.

Yes, I just noticed that. (I rebuilt and ran the tests on two different machines by accident.) It seems like the polling loop is needed after all, which is somewhat surprising to me given that we're using a blocking connect().

I guess this is expected behaviour for TCP. connect(2) returns once the SYN|ACK reply is received, but the child socket isn't complete until the final ACK is received, so there's a window that we have to account for.

  • Re-add the polling loop.

Do the tests still pass for you? After your latest change, I get the "connection wasn't accepted" error every time.

Yes, I just noticed that. (I rebuilt and ran the tests on two different machines by accident.) It seems like the polling loop is needed after all, which is somewhat surprising to me given that we're using a blocking connect().

I guess this is expected behaviour for TCP. connect(2) returns once the SYN|ACK reply is received, but the child socket isn't complete until the final ACK is received, so there's a window that we have to account for.

That makes sense. The listening sockets need to be non-blocking because you don't know which one will receive the next connection. But by making them nonblocking you're also changing the behavior of the connecting sockets. You could probably eliminate the polling loop by making the listening sockets blocking and using select(2) to identify which one got the connection. But I think that would be more trouble than it's worth.

This revision is now accepted and ready to land.Sep 11 2018, 4:16 PM

You could probably eliminate the polling loop by making the listening sockets blocking and using select(2) to identify which one got the connection. But I think that would be more trouble than it's worth.

Right, you can select() for new connections by adding the listening sockets to the read set. But as this is all over the loopback, I didn't bother with that.

Thanks for the review.

This revision was automatically updated to reflect the committed changes.