DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/pcap: support hardware Tx timestamps
@ 2020-06-10 19:39 Vivien Didelot
  2020-06-16 19:02 ` Vivien Didelot
  2020-06-17  8:16 ` Ferruh Yigit
  0 siblings, 2 replies; 7+ messages in thread
From: Vivien Didelot @ 2020-06-10 19:39 UTC (permalink / raw)
  To: dev; +Cc: Ferruh Yigit, patrick.keroulas, 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] net/pcap: support hardware Tx timestamps
  2020-06-10 19:39 [dpdk-dev] [PATCH] net/pcap: support hardware Tx timestamps Vivien Didelot
@ 2020-06-16 19:02 ` Vivien Didelot
  2020-06-17  8:16 ` Ferruh Yigit
  1 sibling, 0 replies; 7+ messages in thread
From: Vivien Didelot @ 2020-06-16 19:02 UTC (permalink / raw)
  To: dev; +Cc: Ferruh Yigit, patrick.keroulas

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] net/pcap: support hardware Tx timestamps
  2020-06-10 19:39 [dpdk-dev] [PATCH] net/pcap: support hardware Tx timestamps Vivien Didelot
  2020-06-16 19:02 ` Vivien Didelot
@ 2020-06-17  8:16 ` Ferruh Yigit
  2020-06-23 22:10   ` Vivien Didelot
  1 sibling, 1 reply; 7+ messages in thread
From: Ferruh Yigit @ 2020-06-17  8:16 UTC (permalink / raw)
  To: Vivien Didelot, dev; +Cc: patrick.keroulas, Olivier MATZ

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] net/pcap: support hardware Tx timestamps
  2020-06-17  8:16 ` Ferruh Yigit
@ 2020-06-23 22:10   ` Vivien Didelot
  2020-06-25 16:35     ` Olivier Matz
  0 siblings, 1 reply; 7+ messages in thread
From: Vivien Didelot @ 2020-06-23 22:10 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: dev, PATRICK KEROULAS, Olivier MATZ

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
>
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] net/pcap: support hardware Tx timestamps
  2020-06-23 22:10   ` Vivien Didelot
@ 2020-06-25 16:35     ` Olivier Matz
  2020-06-25 18:49       ` Vivien Didelot
  0 siblings, 1 reply; 7+ messages in thread
From: Olivier Matz @ 2020-06-25 16:35 UTC (permalink / raw)
  To: Vivien Didelot; +Cc: Ferruh Yigit, dev, PATRICK KEROULAS

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] net/pcap: support hardware Tx timestamps
  2020-06-25 16:35     ` Olivier Matz
@ 2020-06-25 18:49       ` Vivien Didelot
  2020-06-26 13:52         ` Ferruh Yigit
  0 siblings, 1 reply; 7+ messages in thread
From: Vivien Didelot @ 2020-06-25 18:49 UTC (permalink / raw)
  To: Olivier Matz; +Cc: Ferruh Yigit, dev, PATRICK KEROULAS

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] net/pcap: support hardware Tx timestamps
  2020-06-25 18:49       ` Vivien Didelot
@ 2020-06-26 13:52         ` Ferruh Yigit
  0 siblings, 0 replies; 7+ messages in thread
From: Ferruh Yigit @ 2020-06-26 13:52 UTC (permalink / raw)
  To: Vivien Didelot, Olivier Matz; +Cc: dev, PATRICK KEROULAS

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-06-30  7:32 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-10 19:39 [dpdk-dev] [PATCH] net/pcap: support hardware Tx timestamps Vivien Didelot
2020-06-16 19:02 ` Vivien Didelot
2020-06-17  8:16 ` Ferruh Yigit
2020-06-23 22:10   ` Vivien Didelot
2020-06-25 16:35     ` Olivier Matz
2020-06-25 18:49       ` Vivien Didelot
2020-06-26 13:52         ` Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).