DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Morten Brørup" <mb@smartsharesystems.com>
To: "fengchengwen" <fengchengwen@huawei.com>
Cc: "Min Hu (Connor)" <humin29@huawei.com>, <dev@dpdk.org>,
	<ferruh.yigit@intel.com>, <jerinj@marvell.com>,
	<skori@marvell.com>
Subject: Re: [dpdk-dev] [PATCH] eal: fix use wrong time API
Date: Wed, 28 Apr 2021 12:59:26 +0200	[thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35C6171A@smartserver.smartshare.dk> (raw)
In-Reply-To: <5c60416a-b4b3-d002-99a8-55d7e1e1c825@huawei.com>

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of fengchengwen
> Sent: Wednesday, April 28, 2021 12:36 PM
> 
> 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 <fengchengwen@huawei.com>
> >>
> >> 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 <fengchengwen@huawei.com>
> >> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> >> ---
> >>  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

Interesting link! Following the treads there, it looks like CLOCK_MONOTONIC had a bug in some old kernel versions, where in certain circumstances it could jump slightly backwards.

That bug seems to have been fixed, so CLOCK_MONOTONIC should be safe to use. Source: https://bugzilla.redhat.com/show_bug.cgi?id=448449

> 
> >> +
> >>  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.

OK, then just get rid of the CLOCK_TYPE_ID definition and use CLOCK_MONOTONIC instead.

> 
> 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 = &param->end;
> >>  	reply = &param->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
> >>
> >
> 


  reply	other threads:[~2021-04-28 10:59 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-28  8:12 Min Hu (Connor)
2021-04-28  9:24 ` Morten Brørup
2021-04-28 10:36   ` fengchengwen
2021-04-28 10:59     ` Morten Brørup [this message]
2021-04-29  2:12       ` Min Hu (Connor)
2021-04-29  2:10 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
2021-04-29  7:12   ` Morten Brørup
2021-05-04 16:49   ` Thomas Monjalon
2021-05-04 19:12     ` Morten Brørup
2021-05-05  6:14       ` Thomas Monjalon
2021-05-05  6:26         ` Morten Brørup
2021-05-05  6:51           ` Thomas Monjalon
2021-05-05  7:08             ` Morten Brørup
2021-05-05  3:43   ` [dpdk-dev] [PATCH v3] " Chengwen Feng
2021-05-11  7:36     ` Thomas Monjalon
2021-05-11 10:49       ` fengchengwen
2021-05-11 10:41 ` [dpdk-dev] [PATCH v4] " Chengwen Feng
2021-05-12 14:52   ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=98CBD80474FA8B44BF855DF32C47DC35C6171A@smartserver.smartshare.dk \
    --to=mb@smartsharesystems.com \
    --cc=dev@dpdk.org \
    --cc=fengchengwen@huawei.com \
    --cc=ferruh.yigit@intel.com \
    --cc=humin29@huawei.com \
    --cc=jerinj@marvell.com \
    --cc=skori@marvell.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).