On Fri, 24 Jul 2020 16:23:15 -0400
Patrick Keroulas <patrick.kerou...@radio-canada.ca> wrote:

> From: Vivien Didelot <vivien.dide...@gmail.com>
> 
> 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.dide...@gmail.com>
> Signed-off-by: Patrick Keroulas <patrick.kerou...@radio-canada.ca>
> ---
>  doc/guides/rel_notes/release_20_08.rst |  1 +
>  drivers/net/pcap/rte_eth_pcap.c        | 32 +++++++++++++-----------
>  lib/librte_pdump/rte_pdump.c           | 34 ++++++++++++++------------
>  3 files changed, 37 insertions(+), 30 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_20_08.rst 
> b/doc/guides/rel_notes/release_20_08.rst
> index 89822bcb8d..bda2f7567a 100644
> --- a/doc/guides/rel_notes/release_20_08.rst
> +++ b/doc/guides/rel_notes/release_20_08.rst
> @@ -105,6 +105,7 @@ New Features
>    Updated PCAP driver with new features and improvements, including:
>  
>    * Support software Tx nanosecond timestamps precision.
> +  * Support hardware Tx timestamps.
>  
>  * **Updated Broadcom bnxt driver.**
>  
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index 668cbd1fc7..45775951f7 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;
> -
> -     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;
> +calculate_timestamp(const struct rte_mbuf *mbuf, struct timeval *ts) {

DPDK style is to have { on seperate line

> +     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,
> +             };
> +
> +             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);

On Tx pcap you don't want the hardware RX timestamp.
You should update mbuf with software timestamp.

>               header.len = len;
>               header.caplen = caplen;
>               /* rte_pktmbuf_read() returns a pointer to the data directly
> diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
> index 8fd7e13af5..e8963e9dd8 100644
> --- a/lib/librte_pdump/rte_pdump.c
> +++ b/lib/librte_pdump/rte_pdump.c
> @@ -71,21 +71,6 @@ static struct pdump_rxtx_cbs {
>  } rx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT],
>  tx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
>  
> -static uint64_t hz;
> -static uint64_t start_time;
> -static uint64_t start_cycles;
> -
> -static inline void
> -pdump_ts_to_ns(struct rte_mbuf **pkts, uint16_t nb_pkts)
> -{
> -     unsigned int i;
> -
> -     for (i = 0; i < nb_pkts; i++) {
> -             if ((pkts[i]->ol_flags & PKT_RX_TIMESTAMP) && hz)
> -                     pkts[i]->timestamp = start_time +
> -                              (pkts[i]->timestamp - start_cycles) * NS_PER_S 
> / hz;
> -     }
> -}
>  
>  static inline void
>  pdump_copy(struct rte_mbuf **pkts, uint16_t nb_pkts, void *user_params)
> @@ -118,6 +103,23 @@ pdump_copy(struct rte_mbuf **pkts, uint16_t nb_pkts, 
> void *user_params)
>       }
>  }
>  
> +#define NSEC_PER_SEC 1000000000L
> +static uint64_t hz;
> +static uint64_t start_time;
> +static uint64_t start_cycles;
> +
> +static inline void
> +pdump_ts_to_ns(struct rte_mbuf **pkts, uint16_t nb_pkts)
> +{
> +     unsigned int i;
> +
> +     for (i = 0; i < nb_pkts; i++) {
> +             if ((pkts[i]->ol_flags & PKT_RX_TIMESTAMP) && hz)
> +                     pkts[i]->timestamp = start_time +
> +                              (pkts[i]->timestamp - start_cycles) * 
> NSEC_PER_SEC / hz;
> +     }
> +}
> +
>  static uint16_t
>  pdump_rx(uint16_t port __rte_unused, uint16_t qidx __rte_unused,
>       struct rte_mbuf **pkts, uint16_t nb_pkts,
> @@ -154,7 +156,7 @@ pdump_register_rx_callbacks(uint16_t end_q, uint16_t 
> port, uint16_t queue,
>                       rte_eth_read_clock(port, &start_cycles);
>                       rte_eth_get_clock_freq(port, &hz);
>                       gettimeofday(&now, NULL);
> -                     start_time = now.tv_sec * NS_PER_S + now.tv_usec * 1000;
> +                     start_time = now.tv_sec * NSEC_PER_SEC + now.tv_usec * 
> 1000;
>  
>                       if (cbs->cb) {
>                               PDUMP_LOG(ERR,


Actually using the current way through rte_eth_pcap adds a lot
of overhead. I dropped it when doing pcapng.

Perhaps we should work together on the pcapng version.

Reply via email to