<tt>Eric Blake <eblake@redhat.com> wrote on 03/22/2012 06:54:31 PM: </tt> <tt>> > On 03/22/2012 04:49 PM, David Stevens wrote: > > Stefan Berger/Watson/IBM wrote on 03/22/2012 03:04:53 PM: > > > >> > >> I have some concerns about the cancelation of the thread. It can > >> hold the snoop lock and get cancelled while holding it. Next time > >> that lock is grabbed we will get a deadlock... > >> > > > > The snoop lock is acquired in virNWFilterDHCPSnoopEnd(), which > > is where the pthread_cancel() directly (for valid leases) or the > > freeReq()/pthread_cancel() is done. So, the canceler has the lock > > and the canceling thread can't have it, then. > > > > The only other case I see is when the config goes invalid and > > we exit the snooper loop; he frees the snoop_lock() before removing > > his own hash entry, which will cancel the same thread doing > > the remove. Again, it can't have the snoop lock when canceled. > > > > Is there some other case you think I'm missing? > > pthread_cancel() tends to imply that you are properly managing signal > blocking across threads; we haven't used it anywhere else in libvirt, > and I'm extremely wary of pulling it in now, as there's probably a lot > of subtle bugs that it would expose. Are you sure you can't do this in > some other manner without dragging in pthread_cancel()?</tt> <tt>From the patch:</tt> <tt>+ if (pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, &tmp) < 0) + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "pthread_setcanceltype " + "failed; ifname \"%s\"", req->ifname ? + req->ifname : "<NULL>");</tt> <tt>from the man page of above function:</tt> <tt> PTHREAD_CANCEL_ASYNCHRONOUS</tt> <tt> The thread can be canceled at any time. (Typically, it will be</tt> <tt> canceled immediately upon receiving a cancellation request, but</tt> <tt> the system doesn't guarantee this.)</tt> <tt>It seems implementation dependent on when the thread actually die...</tt> <tt>So I can construct a scenario where the canceller holds the lock and the thread doesn't, the thread gets cancelled but not immediately and can still grab the lock and then dies. This is what I concluded to have seen from below stack trace when the snooping thread was running and in rapid succession I was shooting two SIGHUPs at libvirt:</tt> <tt>Thread 11 (Thread 0x7f3c29ce8700 (LWP 12800)):</tt> <tt>#0 0x00000038ec60b3b4 in pthread_cond_wait@@GLIBC_2.3.2 ()</tt> <tt> from /lib64/libpthread.so.0</tt> <tt>#1 0x00007f3c2fe2229a in virCondWait (c=<value optimized out>, </tt> <tt> m=<value optimized out>) at util/threads-pthread.c:117</tt> <tt>#2 0x00007f3c2fe2295b in virThreadPoolWorker (opaque=<value optimized out>)</tt> <tt> at util/threadpool.c:103</tt> <tt>#3 0x00007f3c2fe21f36 in virThreadHelper (data=<value optimized out>)</tt> <tt> at util/threads-pthread.c:161</tt> <tt>#4 0x00000038ec606ccb in start_thread () from /lib64/libpthread.so.0</tt> <tt>#5 0x00000038ec2e0c2d in clone () from /lib64/libc.so.6</tt> <tt>Thread 10 (Thread 0x7f3c294e7700 (LWP 12801)):</tt> <tt>#0 0x00000038ec60b3b4 in pthread_cond_wait@@GLIBC_2.3.2 ()</tt> <tt> from /lib64/libpthread.so.0</tt> <tt>#1 0x00007f3c2fe2229a in virCondWait (c=<value optimized out>, </tt> <tt> m=<value optimized out>) at util/threads-pthread.c:117</tt> <tt>#2 0x00007f3c2fe2295b in virThreadPoolWorker (opaque=<value optimized out>)</tt> <tt> at util/threadpool.c:103</tt> <tt>#3 0x00007f3c2fe21f36 in virThreadHelper (data=<value optimized out>)</tt> <tt> at util/threads-pthread.c:161</tt> <tt>#4 0x00000038ec606ccb in start_thread () from /lib64/libpthread.so.0</tt> <tt>---Type <return> to continue, or q <return> to quit---</tt> <tt>#5 0x00000038ec2e0c2d in clone () from /lib64/libc.so.6</tt> <tt>Thread 9 (Thread 0x7f3c28ce6700 (LWP 12802)):</tt> <tt>#0 0x00000038ec60b3b4 in pthread_cond_wait@@GLIBC_2.3.2 ()</tt> <tt> from /lib64/libpthread.so.0</tt> <tt>#1 0x00007f3c2fe2229a in virCondWait (c=<value optimized out>, </tt> <tt> m=<value optimized out>) at util/threads-pthread.c:117</tt> <tt>#2 0x00007f3c2fe2295b in virThreadPoolWorker (opaque=<value optimized out>)</tt> <tt> at util/threadpool.c:103</tt> <tt>#3 0x00007f3c2fe21f36 in virThreadHelper (data=<value optimized out>)</tt> <tt> at util/threads-pthread.c:161</tt> <tt>#4 0x00000038ec606ccb in start_thread () from /lib64/libpthread.so.0</tt> <tt>#5 0x00000038ec2e0c2d in clone () from /lib64/libc.so.6</tt> <tt>Thread 8 (Thread 0x7f3c284e5700 (LWP 12803)):</tt> <tt>#0 0x00000038ec60b3b4 in pthread_cond_wait@@GLIBC_2.3.2 ()</tt> <tt> from /lib64/libpthread.so.0</tt> <tt>#1 0x00007f3c2fe2229a in virCondWait (c=<value optimized out>, </tt> <tt> m=<value optimized out>) at util/threads-pthread.c:117</tt> <tt>#2 0x00007f3c2fe2295b in virThreadPoolWorker (opaque=<value optimized out>)</tt> <tt> at util/threadpool.c:103</tt> <tt>#3 0x00007f3c2fe21f36 in virThreadHelper (data=<value optimized out>)</tt> <tt> at util/threads-pthread.c:161</tt> <tt>---Type <return> to continue, or q <return> to quit---</tt> <tt>#4 0x00000038ec606ccb in start_thread () from /lib64/libpthread.so.0</tt> <tt>#5 0x00000038ec2e0c2d in clone () from /lib64/libc.so.6</tt> <tt>Thread 7 (Thread 0x7f3c27ce4700 (LWP 12804)):</tt> <tt>#0 0x00000038ec60b3b4 in pthread_cond_wait@@GLIBC_2.3.2 ()</tt> <tt> from /lib64/libpthread.so.0</tt> <tt>#1 0x00007f3c2fe2229a in virCondWait (c=<value optimized out>, </tt> <tt> m=<value optimized out>) at util/threads-pthread.c:117</tt> <tt>#2 0x00007f3c2fe2295b in virThreadPoolWorker (opaque=<value optimized out>)</tt> <tt> at util/threadpool.c:103</tt> <tt>#3 0x00007f3c2fe21f36 in virThreadHelper (data=<value optimized out>)</tt> <tt> at util/threads-pthread.c:161</tt> <tt>#4 0x00000038ec606ccb in start_thread () from /lib64/libpthread.so.0</tt> <tt>#5 0x00000038ec2e0c2d in clone () from /lib64/libc.so.6</tt> <tt>Thread 6 (Thread 0x7f3c274e3700 (LWP 12805)):</tt> <tt>#0 0x00000038ec60b3b4 in pthread_cond_wait@@GLIBC_2.3.2 ()</tt> <tt> from /lib64/libpthread.so.0</tt> <tt>#1 0x00007f3c2fe2229a in virCondWait (c=<value optimized out>, </tt> <tt> m=<value optimized out>) at util/threads-pthread.c:117</tt> <tt>#2 0x00007f3c2fe2295b in virThreadPoolWorker (opaque=<value optimized out>)</tt> <tt> at util/threadpool.c:103</tt> <tt>#3 0x00007f3c2fe21f36 in virThreadHelper (data=<value optimized out>)</tt> <tt>---Type <return> to continue, or q <return> to quit---</tt> <tt> at util/threads-pthread.c:161</tt> <tt>#4 0x00000038ec606ccb in start_thread () from /lib64/libpthread.so.0</tt> <tt>#5 0x00000038ec2e0c2d in clone () from /lib64/libc.so.6</tt> <tt>Thread 5 (Thread 0x7f3c26ce2700 (LWP 12806)):</tt> <tt>#0 0x00000038ec60b3b4 in pthread_cond_wait@@GLIBC_2.3.2 ()</tt> <tt> from /lib64/libpthread.so.0</tt> <tt>#1 0x00007f3c2fe2229a in virCondWait (c=<value optimized out>, </tt> <tt> m=<value optimized out>) at util/threads-pthread.c:117</tt> <tt>#2 0x00007f3c2fe2295b in virThreadPoolWorker (opaque=<value optimized out>)</tt> <tt> at util/threadpool.c:103</tt> <tt>#3 0x00007f3c2fe21f36 in virThreadHelper (data=<value optimized out>)</tt> <tt> at util/threads-pthread.c:161</tt> <tt>#4 0x00000038ec606ccb in start_thread () from /lib64/libpthread.so.0</tt> <tt>#5 0x00000038ec2e0c2d in clone () from /lib64/libc.so.6</tt> <tt>Thread 4 (Thread 0x7f3c264e1700 (LWP 12807)):</tt> <tt>#0 0x00000038ec60b3b4 in pthread_cond_wait@@GLIBC_2.3.2 ()</tt> <tt> from /lib64/libpthread.so.0</tt> <tt>#1 0x00007f3c2fe2229a in virCondWait (c=<value optimized out>, </tt> <tt> m=<value optimized out>) at util/threads-pthread.c:117</tt> <tt>#2 0x00007f3c2fe2295b in virThreadPoolWorker (opaque=<value optimized out>)</tt> <tt> at util/threadpool.c:103</tt> <tt>---Type <return> to continue, or q <return> to quit---</tt> <tt>#3 0x00007f3c2fe21f36 in virThreadHelper (data=<value optimized out>)</tt> <tt> at util/threads-pthread.c:161</tt> <tt>#4 0x00000038ec606ccb in start_thread () from /lib64/libpthread.so.0</tt> <tt>#5 0x00000038ec2e0c2d in clone () from /lib64/libc.so.6</tt> <tt>Thread 3 (Thread 0x7f3c25ce0700 (LWP 12808)):</tt> <tt>#0 0x00000038ec60b3b4 in pthread_cond_wait@@GLIBC_2.3.2 ()</tt> <tt> from /lib64/libpthread.so.0</tt> <tt>#1 0x00007f3c2fe2229a in virCondWait (c=<value optimized out>, </tt> <tt> m=<value optimized out>) at util/threads-pthread.c:117</tt> <tt>#2 0x00007f3c2fe2295b in virThreadPoolWorker (opaque=<value optimized out>)</tt> <tt> at util/threadpool.c:103</tt> <tt>#3 0x00007f3c2fe21f36 in virThreadHelper (data=<value optimized out>)</tt> <tt> at util/threads-pthread.c:161</tt> <tt>#4 0x00000038ec606ccb in start_thread () from /lib64/libpthread.so.0</tt> <tt>#5 0x00000038ec2e0c2d in clone () from /lib64/libc.so.6</tt> <tt>Thread 2 (Thread 0x7f3c254df700 (LWP 12809)):</tt> <tt>#0 0x00000038ec60b3b4 in pthread_cond_wait@@GLIBC_2.3.2 ()</tt> <tt> from /lib64/libpthread.so.0</tt> <tt>#1 0x00007f3c2fe2229a in virCondWait (c=<value optimized out>, </tt> <tt> m=<value optimized out>) at util/threads-pthread.c:117</tt> <tt>#2 0x00007f3c2fe2295b in virThreadPoolWorker (opaque=<value optimized out>)</tt> <tt>---Type <return> to continue, or q <return> to quit---</tt> <tt> at util/threadpool.c:103</tt> <tt>#3 0x00007f3c2fe21f36 in virThreadHelper (data=<value optimized out>)</tt> <tt> at util/threads-pthread.c:161</tt> <tt>#4 0x00000038ec606ccb in start_thread () from /lib64/libpthread.so.0</tt> <tt>#5 0x00000038ec2e0c2d in clone () from /lib64/libc.so.6</tt> <tt>Thread 1 (Thread 0x7f3c2fda2820 (LWP 12799)):</tt> <tt>#0 0x00000038ec60dc9c in __lll_lock_wait () from /lib64/libpthread.so.0</tt> <tt>#1 0x00000038ec609125 in _L_lock_880 () from /lib64/libpthread.so.0</tt> <tt>#2 0x00000038ec609008 in pthread_mutex_lock () from /lib64/libpthread.so.0</tt> <tt>#3 0x00007f3c2fe73818 in virNWFilterObjFindByUUID (nwfilters=0x7f3c1c015008, </tt> <tt> uuid=<value optimized out>) at conf/nwfilter_conf.c:2662</tt> <tt>#4 0x00007f3c2fe738ad in virNWFilterObjAssignDef (conn=0x2055190, </tt> <tt> nwfilters=0x7f3c1c015008, def=0x201ad60) at conf/nwfilter_conf.c:2947</tt> <tt>#5 0x00007f3c2fe73d6f in virNWFilterObjLoad (conn=0x2055190, </tt> <tt> nwfilters=0x7f3c1c015008, configDir=0x7f3c1c089460 "/etc/libvirt/nwfilter")</tt> <tt> at conf/nwfilter_conf.c:3045</tt> <tt>#6 virNWFilterLoadAllConfigs (conn=0x2055190, nwfilters=0x7f3c1c015008, </tt> <tt> configDir=0x7f3c1c089460 "/etc/libvirt/nwfilter")</tt> <tt> at conf/nwfilter_conf.c:3092</tt> <tt>#7 0x00000000004e9084 in nwfilterDriverReload ()</tt> <tt> at nwfilter/nwfilter_driver.c:163</tt> <tt>#8 0x00007f3c2fea2c8f in virStateReload () at libvirt.c:891</tt> <tt>---Type <return> to continue, or q <return> to quit---</tt> <tt>#9 0x0000000000420572 in daemonReloadHandler (srv=<value optimized out>, </tt> <tt> sig=<value optimized out>, opaque=<value optimized out>) at libvirtd.c:1153</tt> <tt>#10 0x00007f3c2fefd4ba in virNetServerSignalEvent (</tt> <tt> watch=<value optimized out>, fd=<value optimized out>, </tt> <tt> events=<value optimized out>, opaque=0x200fe50) at rpc/virnetserver.c:500</tt> <tt>#11 0x00007f3c2fe0f651 in virEventPollDispatchHandles ()</tt> <tt> at util/event_poll.c:490</tt> <tt>#12 virEventPollRunOnce () at util/event_poll.c:637</tt> <tt>#13 0x00007f3c2fe0dd15 in virEventRunDefaultImpl () at util/event.c:247</tt> <tt>#14 0x00007f3c2fefdffd in virNetServerRun (srv=0x200fe50)</tt> <tt> at rpc/virnetserver.c:736</tt> <tt>#15 0x000000000042235d in main (argc=<value optimized out>, </tt> <tt> argv=<value optimized out>) at libvirtd.c:1609</tt> <tt>I bet thread 1 was trying to grab a lock that wasn't properly unlocked due to the async cancel. All the other threads look pretty much the same and I doubt either one holds the lock.</tt> <tt>Maybe we should go with the previous code from a while ago which was setting a flag for the thread to die. It caused other work-arounds to become necessary but at least we don't have to deal with possibly async. deaths of threads holding locks.</tt> <tt> Stefan</tt>