Bug #798: should have a empty() test on list<Messegger *> - Messengers - Ceph

Actions

Copy link

Bug #798

closed

should have a empty() test on list<Messegger *>

Added by longguang yue about 13 years ago. Updated about 5 years ago.

Status:

Can't reproduce

Priority:

Normal

Assignee:

Sage Weil

Category:

Target version:

% Done:

Source:

Tags:

Backport:

Regression:

Severity:

3 - minor

Reviewed:

Affected Versions:

ceph-qa-suite:

Pull request ID:

Crash signature (v1):

Crash signature (v2):

Description

void SimpleMessenger::dispatch_entry(){
...
pipe->pipe_lock.Lock();
list<Message *>& m_queue = pipe->in_q[priority];
If(!m_queue.empty()) {
Message *m = m_queue.front();
m_queue.pop_front();
}

// Message *m = m_queue.front();
     // m_queue.pop_front();

Actions

Copy link

Updated by longguang yue about 13 years ago

void SimpleMessenger::dispatch_entry(){
...
pipe->pipe_lock.Lock();
list<Message *>& m_queue = pipe->in_q[priority];
Message *m = NULL;
If(!m_queue.empty()) {
m = m_queue.front();
m_queue.pop_front();
}

Actions

Copy link

Updated by Sage Weil about 13 years ago

Category set to msgr
Status changed from New to In Progress
Assignee set to Sage Weil

Hi Longguang,

I took a look at this and think it's okay. Basically the invariant is that the pipe should be in the dispatch queue if and only if the pipe queue is non-empty. Actually, the queuing side was a bit sloppy.. this patch cleans it up I think... does that look reasonable? Or is there a race case I'm missing?

Thanks!
sage

Subject: [PATCH] msgr: clean up Pipe::queue_received locking

Ensure we maintain the invariant that a pipe has a non-empty queue IFF
the pipe is queued.

Prompted by #798.

Signed-off-by: Sage Weil <sage.weil@dreamhost.com>
---
 src/msg/SimpleMessenger.cc |   44 +++++++++++++++++++++++++-------------------
 1 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/src/msg/SimpleMessenger.cc b/src/msg/SimpleMessenger.cc
index 88f1b8a..6200627 100644
--- a/src/msg/SimpleMessenger.cc
+++ b/src/msg/SimpleMessenger.cc
@@ -534,38 +534,44 @@ void SimpleMessenger::Pipe::queue_received(Message *m, int priority)
   assert(pipe_lock.is_locked());

   list<Message *>& queue = in_q[priority];
-  bool was_empty;

   if (halt_delivery)
     goto halt;

-  was_empty = queue.empty();
-  queue.push_back(m);
-  
-  //increment queue length counters
-  in_qlen++;
-  messenger->dispatch_queue.qlen_lock.lock();
-  ++messenger->dispatch_queue.qlen;
-  messenger->dispatch_queue.qlen_lock.unlock();
-  
-  if (was_empty) { //this pipe isn't on the endpoint queue
+  if (queue.empty()) {
+    // queue pipe AND message under pipe AND dispatch_queue locks.
     pipe_lock.Unlock();
     messenger->dispatch_queue.lock.Lock();
     pipe_lock.Lock();
-    
+
     if (halt_delivery) {
       messenger->dispatch_queue.lock.Unlock();
       goto halt;
     }
-    
-    dout(20) << "queue_received queuing pipe" << dendl;
-    if (!queue_items.count(priority)) 
-      queue_items[priority] = new xlist<Pipe *>::item(this);
-    if (messenger->dispatch_queue.queued_pipes.empty())
-      messenger->dispatch_queue.cond.Signal();
-    messenger->dispatch_queue.queued_pipes[priority].push_back(queue_items[priority]);
+
+    if (queue.empty()) {
+      dout(20) << "queue_received queuing pipe" << dendl;
+      if (!queue_items.count(priority)) 
+       queue_items[priority] = new xlist<Pipe *>::item(this);
+      if (messenger->dispatch_queue.queued_pipes.empty())
+       messenger->dispatch_queue.cond.Signal();
+      messenger->dispatch_queue.queued_pipes[priority].push_back(queue_items[priority]);
+    }
+
+    queue.push_back(m);
+
     messenger->dispatch_queue.lock.Unlock();
+  } else {
+    // just queue message under pipe lock.
+    queue.push_back(m);
   }
+  
+  // increment queue length counters
+  in_qlen++;
+  messenger->dispatch_queue.qlen_lock.lock();
+  ++messenger->dispatch_queue.qlen;
+  messenger->dispatch_queue.qlen_lock.unlock();
+  
   return;

  halt:
--

Actions

Copy link

Updated by longguang yue about 13 years ago

a exception is throwed out when pop_front()
------gdb msg--------------------------
-----------------
[root@mods2 ~]# whereis cmon
cmon: /usr/bin/cmon
[root@mods2 ~]# gdb /usr/bin/cm -c core.14331
cmds cmon cmp
[root@mods2 ~]# gdb /usr/bin/cmon -c core.14331
GNU gdb (GDB) Fedora (7.0-3.fc12)
Copyright (C) 2009 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law. Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-redhat-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /usr/bin/cmon...Reading symbols from /usr/lib/debug/usr/bin/cmon.debug...done.
done.
[New Thread 14338]
[New Thread 14336]
[New Thread 14331]
[New Thread 14332]
[New Thread 14589]
[New Thread 14368]
[New Thread 14388]
[New Thread 14389]
[New Thread 14340]
[New Thread 14341]
[New Thread 14333]
[New Thread 14369]
[New Thread 14590]
[New Thread 14339]
Reading symbols from /lib64/libpthread.so.0...(no debugging symbols found)...done.
Loaded symbols for /lib64/libpthread.so.0
Reading symbols from /usr/lib64/libcrypto.so.10...(no debugging symbols found)...done.
Loaded symbols for /usr/lib64/libcrypto.so.10
Reading symbols from /usr/lib64/libstdc++.so.6...(no debugging symbols found)...done.
Loaded symbols for /usr/lib64/libstdc++.so.6
Reading symbols from /lib64/libm.so.6...(no debugging symbols found)...done.
Loaded symbols for /lib64/libm.so.6
Reading symbols from /lib64/libgcc_s.so.1...(no debugging symbols found)...done.
Loaded symbols for /lib64/libgcc_s.so.1
Reading symbols from /lib64/libc.so.6...(no debugging symbols found)...done.
Loaded symbols for /lib64/libc.so.6
Reading symbols from /lib64/ld-linux-x86-64.so.2...(no debugging symbols found)...done.
Loaded symbols for /lib64/ld-linux-x86-64.so.2
Reading symbols from /lib64/libdl.so.2...(no debugging symbols found)...done.
Loaded symbols for /lib64/libdl.so.2
Reading symbols from /lib64/libz.so.1...(no debugging symbols found)...done.
Loaded symbols for /lib64/libz.so.1
Core was generated by `/usr/bin/cmon -i 2 -c /tmp/ceph.conf.23467'.
Program terminated with signal 6, Aborted.
#0 0x00007fd99adc96c5 in raise () from /lib64/libc.so.6
Missing separate debuginfos, use: debuginfo-install glibc-2.11.2-1.x86_64 libgcc-4.4.2-7.fc12.x86_64 libstdc++-4.4.2-7.fc12.x86_64 openssl-1.0.0-4.fc12.x86_64 zlib-1.2.3-23.fc12.x86_64
(gdb) bt
#0 0x00007fd99adc96c5 in raise () from /lib64/libc.so.6
#1 0x00007fd99adcaea5 in abort () from /lib64/libc.so.6
#2 0x00007fd99ae06133 in __libc_message () from /lib64/libc.so.6
#3 0x00007fd99ae0bac6 in malloc_printerr () from /lib64/libc.so.6
#4 0x0000000000454cf2 in deallocate (this=<value optimized out>, __p=<value optimized out>)
at /usr/include/c++/4.4.2/ext/new_allocator.h:95
#5 _M_put_node (this=<value optimized out>, __p=<value optimized out>) at /usr/include/c++/4.4.2/bits/stl_list.h:320
#6 _M_erase (this=<value optimized out>, __p=<value optimized out>) at /usr/include/c++/4.4.2/bits/stl_list.h:1431
#7 pop_front (this=<value optimized out>, __p=<value optimized out>) at /usr/include/c++/4.4.2/bits/stl_list.h:906
#8 SimpleMessenger::dispatch_entry (this=<value optimized out>, __p=<value optimized out>) at msg/SimpleMessenger.cc:290
#9 0x000000000044525c in SimpleMessenger::DispatchThread::entry (this=0x20a5818) at msg/SimpleMessenger.h:558
#10 0x000000000045a9da in Thread::_entry_func (arg=<value optimized out>) at common/Thread.h:39
#11 0x00007fd99bc57a3a in start_thread () from /lib64/libpthread.so.0
#12 0x00007fd99ae7577d in clone () from /lib64/libc.so.6
#13 0x0000000000000000 in ?? ()