Simon Arlott wrote:
> On Mon, March 5, 2007 11:19, Johannes Stezenbach wrote:
> > On Mon, Mar 05, 2007 at 01:58:14AM +0100, Oliver Endriss wrote:
> >> Simon Arlott wrote:
> >> > Is any part of the patch going to be applied? I mentioned this
> >> > problem in September last year and it looks like it's existed for
> >> > years (the semaphore locking did the same thing).
> >>
> >> Well, I hoped that someone more familiar with the demuxer stuff would
> >> comment on the patch. I am not very happy about using non-interruptible
> >> lock operations...
>
> Why? If there are deadlocks these should be fixed, not ignored.
Yes, they should be fixed, but they should not require a reboot.
>From 'Linux Device Drivers' (replace 'down' by 'mutex_lock'):
| ...
| down decrements the value of the semaphore and waits as long as need
| be. down_ interruptible does the same, but the operation is
| interruptible. The interruptible version is almost always the one you
| will want; it allows a user-space process that is waiting on a
| semaphore to be interrupted by the user. You do not, as a general
^^^^^^^^^^^^^^^^^^^^^^^^
| rule, want to use noninterruptible operations unless there truly is no
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| alternative. Noninterruptible operations are a good way to create
^^^^^^^^^^^
| unkillable processes (the dreaded D state seen in ps), and annoy
| your users. Using down_interruptible requires some extra care,
| however, if the operation is interrupted, the function returns a
| nonzero value, and the caller does not hold the semaphore. Proper use
| of down_interruptible requires always checking the return value and
| responding accordingly.
| ...
> >> Anyway, I'll apply the patch to HG master if you submit an updated patch:
> >> - Please add a line of comment to each mutex_lock() stating _why_ the
> >> non-interruptible lock has to be used at this place.
>
> What's the point of doing that?
The point is to document why we do not use the interruptible version.
Next year someone might submit a patch replacing mutex_lock by
mutex_lock_interruptible, and no one remembers why mutex_lock is
required at this place.
> > IMHO using mutex_lock_interruptible() is almost always wrong.
> >
> > The only use it has in dvb-core is to recover from locking bugs --
> > if it deadlocks you can Ctrl-C out of it
> > (instead of being left with a non-killable program -> reboot).
>
> This is what lockdep is for.
Is lockdep activated in distribution kernels?
> > But with mutex_lock_interruptible() it's easy to introduce
> > signal handling bugs, which Simon confirmed to exist.
>
> It's also easy to find examples of people needing to rmmod/modprobe
> because dvr0 started returning -EBUSY on open() after they closed
> something.
You can find examples for everything. A reboot because of a deadlock is
worse than a rmmod/insmod cycle. Anyway, that's not the point.
> > Fixing those up without reverting to mutex_lock() way might
> > be possible, but is more difficult.
>
> It'd introduce lot of unneccessary -ERESTARTSYS, just to avoid the
> possiblity of waiting on mutex_lock for a few msecs.
Imho they are not unneccessary, it's a question of programming style.
See above.
Oliver
--
--------------------------------------------------------
VDR Remote Plugin 0.3.9 available at
http://www.escape-edv.de/endriss/vdr/
--------------------------------------------------------
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
