Scylla consumes 100% CPU when compiled with boost version < 1.56

[tgrabiec]

Installation details
Scylla version (or git commit hash): >= 1.2.2
OS (RHEL/CentOS/Ubuntu/AWS AMI): Ubuntu 14.04

Hardware details (for performance issues)
--smp > 1

This is because of this code in seastar:

bool smp_message_queue::pure_poll_tx() const {
#if BOOST_VERSION >= 105600
    return !_tx.a.pending_fifo.empty() && _pending.write_available();
#else
    return true;
#endif
}

When compiled with boost older than 1.56 pure_poll_tx() will always return true causing tracker::impl::compact_on_idle to always return reactor::idle_cpu_handler_result::interrupted_by_higher_priority_task which will prevent seastar from entering sleep mode:

go_to_sleep = handler_result == idle_cpu_handler_result::no_more_work;

This affects our .deb packages for Scylla 1.2.3:

(gdb) disassemble smp_message_queue::pure_poll_tx
Dump of assembler code for function smp_message_queue::pure_poll_tx() const:
   0x00000000004bb300 <+0>: mov    $0x1,%eax
   0x00000000004bb305 <+5>: retq   
End of assembler dump.

[avikivity]

Maybe, instead of finding a clever workaround, we can set up a ppa for boost-1.recent and build with that.

[tgrabiec]

I'm looking at smp::poll_queues (used when there's no task-on-idle) and its result doesn't depend on whether there is room in the txq queue or not, it just calls txq.flush_request_batch();. Couldn't pure_poll_queues() do the same?

The result of poll_queues() does depend on whether there are any completions to process, but pure_poll_queues() doesn't check it at all. Shouldn't it?

[tgrabiec]

Shouldn't we have:

diff --git a/core/reactor.hh b/core/reactor.hh
index d0a6ffa..09cea1f 100644
--- a/core/reactor.hh
+++ b/core/reactor.hh
@@ -1084,7 +1084,9 @@ class smp {
         for (unsigned i = 0; i < count; i++) {
             if (engine().cpu_id() != i) {
                 auto& rxq = _qs[engine().cpu_id()][i];
+                rxq.flush_response_batch();
                 auto& txq = _qs[i][engine()._id];
+                txq.flush_request_batch();
                 if (rxq.pure_poll_rx() || txq.pure_poll_tx()) {
                     return true;
                 }
diff --git a/core/reactor.cc b/core/reactor.cc
index 1b30de5..100f835 100644
--- a/core/reactor.cc
+++ b/core/reactor.cc
@@ -2164,14 +2164,6 @@ void smp_message_queue::move_pending() {
     _sent += nr;
 }

-bool smp_message_queue::pure_poll_tx() const {
-#if BOOST_VERSION >= 105600
-    return !_tx.a.pending_fifo.empty() && _pending.write_available();
-#else
-    return true;
-#endif
-}
-
 void smp_message_queue::submit_item(smp_message_queue::work_item* item) {
     _tx.a.pending_fifo.push_back(item);
     if (_tx.a.pending_fifo.size() >= batch_size) {
@@ -2202,6 +2194,12 @@ void smp_message_queue::flush_response_batch() {
 bool smp_message_queue::pure_poll_rx() const {
     // can't use read_available(), not available on older boost
     // empty() is not const, so need const_cast.
+    return !const_cast<lf_queue&>(_pending).empty();
+}
+
+bool smp_message_queue::pure_poll_tx() const {
+    // can't use read_available(), not available on older boost
+    // empty() is not const, so need const_cast.
     return !const_cast<lf_queue&>(_completed).empty();
 }

[avikivity]

I think poll_queues() is broken. If we have pending responses, but can't push them because the queue is full, we should not go to sleep. If we do, no one will wake us up.

We'll get woken up if we try to read a work_item but the queue is empty. The reverse case is not handled (nor should it be handled, because it is so rare).

So I think the pure version was right and the non-pure version is wrong.

[tgrabiec]

@avikivity Another issue with pure_poll_queues was that pure_poll_rx(), which is called on rxq, was checking _completed instead of _pending

[tgrabiec]

@avikivity As for the case for poll_queues() being broken, for txq being full, aren't we supposed to be woken up by completion sent for the already queued tasks?

[avikivity]

We are, but but what happens when we push a completion?

btw, we might make the queues unidirectional and simplify things a little.

[avikivity]

Actually, unidirecional queues mean cross-cpu frees.

[tgrabiec]

2016-08-05 18:50 GMT+02:00 Avi Kivity notifications@github.com:

We are, but but what happens when we push a completion?

Right, if _completed gets full and we go to sleep, we won't be woken up to
send more completions. But I don't see how the pure version dealt with it.

I think this couldn't happen before
15a92cf, because the number of in-flight
pending requests was never grater than the capacity of _completed.

[tgrabiec]

Ok, I see now. The pure version was not going to sleep unless the completions drained.

[tgrabiec]

But we were missing the call to flush the response batch