Re: [dpdk-dev] [PATCH v3 1/3] rte_ethdev: Add API function to read dev clock

[Tom Barbette <barbette@kth.se>]

@Andrew I applied your comments. Thanks.

On 2019-04-25 20:28, Wiles, Keith wrote:
> What is a raw clock value? It took me a bit to find that it is in nano-seconds need to document that point.

It is not in nanosecond, it has no units. Finding the relation between 
the device clock and the real time is the whole point of this API.

> Using timestamp variable is not very descriptive and some other name needs to be used. Also in the driver it is called *clock.

The problem is that the "timestamp offload" feature filling the 
timestamp field of the pktmbuf is already badly named as it is given 
without time unit. Both of them are not timestamp but raw "clock" 
values, a number of ticks at an unknown frequency, with an unknow time 
base (ie is it the number of ticks since boot? Device is up? 1/1/1970?).
One solution would be to have an union in the pktmbuf, changing the 
timestamp field into a "clock" or "timestamp" field. But it's a bit 
overkill.

I propose to change the read_clock API to reference "clock" instead of 
timestamp everywhere.

> Another question is why does this routine not perform the looping/delaying and calling read_clock and then return frequency instead. If you want a timestamp reading function then this one is not being described that way and I would think it should be done someplace else or should be.

The frequency is one thing, and would allow to give a relative time in 
second between packets. In practice though, we change the frequency (as 
Linux does regarding the TSC ticks) to catch up corrections applied by 
NTP on the source clock.
The second thing is the time reference, to convert the clock time (of 
the packets) to the current wall time. One may want to use the monotonic 
clock, the real clock (we do follow both). Or no system clock at all and 
directly follow an NTP source. The point is, having a function to give 
the frequency is not enough. One can derive the frequency and the time 
reference with this API. I could write a helper in a second step to get 
the frequency out of read_clock. But the time reference implies timers, 
choosing a source, and stuff we don't want here, I think...

>> 	/** Valid if PKT_RX_TIMESTAMP is set. The unit and time reference
>> 	 * are not normalized but are always the same for a given port.
>> +	 * Some devices allow to query rte_eth_read_clock that will return the
>> +	 * current device timestamp.
> 
> The message here is not a good place for this detail, I would put it in the function call doc above.

I can remove these lines, but with read_clock referencing only clock I 
feel like it's even more needed. Someone reading the doc will see "oh, I 
can use timestamp offloading to get precise timing of when my packets 
were received, great ! But it has no unit and no reference... What do I 
do with that?". It miss the last step "how I see... I can look at 
rte_eth_read_clock documentation to read more about conversion."

Thanks !

Tom