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

[Vivien Didelot <vivien.didelot@gmail.com>]

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