[illumos-Developer] missing lwp_exit() in kcfpool_svc()

[Garrett D'Amore]

On Sun, 2011-04-03 at 03:01 -0400, Albert Lee wrote:
> On Sun, Apr 3, 2011 at 12:57 AM, Garrett D'Amore <garrett at nexenta.com> wrote:
> > I've added this comment (but not updated the webrev):
> >
> >                         /*
> >                          * lwp_exit() assumes it is called
> >                          * with the proc lock held.  But the
> >                          * first thing it does is drop it.
> >                          * This ensures that lwp does not
> >                          * exit before lwp_create is done
> >                          * with it.
> >                          */
> >                          mutex_enter(&curproc->p_lock);
> >                          lwp_exit();   /* does not return */
> >
> >
> > Hopefully that is what you were looking for.
> >
> >        - Garrett
> >
> 
> 
> The fix looks correct to me. However, I'm concerned about the workers'
> accesses to of kcfpool->kp_threads. While chunks of kcfpool_svc() are
> synchronised between workers with gswq->gs_lock, kcfpoold's use of
> kp_threads and kp_blockedthreads is only guarded by kp_lock so it is
> possible to race with workers. Inconsistent conditions like
> kp_blockedthreads > kp_threads can be visible in kcfpoold, and may
> cause too few or too many threads to be spawned (although the latter
> case unlikely since it is limited to racing with increments).

A race that causes too many or two few threads to be created is
acceptable, as long as the ultimate accounting for those is correct.
Remember the sizing of the pool is dynamic.

The actual values are not protected by kp_lock, but are updated using
atomics, so they'll always be correct.  kp_lock itself is used pretty
much just for dealing with the condvar for waking up thread pool
manager.

The gs_lock is used to protect the request queue.

There's no overlap between the locks that I can see.

	- Garrett
> 
> -Albert
> 
> > On Fri, 2011-04-01 at 15:26 -0400, Dan McDonald wrote:
> >> On Fri, Apr 01, 2011 at 11:51:18AM -0700, Garrett D'Amore wrote:
> >> >
> >> >  Webrev with the fix is at
> >> > http://mexico.purplecow.org/gdamore/webrev/kcf-fix/
> >> >
> >>
> >> It might be instructive to have a small comment about why you're acquiring
> >> curproc->p_lock prior to calling lwp_exit().
> >>
> >> Otherwise, ship it!
> >>
> >> Dan
> >>
> 
> _______________________________________________
> Developer mailing list
> Developer at lists.illumos.org
> http://lists.illumos.org/m/listinfo/developer