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(-) (limited to 'runtime') 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 From e30fe2842e3ce9f06098c18f0f962596059684ca Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Tue, 30 Sep 2008 15:57:10 +0200 Subject: prevent getnameinfo() from being cancelled ... but removed the mutex, as the problem seems to be in cancel processing, so the mutex is no longer necessary --- runtime/net.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'runtime') diff --git a/runtime/net.c b/runtime/net.c index 892edf4a..31b02f1a 100644 --- a/runtime/net.c +++ b/runtime/net.c @@ -685,11 +685,14 @@ mygetnameinfo(const struct sockaddr *sa, socklen_t salen, char *serv, size_t servlen, int flags) { static pthread_mutex_t mut = PTHREAD_MUTEX_INITIALIZER; + int iCancelStateSave; int i; - pthread_mutex_lock(&mut); + pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &iCancelStateSave); + //pthread_mutex_lock(&mut); i = getnameinfo(sa, salen, host, hostlen, serv, servlen, flags); - pthread_mutex_unlock(&mut); + //pthread_mutex_unlock(&mut); + pthread_setcancelstate(iCancelStateSave, NULL); return i; } -- cgit From 925bea71772a23bc184a1da554fe310f39103b0c Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Wed, 1 Oct 2008 19:01:53 +0200 Subject: bugfix: segfault when main queue was in direct mode --- runtime/msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'runtime') diff --git a/runtime/msg.c b/runtime/msg.c index d5e1fde9..6c1b9ff3 100644 --- a/runtime/msg.c +++ b/runtime/msg.c @@ -342,8 +342,8 @@ CODESTARTobjDestruct(msg) MsgUnlock(pThis); funcDeleteMutex(pThis); } else { - pThis = NULL; /* tell framework not to destructing the object! */ MsgUnlock(pThis); + pThis = NULL; /* tell framework not to destructing the object! */ } ENDobjDestruct(msg) -- cgit From 86f76c1299b7827549a0cd754960347af627e18d Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Thu, 2 Oct 2008 10:44:44 +0200 Subject: removed no longer needed mutex calls problem source is that getnameinfo() is not cancel-safe, not that it is not thread-safe. It is now guarded against cancellation. --- runtime/net.c | 3 --- 1 file changed, 3 deletions(-) (limited to 'runtime') diff --git a/runtime/net.c b/runtime/net.c index 31b02f1a..85d6813b 100644 --- a/runtime/net.c +++ b/runtime/net.c @@ -684,14 +684,11 @@ 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 iCancelStateSave; int i; pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &iCancelStateSave); - //pthread_mutex_lock(&mut); i = getnameinfo(sa, salen, host, hostlen, serv, servlen, flags); - //pthread_mutex_unlock(&mut); pthread_setcancelstate(iCancelStateSave, NULL); return i; } -- cgit From 1908bae50491624d270c73609ea45634c15246fd Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Thu, 2 Oct 2008 11:01:08 +0200 Subject: fixed git merge bug (duplicate code could not be detected) --- runtime/net.c | 19 ------------------- 1 file changed, 19 deletions(-) (limited to 'runtime') diff --git a/runtime/net.c b/runtime/net.c index 38b41389..44c9008a 100644 --- a/runtime/net.c +++ b/runtime/net.c @@ -695,25 +695,6 @@ 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) -{ - int iCancelStateSave; - int i; - - pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &iCancelStateSave); - i = getnameinfo(sa, salen, host, hostlen, serv, servlen, flags); - pthread_setcancelstate(iCancelStateSave, NULL); - 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 -- cgit