From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 7B2D9A0A0E; Wed, 28 Apr 2021 12:36:27 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E6567410E2; Wed, 28 Apr 2021 12:36:26 +0200 (CEST) Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by mails.dpdk.org (Postfix) with ESMTP id F2FF240147 for ; Wed, 28 Apr 2021 12:36:24 +0200 (CEST) Received: from DGGEMS412-HUB.china.huawei.com (unknown [172.30.72.60]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4FVZj56sm9zPtpk; Wed, 28 Apr 2021 18:33:13 +0800 (CST) Received: from [127.0.0.1] (10.40.190.165) by DGGEMS412-HUB.china.huawei.com (10.3.19.212) with Microsoft SMTP Server id 14.3.498.0; Wed, 28 Apr 2021 18:36:15 +0800 To: =?UTF-8?Q?Morten_Br=c3=b8rup?= CC: "Min Hu (Connor)" , , , , References: <1619597563-56716-1-git-send-email-humin29@huawei.com> <98CBD80474FA8B44BF855DF32C47DC35C61719@smartserver.smartshare.dk> From: fengchengwen Message-ID: <5c60416a-b4b3-d002-99a8-55d7e1e1c825@huawei.com> Date: Wed, 28 Apr 2021 18:36:15 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35C61719@smartserver.smartshare.dk> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [10.40.190.165] X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH] eal: fix use wrong time API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 2021/4/28 17:24, Morten Brørup wrote: >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Min Hu (Connor) >> Sent: Wednesday, April 28, 2021 10:13 AM >> >> From: Chengwen Feng >> >> Currently, the mp uses gettimeofday() API to get the time, and used as >> timeout parameter. >> >> But the time which gets from gettimeofday() API isn't monotonically >> increasing. The process may fail if the system time is changed. >> >> This fixes it by using clock_gettime() API with monotonic attribution. >> >> Fixes: 783b6e54971d ("eal: add synchronous multi-process >> communication") >> Fixes: f05e26051c15 ("eal: add IPC asynchronous request") >> Cc: stable@dpdk.org >> >> Signed-off-by: Chengwen Feng >> Signed-off-by: Min Hu (Connor) >> --- >> lib/eal/common/eal_common_proc.c | 45 +++++++++++++++++--------------- >> -------- >> 1 file changed, 19 insertions(+), 26 deletions(-) >> >> diff --git a/lib/eal/common/eal_common_proc.c >> b/lib/eal/common/eal_common_proc.c >> index 6d1af3c..7f08826 100644 >> --- a/lib/eal/common/eal_common_proc.c >> +++ b/lib/eal/common/eal_common_proc.c >> @@ -40,6 +40,12 @@ static char mp_dir_path[PATH_MAX]; /* The directory >> path for all mp sockets */ >> static pthread_mutex_t mp_mutex_action = PTHREAD_MUTEX_INITIALIZER; >> static char peer_name[PATH_MAX]; >> >> +#ifdef CLOCK_MONOTONIC_RAW /* Defined in glibc bits/time.h */ >> +#define CLOCK_TYPE_ID CLOCK_MONOTONIC_RAW >> +#else >> +#define CLOCK_TYPE_ID CLOCK_MONOTONIC >> +#endif > > Just out of curiosity: Why do you prefer CLOCK_MONOTONIC_RAW over CLOCK_MONOTONIC? > there may slightly difference, the CLOCK_MONOTONIC_RAW is totally local oscillator (pls see below link), just preferred in engineering practice. https://stackoverflow.com/questions/14270300/what-is-the-difference-between-clock-monotonic-clock-monotonic-raw >> + >> struct action_entry { >> TAILQ_ENTRY(action_entry) next; >> char action_name[RTE_MP_MAX_NAME_LEN]; >> @@ -490,14 +496,8 @@ async_reply_handle_thread_unsafe(void *arg) >> struct pending_request *req = (struct pending_request *)arg; >> enum async_action action; >> struct timespec ts_now; >> - struct timeval now; >> >> - if (gettimeofday(&now, NULL) < 0) { >> - RTE_LOG(ERR, EAL, "Cannot get current time\n"); >> - goto no_trigger; >> - } >> - ts_now.tv_nsec = now.tv_usec * 1000; >> - ts_now.tv_sec = now.tv_sec; >> + clock_gettime(CLOCK_TYPE_ID, &ts_now); >> >> action = process_async_request(req, &ts_now); >> >> @@ -896,6 +896,7 @@ mp_request_sync(const char *dst, struct rte_mp_msg >> *req, >> struct rte_mp_reply *reply, const struct timespec *ts) >> { >> int ret; >> + pthread_condattr_t attr; >> struct rte_mp_msg msg, *tmp; >> struct pending_request pending_req, *exist; >> >> @@ -904,7 +905,9 @@ mp_request_sync(const char *dst, struct rte_mp_msg >> *req, >> strlcpy(pending_req.dst, dst, sizeof(pending_req.dst)); >> pending_req.request = req; >> pending_req.reply = &msg; >> - pthread_cond_init(&pending_req.sync.cond, NULL); >> + pthread_condattr_init(&attr); >> + pthread_condattr_setclock(&attr, CLOCK_MONOTONIC); > > Shouldn't CLOCK_MONOTONIC be CLOCK_TYPE_ID here too? After reading the source code, it only support CLOCK_MONOTONIC and CLOCK_REALTIME (pls see below link), so cant't use CLOCK_TYPE_ID here. https://code.woboq.org/userspace/glibc/nptl/pthread_condattr_setclock.c.html#pthread_condattr_setclock will fix in v2 by make CLOCK_TYPE_ID equal CLOCK_MONOTONIC. thanks > >> + pthread_cond_init(&pending_req.sync.cond, &attr); >> >> exist = find_pending_request(dst, req->name); >> if (exist) { >> @@ -967,8 +970,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct >> rte_mp_reply *reply, >> int dir_fd, ret = -1; >> DIR *mp_dir; >> struct dirent *ent; >> - struct timeval now; >> - struct timespec end; >> + struct timespec now, end; >> const struct internal_config *internal_conf = >> eal_get_internal_configuration(); >> >> @@ -987,15 +989,10 @@ rte_mp_request_sync(struct rte_mp_msg *req, >> struct rte_mp_reply *reply, >> return -1; >> } >> >> - if (gettimeofday(&now, NULL) < 0) { >> - RTE_LOG(ERR, EAL, "Failed to get current time\n"); >> - rte_errno = errno; >> - goto end; >> - } >> - >> - end.tv_nsec = (now.tv_usec * 1000 + ts->tv_nsec) % 1000000000; >> + clock_gettime(CLOCK_TYPE_ID, &now); >> + end.tv_nsec = (now.tv_nsec + ts->tv_nsec) % 1000000000; >> end.tv_sec = now.tv_sec + ts->tv_sec + >> - (now.tv_usec * 1000 + ts->tv_nsec) / 1000000000; >> + (now.tv_nsec + ts->tv_nsec) / 1000000000; >> >> /* for secondary process, send request to the primary process >> only */ >> if (rte_eal_process_type() == RTE_PROC_SECONDARY) { >> @@ -1069,7 +1066,7 @@ rte_mp_request_async(struct rte_mp_msg *req, >> const struct timespec *ts, >> int dir_fd, ret = 0; >> DIR *mp_dir; >> struct dirent *ent; >> - struct timeval now; >> + struct timespec now; >> struct timespec *end; >> bool dummy_used = false; >> const struct internal_config *internal_conf = >> @@ -1086,11 +1083,6 @@ rte_mp_request_async(struct rte_mp_msg *req, >> const struct timespec *ts, >> return -1; >> } >> >> - if (gettimeofday(&now, NULL) < 0) { >> - RTE_LOG(ERR, EAL, "Failed to get current time\n"); >> - rte_errno = errno; >> - return -1; >> - } >> copy = calloc(1, sizeof(*copy)); >> dummy = calloc(1, sizeof(*dummy)); >> param = calloc(1, sizeof(*param)); >> @@ -1108,9 +1100,10 @@ rte_mp_request_async(struct rte_mp_msg *req, >> const struct timespec *ts, >> end = ¶m->end; >> reply = ¶m->user_reply; >> >> - end->tv_nsec = (now.tv_usec * 1000 + ts->tv_nsec) % 1000000000; >> + clock_gettime(CLOCK_TYPE_ID, &now); >> + end->tv_nsec = (now.tv_nsec + ts->tv_nsec) % 1000000000; >> end->tv_sec = now.tv_sec + ts->tv_sec + >> - (now.tv_usec * 1000 + ts->tv_nsec) / 1000000000; >> + (now.tv_nsec + ts->tv_nsec) / 1000000000; >> reply->nb_sent = 0; >> reply->nb_received = 0; >> reply->msgs = NULL; >> -- >> 2.7.4 >> >