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

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

[Tom Barbette]

Ferruh Yigit wrote :
> Is this a common enough feature to include into ethdev abstraction layer? Or a
> feature for a single vendor?

I found reference to mbuf’s timestamp field only in MLX5. I think it is the only one to support timestamp offloading. This new API is only useful to make sense out of the timestamp value. And without this patch, timestamp offloading is completely useless…

What would be the other way ? Define something in mlx5 header and ask clients to check for the driver and call the specific API ?

I see reference to timestamp offloading in Netronome Agilio, CC-ing maintainers. Is timestamp offloading a feature you could potentially provide ? Would it be host time reference or a value that need conversion with an API like this?

Tom

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

[Shahaf Shuler]

Ferruh,
I share the same thoughts as Tom here.

>Ferruh Yigit wrote :
>> Is this a common enough feature to include into ethdev abstraction layer? Or a
>> feature for a single vendor?
>
>I found reference to mbuf’s timestamp field only in MLX5. I think it is the only one to support timestamp offloading. This new API is only useful to make sense out of the timestamp value. And without this patch, timestamp offloading is completely useless…
>
>What would be the other way ? Define something in mlx5 header and ask clients to check for the driver and call the specific API ?
>
>I see reference to timestamp offloading in Netronome Agilio, CC-ing maintainers. Is timestamp offloading a feature you could potentially provide ? Would it be host time reference or a value that need conversion with an API like this?

I don’t think that the number of vendors which implement the feature at current time is the qualifier for a feature to enter. Rather we should consider how generic it is and its need in the world of networking (since it is ethdev).
IMO, It is perfectly reasonable to expose a generic channel to read the device clock, same as reading device register (which exists).

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

[Ferruh Yigit]

On 12/23/2018 6:06 AM, Shahaf Shuler wrote:
> Ferruh,
> 
> I share the same thoughts as Tom here.
> 
>  
> 
>>Ferruh Yigit wrote :
> 
>>> Is this a common enough feature to include into ethdev abstraction layer? Or a
> 
>>> feature for a single vendor?
> 
>> 
> 
>>I found reference to mbuf’s timestamp field only in MLX5. I think it is the
> only one to support timestamp offloading. This new API is only useful to make
> sense out of the timestamp value. And without this patch, timestamp offloading
> is completely useless…

Why timestamp offloading become useless? When timestamp offloading enabled,
device fills 'mbuf.timestamp' and you can use it.

For your case this timestamp for mlx is device clock and you are adding this API
to be able to convert device clock to real time, this is not something enables
the timestamp offload.

Technically driver can set the 'mbuf.timestamp' with the real clock right, if it
is required? Or this can be defined by a devarg?

Also this relates to how other HW vendors implemented this, if it is common
approach to fill the timestamp with the device clock and there is way to clock
reference from device, this may make sense. If other vendors already providing
the real time on timestamp, this needs to be handled in mlx driver.
That is why it is good to know what other vendors need / use this?

> 
>> 
> 
>>What would be the other way ? Define something in mlx5 header and ask clients
> to check for the driver and call the specific API ?
> 
>> 
> 
>>I see reference to timestamp offloading in Netronome Agilio, CC-ing
> maintainers. Is timestamp offloading a feature you could potentially provide ?
> Would it be host time reference or a value that need conversion with an API like
> this?
> 
>  
> 
> I don’t think that the number of vendors which implement the feature at current
> time is the qualifier for a feature to enter. Rather we should consider how
> generic it is and its need in the world of networking (since it is ethdev).
> 
> IMO, It is perfectly reasonable to expose a generic channel to read the device
> clock, same as reading device register (which exists).
> 

The concern is ethdev bloats with single vendor specific APIs. In the past we
moved some of the ethdev APIs as PMD specific APIs to PMDs. It is not nice to
have "PMD specific APIs" but that was the trade off for more usable ethdev API.

And according techboard discussion [1] for new API():

"
If the API is about HW abstraction, at least one driver
should be implemented. Preferably two.
"

I am questioning if is there possible second one, and how is their
implementation is like.


[1] https://mails.dpdk.org/archives/dev/2018-November/118697.html

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

[Tom Barbette]

Ferruh Yigit wrote:
> Why timestamp offloading become useless? When timestamp offloading enabled,
> device fills 'mbuf.timestamp' and you can use it.
But the frequency is unknown, and the reference time neither. So it can be used only to know that "some time passed" between packets.

> For your case this timestamp for mlx is device clock and you are adding this API
> to be able to convert device clock to real time, this is not something enables
> the timestamp offload.
I get your point, but a keyboard is highly required to use a computer. It's pretty much useless without it. Without this API, the timestamp offload makes no sense. It's a random number generator at best...

> Technically driver can set the 'mbuf.timestamp' with the real clock right, if it
> is required? Or this can be defined by a devarg?
I don't think so. Device have no sense of system time. And doing it in the driver is tricky because it depends on the user needs. Catch-up with NTP updates would need a timer and various parameters... Hence we prefer to give a simple working code, and users may do this if they want.


For the other comments it's not my call... I would just underline that timestamp offload is not usable in the current state, and there is a lot of use case for monitoring latency-sensitive applications.

Tom

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

[Yigit, Ferruh]

On 1/8/2019 11:30 AM, Tom Barbette wrote:
> Ferruh Yigit wrote:
>> Why timestamp offloading become useless? When timestamp offloading enabled,
>> device fills 'mbuf.timestamp' and you can use it.
> But the frequency is unknown, and the reference time neither. So it can be used only to know that "some time passed" between packets.
> 
>> For your case this timestamp for mlx is device clock and you are adding this API
>> to be able to convert device clock to real time, this is not something enables
>> the timestamp offload.
> I get your point, but a keyboard is highly required to use a computer. It's pretty much useless without it. Without this API, the timestamp offload makes no sense. It's a random number generator at best...
> 
>> Technically driver can set the 'mbuf.timestamp' with the real clock right, if it
>> is required? Or this can be defined by a devarg?
> I don't think so. Device have no sense of system time. And doing it in the driver is tricky because it depends on the user needs. Catch-up with NTP updates would need a timer and various parameters... Hence we prefer to give a simple working code, and users may do this if they want.
> 
> 
> For the other comments it's not my call... I would just underline that timestamp offload is not usable in the current state, and there is a lot of use case for monitoring latency-sensitive applications.

Hi Thomas, Andrew,

CAn you please comment on patch, it adds a new 'rte_eth_read_clock()' API to
read device clock to read timestamp value, later to use this value to map to the
actual time.
So that can convert timestamp information from each packet into real time.

My question was if this is common requirement or specific to single device? And
if can be handles in driver level.

Thanks,
ferruh

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

[Yigit, Ferruh]

On 1/8/2019 11:30 AM, Tom Barbette wrote:
> Ferruh Yigit wrote:
>> Why timestamp offloading become useless? When timestamp offloading enabled,
>> device fills 'mbuf.timestamp' and you can use it.
> But the frequency is unknown, and the reference time neither. So it can be used only to know that "some time passed" between packets.
> 
>> For your case this timestamp for mlx is device clock and you are adding this API
>> to be able to convert device clock to real time, this is not something enables
>> the timestamp offload.
> I get your point, but a keyboard is highly required to use a computer. It's pretty much useless without it. Without this API, the timestamp offload makes no sense. It's a random number generator at best...
> 
>> Technically driver can set the 'mbuf.timestamp' with the real clock right, if it
>> is required? Or this can be defined by a devarg?
> I don't think so. Device have no sense of system time. And doing it in the driver is tricky because it depends on the user needs. Catch-up with NTP updates would need a timer and various parameters... Hence we prefer to give a simple working code, and users may do this if they want.
> 
> 
> For the other comments it's not my call... I would just underline that timestamp offload is not usable in the current state, and there is a lot of use case for monitoring latency-sensitive applications.

Hi Thomas, Andrew,

CAn you please comment on patch, it adds a new 'rte_eth_read_clock()' API to
read device clock to read timestamp value, later to use this value to map to the
actual time.
So that can convert timestamp information from each packet into real time.

My question was if this is common requirement or specific to single device? And
if can be handles in driver level.

Thanks,
ferruh

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

[Thomas Monjalon]

19/03/2019 14:32, Yigit, Ferruh:
> On 1/8/2019 11:30 AM, Tom Barbette wrote:
> > Ferruh Yigit wrote:
> >> Why timestamp offloading become useless? When timestamp offloading enabled,
> >> device fills 'mbuf.timestamp' and you can use it.
> > But the frequency is unknown, and the reference time neither. So it can be used only to know that "some time passed" between packets.
> > 
> >> For your case this timestamp for mlx is device clock and you are adding this API
> >> to be able to convert device clock to real time, this is not something enables
> >> the timestamp offload.
> > I get your point, but a keyboard is highly required to use a computer. It's pretty much useless without it. Without this API, the timestamp offload makes no sense. It's a random number generator at best...
> > 
> >> Technically driver can set the 'mbuf.timestamp' with the real clock right, if it
> >> is required? Or this can be defined by a devarg?
> > I don't think so. Device have no sense of system time. And doing it in the driver is tricky because it depends on the user needs. Catch-up with NTP updates would need a timer and various parameters... Hence we prefer to give a simple working code, and users may do this if they want.
> > 
> > 
> > For the other comments it's not my call... I would just underline that timestamp offload is not usable in the current state, and there is a lot of use case for monitoring latency-sensitive applications.
> 
> Hi Thomas, Andrew,
> 
> CAn you please comment on patch, it adds a new 'rte_eth_read_clock()' API to
> read device clock to read timestamp value, later to use this value to map to the
> actual time.
> So that can convert timestamp information from each packet into real time.

The approach is smart in my opinion.
It is requesting the time generator (at its source) and allowing
the app to do any kind of time handling strategy.

> My question was if this is common requirement or specific to single device?

It will work with any device providing some timestamps.
There is nothing specific here in my opinion.

> And if can be handles in driver level.

Yes, it may be handled differently.
But this approach looks to be the most flexible and reliable.

Acked-by: Thomas Monjalon <thomas@monjalon.net>

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

[Thomas Monjalon]

19/03/2019 14:32, Yigit, Ferruh:
> On 1/8/2019 11:30 AM, Tom Barbette wrote:
> > Ferruh Yigit wrote:
> >> Why timestamp offloading become useless? When timestamp offloading enabled,
> >> device fills 'mbuf.timestamp' and you can use it.
> > But the frequency is unknown, and the reference time neither. So it can be used only to know that "some time passed" between packets.
> > 
> >> For your case this timestamp for mlx is device clock and you are adding this API
> >> to be able to convert device clock to real time, this is not something enables
> >> the timestamp offload.
> > I get your point, but a keyboard is highly required to use a computer. It's pretty much useless without it. Without this API, the timestamp offload makes no sense. It's a random number generator at best...
> > 
> >> Technically driver can set the 'mbuf.timestamp' with the real clock right, if it
> >> is required? Or this can be defined by a devarg?
> > I don't think so. Device have no sense of system time. And doing it in the driver is tricky because it depends on the user needs. Catch-up with NTP updates would need a timer and various parameters... Hence we prefer to give a simple working code, and users may do this if they want.
> > 
> > 
> > For the other comments it's not my call... I would just underline that timestamp offload is not usable in the current state, and there is a lot of use case for monitoring latency-sensitive applications.
> 
> Hi Thomas, Andrew,
> 
> CAn you please comment on patch, it adds a new 'rte_eth_read_clock()' API to
> read device clock to read timestamp value, later to use this value to map to the
> actual time.
> So that can convert timestamp information from each packet into real time.

The approach is smart in my opinion.
It is requesting the time generator (at its source) and allowing
the app to do any kind of time handling strategy.

> My question was if this is common requirement or specific to single device?

It will work with any device providing some timestamps.
There is nothing specific here in my opinion.

> And if can be handles in driver level.

Yes, it may be handled differently.
But this approach looks to be the most flexible and reliable.

Acked-by: Thomas Monjalon <thomas@monjalon.net>

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

[Andrew Rybchenko]

On 3/20/19 5:48 PM, Thomas Monjalon wrote:
> 19/03/2019 14:32, Yigit, Ferruh:
>> On 1/8/2019 11:30 AM, Tom Barbette wrote:
>>> Ferruh Yigit wrote:
>>>> Why timestamp offloading become useless? When timestamp offloading enabled,
>>>> device fills 'mbuf.timestamp' and you can use it.
>>> But the frequency is unknown, and the reference time neither. So it can be used only to know that "some time passed" between packets.
>>>
>>>> For your case this timestamp for mlx is device clock and you are adding this API
>>>> to be able to convert device clock to real time, this is not something enables
>>>> the timestamp offload.
>>> I get your point, but a keyboard is highly required to use a computer. It's pretty much useless without it. Without this API, the timestamp offload makes no sense. It's a random number generator at best...
>>>
>>>> Technically driver can set the 'mbuf.timestamp' with the real clock right, if it
>>>> is required? Or this can be defined by a devarg?
>>> I don't think so. Device have no sense of system time. And doing it in the driver is tricky because it depends on the user needs. Catch-up with NTP updates would need a timer and various parameters... Hence we prefer to give a simple working code, and users may do this if they want.
>>>
>>>
>>> For the other comments it's not my call... I would just underline that timestamp offload is not usable in the current state, and there is a lot of use case for monitoring latency-sensitive applications.
>> Hi Thomas, Andrew,
>>
>> CAn you please comment on patch, it adds a new 'rte_eth_read_clock()' API to
>> read device clock to read timestamp value, later to use this value to map to the
>> actual time.
>> So that can convert timestamp information from each packet into real time.
> The approach is smart in my opinion.
> It is requesting the time generator (at its source) and allowing
> the app to do any kind of time handling strategy.
>
>> My question was if this is common requirement or specific to single device?
> It will work with any device providing some timestamps.
> There is nothing specific here in my opinion.
>
>> And if can be handles in driver level.
> Yes, it may be handled differently.
> But this approach looks to be the most flexible and reliable.
>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>

Taking into account that timestamp in mbuf is not normalized (neither
unit nor reference) and the API helps to normalize units, it makes sense.
I recall discussion about timestamp if should be normalized or
not, the decision was to keep it undefined.

Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>

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

[Andrew Rybchenko]

On 3/20/19 5:48 PM, Thomas Monjalon wrote:
> 19/03/2019 14:32, Yigit, Ferruh:
>> On 1/8/2019 11:30 AM, Tom Barbette wrote:
>>> Ferruh Yigit wrote:
>>>> Why timestamp offloading become useless? When timestamp offloading enabled,
>>>> device fills 'mbuf.timestamp' and you can use it.
>>> But the frequency is unknown, and the reference time neither. So it can be used only to know that "some time passed" between packets.
>>>
>>>> For your case this timestamp for mlx is device clock and you are adding this API
>>>> to be able to convert device clock to real time, this is not something enables
>>>> the timestamp offload.
>>> I get your point, but a keyboard is highly required to use a computer. It's pretty much useless without it. Without this API, the timestamp offload makes no sense. It's a random number generator at best...
>>>
>>>> Technically driver can set the 'mbuf.timestamp' with the real clock right, if it
>>>> is required? Or this can be defined by a devarg?
>>> I don't think so. Device have no sense of system time. And doing it in the driver is tricky because it depends on the user needs. Catch-up with NTP updates would need a timer and various parameters... Hence we prefer to give a simple working code, and users may do this if they want.
>>>
>>>
>>> For the other comments it's not my call... I would just underline that timestamp offload is not usable in the current state, and there is a lot of use case for monitoring latency-sensitive applications.
>> Hi Thomas, Andrew,
>>
>> CAn you please comment on patch, it adds a new 'rte_eth_read_clock()' API to
>> read device clock to read timestamp value, later to use this value to map to the
>> actual time.
>> So that can convert timestamp information from each packet into real time.
> The approach is smart in my opinion.
> It is requesting the time generator (at its source) and allowing
> the app to do any kind of time handling strategy.
>
>> My question was if this is common requirement or specific to single device?
> It will work with any device providing some timestamps.
> There is nothing specific here in my opinion.
>
>> And if can be handles in driver level.
> Yes, it may be handled differently.
> But this approach looks to be the most flexible and reliable.
>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>

Taking into account that timestamp in mbuf is not normalized (neither
unit nor reference) and the API helps to normalize units, it makes sense.
I recall discussion about timestamp if should be normalized or
not, the decision was to keep it undefined.

Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>

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

[Ferruh Yigit]

On 3/20/2019 3:57 PM, Andrew Rybchenko wrote:
> On 3/20/19 5:48 PM, Thomas Monjalon wrote:
>> 19/03/2019 14:32, Yigit, Ferruh:
>>> On 1/8/2019 11:30 AM, Tom Barbette wrote:
>>>> Ferruh Yigit wrote:
>>>>> Why timestamp offloading become useless? When timestamp offloading enabled,
>>>>> device fills 'mbuf.timestamp' and you can use it.
>>>> But the frequency is unknown, and the reference time neither. So it can be used only to know that "some time passed" between packets.
>>>>
>>>>> For your case this timestamp for mlx is device clock and you are adding this API
>>>>> to be able to convert device clock to real time, this is not something enables
>>>>> the timestamp offload.
>>>> I get your point, but a keyboard is highly required to use a computer. It's pretty much useless without it. Without this API, the timestamp offload makes no sense. It's a random number generator at best...
>>>>
>>>>> Technically driver can set the 'mbuf.timestamp' with the real clock right, if it
>>>>> is required? Or this can be defined by a devarg?
>>>> I don't think so. Device have no sense of system time. And doing it in the driver is tricky because it depends on the user needs. Catch-up with NTP updates would need a timer and various parameters... Hence we prefer to give a simple working code, and users may do this if they want.
>>>>
>>>>
>>>> For the other comments it's not my call... I would just underline that timestamp offload is not usable in the current state, and there is a lot of use case for monitoring latency-sensitive applications.
>>> Hi Thomas, Andrew,
>>>
>>> CAn you please comment on patch, it adds a new 'rte_eth_read_clock()' API to
>>> read device clock to read timestamp value, later to use this value to map to the
>>> actual time.
>>> So that can convert timestamp information from each packet into real time.
>> The approach is smart in my opinion.
>> It is requesting the time generator (at its source) and allowing
>> the app to do any kind of time handling strategy.
>>
>>> My question was if this is common requirement or specific to single device?
>> It will work with any device providing some timestamps.
>> There is nothing specific here in my opinion.
>>
>>> And if can be handles in driver level.
>> Yes, it may be handled differently.
>> But this approach looks to be the most flexible and reliable.
>>
>> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> 
> Taking into account that timestamp in mbuf is not normalized (neither
> unit nor reference) and the API helps to normalize units, it makes sense.
> I recall discussion about timestamp if should be normalized or
> not, the decision was to keep it undefined.
> 
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 

Hi Tom,

mlx patch doesn't apply cleanly, can you please send a new version on top of
latest head?

Thanks,
ferruh

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

[Ferruh Yigit]

On 3/20/2019 3:57 PM, Andrew Rybchenko wrote:
> On 3/20/19 5:48 PM, Thomas Monjalon wrote:
>> 19/03/2019 14:32, Yigit, Ferruh:
>>> On 1/8/2019 11:30 AM, Tom Barbette wrote:
>>>> Ferruh Yigit wrote:
>>>>> Why timestamp offloading become useless? When timestamp offloading enabled,
>>>>> device fills 'mbuf.timestamp' and you can use it.
>>>> But the frequency is unknown, and the reference time neither. So it can be used only to know that "some time passed" between packets.
>>>>
>>>>> For your case this timestamp for mlx is device clock and you are adding this API
>>>>> to be able to convert device clock to real time, this is not something enables
>>>>> the timestamp offload.
>>>> I get your point, but a keyboard is highly required to use a computer. It's pretty much useless without it. Without this API, the timestamp offload makes no sense. It's a random number generator at best...
>>>>
>>>>> Technically driver can set the 'mbuf.timestamp' with the real clock right, if it
>>>>> is required? Or this can be defined by a devarg?
>>>> I don't think so. Device have no sense of system time. And doing it in the driver is tricky because it depends on the user needs. Catch-up with NTP updates would need a timer and various parameters... Hence we prefer to give a simple working code, and users may do this if they want.
>>>>
>>>>
>>>> For the other comments it's not my call... I would just underline that timestamp offload is not usable in the current state, and there is a lot of use case for monitoring latency-sensitive applications.
>>> Hi Thomas, Andrew,
>>>
>>> CAn you please comment on patch, it adds a new 'rte_eth_read_clock()' API to
>>> read device clock to read timestamp value, later to use this value to map to the
>>> actual time.
>>> So that can convert timestamp information from each packet into real time.
>> The approach is smart in my opinion.
>> It is requesting the time generator (at its source) and allowing
>> the app to do any kind of time handling strategy.
>>
>>> My question was if this is common requirement or specific to single device?
>> It will work with any device providing some timestamps.
>> There is nothing specific here in my opinion.
>>
>>> And if can be handles in driver level.
>> Yes, it may be handled differently.
>> But this approach looks to be the most flexible and reliable.
>>
>> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> 
> Taking into account that timestamp in mbuf is not normalized (neither
> unit nor reference) and the API helps to normalize units, it makes sense.
> I recall discussion about timestamp if should be normalized or
> not, the decision was to keep it undefined.
> 
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 

Hi Tom,

mlx patch doesn't apply cleanly, can you please send a new version on top of
latest head?

Thanks,
ferruh

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

[Ferruh Yigit]

On 12/19/2018 1:49 PM, Tom Barbette wrote:
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Read the current clock counter of an Ethernet device
> + *
> + * This returns the current raw clock value of an Ethernet device.
> + * The value returned here is from the same clock than the one
> + * filling timestamp field of RX packets. Therefore it can be used
> + * to compute a precise conversion of the device clock to the real time.
> + *
> + * E.g, a simple heuristic to derivate the frequency would be:
> + * uint64_t start, end;
> + * rte_eth_read_clock(port, start);
> + * rte_delay_ms(100);
> + * rte_eth_read_clock(port, end);
> + * double freq = (end - start) * 10;
> + *
> + * Compute a common reference with:
> + * uint64_t base_time_sec = current_time();
> + * uint64_t base_clock;
> + * rte_eth_read_clock(port, base_clock);
> + *
> + * Then, convert the raw mbuf timestamp with:
> + * base_time_sec + (double)(mbuf->timestamp - base_clock) / freq;
> + *
> + * This simple example will not provide a very good accuracy. One must
> + * at least measure multiple times the frequency and do a regression.
> + * To avoid deviation from the system time, the common reference can
> + * be repeated from time to time. The integer division can also be
> + * converted by a multiplication and a shift for better performance.
> + *
> + * @param port_id
> + * The port identifier of the Ethernet device.
> + * @param time
> + * Pointer to the uint64_t that holds the raw clock value.
> + *
> + * @return
> + * - 0: Success.
> + * - -ENODEV: The port ID is invalid.
> + * - -ENOTSUP: The function is not supported by the Ethernet driver.
> + */
> +int __rte_experimental rte_eth_read_clock(uint16_t port_id, uint64_t *time);

Is this a common enough feature to include into ethdev abstraction layer? Or a
feature for a single vendor?
I would like to get more input from other vendors if this is something they can
benefit from?

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

[Shahaf Shuler]

Wednesday, December 19, 2018 3:50 PM, Tom Barbette:
> Subject: [PATCH 1/3] rte_ethdev: Add API function to read dev clock
> 
> Add rte_eth_read_clock to read the raw clock of a devide.
> 
> The main use is to get the device clock conversion co-efficients to be able to
> translate the raw clock of the timestamp field of the pkt mbuf to a local synced
> time value.
> 
> This function was missing to allow users to convert the RX timestamp field to
> real time without the complexity of the rte_timesync* facility. One can
> derivate the clock frequency by calling twice read_clock and then keep a
> common time base.
> 
> Signed-off-by: Tom Barbette <barbette@kth.se>

Acked-by: Shahaf Shuler <shahafs@mellanox.com>

> ---
>  doc/guides/nics/features.rst             |  1 +
>  lib/librte_ethdev/rte_ethdev.c           | 13 +++++++
>  lib/librte_ethdev/rte_ethdev.h           | 44 ++++++++++++++++++++++++
>  lib/librte_ethdev/rte_ethdev_core.h      |  6 ++++
>  lib/librte_ethdev/rte_ethdev_version.map |  1 +
>  lib/librte_mbuf/rte_mbuf.h               |  2 ++
>  6 files changed, 67 insertions(+)
> 
> diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst index
> d3f904839..45484a30f 100644
> --- a/doc/guides/nics/features.rst
> +++ b/doc/guides/nics/features.rst
> @@ -602,6 +602,7 @@ Supports Timestamp.
>  * **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_TIMESTAMP``.
>  * **[provides] mbuf**: ``mbuf.timestamp``.
>  * **[provides] rte_eth_dev_info**: ``rx_offload_capa,rx_queue_offload_capa:
> DEV_RX_OFFLOAD_TIMESTAMP``.
> +* **[implements] eth_dev_ops**: ``read_clock``.
> 
>  .. _nic_features_macsec_offload:
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 9d5107dce..97f92d11a 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -4124,6 +4124,19 @@ rte_eth_timesync_write_time(uint16_t port_id,
> const struct timespec *timestamp)
>  								timestamp));
>  }
> 
> +int
> +rte_eth_read_clock(uint16_t port_id, uint64_t *timestamp) {
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->read_clock, -
> ENOTSUP);
> +	return eth_err(port_id, (*dev->dev_ops->read_clock)(dev,
> +								timestamp));
> +}
> +
>  int
>  rte_eth_dev_get_reg_info(uint16_t port_id, struct rte_dev_reg_info *info)  {
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 1960f3a2d..5a831db29 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -3642,6 +3642,50 @@ int rte_eth_timesync_read_time(uint16_t port_id,
> struct timespec *time);
>   */
>  int rte_eth_timesync_write_time(uint16_t port_id, const struct timespec
> *time);
> 
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Read the current clock counter of an Ethernet device
> + *
> + * This returns the current raw clock value of an Ethernet device.
> + * The value returned here is from the same clock than the one
> + * filling timestamp field of RX packets. Therefore it can be used
> + * to compute a precise conversion of the device clock to the real time.
> + *
> + * E.g, a simple heuristic to derivate the frequency would be:
> + * uint64_t start, end;
> + * rte_eth_read_clock(port, start);
> + * rte_delay_ms(100);
> + * rte_eth_read_clock(port, end);
> + * double freq = (end - start) * 10;
> + *
> + * Compute a common reference with:
> + * uint64_t base_time_sec = current_time();
> + * uint64_t base_clock;
> + * rte_eth_read_clock(port, base_clock);
> + *
> + * Then, convert the raw mbuf timestamp with:
> + * base_time_sec + (double)(mbuf->timestamp - base_clock) / freq;
> + *
> + * This simple example will not provide a very good accuracy. One must
> + * at least measure multiple times the frequency and do a regression.
> + * To avoid deviation from the system time, the common reference can
> + * be repeated from time to time. The integer division can also be
> + * converted by a multiplication and a shift for better performance.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param time
> + *   Pointer to the uint64_t that holds the raw clock value.
> + *
> + * @return
> + *   - 0: Success.
> + *   - -ENODEV: The port ID is invalid.
> + *   - -ENOTSUP: The function is not supported by the Ethernet driver.
> + */
> +int __rte_experimental rte_eth_read_clock(uint16_t port_id, uint64_t
> +*time);
> +
>  /**
>   * Config l2 tunnel ether type of an Ethernet device for filtering specific
>   * tunnel packets by ether type.
> diff --git a/lib/librte_ethdev/rte_ethdev_core.h
> b/lib/librte_ethdev/rte_ethdev_core.h
> index 8f03f83f6..86806b3eb 100644
> --- a/lib/librte_ethdev/rte_ethdev_core.h
> +++ b/lib/librte_ethdev/rte_ethdev_core.h
> @@ -322,6 +322,10 @@ typedef int (*eth_timesync_write_time)(struct
> rte_eth_dev *dev,
>  				       const struct timespec *timestamp);  /**<
> @internal Function used to get time from the device clock */
> 
> +typedef int (*eth_read_clock)(struct rte_eth_dev *dev,
> +				      uint64_t *timestamp);
> +/**< @internal Function used to get the current value of the device
> +clock. */
> +
>  typedef int (*eth_get_reg_t)(struct rte_eth_dev *dev,
>  				struct rte_dev_reg_info *info);
>  /**< @internal Retrieve registers  */
> @@ -496,6 +500,8 @@ struct eth_dev_ops {
>  	eth_timesync_read_time     timesync_read_time; /** Get the device
> clock time. */
>  	eth_timesync_write_time    timesync_write_time; /** Set the device
> clock time. */
> 
> +	eth_read_clock             read_clock;
> +
>  	eth_xstats_get_by_id_t     xstats_get_by_id;
>  	/**< Get extended device statistic values by ID. */
>  	eth_xstats_get_names_by_id_t xstats_get_names_by_id; diff --git
> a/lib/librte_ethdev/rte_ethdev_version.map
> b/lib/librte_ethdev/rte_ethdev_version.map
> index 92ac3de25..12d6c3c1d 100644
> --- a/lib/librte_ethdev/rte_ethdev_version.map
> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> @@ -249,6 +249,7 @@ EXPERIMENTAL {
>  	rte_eth_switch_domain_free;
>  	rte_flow_conv;
>  	rte_flow_expand_rss;
> +	rte_eth_read_clock;
>  	rte_mtr_capabilities_get;
>  	rte_mtr_create;
>  	rte_mtr_destroy;
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h index
> 3dbc6695e..4ceefa913 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -605,6 +605,8 @@ struct rte_mbuf {
> 
>  	/** 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.
>  	 */
>  	uint64_t timestamp;
> 
> --
> 2.17.1

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

[Tom Barbette]

Add rte_eth_read_clock to read the raw clock of a devide.

The main use is to get the device clock conversion co-efficients to be
able to translate the raw clock of the timestamp field of the pkt mbuf
to a local synced time value.

This function was missing to allow users to convert the RX timestamp field
to real time without the complexity of the rte_timesync* facility. One can
derivate the clock frequency by calling twice read_clock and then keep a
common time base.

Signed-off-by: Tom Barbette <barbette@kth.se>
---
 doc/guides/nics/features.rst             |  1 +
 lib/librte_ethdev/rte_ethdev.c           | 13 +++++++
 lib/librte_ethdev/rte_ethdev.h           | 44 ++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_core.h      |  6 ++++
 lib/librte_ethdev/rte_ethdev_version.map |  1 +
 lib/librte_mbuf/rte_mbuf.h               |  2 ++
 6 files changed, 67 insertions(+)

diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
index d3f904839..45484a30f 100644
--- a/doc/guides/nics/features.rst
+++ b/doc/guides/nics/features.rst
@@ -602,6 +602,7 @@ Supports Timestamp.
 * **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_TIMESTAMP``.
 * **[provides] mbuf**: ``mbuf.timestamp``.
 * **[provides] rte_eth_dev_info**: ``rx_offload_capa,rx_queue_offload_capa: DEV_RX_OFFLOAD_TIMESTAMP``.
+* **[implements] eth_dev_ops**: ``read_clock``.
 
 .. _nic_features_macsec_offload:
 
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 9d5107dce..97f92d11a 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -4124,6 +4124,19 @@ rte_eth_timesync_write_time(uint16_t port_id, const struct timespec *timestamp)
 								timestamp));
 }
 
+int
+rte_eth_read_clock(uint16_t port_id, uint64_t *timestamp)
+{
+	struct rte_eth_dev *dev;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	dev = &rte_eth_devices[port_id];
+
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->read_clock, -ENOTSUP);
+	return eth_err(port_id, (*dev->dev_ops->read_clock)(dev,
+								timestamp));
+}
+
 int
 rte_eth_dev_get_reg_info(uint16_t port_id, struct rte_dev_reg_info *info)
 {
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 1960f3a2d..5a831db29 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -3642,6 +3642,50 @@ int rte_eth_timesync_read_time(uint16_t port_id, struct timespec *time);
  */
 int rte_eth_timesync_write_time(uint16_t port_id, const struct timespec *time);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Read the current clock counter of an Ethernet device
+ *
+ * This returns the current raw clock value of an Ethernet device.
+ * The value returned here is from the same clock than the one
+ * filling timestamp field of RX packets. Therefore it can be used
+ * to compute a precise conversion of the device clock to the real time.
+ *
+ * E.g, a simple heuristic to derivate the frequency would be:
+ * uint64_t start, end;
+ * rte_eth_read_clock(port, start);
+ * rte_delay_ms(100);
+ * rte_eth_read_clock(port, end);
+ * double freq = (end - start) * 10;
+ *
+ * Compute a common reference with:
+ * uint64_t base_time_sec = current_time();
+ * uint64_t base_clock;
+ * rte_eth_read_clock(port, base_clock);
+ *
+ * Then, convert the raw mbuf timestamp with:
+ * base_time_sec + (double)(mbuf->timestamp - base_clock) / freq;
+ *
+ * This simple example will not provide a very good accuracy. One must
+ * at least measure multiple times the frequency and do a regression.
+ * To avoid deviation from the system time, the common reference can
+ * be repeated from time to time. The integer division can also be
+ * converted by a multiplication and a shift for better performance.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param time
+ *   Pointer to the uint64_t that holds the raw clock value.
+ *
+ * @return
+ *   - 0: Success.
+ *   - -ENODEV: The port ID is invalid.
+ *   - -ENOTSUP: The function is not supported by the Ethernet driver.
+ */
+int __rte_experimental rte_eth_read_clock(uint16_t port_id, uint64_t *time);
+
 /**
  * Config l2 tunnel ether type of an Ethernet device for filtering specific
  * tunnel packets by ether type.
diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
index 8f03f83f6..86806b3eb 100644
--- a/lib/librte_ethdev/rte_ethdev_core.h
+++ b/lib/librte_ethdev/rte_ethdev_core.h
@@ -322,6 +322,10 @@ typedef int (*eth_timesync_write_time)(struct rte_eth_dev *dev,
 				       const struct timespec *timestamp);
 /**< @internal Function used to get time from the device clock */
 
+typedef int (*eth_read_clock)(struct rte_eth_dev *dev,
+				      uint64_t *timestamp);
+/**< @internal Function used to get the current value of the device clock. */
+
 typedef int (*eth_get_reg_t)(struct rte_eth_dev *dev,
 				struct rte_dev_reg_info *info);
 /**< @internal Retrieve registers  */
@@ -496,6 +500,8 @@ struct eth_dev_ops {
 	eth_timesync_read_time     timesync_read_time; /** Get the device clock time. */
 	eth_timesync_write_time    timesync_write_time; /** Set the device clock time. */
 
+	eth_read_clock             read_clock;
+
 	eth_xstats_get_by_id_t     xstats_get_by_id;
 	/**< Get extended device statistic values by ID. */
 	eth_xstats_get_names_by_id_t xstats_get_names_by_id;
diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index 92ac3de25..12d6c3c1d 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -249,6 +249,7 @@ EXPERIMENTAL {
 	rte_eth_switch_domain_free;
 	rte_flow_conv;
 	rte_flow_expand_rss;
+	rte_eth_read_clock;
 	rte_mtr_capabilities_get;
 	rte_mtr_create;
 	rte_mtr_destroy;
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 3dbc6695e..4ceefa913 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -605,6 +605,8 @@ struct rte_mbuf {
 
 	/** 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.
 	 */
 	uint64_t timestamp;
 
-- 
2.17.1