PERFORCE change 125520 for review

[Kip Macy]

On 9/17/07, Julian Elischer <julian at elischer.org> wrote:
> Marko Zec wrote:
> > On Monday 17 September 2007 20:03:59 Julian Elischer wrote:
> >> Marko Zec wrote:
> >>> http://perforce.freebsd.org/chv.cgi?CH=125520
> >>>
> >>> Change 125520 by zec at zec_tpx32 on 2007/08/21 23:51:39
> >>>
> >>>     Given that ng_pipe nodes can be connected into arbitrary
> >>>     topologies, and therefore it is possible for ngp_rcvdata()
> >>>     to be recursively called from a single thread, it is
> >>>     necessary to explicitly allow for the ng_pipe_giant mutex
> >>>     to be recursively acquireable.
> >> OR use a different locking scheme.
> >
> > That's right, but I'm just wondering is there anything fundamentally
> > wrong with lock recursing (both in general as well as in this
> > particular case)?
>
> we are trying as a general rule trying to keep lock recursion to an absolute
> minimum. It can make debugging other things very hard. and can introduce
> bugs that are hard to find..
>
> Generally a bad idea. If you don't know you are recursing, how can you
> avoid the problems you don't know about? (sounds silly but..)

Just to add my 0.02$ from recent experiences ...
Lock recursion creates the following problems:
     - lock is often held much longer than it really needs to be
     - it is no longer possible from code inspection to determine
where the lock is or is not held
     - it makes it near impossible to mix shared and exclusive access
to a lock, acquiring an exclusive lock that you already hold shared
results in a deadlock and vice versa
    - it makes lock ordering more complicated to infer and work with

So ... from code reading experience recently recursive locks are
typically needed for code where the control flow pre-dates any
thoughts of locking.

    -Kip






>
>
> >
> > Marko
> >
> >> i.e. reference counts or something.
> >>
> >>> Affected files ...
> >>>
> >>> .. //depot/projects/vimage/src/sys/netgraph/ng_pipe.c#2 edit
> >>>
> >>> Differences ...
> >>>
> >>> ==== //depot/projects/vimage/src/sys/netgraph/ng_pipe.c#2 (text+ko)
> >>> ====
> >>>
> >>> @@ -1028,7 +1028,7 @@
> >>>                     error = EEXIST;
> >>>             else {
> >>>                     mtx_init(&ng_pipe_giant, "ng_pipe_giant", NULL,
> >>> -                       MTX_DEF);
> >>> +                       MTX_DEF | MTX_RECURSE);
> >>>                     LIST_INIT(&node_head);
> >>>                     LIST_INIT(&hook_head);
> >>>                     ds_handle = timeout((timeout_t *) &pipe_scheduler,
> >
>
>