Re: Making the transport layer more robust

[Mark Wielaard <mjw at redhat dot com>]

On Mon, 2011-08-15 at 11:30 -0700, Josh Stone wrote:
> On 08/12/2011 10:43 AM, Mark Wielaard wrote:
> > commit 46ac9ed5bad86641e552bee4e42a2d973ffc12d0
> > Author: Mark Wielaard <mjw@redhat.com>
> > Date:   Fri Aug 12 19:34:20 2011 +0200
> > 
> >     Remove _stp_ctl_work_timer from module transport layer.
> >     
> >     The _stp_ctl_work_timer would trigger every 20ms to check whether
> >     there were cmd messages queued, but not announced yet and to
> >     check the _stp_exit_flag was set.
> >     
> >     This commit makes all control messages announce themselves and
> >     check the _stp_exit_flag in the _stp_ctl_read_cmd loop (delivery
> >     is still possibly delayed since the messages are just pushed on
> >     a wait queue).
> 
> This has unfortunately left open an opportunity for deadlock.  The
> kernel wake_up infrastructure takes a spinlock on the wait queue.  If
> the probe context happens to fire while that lock is held, either via a
> direct probe on something called by wake_up or indirectly via NMI, then
> the handler must not call anything that would attempt the same lock.
> But this commit is triggering a wake_up on ctl prints, and commit
> a85c8aff triggers the same on exit().
> 
> For example, __wake_up_common is called with a lock held, and then
> either of these will cause a deadlock:
> 
>   probe kernel.function("__wake_up_common") { warn(pp()) }
> 
>   probe kernel.function("__wake_up_common") { exit() }
> 
> This issue in general is very similar to PR2525.  We must take care not
> to call any blocking code from arbitrary probe context.

Thanks for catching that. I am surprised none of our tests triggered
this. I added a nasty testcase based on the above example and reverted
most of the above two commit, reintroducing the timer on the kernel side
(luckily we can still keep the poll/select implementation so we won't be
busy polling on the user side at least). I also tried to explicitly
document all the "safe" places in the patch.

commit fc67febc6733e5803e6883a3757abda6268a953a
Author: Mark Wielaard <mjw@redhat.com>
Date:   Tue Aug 16 14:31:29 2011 +0200

  Reintroduce timer for transport cmd channel, don't wake_up unconditionally.
    
  Revert parts of commit a85c8a "runtime/io.c: Explicitly signal setting of
  _stp_exit_flag" and commit 46ac9e "Remove _stp_ctl_work_timer from module
  transport layer". Introduce a new test wake_up.exp that shows a deadlock
  when sending cmd messages and waking up the reader immediately.
    
  Renamed _stp_ctl_write to _stp_ctl_send, which can be called from
  everywhere. Rename _stp_ctl_send to _stp_ctl_send_notify that can be
  called from user context in the transport layer itself (this will
  immediately notify any readers). Document all places that use
  _stp_ctl_send_notify directly to clarify why that is safe.
    
  See http://sourceware.org/ml/systemtap/2011-q3/msg00163.html

Cheers,

Mark