Date: Tue, 20 Oct 1998 11:33:19 -0400 (EDT) From: Alfred Perlstein <bright@hotjobs.com> To: "Richard Seaman, Jr." <lists@tar.com> Cc: HighWind Software Information <info@highwind.com>, "current@freebsd.org" <current@FreeBSD.ORG> Subject: Re: Another Serious libc_r problem Message-ID: <Pine.BSF.4.05.9810201122440.18282-100000@porkfriedrice.ny.genx.net> In-Reply-To: <199810201330.IAA09508@ns.tar.com>
next in thread | previous in thread | raw e-mail | index | archive | help
I think this patch may be over corecting the problem by not locking around the mutex lock after being woken up. Take a look at what i tried, it "batches" wakeups on a cond_broadcast as to avoid a tight loop on spinlock aquiring/release and also seems to fix the problem. It would be great to have an extensive test suite that stressed pthread functionality though. The patch is at the end of the message. Alfred Perlstein - Programmer, HotJobs Inc. - www.hotjobs.com -- There are operating systems, and then there's FreeBSD. -- http://www.freebsd.org/ 3.0-current On Tue, 20 Oct 1998, Richard Seaman, Jr. wrote: > On Tue, 20 Oct 98 08:01:12 -0500, Richard Seaman, Jr. wrote: > > >I will try to look at this later, if I have time. However, there are > >others who know a lot more about this than I do who could probably > >do a better fix. > > You could try the following patch. However, someone else should look at > it too before its committed. > this looks wrong, although mutex aquiring is atomic we really should lock on the conditional, no? > *** 148,154 **** > --- 146,156 ---- > * Queue the running thread for the condition > * variable: > */ > + > + /* Lock the condition variable structure: */ > + _SPINLOCK(&(*cond)->lock); > _thread_queue_enq(&(*cond)->c_queue, _thread_run); > + _SPINUNLOCK(&(*cond)->lock); > > /* Unlock the mutex: */ > pthread_mutex_unlock(mutex); > *************** > *** 156,171 **** > /* Wait forever: */ > _thread_run->wakeup_time.tv_sec = -1; > > - /* Unlock the condition variable structure: */ > - _SPINUNLOCK(&(*cond)->lock); > > /* Schedule the next thread: */ > _thread_kern_sched_state(PS_COND_WAIT, > __FILE__, __LINE__); > > - /* Lock the condition variable structure: */ > - _SPINLOCK(&(*cond)->lock); > - > /* Lock the mutex: */ > rval = pthread_mutex_lock(mutex); > break; --- uthread_cond.c Tue Oct 20 11:31:31 1998 +++ /root/uthread_fix.c Tue Oct 20 11:31:12 1998 @@ -37,6 +37,11 @@ #include <pthread.h> #include "pthread_private.h" +/* number of signals to batch for pthread_cond_broadcast() to avoid + * expensive looping on a spinlock + */ +#define _PTHREAD_MBROADCAST 10 + int pthread_cond_init(pthread_cond_t * cond, const pthread_condattr_t * cond_attr) { @@ -164,7 +169,6 @@ __FILE__, __LINE__); /* Lock the condition variable structure: */ - _SPINLOCK(&(*cond)->lock); /* Lock the mutex: */ rval = pthread_mutex_lock(mutex); @@ -174,11 +178,11 @@ default: /* Return an invalid argument error: */ rval = EINVAL; + _SPINUNLOCK(&(*cond)->lock); break; } /* Unlock the condition variable structure: */ - _SPINUNLOCK(&(*cond)->lock); } /* Return the completion status: */ @@ -284,21 +288,19 @@ /* Fast condition variable: */ case COND_TYPE_FAST: /* Bring the next thread off the condition queue: */ - if ((pthread = _thread_queue_deq(&(*cond)->c_queue)) != NULL) { + pthread = _thread_queue_deq(&(*cond)->c_queue); + _SPINUNLOCK(&(*cond)->lock); /* Allow the thread to run: */ - PTHREAD_NEW_STATE(pthread,PS_RUNNING); - } + if(pthread) PTHREAD_NEW_STATE(pthread,PS_RUNNING); break; /* Trap invalid condition variable types: */ default: /* Return an invalid argument error: */ rval = EINVAL; + _SPINUNLOCK(&(*cond)->lock); break; } - - /* Unlock the condition variable structure: */ - _SPINUNLOCK(&(*cond)->lock); } /* Return the completion status: */ @@ -312,12 +314,12 @@ int status; pthread_t pthread; + pthread_t nospin[_PTHREAD_MBROADCAST]; + int i,j; + if (cond == NULL || *cond == NULL) rval = EINVAL; else { - /* Lock the condition variable structure: */ - _SPINLOCK(&(*cond)->lock); - /* Process according to condition variable type: */ switch ((*cond)->c_type) { /* Fast condition variable: */ @@ -325,12 +327,23 @@ /* * Enter a loop to bring all threads off the * condition queue: + * to avoid heavy spinlocking we "batch" unblocking */ - while ((pthread = - _thread_queue_deq(&(*cond)->c_queue)) != NULL) { - /* Allow the thread to run: */ - PTHREAD_NEW_STATE(pthread,PS_RUNNING); - } + do{ + /* lock for queue access */ + _SPINLOCK(&(*cond)->lock); + for( i = 0; i < _PTHREAD_MBROADCAST ; i++){ + /* fill array, early out on NULL */ + if( (nospin[i] = _thread_queue_deq(&(*cond)->c_queue) ) == NULL){ + break; + } + } + _SPINUNLOCK(&(*cond)->lock); + /* unlock to prevent deadlock */ + for( j = 0; j < i ; j++){ + PTHREAD_NEW_STATE(nospin[j],PS_RUNNING); + } + }while(nospin[j]); break; /* Trap invalid condition variable types: */ @@ -340,8 +353,6 @@ break; } - /* Unlock the condition variable structure: */ - _SPINUNLOCK(&(*cond)->lock); } /* Return the completion status: */ To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.4.05.9810201122440.18282-100000>