Re: memcpy of struct buf, or similar?

[Chris Gilbert <chris%dokein.co.uk@localhost>]

Chris Gilbert wrote:
> Chris Gilbert wrote:
>> Attached is a slightly revised patch, to get rid of the changes to
>> whitespace and sleeptab_lookup being moved.
>>
>> I also added the KASSERT back to the cv_unsleep, as the lwp lock should
>> be held at that point, and the lwp lock will be set to the sleepq lock,
>> so it's safe to look at the sleepq.
>>
>> Note there is an assumption that cv_has_waiters will be called with the
>> mutex used to access the cv held, and so locking the sleepq lock will
>> make sure we get a consistent state.
> 
> I've now checked that patch in, as it fixes the issue.
> 
> I've also produced that attached patch, which brings back cv_waiters.
> However, I'm not seeing any performance gain with it using build.sh,
> perhaps a specific benchmark would show a gain.  In theory there should
> be some gain as we don't have to lock the sleepq if it's empty, but it's
> probably lost in the noise of build.sh

Actually reviewing the patch again shows that the patch is broken
(although it works when used), cv_unsleep needs to decrement cv_waiters,
but doesn't have the callers mutex held, and so cv_waiter updates need
to be protected by the sleepq/lwp lock being held.

cv_waiters reads are safe without the sleepq held as the worst that
could happen is they read that there are workers, and cause a spurious
wakeup to happen.

Performance for build.sh seems to be on-par with the code without the patch.

Thanks,
Chris

Index: kern/kern_condvar.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_condvar.c,v
retrieving revision 1.23
diff -u -p -r1.23 kern_condvar.c
--- kern/kern_condvar.c 15 Jun 2008 09:56:18 -0000      1.23
+++ kern/kern_condvar.c 15 Jun 2008 14:46:48 -0000
@@ -96,6 +96,7 @@ cv_init(kcondvar_t *cv, const char *wmes
        KASSERT(wmesg != NULL);
        cv->cv_wmesg = wmesg;
        sleepq_init(CV_SLEEPQ(cv));
+       cv->cv_waiters = 0;
 }
 
 /*
@@ -111,6 +112,7 @@ cv_destroy(kcondvar_t *cv)
 #ifdef DIAGNOSTIC
        KASSERT(cv_is_valid(cv));
        cv->cv_wmesg = deadcv;
+       cv->cv_waiters = -3;
 #endif
 }
 
@@ -134,6 +136,7 @@ cv_enter(kcondvar_t *cv, kmutex_t *mtx, 
        l->l_kpriority = true;
        (void)sleeptab_lookup(&sleeptab, cv, &mp);
        sq = CV_SLEEPQ(cv);
+       cv->cv_waiters++;
        sleepq_enter(sq, l, mp);
        sleepq_enqueue(sq, cv, cv->cv_wmesg, &cv_syncobj);
        mutex_exit(mtx);
@@ -183,6 +186,8 @@ cv_unsleep(lwp_t *l, bool cleanup)
        KASSERT(cv_is_valid(cv));
        KASSERT(!TAILQ_EMPTY(CV_SLEEPQ(cv)));
 
+       cv->cv_waiters--;
+
        return sleepq_unsleep(l, cleanup);
 }
 
@@ -278,8 +283,8 @@ cv_signal(kcondvar_t *cv)
 
        /* LOCKDEBUG_WAKEUP(CV_DEBUG_P(cv), cv, CV_RA); */
        KASSERT(cv_is_valid(cv));
-
-       cv_wakeup_one(cv);
+       if (__predict_false(cv->cv_waiters > 0))
+               cv_wakeup_one(cv);
 }
 
 static void __noinline
@@ -299,6 +304,7 @@ cv_wakeup_one(kcondvar_t *cv)
                mutex_spin_exit(mp);
                return;
        }
+       cv->cv_waiters--;
        KASSERT(l->l_sleepq == sq);
        KASSERT(l->l_mutex == mp);
        KASSERT(l->l_wchan == cv);
@@ -328,7 +334,8 @@ cv_broadcast(kcondvar_t *cv)
        /* LOCKDEBUG_WAKEUP(CV_DEBUG_P(cv), cv, CV_RA); */
        KASSERT(cv_is_valid(cv));
 
-       cv_wakeup_all(cv);
+       if (__predict_false(cv->cv_waiters > 0))
+               cv_wakeup_all(cv);
 }
 
 static void __noinline
@@ -351,6 +358,7 @@ cv_wakeup_all(kcondvar_t *cv)
                next = TAILQ_NEXT(l, l_sleepchain);
                swapin |= sleepq_remove(sq, l);
        }
+       cv->cv_waiters = 0;
        mutex_spin_exit(mp);
 
        /*
@@ -387,18 +395,7 @@ cv_wakeup(kcondvar_t *cv)
 bool
 cv_has_waiters(kcondvar_t *cv)
 {
-       bool result;
-       kmutex_t *mp;
-       sleepq_t *sq;
-
-       sq = CV_SLEEPQ(cv);
-       (void)sleeptab_lookup(&sleeptab, cv, &mp);
-
-       /* we can only get a valid result with the sleepq locked */
-       result = !TAILQ_EMPTY(sq);
-
-       mutex_spin_exit(mp);
-       return result;
+       return (cv->cv_waiters > 0);
 }
 
 /*
@@ -411,5 +408,11 @@ bool
 cv_is_valid(kcondvar_t *cv)
 {
 
-       return cv->cv_wmesg != deadcv && cv->cv_wmesg != NULL;
+       if (cv->cv_wmesg == deadcv || cv->cv_wmesg == NULL)
+               return false;
+
+       if (cv->cv_waiters < 0)
+               return false;
+
+       return true;
 }
Index: sys/condvar.h
===================================================================
RCS file: /cvsroot/src/sys/sys/condvar.h,v
retrieving revision 1.10
diff -u -p -r1.10 condvar.h
--- sys/condvar.h       31 May 2008 13:36:25 -0000      1.10
+++ sys/condvar.h       15 Jun 2008 14:46:49 -0000
@@ -44,6 +44,7 @@
 typedef struct kcondvar {
        void            *cv_opaque[2];  /* sleep queue */
        const char      *cv_wmesg;      /* description for /bin/ps */
+       int             cv_waiters;     /* number of waiters */
 } kcondvar_t;
 
 #ifdef _KERNEL