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

[Olivier Matz <olivier.matz@6wind.com>]

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