On 2021/4/28 17:24, Morten Brørup wrote:
>> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Min Hu (Connor)
>> Sent: Wednesday, April 28, 2021 10:13 AM
>>
>> From: Chengwen Feng <fengcheng...@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: sta...@dpdk.org
>>
>> Signed-off-by: Chengwen Feng <fengcheng...@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humi...@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

>> +
>>  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 = &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 via email to