@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
