msg/async: remove file event lock #10090

yuyuyu101 · 2016-07-01T11:15:35Z

prepare for the next async messenger backend framework

yuyuyu101 · 2016-07-06T03:47:31Z

passed http://pulpito.ceph.com/haomai-2016-07-05_12:29:30-rados-wip-haomai-testing---basic-smithi/

http://pulpito.ceph.com/haomai-2016-07-05_21:09:56-rados-wip-haomai-testing-distro-basic-smithi/

yuyuyu101 · 2016-07-06T03:47:44Z

http://pulpito.ceph.com/haomai-2016-07-05_12:29:30-rados-wip-haomai-testing---basic-smithi

tchaikov · 2016-07-06T03:59:02Z

src/msg/async/net_handler.h

@@ -30,6 +30,7 @@ namespace ceph {
    int set_nonblock(int sd);
    void set_close_on_exec(int sd);
    void set_socket_options(int sd);
+    void set_socket_options(int sd, bool nodelay, int size);


now we have two set_socket_options()?

yuyuyu101 · 2016-07-06T04:16:48Z

@tchaikov thanks!!! it's a rebase conflicting

tchaikov · 2016-07-07T08:01:13Z

src/msg/async/Event.cc

@@ -126,10 +126,8 @@ EventCenter::~EventCenter()
  }
  assert(time_events.empty());

-  if (notify_receive_fd >= 0) {
-    delete_file_event(notify_receive_fd, EVENT_READABLE);
+  if (notify_receive_fd >= 0)


could you explain why there is no need to delete delete_file_event() in the dtor?

because delete_file_event is used to delete fd from epoll pool. And we will the whole epoll in the following.

will what? i guess you mean "nuke" or something? and what does "in the following" stands for? seems this commit does not look right by its own, could you move it to the commit where "we will nuke the whole epoll"?

'delete driver"

sorry, delete the epoll in "delete driver"

tchaikov · 2016-07-07T08:43:55Z

so each event thread which own EventCenter must operate EventCenter in local thread

i see you want to have a 1:1 mapping from EventCenter to thread, as you label this PR with performance, but maybe you can put more details on the rationale of why this improves the performance in one of the commits, and accompany it with a micro benchmark on the improvement?

yuyuyu101 · 2016-07-07T08:46:13Z

@tchaikov actually the performance isn't the main purpose of this PR. Local eventcenter is required by dpdk. Since this pr is a part of #9230

yuyuyu101 · 2016-07-07T08:46:24Z

the performance is the side effect

tchaikov · 2016-07-07T09:00:49Z

okay, i can hardly tell the intention of this move without the context.

yuyuyu101 · 2016-07-07T09:03:07Z

@tchaikov sorry, I need to clarify at first.

tchaikov · 2016-07-07T09:09:26Z

sorry, i am not familiar with this area or its background. what interested me was #10056 which addresses some of qa run failures. let me know if you could document the commit a little bit. or maybe we can have some other expert in (async) msgr to review this changeset so less latency is expected.

tchaikov · 2016-07-07T10:10:19Z

src/msg/async/AsyncConnection.cc

+            existing->delay_state->set_center(new_center);
+        } else if (existing->state == STATE_CLOSED) {
+          ::close(new_fd);
+          return ;


could remove this line.

no, if continue, we will dispatch the later event which we wan't

tchaikov · 2016-07-12T13:07:39Z

src/msg/async/AsyncConnection.h

      ::shutdown(sd, SHUT_RDWR);
+      ::close(sd);


is sd dupped before? if not, ::close() would suffice in this case.

tchaikov · 2016-07-12T13:43:59Z

reviewed. i don't think this PR is a refactor. it has following non-trivial changes

fixes on cleanup
and shards the fds by the thread handling them
as a side effect, the lock for inter-thread resource sharing is removed.

other than some nits, generally looks good.

yuyuyu101 · 2016-07-12T15:29:51Z

@tchaikov thanks!

We are make each AsyncConnection/AsyncMessenger only modify its file event in event thread. So make sure create/delete_file_event aren't directly called. Signed-off-by: Haomai Wang <haomai@xsky.com>

Signed-off-by: Haomai Wang <haomai@xsky.com>

Previously we only exchange fd when replacing, now we will introduce dpdk plugin in the near future. It needs all fd used locally which not like kernel socket shared by all cores. So we need to add EventCenter swapping to let each socket is associated to EventCenter. Signed-off-by: Haomai Wang <haomai@xsky.com>

EventCenter::init is called by other thread instead of event thread, so we need to move create_file_event to set_owner which is called by event thread. Signed-off-by: Haomai Wang <haomai@xsky.com>

When we create event thread, it need a little time to enter event loop(like calling set_owner), if caller is going to call create_file_event before event thread enter event loop, it will trigger assert. Signed-off-by: Haomai Wang <haomai@xsky.com>

because if we are in STATE_CLOSED, fd must be -1 Signed-off-by: Haomai Wang <haomai@xsky.com>

…nished Fixes: http://tracker.ceph.com/issues/16552 Signed-off-by: Haomai Wang <haomai@xsky.com>

Now all EventCenter will exists within one thread, it will let all file events api changes without locks. Signed-off-by: Haomai Wang <haomai@xsky.com>

Signed-off-by: Haomai Wang <haomai@xsky.com>

Let cleanup resources things all in shutdown_socket Signed-off-by: Haomai Wang <haomai@xsky.com>

When replacing and someone called mark_down, it will delete_time_event which isn't allowed. Because we're exchaning EventCenter now! Signed-off-by: Haomai Wang <haomai@xsky.com>

Signed-off-by: Haomai Wang <haomai@xsky.com>

Fixes: http://tracker.ceph.com/issues/16554 Signed-off-by: Haomai Wang <haomai@xsky.com>

…lacing When replacing, we don't expect any AsyncConnection dispatch new event which will cause thing chaos Signed-off-by: Haomai Wang <haomai@xsky.com>

Signed-off-by: Haomai Wang <haomai@xsky.com>

Otherwise if message in queue, we will continue to reconnect right now, it won't meet our expectation that we want our connect request delay Signed-off-by: Haomai Wang <haomai@xsky.com>

because we want to get the right log sequence which mixes ceph logginer and cerr. Otherwise, cerr output make the logs a little disordered. Signed-off-by: Haomai Wang <haomai@xsky.com>

yuyuyu101 · 2016-07-12T15:53:05Z

mostly done

1. A -> B 2. goto standby 3. B mark down 4. A reconnect to B 5. got reset session and dispatch remote reset 6. because remote reset is executed in DispatchQueue, it will be delayed 7. A -> B successfully and begin to send message 8. assert because we found the first message is missing but it's reasonble if policy.resetcheck is true Signed-off-by: Haomai Wang <haomai@xsky.com>

yuyuyu101 added core performance wip-haomai-testing labels Jul 1, 2016

yuyuyu101 force-pushed the wip-remove-async-lock branch 4 times, most recently from 2281f9b to 1705c62 Compare July 4, 2016 14:39

yuyuyu101 mentioned this pull request Jul 5, 2016

msg/async/AsyncMessenger: make sure all connection cleanup handler finished #10056

Closed

yuyuyu101 force-pushed the wip-remove-async-lock branch from 0925284 to e8622c1 Compare July 5, 2016 06:53

tchaikov reviewed Jul 6, 2016
View reviewed changes

yuyuyu101 force-pushed the wip-remove-async-lock branch from e8622c1 to 31de313 Compare July 6, 2016 04:16

yuyuyu101 force-pushed the wip-remove-async-lock branch from 31de313 to 7d8c863 Compare July 6, 2016 09:56

yuyuyu101 assigned tchaikov Jul 6, 2016

tchaikov reviewed Jul 7, 2016
View reviewed changes

tchaikov removed their assignment Jul 7, 2016

tchaikov reviewed Jul 7, 2016
View reviewed changes

yuyuyu101 force-pushed the wip-remove-async-lock branch 3 times, most recently from bb1c912 to 2440b0f Compare July 7, 2016 14:27

tchaikov reviewed Jul 12, 2016
View reviewed changes

tchaikov assigned yuyuyu101 Jul 12, 2016

yuyuyu101 and others added 9 commits July 12, 2016 23:37

AsyncMessenger: make create/delete_file_event within event thread

478dd90

We are make each AsyncConnection/AsyncMessenger only modify its file event in event thread. So make sure create/delete_file_event aren't directly called. Signed-off-by: Haomai Wang <haomai@xsky.com>

msg/async/Event: fix delete_time_event leak

96aa327

Signed-off-by: Haomai Wang <haomai@xsky.com>

msg/async: make EventCenter notify file event creating when set_owner

f5d1c05

EventCenter::init is called by other thread instead of event thread, so we need to move create_file_event to set_owner which is called by event thread. Signed-off-by: Haomai Wang <haomai@xsky.com>

msg/async: remove close socket line

fdb9c7d

because if we are in STATE_CLOSED, fd must be -1 Signed-off-by: Haomai Wang <haomai@xsky.com>

msg/async/AsyncMessenger: make sure all connection cleanup handler fi…

af1d3ed

…nished Fixes: http://tracker.ceph.com/issues/16552 Signed-off-by: Haomai Wang <haomai@xsky.com>

Event: remove file_lock

24b3c27

Now all EventCenter will exists within one thread, it will let all file events api changes without locks. Signed-off-by: Haomai Wang <haomai@xsky.com>

msg/async: add assert to ensure create/delete file event within thread

37629d5

Signed-off-by: Haomai Wang <haomai@xsky.com>

yuyuyu101 force-pushed the wip-remove-async-lock branch from 9df59d3 to 3c595c2 Compare July 12, 2016 15:49

yuyuyu101 added 8 commits July 12, 2016 23:50

msg/async/AsyncConnection: simplify shutdown socket flow

fe5770f

Let cleanup resources things all in shutdown_socket Signed-off-by: Haomai Wang <haomai@xsky.com>

msg/async: dont delete time event when exchange eventcenter

8143fff

When replacing and someone called mark_down, it will delete_time_event which isn't allowed. Because we're exchaning EventCenter now! Signed-off-by: Haomai Wang <haomai@xsky.com>

test_async_driver: accept new api changes

9a47ea4

Signed-off-by: Haomai Wang <haomai@xsky.com>

msg/async: hold lock avoid state changing

97e6850

Fixes: http://tracker.ceph.com/issues/16554 Signed-off-by: Haomai Wang <haomai@xsky.com>

msg/async/AsyncConnection: add REPLACING write status to indicate rep…

64405c9

…lacing When replacing, we don't expect any AsyncConnection dispatch new event which will cause thing chaos Signed-off-by: Haomai Wang <haomai@xsky.com>

test_msgr: add server_msgr cleanup

63d5c25

Signed-off-by: Haomai Wang <haomai@xsky.com>

msg/async/AsyncConnection: make sure no STATE_WAIT -> STATE_STANDBY

f04f218

Otherwise if message in queue, we will continue to reconnect right now, it won't meet our expectation that we want our connect request delay Signed-off-by: Haomai Wang <haomai@xsky.com>

ceph_test_msgr: use ceph log infrastructure to output

d1268a6

because we want to get the right log sequence which mixes ceph logginer and cerr. Otherwise, cerr output make the logs a little disordered. Signed-off-by: Haomai Wang <haomai@xsky.com>

yuyuyu101 force-pushed the wip-remove-async-lock branch from 3c595c2 to bcc07cd Compare July 12, 2016 15:51

yuyuyu101 force-pushed the wip-remove-async-lock branch from bcc07cd to 96943ee Compare July 12, 2016 15:59

yuyuyu101 merged commit e489cd4 into ceph:master Jul 12, 2016

yuyuyu101 deleted the wip-remove-async-lock branch July 12, 2016 16:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

msg/async: remove file event lock #10090

msg/async: remove file event lock #10090

yuyuyu101 commented Jul 1, 2016

yuyuyu101 commented Jul 6, 2016

yuyuyu101 commented Jul 6, 2016

tchaikov Jul 6, 2016

yuyuyu101 commented Jul 6, 2016

tchaikov Jul 7, 2016

yuyuyu101 Jul 7, 2016

tchaikov Jul 7, 2016 •

edited

yuyuyu101 Jul 7, 2016

yuyuyu101 Jul 7, 2016 •

edited

tchaikov commented Jul 7, 2016 •

edited

yuyuyu101 commented Jul 7, 2016

yuyuyu101 commented Jul 7, 2016

tchaikov commented Jul 7, 2016 •

edited

yuyuyu101 commented Jul 7, 2016

tchaikov commented Jul 7, 2016 •

edited

tchaikov Jul 7, 2016

yuyuyu101 Jul 7, 2016

tchaikov Jul 12, 2016

tchaikov commented Jul 12, 2016 •

edited

yuyuyu101 commented Jul 12, 2016

yuyuyu101 commented Jul 12, 2016

msg/async: remove file event lock #10090

msg/async: remove file event lock #10090

Conversation

yuyuyu101 commented Jul 1, 2016

yuyuyu101 commented Jul 6, 2016

yuyuyu101 commented Jul 6, 2016

tchaikov Jul 6, 2016

Choose a reason for hiding this comment

yuyuyu101 commented Jul 6, 2016

tchaikov Jul 7, 2016

Choose a reason for hiding this comment

yuyuyu101 Jul 7, 2016

Choose a reason for hiding this comment

tchaikov Jul 7, 2016 • edited

Choose a reason for hiding this comment

yuyuyu101 Jul 7, 2016

Choose a reason for hiding this comment

yuyuyu101 Jul 7, 2016 • edited

Choose a reason for hiding this comment

tchaikov commented Jul 7, 2016 • edited

yuyuyu101 commented Jul 7, 2016

yuyuyu101 commented Jul 7, 2016

tchaikov commented Jul 7, 2016 • edited

yuyuyu101 commented Jul 7, 2016

tchaikov commented Jul 7, 2016 • edited

tchaikov Jul 7, 2016

Choose a reason for hiding this comment

yuyuyu101 Jul 7, 2016

Choose a reason for hiding this comment

tchaikov Jul 12, 2016

Choose a reason for hiding this comment

tchaikov commented Jul 12, 2016 • edited

yuyuyu101 commented Jul 12, 2016

yuyuyu101 commented Jul 12, 2016

tchaikov Jul 7, 2016 •

edited

yuyuyu101 Jul 7, 2016 •

edited

tchaikov commented Jul 7, 2016 •

edited

tchaikov commented Jul 7, 2016 •

edited

tchaikov commented Jul 7, 2016 •

edited

tchaikov commented Jul 12, 2016 •

edited