From 5a1a73b4326d97789b5640225be0e25cb8dd8de7 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Tue, 30 Sep 2008 14:20:01 +0200 Subject: improved threading - changed sequence when awakening thread - removed no longer needed condition variable - EXPERIMENTALLY added mutex guarding to hostname lookups this is to be removed if it does not have any verifyable useful effect --- runtime/net.c | 26 ++++++++++++++++++++++---- runtime/queue.c | 7 ++++++- runtime/wti.c | 9 ++++----- runtime/wti.h | 1 - runtime/wtp.c | 5 +++-- 5 files changed, 35 insertions(+), 13 deletions(-) diff --git a/runtime/net.c b/runtime/net.c index f5b8f46a..892edf4a 100644 --- a/runtime/net.c +++ b/runtime/net.c @@ -674,6 +674,25 @@ finalize_it: } + +/* This is a synchronized getnameinfo() version, because we learned + * (via drd/valgrind) that getnameinfo() seems to have some multi-threading + * issues. -- rgerhards, 2008-09-30 + */ +static int +mygetnameinfo(const struct sockaddr *sa, socklen_t salen, + char *host, size_t hostlen, + char *serv, size_t servlen, int flags) +{ + static pthread_mutex_t mut = PTHREAD_MUTEX_INITIALIZER; + int i; + + pthread_mutex_lock(&mut); + i = getnameinfo(sa, salen, host, hostlen, serv, servlen, flags); + pthread_mutex_unlock(&mut); + return i; +} + /* Print an allowed sender list. The caller must tell us which one. * iListToPrint = 1 means UDP, 2 means TCP * rgerhards, 2005-09-27 @@ -708,7 +727,7 @@ void PrintAllowedSenders(int iListToPrint) if (F_ISSET(pSender->allowedSender.flags, ADDR_NAME)) dbgprintf ("\t%s\n", pSender->allowedSender.addr.HostWildcard); else { - if(getnameinfo (pSender->allowedSender.addr.NetAddr, + if(mygetnameinfo (pSender->allowedSender.addr.NetAddr, SALEN(pSender->allowedSender.addr.NetAddr), (char*)szIP, 64, NULL, 0, NI_NUMERICHOST) == 0) { dbgprintf ("\t%s/%u\n", szIP, pSender->SignificantBits); @@ -956,7 +975,6 @@ should_use_so_bsdcompat(void) #define SO_BSDCOMPAT 0 #endif - /* get the hostname of the message source. This was originally in cvthname() * but has been moved out of it because of clarity and fuctional separation. * It must be provided by the socket we received the message on as well as @@ -982,7 +1000,7 @@ gethname(struct sockaddr_storage *f, uchar *pszHostFQDN, uchar *ip) assert(f != NULL); assert(pszHostFQDN != NULL); - error = getnameinfo((struct sockaddr *)f, SALEN((struct sockaddr *)f), + error = mygetnameinfo((struct sockaddr *)f, SALEN((struct sockaddr *)f), (char*) ip, NI_MAXHOST, NULL, 0, NI_NUMERICHOST); if (error) { @@ -997,7 +1015,7 @@ gethname(struct sockaddr_storage *f, uchar *pszHostFQDN, uchar *ip) sigaddset(&nmask, SIGHUP); pthread_sigmask(SIG_BLOCK, &nmask, &omask); - error = getnameinfo((struct sockaddr *)f, SALEN((struct sockaddr *) f), + error = mygetnameinfo((struct sockaddr *)f, SALEN((struct sockaddr *) f), (char*)pszHostFQDN, NI_MAXHOST, NULL, 0, NI_NAMEREQD); if (error == 0) { diff --git a/runtime/queue.c b/runtime/queue.c index 3fae4aa7..76c2f10f 100644 --- a/runtime/queue.c +++ b/runtime/queue.c @@ -1430,8 +1430,13 @@ queueDequeueConsumable(queue_t *pThis, wti_t *pWti, int iCancelStateSave) pthread_cond_broadcast(&pThis->belowLightDlyWtrMrk); } - d_pthread_mutex_unlock(pThis->mut); + /* rgerhards, 2008-09-30: I reversed the order of cond_signal und mutex_unlock + * as of the pthreads recommendation on predictable scheduling behaviour. I don't see + * any problems caused by this, but I add this comment in case some will be seen + * in the next time. + */ pthread_cond_signal(&pThis->notFull); + d_pthread_mutex_unlock(pThis->mut); pthread_setcancelstate(iCancelStateSave, NULL); /* WE ARE NO LONGER PROTECTED BY THE MUTEX */ diff --git a/runtime/wti.c b/runtime/wti.c index 13554232..365b25d5 100644 --- a/runtime/wti.c +++ b/runtime/wti.c @@ -113,6 +113,9 @@ wtiSetState(wti_t *pThis, qWrkCmd_t tCmd, int bActiveOnly, int bLockMutex) wtiGetDbgHdr(pThis), tCmd, pThis->tCurrCmd); } else { dbgprintf("%s: receiving command %d\n", wtiGetDbgHdr(pThis), tCmd); + /* we could replace this with a simple if, but we leave the switch in in case we need + * to add something at a later stage. -- rgerhards, 2008-09-30 + */ switch(tCmd) { case eWRKTHRD_TERMINATING: /* TODO: re-enable meaningful debug msg! (via function callback?) @@ -123,10 +126,8 @@ wtiSetState(wti_t *pThis, qWrkCmd_t tCmd, int bActiveOnly, int bLockMutex) pthread_cond_signal(&pThis->condExitDone); dbgprintf("%s: worker terminating\n", wtiGetDbgHdr(pThis)); break; - case eWRKTHRD_RUNNING: - pthread_cond_signal(&pThis->condInitDone); - break; /* these cases just to satisfy the compiler, we do (yet) not act an them: */ + case eWRKTHRD_RUNNING: case eWRKTHRD_STOPPED: case eWRKTHRD_RUN_CREATED: case eWRKTHRD_RUN_INIT: @@ -190,7 +191,6 @@ CODESTARTobjDestruct(wti) d_pthread_mutex_unlock(&pThis->mut); /* actual destruction */ - pthread_cond_destroy(&pThis->condInitDone); pthread_cond_destroy(&pThis->condExitDone); pthread_mutex_destroy(&pThis->mut); @@ -202,7 +202,6 @@ ENDobjDestruct(wti) /* Standard-Constructor for the wti object */ BEGINobjConstruct(wti) /* be sure to specify the object type also in END macro! */ - pthread_cond_init(&pThis->condInitDone, NULL); pthread_cond_init(&pThis->condExitDone, NULL); pthread_mutex_init(&pThis->mut, NULL); ENDobjConstruct(wti) diff --git a/runtime/wti.h b/runtime/wti.h index b3d92473..0cd6744e 100644 --- a/runtime/wti.h +++ b/runtime/wti.h @@ -35,7 +35,6 @@ typedef struct wti_s { qWrkCmd_t tCurrCmd; /* current command to be carried out by worker */ obj_t *pUsrp; /* pointer to an object meaningful for current user pointer (e.g. queue pUsr data elemt) */ wtp_t *pWtp; /* my worker thread pool (important if only the work thread instance is passed! */ - pthread_cond_t condInitDone; /* signaled when the thread startup is done (once per thread existance) */ pthread_cond_t condExitDone; /* signaled when the thread exit is done (once per thread existance) */ pthread_mutex_t mut; int bShutdownRqtd; /* shutdown for this thread requested? 0 - no , 1 - yes */ diff --git a/runtime/wtp.c b/runtime/wtp.c index 54f12b2b..734c8d57 100644 --- a/runtime/wtp.c +++ b/runtime/wtp.c @@ -304,11 +304,12 @@ wtpShutdownAll(wtp_t *pThis, wtpState_t tShutdownCmd, struct timespec *ptTimeout rsRetVal wtpSignalWrkrTermination(wtp_t *pThis) { DEFiRet; - /* I leave the mutex code here out as it give as deadlocks. I think it is not really + /* I leave the mutex code here out as it gives us deadlocks. I think it is not really * needed and we are on the safe side. I leave this comment in if practice proves us - * wrong. The whole thing should be removed after half a your or year if we see there + * wrong. The whole thing should be removed after half a year or year if we see there * actually is no issue (or revisit it from a theoretical POV). * rgerhards, 2008-01-28 + * revisited 2008-09-30, still a bit unclear, leave in */ /*TODO: mutex or not mutex, that's the question ;)DEFVARS_mutexProtection;*/ -- cgit