FreeBSD/EventKqueue.{h,cc} Added code to restore events on (thread)fork #11430

wjwithagen · 2016-10-11T15:32:09Z

According the FreeBSD man page of kqueue(), the kq-descriptors become invalid
upon fork. It looks like the same happens when a kq-handle is created and then
a thread is created.

So we keep a list of assigned events with each kq-handle and when the handle
has beccome invalid, we recreated the kq-handle and the events that go with it.

Signed-off-by: Willem Jan Withagen wjw@digiware.nl

wjwithagen · 2016-10-11T18:59:15Z

@yuyuyu101
I've taken that rather rigorous approach to redo the events on a kqfd after the thread has forked.
But it seems to work, since all tests that are available run to completion. (This is for the first time.)

So would you care to review the code, and comment....
More that happy to fix any suggestions.

liewegas · 2016-10-11T19:24:23Z

@yuyuyu101

wjwithagen · 2016-10-11T20:27:30Z

@liewegas
I pinged @yuyuyu101 already to see what he thinks of the code.
Probably is not going to win a first prize in a beauty contest, but it does get the thing done.

yuyuyu101

basicly looks good. But why we not add handle_fork in EventDriver interface which may more clean?

yuyuyu101 · 2016-10-12T08:01:41Z

src/msg/async/EventKqueue.h

  CephContext *cct;
  int size;

+ private:


nit: don't need to private here

yuyuyu101 · 2016-10-12T08:06:15Z

src/msg/async/EventKqueue.cc

+                             << cpp_strerror(errno) << dendl;
+      return -errno;
+    }
+  }


I think we can move these checks to a function to make it shared

yuyuyu101 · 2016-10-12T08:07:40Z

src/msg/async/EventKqueue.cc

+    ldout(cct,10) << __func__ << " We changed thread from " << mythread << " to " << pthread_self() << dendl;
+    mythread = pthread_self();
+    kqfd = -1;
+  } else if ((kqfd != -1) && (test_kqfd() < 0)) {


if thread id isn't changed, it may destroy kqfd?

@yuyuyu101
I have seen cases where it triggered an "invalid" kqfd. But perhaps that was during testing and the code still had bugs. Lets keep this code in for now. And in due time if we full understand what is going on, perhaps change it into an 'assert()' if this actually should not happen. But now I'm not 100% certain.

@yuyuyu101
Or even just return a negative result. The layer above is not equipped to handle kqueue-event faults, and assert in most cases anyways.

yuyuyu101 · 2016-10-12T08:08:28Z

src/msg/async/EventKqueue.h

+  struct SaveEvent{
+    int fd;
+    int mask;
+  };


I think we can move struct kevent to this structure.

@yuyuyu101
That would actually make it hard to use res_events[] the structure that kevent() can return the triggerd events in. So we'd again need to alloc(nevents-size) an array for that, or do that on the stack. Blowing the stack out of proportion on every call to event_wait()

yuyuyu101 · 2016-10-12T08:09:11Z

src/msg/async/EventKqueue.cc

+    }
+  }
+
+  ldout(cct,10) << __func__ << " add event kqfd = " << kqfd << " fd = " << fd 


20 should be better. 10 is too verbose

@yuyuyu101
Will fix all verbose levels, since they are too low.

yuyuyu101 · 2016-10-12T08:09:15Z

src/msg/async/EventKqueue.cc

-  int mask = cur_mask & delmask;
+  int mask = cur_mask & del_mask;
+
+  ldout(cct,10) << __func__ << " delete event kqfd = " << kqfd 


yuyuyu101 · 2016-10-12T08:09:32Z

src/msg/async/EventKqueue.cc

@@ -99,22 +211,66 @@ int KqueueDriver::event_wait(vector<FiredFileEvent> &fired_events, struct timeva
  int retval, numevents = 0;
  struct timespec timeout;

+  ldout(cct,10) << __func__ << " kqfd = " << kqfd << dendl;


I hope we don't print this or improve this to 30

@wjwithagen this line... we don't want to print level 10 log when entering event_wait each time

yuyuyu101 · 2016-10-12T08:09:44Z

src/msg/async/EventKqueue.cc

  }

-  if (retval > 0) {
+  ldout(cct,10) << __func__ << " kevent retval: "


yuyuyu101 · 2016-10-12T08:09:55Z

src/msg/async/EventKqueue.cc

    int j;

    numevents = retval;
    fired_events.resize(numevents);
    for (j = 0; j < numevents; j++) {
      int mask = 0;
-      struct kevent *e = events + j;
+      struct kevent *e = res_events + j;
+      ldout(cct,10) << __func__ << " j: " << j << dendl;


hope we don't need this

wjwithagen · 2016-10-12T09:00:42Z

@yuyuyu101
Glad you think it is not a total mess.... I will work on the changes.

And yes, I would love to do this at a higher level, but on the other hand it would perhaps obfuscate the code at that level. This is obviously a Kqueue issue, and as such should be hidden in the kqueue-code.
Perhaps for me the most important thing: I'm not 100% confident that I understand all details of the thread creation in the stack module.
Perhaps we should later on, when the dust has settled, and we are quite sure that this is a workable solution that has passed QA.

wjwithagen · 2016-10-12T09:02:26Z

@yuyuyu101
If you want to go the handle_fork way, you will have to give me some pointers/example code of what your idea is. And I'll start thinking about it, and we can see if it delivers cleaner code.

yuyuyu101 · 2016-10-12T09:22:00Z

@wjwithagen hmm, I think we can go through this way at first. Let's see it will work firstly. Then if available, I could just refactor this workable codes and make it in higher level. So let's make it first!

Thanks!!

wjwithagen · 2016-10-12T09:38:19Z

@yuyuyu101
Great, thanx for the positive feedback.

wjwithagen · 2016-10-12T10:35:16Z

@yuyuyu101
I think I addressed most/all of your comments?
If you are oke with this, I'll rebase it again into 2 commits.

yuyuyu101 · 2016-10-12T15:07:55Z

src/msg/async/EventKqueue.cc

    int j;

    numevents = retval;
    fired_events.resize(numevents);
    for (j = 0; j < numevents; j++) {
      int mask = 0;
-      struct kevent *e = events + j;
+      struct kevent *e = res_events + j;
+      ldout(cct,20) << __func__ << " j: " << j << dendl;


I dont know why need to print j.... since we already know the number of events returned

@yuyuyu101
This is really debug stuff to make sure all events are processed.
So yes it can go.

wjwithagen · 2016-10-12T15:16:55Z

@yuyuyu101
Ping...

yuyuyu101 · 2016-10-12T16:28:19Z

src/msg/async/EventKqueue.cc

@@ -22,34 +22,129 @@
 #undef dout_prefix
 #define dout_prefix *_dout << "KqueueDriver."

+#define KEVENT_NOWAIT 0


@yuyuyu101
Leftover from previous code. Usually I replace hard values with descriptive constants.
I'll see if it still fits the purpose.

yuyuyu101 · 2016-10-12T16:29:49Z

src/msg/async/EventKqueue.cc

+        ldout(cct,0) << __func__ << " unable to add event: "
+                     << cpp_strerror(errno) << dendl;
+        return -errno;
+      }


I think you need to reset num = 0 here? otherwise, num will continue to grow.....

yuyuyu101 · 2016-10-12T16:30:56Z

src/msg/async/EventKqueue.cc

@@ -99,22 +211,66 @@ int KqueueDriver::event_wait(vector<FiredFileEvent> &fired_events, struct timeva
  int retval, numevents = 0;
  struct timespec timeout;

+  ldout(cct,10) << __func__ << " kqfd = " << kqfd << dendl;


yuyuyu101 · 2016-10-12T16:31:37Z

src/msg/async/EventKqueue.h

 public:
-  explicit KqueueDriver(CephContext *c): kqfd(-1), events(NULL), cct(c), size(0) {}
+  explicit KqueueDriver(CephContext *c): kqfd(-1), res_events(NULL), cct(c), size(0) {}


need to init sav_events = 0

yuyuyu101 · 2016-10-12T16:31:49Z

src/msg/async/EventKqueue.h

+    int mask;
+  };
+  struct SaveEvent *sav_events;
+  int sav_index;


Right the idea was to keep save usage here.
Best to name it sav_max and keep the size of sav_events for resizing.

yuyuyu101 · 2016-10-12T16:34:20Z

src/msg/async/EventKqueue.h

+    int fd;
+    int mask;
+  };
+  struct SaveEvent *sav_events;


because you save all ongoing kqueue events, I think you need to resize in KqueueDriver::resize_events in case of fd is exceed the init number

@yuyuyu101
Right, I was hoping not to run into that :), but it does require action.
Would be a lot when 1 OSD has like 5000 fd's open.

wjwithagen · 2016-10-13T08:28:05Z

@yuyuyu101
Good review, I'm currently also busy with other things, so it might take more than a few hours to rework this.

wjwithagen · 2016-10-14T07:33:51Z

@yuyuyu101
I think I got all the issues you touched and did a bit of cleaning up.
Especially catching the num=0 was a nice one, could be the reason that I sometimes saw restore_event errors, and reset the FD. Probably events were reinserted that were already there.
So can you do another review?

yuyuyu101 · 2016-10-14T07:41:30Z

src/msg/async/EventKqueue.cc

+    }
+	memset(&sav_events[size], 0, sizeof(struct SaveEvent)*(newsize-sav_max));
+	sav_max = newsize;
+  }


looks format is totally unaligned

yuyuyu101 · 2016-10-14T07:42:31Z

others look better to me

wjwithagen · 2016-10-14T07:55:03Z

@yuyuyu101
You are right. Looks like vi did not pick up on tabbing.
Fixed that.

yuyuyu101

LGTM

yuyuyu101 · 2016-10-14T08:01:28Z

Because the qa won't compile out Kqueue driver, I don't think we need qa process. But before merging, I want to make sure @wjwithagen have tested the fresh commits. And got ack from you....

wjwithagen · 2016-10-15T10:57:23Z

@yuyuyu101
Before I left yesterday, i fired up a testrun with this code in my work tree:

99% tests passed, 1 tests failed out of 129
Total Test time (real) = 3531.31 sec
The following tests FAILED:
         12 - run-tox-ceph-disk (Failed)

And the failure on the last on is expected, because I have not done any work on it.
So I think it is good to go.

…ead)fork According the FreeBSD man page of kqueue(), the kq-descriptors become invalid upon fork. It looks like the same happens when a kq-handle is created and then a thread is created. So we keep a list of assigned events with each kq-handle and when the handle has beccome invalid, we recreated the kq-handle and the events that go with it. Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>

wjwithagen · 2016-10-15T11:10:54Z

@yuyuyu101
Rebased to compress the commits into 3 pieces.

wjwithagen · 2016-10-16T18:14:55Z

@yuyuyu101
thank ....

liewegas added the common label Oct 11, 2016

yuyuyu101 requested changes Oct 12, 2016

View reviewed changes

yuyuyu101 reviewed Oct 12, 2016

View reviewed changes

yuyuyu101 reviewed Oct 14, 2016

View reviewed changes

yuyuyu101 approved these changes Oct 14, 2016

View reviewed changes

wjwithagen added 2 commits October 15, 2016 13:04

src/msg/async/EventKqueue.cc: adjust logging levels.

a534069

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>

wjwithagen force-pushed the wip-wjw-freebsd-EventKqueue branch from 1fdcc42 to 1fdd2b4 Compare October 15, 2016 11:05

src/msg/async/EventKqueue: refactor and add test_thread_change()

9eeb491

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>

wjwithagen force-pushed the wip-wjw-freebsd-EventKqueue branch from 1fdd2b4 to 9eeb491 Compare October 15, 2016 11:10

yuyuyu101 merged commit 21d5f49 into ceph:master Oct 16, 2016

wjwithagen deleted the wip-wjw-freebsd-EventKqueue branch January 4, 2017 10:22

FreeBSD/EventKqueue.{h,cc} Added code to restore events on (thread)fork #11430

FreeBSD/EventKqueue.{h,cc} Added code to restore events on (thread)fork #11430

Conversation

wjwithagen commented Oct 11, 2016

wjwithagen commented Oct 11, 2016

liewegas commented Oct 11, 2016

wjwithagen commented Oct 11, 2016

yuyuyu101 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwithagen commented Oct 12, 2016

wjwithagen commented Oct 12, 2016

yuyuyu101 commented Oct 12, 2016

wjwithagen commented Oct 12, 2016

wjwithagen commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwithagen commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwithagen commented Oct 13, 2016

wjwithagen commented Oct 14, 2016

Choose a reason for hiding this comment

yuyuyu101 commented Oct 14, 2016

wjwithagen commented Oct 14, 2016

yuyuyu101 left a comment

Choose a reason for hiding this comment

yuyuyu101 commented Oct 14, 2016

wjwithagen commented Oct 15, 2016

wjwithagen commented Oct 15, 2016

wjwithagen commented Oct 16, 2016