Re: [dpdk-dev] [PATCH v2] eal: fix use wrong time API

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, May 5, 2021 8:51 AM
> 
> 05/05/2021 08:26, Morten Brørup:
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas
> Monjalon
> > > Sent: Wednesday, May 5, 2021 8:14 AM
> > >
> > > 04/05/2021 21:12, Morten Brørup:
> > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas
> > > Monjalon
> > > > > Sent: Tuesday, May 4, 2021 6:50 PM
> > > > >
> > > > > 29/04/2021 04:10, Min Hu (Connor):
> > > > > > 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>
> > > > > > ---
> > > > > [...]
> > > > > > --- a/lib/eal/common/eal_common_proc.c	
> > > > > > +++ b/lib/eal/common/eal_common_proc.c
> > > > > > -	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_MONOTONIC, &ts_now);
> > > > >
> > > > > Why not testing the return value?
> > > >
> > > > Because it is guaranteed not to fail. Ref:
> > > > https://linux.die.net/man/3/clock_gettime
> > > > https://www.freebsd.org/cgi/man.cgi?query=clock_gettime
> > >
> > > I see "return 0 for success, or -1 for failure".
> > > Where is it said it cannot fail?
> >
> > I'm sorry about being unclear. Referring to the "Errors" chapter in
> the function's man page, this function call is guaranteed not to fail
> with these parameters. So there is no need to check the return value.
> 
> I don't agree.
> Especially for this error:
> "The clk_id specified is not supported on this system."
> How can you be sure it is always supported for any system
> we try to run this code now and in future experiments?
> 

I referred to both Linux and BSD man pages because they say that it is defined there. This should cover current supported systems, excl. Windows, which doesn't have gettimeofday() or clock_gettime() anyway.

However, for future operating systems that might be supported by DPDK, you are correct that CLOCK_MONOTONIC is optional.
Ref: https://pubs.opengroup.org/onlinepubs/009695399/basedefs/time.h.html

This can be fixed by adding:
#if !defined(CLOCK_MONOTONIC)
#error CLOCK_MONOTONIC is not defined.
#endif

If we only check the return value at runtime, the function would fail every time on a system without CLOCK_MONOTONIC, and eal_common_proc.c would need to be fixed anyway. It's better to catch at compile time.