Ok, it could be done in a separate ticket. But if this is a temporary solution then I think you shouldn't add a common helper function for reading the system clock as a time_t. It could encourage others to start using a low-resolution clock for time measurements. Maybe better to put it in ImmModel.cc and remove it when you have implemented the other ticket?
regards, Anders Widell On 03/16/2016 10:52 AM, Zoran Milinkovic wrote: > Hi Anders, > > Your comment can be done in a new ticket. > Ticket #1617 is to switch from system to monotonic time. > > Thanks, > Zoran > > > -----Original Message----- > From: Anders Widell > Sent: Wednesday, March 16, 2016 10:44 AM > To: [email protected]; Zoran Milinkovic; Hung Duc Nguyen > Cc: [email protected] > Subject: Re: [PATCH 1 of 1] imm: changing from system time to monotonic time > [#1617] > > Hi! > > One comment here: wouldn't it be better to change the type of mCreateTime to > "struct timespec"? By using time_t we get a very low resolution of time > measurements. A timeout of six seconds could result in an actual timeout of > only five seconds, if we happen to read the system clock just before the > second dial moves to the next second. By using struct timespec instead of > time_t, you don't have to introduce new helper functions and the resolution > will be nanoseconds instead of seconds. > > Another thing to note is that I have introduced C++ helper functions for > dealing with struct timespec in ticket [#836]. These include operators for > arithmetic operations and comparisons. Another option in C++ is to use > std::chrono::steady_clock from the standard library, if you think that is > easier. > > regards, > Anders Widell > > On 03/16/2016 08:36 AM, [email protected] wrote: >> osaf/libs/core/common/include/osaf_time.h | 20 +++++++++++++++++++ >> osaf/services/saf/immsv/immnd/ImmModel.cc | 29 >> +++++++++++++++------------ >> osaf/services/saf/immsv/immnd/ImmSearchOp.hh | 3 +- >> osaf/services/saf/immsv/immnd/immnd_evt.c | 7 +++-- >> osaf/services/saf/immsv/immnd/immnd_proc.c | 6 +++- >> 5 files changed, 46 insertions(+), 19 deletions(-) >> >> >> diff --git a/osaf/libs/core/common/include/osaf_time.h >> b/osaf/libs/core/common/include/osaf_time.h >> --- a/osaf/libs/core/common/include/osaf_time.h >> +++ b/osaf/libs/core/common/include/osaf_time.h >> @@ -89,6 +89,18 @@ extern void osaf_nanosleep(const struct >> static inline void osaf_clock_gettime(clockid_t i_clk_id, >> struct timespec* o_ts); >> >> + >> +/** >> + * @brief Get the time in seconds >> + * >> + * This is a convenience function that behaves exactly like the POSIX >> +function >> + * clock_gettime(3P), except that it will abort the process instead >> +of returning >> + * an error code in case of a failure. The Output vlaue passed will be in >> seconds. >> + */ >> +static inline void osaf_clock_gettime_sec(clockid_t i_clk_id, >> + time_t* o_t); >> + >> + >> /** >> * @brief Normalize a timespec structure. >> * >> @@ -257,6 +269,14 @@ static inline void osaf_clock_gettime(cl >> if (clock_gettime(i_clk_id, o_ts) != 0) osaf_abort(i_clk_id); >> } >> >> +static inline void osaf_clock_gettime_sec(clockid_t i_clk_id, >> + time_t* o_t) >> +{ >> + struct timespec o_ts; >> + if (clock_gettime(i_clk_id, &o_ts) != 0) osaf_abort(i_clk_id); >> + *o_t = o_ts.tv_sec; >> +} >> + >> static inline void osaf_normalize_timespec(const struct timespec* i_ts, >> struct timespec* o_nrm) >> { >> diff --git a/osaf/services/saf/immsv/immnd/ImmModel.cc >> b/osaf/services/saf/immsv/immnd/ImmModel.cc >> --- a/osaf/services/saf/immsv/immnd/ImmModel.cc >> +++ b/osaf/services/saf/immsv/immnd/ImmModel.cc >> @@ -27,6 +27,7 @@ >> #include "immnd.h" >> #include "osaf_unicode.h" >> #include "osaf_extended_name.h" >> +#include "osaf_time.h" >> >> // Local types >> #define DEFAULT_TIMEOUT_SEC 6 /* Should be saImmOiTimeout in >> SaImmMngt */ @@ -44,8 +45,7 @@ struct ContinuationInfo2 >> ContinuationInfo2():mCreateTime(0), mConn(0), mTimeout(0), >> mImplId(0){} >> ContinuationInfo2(SaUint32T conn, SaUint32T timeout):mConn(conn), >> mTimeout(timeout), >> mImplId(0) >> - {mCreateTime = time(NULL);osafassert(mCreateTime >= ((time_t) 0));} >> - >> + {osaf_clock_gettime_sec(CLOCK_MONOTONIC, >> + &mCreateTime);osafassert(mCreateTime >= ((time_t) 0));} >> time_t mCreateTime; >> SaUint32T mConn; >> SaUint32T mTimeout; //0=> no timeout. Otherwise timeout in SECONDS. >> @@ -1121,7 +1121,8 @@ immModel_cleanTheBasement(IMMND_CB *cb, >> osafassert(ix==(*pbePrtoReqArrSize)); >> } >> >> - time_t now = time(NULL); >> + time_t now; >> + osaf_clock_gettime_sec(CLOCK_MONOTONIC, &now); >> osafassert(now >= ((time_t) 0)); >> time_t nextSearch = 0; >> time_t opSearchTime; >> @@ -5416,7 +5417,7 @@ ImmModel::ccbApply(SaUint32T ccbId, >> implAssoc->mWaitForImplAck = true; >> implAssoc->mContinuationId = sLastContinuationId;/* >> incremented above */ >> if(ccb->mWaitStartTime == 0) { >> - ccb->mWaitStartTime = time(NULL); >> + osaf_clock_gettime_sec(CLOCK_MONOTONIC, >> + &ccb->mWaitStartTime); >> osafassert(ccb->mWaitStartTime >= ((time_t) 0)); >> TRACE("Wait timer for completed started for ccb:%u", >> ccb->mId); >> @@ -6263,7 +6264,7 @@ ImmModel::ccbTerminate(SaUint32T ccbId) >> /* Retain the ccb info to allow ccb result recovery. */ >> >> if(ccb->mWaitStartTime == 0) { >> - ccb->mWaitStartTime = time(NULL); >> + osaf_clock_gettime_sec(CLOCK_MONOTONIC, >> + &ccb->mWaitStartTime); >> osafassert(ccb->mWaitStartTime >= ((time_t) 0)); >> TRACE_5("Ccb Wait-time for GC set. State: %u/%s", ccb->mState, >> (ccb->mState == IMM_CCB_COMMITTED)?"COMMITTED": >> @@ -8037,7 +8038,7 @@ SaAisErrorT ImmModel::ccbObjectCreate(Im >> TRACE_5("THERE IS AN IMPLEMENTER %u conn:%u node:%x >> name:%s\n", >> object->mImplementer->mId, *implConn, *implNodeId, >> object->mImplementer->mImplementerName.c_str()); >> - ccb->mWaitStartTime = time(NULL); >> + osaf_clock_gettime_sec(CLOCK_MONOTONIC, >> + &ccb->mWaitStartTime); >> osafassert(ccb->mWaitStartTime >= ((time_t) 0)); >> } else if(className == immMngtClass) { >> if(sImmNodeState == IMM_NODE_LOADING) { @@ -9239,7 >> +9240,7 @@ ImmModel::ccbObjectModify(const ImmsvOmC >> object->mImplementer->mId, *implConn, *implNodeId, >> object->mImplementer->mImplementerName.c_str()); >> >> - ccb->mWaitStartTime = time(NULL); >> + osaf_clock_gettime_sec(CLOCK_MONOTONIC, >> + &ccb->mWaitStartTime); >> osafassert(ccb->mWaitStartTime >= ((time_t) 0)); >> } else if(ccb->mCcbFlags & SA_IMM_CCB_REGISTERED_OI) { >> if((object->mImplementer == NULL) && @@ -9808,7 +9809,7 >> @@ ImmModel::deleteObject(ObjectMap::iterat >> >> SaUint32T implConn = oi->second->mImplementer->mConn; >> >> - ccb->mWaitStartTime = time(NULL); >> + osaf_clock_gettime_sec(CLOCK_MONOTONIC, >> + &ccb->mWaitStartTime); >> osafassert(ccb->mWaitStartTime >= ((time_t) 0)); >> /* TODO: Resetting the ccb timer for each deleted object here. >> Not so efficient. Should set it only when all objects >> @@ -10150,7 +10151,7 @@ ImmModel::ccbWaitForCompletedAck(SaUint3 >> objects write locked by the ccb) until we know the outcome. >> Restart the timer to catch ccbs hung waiting on PBE. >> */ >> - ccb->mWaitStartTime = time(NULL); >> + osaf_clock_gettime_sec(CLOCK_MONOTONIC, >> + &ccb->mWaitStartTime); >> osafassert(ccb->mWaitStartTime >= ((time_t) 0)); >> return true; /* Wait for PBE commit*/ >> } else { >> @@ -12825,7 +12826,8 @@ ImmModel::getOldCriticalCcbs(IdVector& c >> *pbeConnPtr = 0; >> *pbeIdPtr = 0; >> CcbVector::iterator i; >> - time_t now = time(NULL); >> + time_t now; >> + osaf_clock_gettime_sec(CLOCK_MONOTONIC, &now); >> osafassert(now >= ((time_t) 0)); >> for(i=sCcbVector.begin(); i!=sCcbVector.end(); ++i) { >> if((*i)->mState == IMM_CCB_CRITICAL && @@ -13046,7 +13048,8 >> @@ ImmModel::cleanTheBasement(InvocVector& >> InvocVector& searchReqs, IdVector& ccbs, IdVector& pbePrtoReqs, >> bool iAmCoord) >> { >> - time_t now = time(NULL); >> + time_t now; >> + osaf_clock_gettime_sec(CLOCK_MONOTONIC, &now); >> osafassert(now >= ((time_t) 0)); >> ContinuationMap2::iterator ci2; >> ImplementerEvtMap::iterator iem; @@ -13455,7 +13458,7 @@ >> ImmModel::implementerSet(const IMMSV_OCT >> ccb->mImplementers[info->mId] = >> oldImplAssoc; >> TRACE_7("Replaced implid %u with %u", >> oldImplId, info->mId); >> ccb->mPbeRestartId = info->mId; >> - ccb->mWaitStartTime = time(NULL);/*Reset >> timer on new impl*/ >> + >> + osaf_clock_gettime_sec(CLOCK_MONOTONIC, >> + &ccb->mWaitStartTime);/*Reset timer on new impl*/ >> osafassert(ccb->mWaitStartTime >= >> ((time_t) 0)); >> /* Can only be one PBE impl asoc*/ >> break; /* out of for(isi = >> ccb->mImplementers....*/ @@ -17877,7 +17880,7 @@ >> ImmModel::finalizeSync(ImmsvOmFinalizeSy >> newCcb->mOriginatingConn = 0; >> newCcb->mVeto = SA_AIS_OK; >> newCcb->mState = (ImmCcbState) (prt45allowed ? >> ol->ccbState : (ol->ccbState + 2)); >> - newCcb->mWaitStartTime = time(NULL); >> + osaf_clock_gettime_sec(CLOCK_MONOTONIC, >> + &newCcb->mWaitStartTime); >> if(newCcb->mWaitStartTime < ((time_t) 0)) { >> LOG_ER("newCcb->mWaitStartTime < 0"); >> err = SA_AIS_ERR_FAILED_OPERATION; diff --git >> a/osaf/services/saf/immsv/immnd/ImmSearchOp.hh >> b/osaf/services/saf/immsv/immnd/ImmSearchOp.hh >> --- a/osaf/services/saf/immsv/immnd/ImmSearchOp.hh >> +++ b/osaf/services/saf/immsv/immnd/ImmSearchOp.hh >> @@ -23,6 +23,7 @@ >> #include <string> >> #include <list> >> #include <time.h> >> +#include "osaf_time.h" >> >> >> struct SearchAttribute >> @@ -85,7 +86,7 @@ public: >> bool isAccessor() {return mIsAccessor;} >> bool isNonExtendedNameSet() {return mNonExtendedName;} >> time_t getLastSearchTime() { return mLastSearch; } >> - void updateSearchTime() { mLastSearch = time(NULL); } >> + void updateSearchTime() { >> osaf_clock_gettime_sec(CLOCK_MONOTONIC, &mLastSearch); } >> void* syncOsi; >> void* attrNameList; >> void* classInfo; >> diff --git a/osaf/services/saf/immsv/immnd/immnd_evt.c >> b/osaf/services/saf/immsv/immnd/immnd_evt.c >> --- a/osaf/services/saf/immsv/immnd/immnd_evt.c >> +++ b/osaf/services/saf/immsv/immnd/immnd_evt.c >> @@ -33,6 +33,7 @@ >> #include "ncssysf_mem.h" >> #include "mds_papi.h" >> #include "osaf_extended_name.h" >> +#include "osaf_time.h" >> >> /* Adjust to 90% of MDS_DIRECT_BUF_MAXSIZE */ >> #define IMMND_SEARCH_BUNDLE_SIZE ((MDS_DIRECT_BUF_MAXSIZE / 100) * >> 90) @@ -8384,7 +8385,7 @@ uint32_t immnd_evt_proc_abort_sync(IMMND >> cb->mState = IMM_SERVER_LOADING_PENDING; >> LOG_WA("SERVER STATE: IMM_SERVER_SYNC_CLIENT --> >> IMM_SERVER_LOADING_PENDING (sync aborted)"); >> cb->mStep = 0; >> - cb->mJobStart = time(NULL); >> + osaf_clock_gettime_sec(CLOCK_MONOTONIC, &cb->mJobStart); >> osafassert(cb->mJobStart >= ((time_t) 0)); >> cb->mMyEpoch = 0; >> cb->mSync = false; >> @@ -8519,7 +8520,7 @@ static uint32_t immnd_evt_proc_start_syn >> immModel_setScAbsenceAllowed(cb); >> } else if ((cb->mState == IMM_SERVER_SYNC_CLIENT) && >> (immnd_syncComplete(cb, SA_FALSE, cb->mStep))) { >> cb->mStep = 0; >> - cb->mJobStart = time(NULL); >> + osaf_clock_gettime_sec(CLOCK_MONOTONIC, &cb->mJobStart); >> osafassert(cb->mJobStart >= ((time_t) 0)); >> cb->mState = IMM_SERVER_READY; >> immnd_ackToNid(NCSCC_RC_SUCCESS); >> @@ -8653,7 +8654,7 @@ static uint32_t immnd_evt_proc_loading_o >> LOG_NO("SERVER STATE: IMM_SERVER_LOADING_PENDING --> >> IMM_SERVER_LOADING_CLIENT (materialized by proc_loading_ok)"); >> cb->mState = IMM_SERVER_LOADING_CLIENT; >> cb->mStep = 0; >> - cb->mJobStart = time(NULL); >> + osaf_clock_gettime_sec(CLOCK_MONOTONIC, &cb->mJobStart); >> osafassert(cb->mJobStart >= ((time_t) 0)); >> cb->mAccepted = true; >> if(cb->mCanBeCoord) {cb->mIsOtherScUp = true;} diff >> --git >> a/osaf/services/saf/immsv/immnd/immnd_proc.c >> b/osaf/services/saf/immsv/immnd/immnd_proc.c >> --- a/osaf/services/saf/immsv/immnd/immnd_proc.c >> +++ b/osaf/services/saf/immsv/immnd/immnd_proc.c >> @@ -35,6 +35,7 @@ >> #include "immnd.h" >> #include "immsv_api.h" >> #include "immnd_init.h" >> +#include "osaf_time.h" >> >> static const char *loaderBase = "osafimmloadd"; >> static const char *pbeBase = "osafimmpbed"; @@ -597,7 +598,7 @@ void >> immnd_announceDump(IMMND_CB *cb) >> /* Reset jobStart timer to delay potential start of sync. >> Reduces risk of epoch race with dump. >> */ >> - cb->mJobStart = time(NULL); >> + osaf_clock_gettime_sec(CLOCK_MONOTONIC, &cb->mJobStart); >> osafassert(cb->mJobStart >= ((time_t) 0)); >> } >> } >> @@ -1648,7 +1649,8 @@ uint32_t immnd_proc_server(uint32_t *tim >> uint32_t rc = NCSCC_RC_SUCCESS; >> int32_t coord, newEpoch; >> int32_t printFrq = (*timeout > 100) ? 5 : 50; >> - time_t now = time(NULL); >> + time_t now; >> + osaf_clock_gettime_sec(CLOCK_MONOTONIC, &now); >> osafassert(now >= ((time_t) 0)); >> uint32_t jobDuration = (uint32_t) now - cb->mJobStart; >> SaBoolT pbeImmndDeadlock=SA_FALSE; ------------------------------------------------------------------------------ Transform Data into Opportunity. Accelerate data analysis in your applications with Intel Data Analytics Acceleration Library. Click to learn more. http://pubads.g.doubleclick.net/gampad/clk?id=278785231&iu=/4140 _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
