* Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> [2011-06-30 10:34:35 -0400]: > On Wed, 2011-06-29 at 16:52 -0400, Gustavo F. Padovan wrote: > > * Gustavo F. Padovan <padovan@xxxxxxxxxxxxxx> [2011-06-29 17:24:56 -0300]: > > > > > * Ilia, Kolominsky <iliak@xxxxxx> [2011-06-26 09:16:58 +0200]: > > > > > > > Hi! > > > > IMHO the fix isnt good due to possible race condition which > > > > will destroy session/task objects - either by a call to kthread_stop > > > > from the timer func or reentry to hidp_del_connection() on > > > > smp platforms. > .... > > This should fix the timer issue. Please test. > > > > Gustavo > > Hi Ilia & Gustavo, > > After Ilia pointed out the problem with the timer function, I went back > and reviewed *all* the synchronization code relevant to the hid session > thread. > > A number of problems were introduced with commit aabf6f89 - when the > session thread was converted from a kernel_thread to a kthread. Although > a kthread is a better choice for representing the session thread, the > naive conversion of atomic/wakeup to kthread_stop() was inappropriate. > > kthread_stop() has usage semantics that different significantly from > atomic/wakeup. As we already know, because kthread_stop() blocks on > thread completion, it can introduce deadlocks in code that already uses > exclusion mechanisms. Even with Ilia's new patch, consider the following > sequence: > > Thread 0 Thread 1 Thread 2 > in hidp_del_connection in hidp session > claim r/w sem . > timer triggers . > kthread_stop() --------->. > *blocks on thread 2* exits loop > *blocks for r/w sem* > in hidp_del_timer > del_timer_sync() > *blocks on thread 1* > > Deadlock occurs because: > + thread 0 holds reader lock but is waiting for the timer function on > thread 1 to finish > + thread 1 has stopped kthread and is waiting for thread completion > + thread 2 (aka kthread) is waiting to claim writer lock held by thread > 0 > > In addition to the deadlocks and races, kthread_stop() is being called > by hidp_process_hid_control() which is *in the session thread context* - > kthread_stop() cannot be called on itself! > > I've been testing patch v2 since Monday which continues to use kthread > but reverts back to the old behavior of atomic/wakeup. It also fixes the > potential for a lost wakeup. Of course, "testing" means running it and > looking at it carefully - as someone on IRC pointed out, kthread really > needs to get instrumented with lockdep. Where is this v2 patch? I wanna take a look on it. Gustavo -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
