[dpdk-dev] [PATCH] net/pcap: support hardware Tx timestamps

[dpdk-dev] [PATCH] net/pcap: support hardware Tx timestamps

[Vivien Didelot]

When hardware timestamping is enabled on Rx path, system time should
no longer be used to calculate the timestamps when dumping packets.

Instead, use the value stored by the driver in mbuf->timestamp
and assume it is already converted to nanoseconds (otherwise the
application may edit the packet headers itself afterwards).

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
Signed-off-by: Patrick Keroulas <patrick.keroulas@radio-canada.ca>
---
 doc/guides/rel_notes/release_20_08.rst |  1 +
 drivers/net/pcap/rte_eth_pcap.c        | 30 +++++++++++++++-----------
 2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/doc/guides/rel_notes/release_20_08.rst b/doc/guides/rel_notes/release_20_08.rst
index 7a67c960c..cedceaf9d 100644
--- a/doc/guides/rel_notes/release_20_08.rst
+++ b/doc/guides/rel_notes/release_20_08.rst
@@ -61,6 +61,7 @@ New Features
   Updated PCAP driver with new features and improvements, including:
 
   * Support software Tx nanosecond timestamps precision.
+  * Support hardware Tx timestamps.
 
 * **Updated Mellanox mlx5 driver.**
 
diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 13a3d0ac7..3d80b699b 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -290,19 +290,23 @@ eth_null_rx(void *queue __rte_unused,
 #define NSEC_PER_SEC	1000000000L
 
 static inline void
-calculate_timestamp(struct timeval *ts) {
-	uint64_t cycles;
-	struct timeval cur_time;
+calculate_timestamp(const struct rte_mbuf *mbuf, struct timeval *ts) {
+	if (mbuf->ol_flags & PKT_RX_TIMESTAMP) {
+		ts->tv_sec = mbuf->timestamp / NSEC_PER_SEC;
+		ts->tv_usec = mbuf->timestamp % NSEC_PER_SEC;
+	} else {
+		uint64_t cycles = rte_get_timer_cycles() - start_cycles;
+		struct timeval cur_time = {
+			.tv_sec = cycles / hz,
+			.tv_usec = (cycles % hz) * NSEC_PER_SEC / hz,
+		};
 
-	cycles = rte_get_timer_cycles() - start_cycles;
-	cur_time.tv_sec = cycles / hz;
-	cur_time.tv_usec = (cycles % hz) * NSEC_PER_SEC / hz;
-
-	ts->tv_sec = start_time.tv_sec + cur_time.tv_sec;
-	ts->tv_usec = start_time.tv_usec + cur_time.tv_usec;
-	if (ts->tv_usec >= NSEC_PER_SEC) {
-		ts->tv_usec -= NSEC_PER_SEC;
-		ts->tv_sec += 1;
+		ts->tv_sec = start_time.tv_sec + cur_time.tv_sec;
+		ts->tv_usec = start_time.tv_usec + cur_time.tv_usec;
+		if (ts->tv_usec >= NSEC_PER_SEC) {
+			ts->tv_usec -= NSEC_PER_SEC;
+			ts->tv_sec += 1;
+		}
 	}
 }
 
@@ -339,7 +343,7 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 			caplen = sizeof(temp_data);
 		}
 
-		calculate_timestamp(&header.ts);
+		calculate_timestamp(mbuf, &header.ts);
 		header.len = len;
 		header.caplen = caplen;
 		/* rte_pktmbuf_read() returns a pointer to the data directly
-- 
2.27.0

Re: [dpdk-dev] [PATCH] net/pcap: support hardware Tx timestamps

[Vivien Didelot]

Hi Ferruh,

On Wed, 10 Jun 2020 15:39:38 -0400, Vivien Didelot <vivien.didelot@gmail.com> wrote:
> When hardware timestamping is enabled on Rx path, system time should
> no longer be used to calculate the timestamps when dumping packets.
> 
> Instead, use the value stored by the driver in mbuf->timestamp
> and assume it is already converted to nanoseconds (otherwise the
> application may edit the packet headers itself afterwards).
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> Signed-off-by: Patrick Keroulas <patrick.keroulas@radio-canada.ca>

Any feedback on this patch?


Thank you,

	Vivien

Re: [dpdk-dev] [PATCH] net/pcap: support hardware Tx timestamps

[Ferruh Yigit]

On 6/10/2020 8:39 PM, Vivien Didelot wrote:
> When hardware timestamping is enabled on Rx path, system time should
> no longer be used to calculate the timestamps when dumping packets.
> 
> Instead, use the value stored by the driver in mbuf->timestamp
> and assume it is already converted to nanoseconds (otherwise the
> application may edit the packet headers itself afterwards).
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> Signed-off-by: Patrick Keroulas <patrick.keroulas@radio-canada.ca>
> ---
>  doc/guides/rel_notes/release_20_08.rst |  1 +
>  drivers/net/pcap/rte_eth_pcap.c        | 30 +++++++++++++++-----------
>  2 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_20_08.rst b/doc/guides/rel_notes/release_20_08.rst
> index 7a67c960c..cedceaf9d 100644
> --- a/doc/guides/rel_notes/release_20_08.rst
> +++ b/doc/guides/rel_notes/release_20_08.rst
> @@ -61,6 +61,7 @@ New Features
>    Updated PCAP driver with new features and improvements, including:
>  
>    * Support software Tx nanosecond timestamps precision.
> +  * Support hardware Tx timestamps.
>  
>  * **Updated Mellanox mlx5 driver.**
>  
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index 13a3d0ac7..3d80b699b 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -290,19 +290,23 @@ eth_null_rx(void *queue __rte_unused,
>  #define NSEC_PER_SEC	1000000000L
>  
>  static inline void
> -calculate_timestamp(struct timeval *ts) {
> -	uint64_t cycles;
> -	struct timeval cur_time;
> +calculate_timestamp(const struct rte_mbuf *mbuf, struct timeval *ts) {
> +	if (mbuf->ol_flags & PKT_RX_TIMESTAMP) {
> +		ts->tv_sec = mbuf->timestamp / NSEC_PER_SEC;
> +		ts->tv_usec = mbuf->timestamp % NSEC_PER_SEC;

Hi Vivien,

No objection from pcap PMD point of view.

But should we have a Tx mbuf flag, 'PKT_TX_TIMESTAMP', for applications to
request drivers to use the timestamp field on Tx path? Not sure if there can be
any problem on using Rx flag on both direction?

Also the metric is not defined for the 'mbuf->timestamp', it doesn't need to be
nanoseconds, not sure if it is correct to assume it is. Or should we define a
metric for timestamp on the Tx path?

cc'ed Oliver, I think he can comment better on above two questions.

Thanks,
ferruh

Re: [dpdk-dev] [PATCH] net/pcap: support hardware Tx timestamps

[Vivien Didelot]

Hi Oliver,

Surprisingly, dumping PCAP with hardware timestamps seems to be a niche,
but we do need this feature for our network analyzing tool.

Do you guys have objections for this patch?

Regards,
Vivien


On Wed, Jun 17, 2020 at 4:16 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 6/10/2020 8:39 PM, Vivien Didelot wrote:
> > When hardware timestamping is enabled on Rx path, system time should
> > no longer be used to calculate the timestamps when dumping packets.
> >
> > Instead, use the value stored by the driver in mbuf->timestamp
> > and assume it is already converted to nanoseconds (otherwise the
> > application may edit the packet headers itself afterwards).
> >
> > Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> > Signed-off-by: Patrick Keroulas <patrick.keroulas@radio-canada.ca>
> > ---
> >  doc/guides/rel_notes/release_20_08.rst |  1 +
> >  drivers/net/pcap/rte_eth_pcap.c        | 30 +++++++++++++++-----------
> >  2 files changed, 18 insertions(+), 13 deletions(-)
> >
> > diff --git a/doc/guides/rel_notes/release_20_08.rst
> b/doc/guides/rel_notes/release_20_08.rst
> > index 7a67c960c..cedceaf9d 100644
> > --- a/doc/guides/rel_notes/release_20_08.rst
> > +++ b/doc/guides/rel_notes/release_20_08.rst
> > @@ -61,6 +61,7 @@ New Features
> >    Updated PCAP driver with new features and improvements, including:
> >
> >    * Support software Tx nanosecond timestamps precision.
> > +  * Support hardware Tx timestamps.
> >
> >  * **Updated Mellanox mlx5 driver.**
> >
> > diff --git a/drivers/net/pcap/rte_eth_pcap.c
> b/drivers/net/pcap/rte_eth_pcap.c
> > index 13a3d0ac7..3d80b699b 100644
> > --- a/drivers/net/pcap/rte_eth_pcap.c
> > +++ b/drivers/net/pcap/rte_eth_pcap.c
> > @@ -290,19 +290,23 @@ eth_null_rx(void *queue __rte_unused,
> >  #define NSEC_PER_SEC 1000000000L
> >
> >  static inline void
> > -calculate_timestamp(struct timeval *ts) {
> > -     uint64_t cycles;
> > -     struct timeval cur_time;
> > +calculate_timestamp(const struct rte_mbuf *mbuf, struct timeval *ts) {
> > +     if (mbuf->ol_flags & PKT_RX_TIMESTAMP) {
> > +             ts->tv_sec = mbuf->timestamp / NSEC_PER_SEC;
> > +             ts->tv_usec = mbuf->timestamp % NSEC_PER_SEC;
>
> Hi Vivien,
>
> No objection from pcap PMD point of view.
>
> But should we have a Tx mbuf flag, 'PKT_TX_TIMESTAMP', for applications to
> request drivers to use the timestamp field on Tx path? Not sure if there
> can be
> any problem on using Rx flag on both direction?
>
> Also the metric is not defined for the 'mbuf->timestamp', it doesn't need
> to be
> nanoseconds, not sure if it is correct to assume it is. Or should we
> define a
> metric for timestamp on the Tx path?
>
> cc'ed Oliver, I think he can comment better on above two questions.
>
> Thanks,
> ferruh
>
>

Re: [dpdk-dev] [PATCH] net/pcap: support hardware Tx timestamps

[Olivier Matz]

Hi Vivien,

On Tue, Jun 23, 2020 at 06:10:09PM -0400, Vivien Didelot wrote:
> 
> On Wed, Jun 17, 2020 at 4:16 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> > On 6/10/2020 8:39 PM, Vivien Didelot wrote:
> > > When hardware timestamping is enabled on Rx path, system time should
> > > no longer be used to calculate the timestamps when dumping packets.
> > >
> > > Instead, use the value stored by the driver in mbuf->timestamp
> > > and assume it is already converted to nanoseconds (otherwise the
> > > application may edit the packet headers itself afterwards).
> > >
> > > Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> > > Signed-off-by: Patrick Keroulas <patrick.keroulas@radio-canada.ca>
> > > ---
> > >  doc/guides/rel_notes/release_20_08.rst |  1 +
> > >  drivers/net/pcap/rte_eth_pcap.c        | 30 +++++++++++++++-----------
> > >  2 files changed, 18 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/doc/guides/rel_notes/release_20_08.rst
> > b/doc/guides/rel_notes/release_20_08.rst
> > > index 7a67c960c..cedceaf9d 100644
> > > --- a/doc/guides/rel_notes/release_20_08.rst
> > > +++ b/doc/guides/rel_notes/release_20_08.rst
> > > @@ -61,6 +61,7 @@ New Features
> > >    Updated PCAP driver with new features and improvements, including:
> > >
> > >    * Support software Tx nanosecond timestamps precision.
> > > +  * Support hardware Tx timestamps.
> > >
> > >  * **Updated Mellanox mlx5 driver.**
> > >
> > > diff --git a/drivers/net/pcap/rte_eth_pcap.c
> > b/drivers/net/pcap/rte_eth_pcap.c
> > > index 13a3d0ac7..3d80b699b 100644
> > > --- a/drivers/net/pcap/rte_eth_pcap.c
> > > +++ b/drivers/net/pcap/rte_eth_pcap.c
> > > @@ -290,19 +290,23 @@ eth_null_rx(void *queue __rte_unused,
> > >  #define NSEC_PER_SEC 1000000000L
> > >
> > >  static inline void
> > > -calculate_timestamp(struct timeval *ts) {
> > > -     uint64_t cycles;
> > > -     struct timeval cur_time;
> > > +calculate_timestamp(const struct rte_mbuf *mbuf, struct timeval *ts) {
> > > +     if (mbuf->ol_flags & PKT_RX_TIMESTAMP) {
> > > +             ts->tv_sec = mbuf->timestamp / NSEC_PER_SEC;
> > > +             ts->tv_usec = mbuf->timestamp % NSEC_PER_SEC;
> >
> > Hi Vivien,
> >
> > No objection from pcap PMD point of view.
> >
> > But should we have a Tx mbuf flag, 'PKT_TX_TIMESTAMP', for applications to
> > request drivers to use the timestamp field on Tx path? Not sure if there
> > can be
> > any problem on using Rx flag on both direction?
> >
> > Also the metric is not defined for the 'mbuf->timestamp', it doesn't need
> > to be
> > nanoseconds, not sure if it is correct to assume it is. Or should we
> > define a
> > metric for timestamp on the Tx path?
> >
> > cc'ed Oliver, I think he can comment better on above two questions.
> >
> > Thanks,
> > ferruh
> >
> >
> Hi Oliver,
> 
> Surprisingly, dumping PCAP with hardware timestamps seems to be a niche,
> but we do need this feature for our network analyzing tool.
> 
> Do you guys have objections for this patch?
> 
> Regards,
> Vivien
> 

As said by Ferruh, the unit of timestamp in mbuf is not normalized to
nanosecs, as seen in rte_mbuf_core.h:

	/** 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;

Using the timestamp generated from a port which is not a pmd-pcap would
require a conversion, using rte_eth_read_clock() on mbuf->port (assuming
it was not modified, which should be true except if event eth Tx adapter
is used).

Also, note that the timestamp corresponds to the Rx timestamp. I think it
could be an issue in case the mbuf is reassembled by the application: the
timestamp in reassembled mbuf would be the one from the first fragment.

So, I share Ferruh's concerns.

If the problem is about calculate_timestamp() speed, I think there is
some room for optimization: 1e6 is a float, so it will probably slow
down the function. I think the following should also work, and would
be faster:

	static inline void
	calculate_timestamp(struct timeval *ts)
	{
		uint64_t cycles;
		struct timeval cur_time;

		cycles = rte_get_timer_cycles() - start_cycles;
		cur_time.tv_sec = cycles / hz;
		cycles -= cur_time.tv_sec * hz;
		cur_time.tv_usec = (cycles * 1000000) / hz;
		timeradd(&start_time, &cur_time, ts);
	}


I also think the call to timeradd() could be removed if an offset
is added to start_cycles.

Regards,
Olivier

Re: [dpdk-dev] [PATCH] net/pcap: support hardware Tx timestamps

[Vivien Didelot]

Hi Olivier,

On Thu, 25 Jun 2020 18:35:32 +0200, Olivier Matz <olivier.matz@6wind.com> wrote:
> As said by Ferruh, the unit of timestamp in mbuf is not normalized to
> nanosecs, as seen in rte_mbuf_core.h:
> 
> 	/** 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;
> 
> Using the timestamp generated from a port which is not a pmd-pcap would
> require a conversion, using rte_eth_read_clock() on mbuf->port (assuming
> it was not modified, which should be true except if event eth Tx adapter
> is used).
> 
> Also, note that the timestamp corresponds to the Rx timestamp. I think it
> could be an issue in case the mbuf is reassembled by the application: the
> timestamp in reassembled mbuf would be the one from the first fragment.
> 
> So, I share Ferruh's concerns.

I think this is not totally true depending on the driver. For instance, we
use mlx5 which already provides a timestamp conversion to nanosecs through
libibverbs. Let me resend this patch alongside Patrick's mlx5 patches that
implemented the needed glue, so that you may understand better the big picture
of how we enabled hardware timestamping in PCAP capture using mlx5 and pdump.


Regards,

	Vivien

Re: [dpdk-dev] [PATCH] net/pcap: support hardware Tx timestamps

[Ferruh Yigit]

On 6/25/2020 7:49 PM, Vivien Didelot wrote:
> Hi Olivier,
> 
> On Thu, 25 Jun 2020 18:35:32 +0200, Olivier Matz <olivier.matz@6wind.com> wrote:
>> As said by Ferruh, the unit of timestamp in mbuf is not normalized to
>> nanosecs, as seen in rte_mbuf_core.h:
>>
>> 	/** 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;
>>
>> Using the timestamp generated from a port which is not a pmd-pcap would
>> require a conversion, using rte_eth_read_clock() on mbuf->port (assuming
>> it was not modified, which should be true except if event eth Tx adapter
>> is used).
>>
>> Also, note that the timestamp corresponds to the Rx timestamp. I think it
>> could be an issue in case the mbuf is reassembled by the application: the
>> timestamp in reassembled mbuf would be the one from the first fragment.
>>
>> So, I share Ferruh's concerns.
> 
> I think this is not totally true depending on the driver. For instance, we
> use mlx5 which already provides a timestamp conversion to nanosecs through
> libibverbs. Let me resend this patch alongside Patrick's mlx5 patches that
> implemented the needed glue, so that you may understand better the big picture
> of how we enabled hardware timestamping in PCAP capture using mlx5 and pdump.
> 

Hi Vivien,

Not sure a specific driver doing the conversion solves the issue. The check is
to 'PKT_RX_TIMESTAMP' which is generic.