drivers/unix_socket: Let clients to freely connect/disconnect by llvilanova · Pull Request #613 · NetSys/bess

llvilanova · 2017-08-21T09:01:49Z

In order to allow external controllers (connected through a UNIX socket) truly
independent of bessd, client disconnection must always be detected, regardless
of whether the port is used or not. This is done by keeping a thread
around (using epoll).

Before, this driver would only reliably detect disconnects when the port is used
to receive packets. When used to send packets, the port would only detect
disconnects on an actual send, which depends on the processed packets; this led
to only some instances accepting new clients, making external program
reconnection impossible.

In order to allow external controllers (connected through a UNIX socket) truly independent of bessd, client disconnection must always be detected, regardless of whether the port is used or not. This is done by keeping a thread around (using epoll). Before, this driver would only reliably detect disconnects when the port is used to receive packets. When used to send packets, the port would only detect disconnects on an actual send, which depends on the processed packets; this led to only some instances accepting new clients, making external program reconnection impossible.

llvilanova · 2017-08-21T10:09:23Z

I'm not sure how this patch affects the failed test. Any ideas?

sangjinhan · 2017-08-21T21:09:48Z

Seems like the failure happens with 32bit build and it is reproducible. Trying to debug it...

codecov · 2017-08-22T04:40:00Z

Codecov Report

Merging #613 into master will increase coverage by 0.02%.
The diff coverage is 77.58%.

@@            Coverage Diff             @@
##           master     #613      +/-   ##
==========================================
+ Coverage   69.29%   69.32%   +0.02%     
==========================================
  Files         204      204              
  Lines       13037    13061      +24     
==========================================
+ Hits         9034     9054      +20     
- Misses       4003     4007       +4

Impacted Files	Coverage Δ
core/drivers/unix_socket.h	`100% <100%> (ø)`	⬆️
core/drivers/unix_socket.cc	`80.91% <77.19%> (-4.14%)`	⬇️
core/modules/flowgen.cc	`74.07% <0%> (+0.37%)`	⬆️
core/dpdk.cc	`82.55% <0%> (+4.65%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ee1973...8cd0de6. Read the comment docs.

llvilanova · 2017-08-22T11:01:39Z

Is there some way for me to easily test the failing experiment on that build?

Also, is it possible that DeInit is being called before Init? (I've checked this never happens when Init returns a command error) That could trigger a file descriptor close before properly setting the descriptors (they should all be zero from the constructor).

…n error

…d up

llvilanova · 2017-08-22T17:11:21Z

I just did a cleanup to ensure there's no threads or FDs left behind when a module is destroyed, but it still shows the same failure.

If you give me a command line to reproduce the failing build and specific test I'll give it a shot in the debugger.

sangjinhan · 2017-08-22T17:57:00Z

I don't know how to reliably reproduce the issue locally. What I did was using Travis with my private fork of this repo. The issue seems to happen only with 32bit builds.

What I realized is that bessctl/conf/testing/module_tests/vlan.py opens lots of (more precisely, 450) UnixSocketPort. Given that each port now opens three (not two) file descriptors, my theory is that it may exceed the file descriptor limit of 1024. For processes launched by root, maybe this limit only applies to 32bit programs. Could you update the file as below?

-        return [VLANSplit(), 1, 150, expected]
+        return [VLANSplit(), 1, 30, expected]

-    OUTPUT_TEST_INPUTS.append(output_test([1, 100, 77, -1, 149, 50, 100, -1]))
-    OUTPUT_TEST_INPUTS.append(output_test([100, 77, -1, 149, 50, 100, -1, 33, 70]))
-    OUTPUT_TEST_INPUTS.append(output_test([100, 77, -1, 149, 50, 100, -1, 33, 70], True))
+    OUTPUT_TEST_INPUTS.append(output_test([1, 17, -1, 29, 10, 13, 7]))
+    OUTPUT_TEST_INPUTS.append(output_test([1, 17, -1, 29, 10, 13, 7], True))

llvilanova · 2017-08-22T18:23:27Z

I'm on it.

It seems that 32-bit builds reach the limit of open file descriptors.

sangjinhan · 2017-08-22T20:20:20Z

Two requests regarding file descriptor usage:

Instead of using epoll, could you switch to select or poll?
Instead of using pipe, you can terminate the thread with pthread_cancel or pthread_kill
- Let's have a separate function for the thread, rather than a large lambda block.

llvilanova · 2017-08-22T22:58:04Z

Can't use poll because wakeup conditions apply to all FDs (therefore the thread will be woken up too often - i.e., every time there's data on the client socket, not only when it's closed, since we need to wait for available reads on the accept socket).

Can't use select either because socket disconnect can only be detected together with available read events (i.e., will wake up too often when the client socket is readable, not only when it's closed).

So, is it OK to keep it with epoll?

sangjinhan · 2017-08-22T23:12:31Z

I am not sure if I understood correctly. You can add client_fd_ only to exceptfds for select(), or set it only with POLLRDHUP for poll()? Just like epoll, you should be able to specify what events you want to monitor for individual file descriptors. I vaguely remember that all select/poll/epoll are implemented with the same backend code in the kernel.

Maybe I am missing something here. If you find epoll is the only way to reliably detect disconnection of client_fd_, please go ahead.

Using signals instead of pipes minimizes the number of open file descriptors (two FDs per instance).

Using ppoll() instead of epoll_pwait() saves us from using one additional file descriptor per instance, without introducing any measurable performance overhead.

llvilanova · 2017-08-23T00:09:11Z

Ooops, sorry. I read poll too quickly and misunderstood it; yes it's a suitable replacement to epoll_wait.

For the record, the limitations on select still stand: there is no equivalent to EPOLLHUP/POLLHUP in select, and one must instead add a FD into readfds and check if a read returns zero (overkill for this FD monitoring thread).

Only one client connection is supported now, so ignore new ones as long as the current connection is not severed.

sangjinhan

Thank you so much for the update. I'll merge once those minor comments are addressed.

sangjinhan · 2017-08-23T20:00:16Z


-  recv_skip_cnt_ = 0;
+  while (true) {
+    fds[1].fd = client_fd_;


Would you add a comment that negative file descriptors are ignored by poll() for clafirication?

sangjinhan · 2017-08-23T20:01:44Z

+      if (errno == EINTR) {
+        continue;
+      } else {
+        PLOG(ERROR) << "[UnixSocketPort]:epoll_wait()";


Feel free to ignore this: you don't need to add [UnixSocketPort]: to the log message since the filename will be automatically added by PLOG. Also, epoll_wait() -> ppoll()

sangjinhan · 2017-08-23T20:02:20Z

+    } else if (fds[1].revents & (POLLRDHUP | POLLHUP)) {
+      // connection dropped by client
+      int fd = client_fd_;
+      client_fd_ = -1;


kNotConnectedFd in place of -1

sangjinhan · 2017-08-23T20:03:54Z

-  // Relaunch the accept thread.
-  std::thread accept_thread(AcceptThreadMain, reinterpret_cast<void *>(this));
-  accept_thread.detach();
+static void AcceptThreadHandler(int sig) {


Minor: You can just omit the argument name (just static void AcceptThreadHandler(int) {) to indicate the argument will be unused.

sangjinhan · 2017-08-23T20:05:26Z

+  }
+
+
+  accept_thread_ = std::thread([&]() {


[this] can be used in place of [&].

sangjinhan · 2017-08-23T20:10:04Z

 // ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 // POSSIBILITY OF SUCH DAMAGE.

+#include <signal.h>


#include <poll.h> should appear first.

sangjinhan · 2017-08-23T20:12:43Z

 #define BESS_DRIVERS_UNIXSOCKET_H_

 #include <assert.h>
+#include <atomic>


#include <poll.h> #include <atomic> #include <cassert> #include <cerrno> #include <cstdio>

The Google C++ Style Guide says all C++ headers should appear after C headers. Each group needs to be alphabetically sorted.

Do we need cassert and cstdio? Also, poll.h and cerrno seem only necessary in the .cc file, not in the header.

sangjinhan · 2017-08-24T21:29:46Z

Merged, thanks!

Lluís Vilanova added 2 commits August 22, 2017 18:18

drivers/unix_socket: Make sure no FDs are leaked during initializatio…

55ea028

…n error

drivers/unix_socket: Ensure accept thread and FDs are properly cleane…

a610eb5

…d up

Merge branch 'master' into fix-unix-socket

840d13d

Lluís Vilanova added 2 commits August 22, 2017 21:25

testing: [vlan] Limit number of UnixSocketPort instances created by test

c1c1b55

It seems that 32-bit builds reach the limit of open file descriptors.

drivers/unix_socket: Cleanup stray code

520f66b

changlan requested review from shwang and removed request for shwang August 22, 2017 20:14

drivers/unix_socket: Move accept thread body to a separate function

fc9d8e4

Lluís Vilanova added 2 commits August 23, 2017 02:39

drivers/unix_socket: Use a signal to activate exit of accept thread

c9c6218

Using signals instead of pipes minimizes the number of open file descriptors (two FDs per instance).

drivers/unix_socket: Use ppoll to monitor file descriptors

36c7ed7

Using ppoll() instead of epoll_pwait() saves us from using one additional file descriptor per instance, without introducing any measurable performance overhead.

Lluís Vilanova added 2 commits August 23, 2017 03:10

drivers/unix_socket: Ignore additional clients

5f4dbcb

Only one client connection is supported now, so ignore new ones as long as the current connection is not severed.

drivers/unix_socket: Keep compiler happy about warnings

c8e9e98

sangjinhan reviewed Aug 23, 2017

View reviewed changes

drivers/unix_socket: Code cleanup

8cd0de6

sangjinhan approved these changes Aug 24, 2017

View reviewed changes

sangjinhan merged commit 0b4ffeb into NetSys:master Aug 24, 2017

llvilanova deleted the fix-unix-socket branch August 27, 2017 12:19

Conversation

llvilanova commented Aug 21, 2017

Uh oh!

llvilanova commented Aug 21, 2017

Uh oh!

sangjinhan commented Aug 21, 2017

Uh oh!

codecov Bot commented Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

llvilanova commented Aug 22, 2017

Uh oh!

llvilanova commented Aug 22, 2017

Uh oh!

sangjinhan commented Aug 22, 2017

Uh oh!

llvilanova commented Aug 22, 2017

Uh oh!

sangjinhan commented Aug 22, 2017

Uh oh!

llvilanova commented Aug 22, 2017

Uh oh!

sangjinhan commented Aug 22, 2017

Uh oh!

llvilanova commented Aug 23, 2017

Uh oh!

sangjinhan left a comment

Choose a reason for hiding this comment

Uh oh!

sangjinhan Aug 23, 2017

Choose a reason for hiding this comment

Uh oh!

sangjinhan Aug 23, 2017

Choose a reason for hiding this comment

Uh oh!

sangjinhan Aug 23, 2017

Choose a reason for hiding this comment

Uh oh!

sangjinhan Aug 23, 2017

Choose a reason for hiding this comment

Uh oh!

sangjinhan Aug 23, 2017

Choose a reason for hiding this comment

Uh oh!

sangjinhan Aug 23, 2017

Choose a reason for hiding this comment

Uh oh!

sangjinhan Aug 23, 2017

Choose a reason for hiding this comment

Uh oh!

sangjinhan commented Aug 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Aug 22, 2017 •

edited

Loading