Re: [dpdk-dev] [PATCH v2 3/3] rxtx_callbacks: Add support for HW timestamp

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 3/27/2019 6:19 AM, Tom Barbette wrote:
> Use rxtx callback to demonstrate a way to use rte_eth_read_clock to
> convert the hardware timestamps to an amount of cycles.
> 
> This allows to get the amount of time the packet spent since its entry
> in the device. While the regular latency only shows the latency from
> when it entered the software stack.
> 
> Signed-off-by: Tom Barbette <barbette@kth.se>
<...>

> @@ -50,6 +50,8 @@ include $(RTE_SDK)/mk/rte.vars.mk
>  
>  CFLAGS += $(WERROR_FLAGS)
>  
> +
> +CFLAGS += -DALLOW_EXPERIMENTAL_API

Can you please add the experimental API as a comment.

>  # workaround for a gcc bug with noreturn attribute
>  # http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12603
>  ifeq ($(CONFIG_RTE_TOOLCHAIN_GCC),y)
> diff --git a/examples/rxtx_callbacks/main.c b/examples/rxtx_callbacks/main.c
> index 2058be627..7000464e1 100644
> --- a/examples/rxtx_callbacks/main.c
> +++ b/examples/rxtx_callbacks/main.c
> @@ -10,6 +10,8 @@
>  #include <rte_lcore.h>
>  #include <rte_mbuf.h>
>  
> +#include <getopt.h>

libc includes are already together above, can you move this above

> +
>  #define RX_RING_SIZE 1024
>  #define TX_RING_SIZE 1024
>  
> @@ -17,6 +19,9 @@
>  #define MBUF_CACHE_SIZE 250
>  #define BURST_SIZE 32
>  
> +static const char usage[] =
> +	"%s EAL_ARGS -- [-t]\n";
> +
>  static const struct rte_eth_conf port_conf_default = {
>  	.rxmode = {
>  		.max_rx_pkt_len = ETHER_MAX_LEN,
> @@ -25,9 +30,14 @@ static const struct rte_eth_conf port_conf_default = {
>  
>  static struct {
>  	uint64_t total_cycles;
> +	uint64_t total_queue_cycles;
>  	uint64_t total_pkts;
>  } latency_numbers;
>  
> +int hw_timestamping;
> +
> +#define TICKS_PER_CYCLE_SHIFT 16
> +uint64_t ticks_per_cycle_mult;

I am aware this is single .c file application, but as a good practice can you
please make above global variables static.

<...>

> @@ -95,9 +126,20 @@ port_init(uint16_t port, struct rte_mempool *mbuf_pool)
>  	if (retval != 0)
>  		return retval;
>  
> +	rxconf = dev_info.default_rxconf;
> +
> +	if (hw_timestamping) {
> +		if (!(dev_info.rx_offload_capa & DEV_RX_OFFLOAD_TIMESTAMP)) {
> +			printf("\nERROR: Port %u does not support hardware timestamping\n"
> +					, port);
> +			return -1;
> +		}
> +		rxconf.offloads |= DEV_RX_OFFLOAD_TIMESTAMP;
> +	}

Same comment as ethdev one, above code assume if driver announces
'DEV_RX_OFFLOAD_TIMESTAMP' capability, it have to implement 'read_clock'
dev_ops, should it be the case?

Write now only mlx implements it so this is not a problem at all, but I don't
know if all PMDs supports timestamping packets must implement 'read_clock'.

<...>

> @@ -6,6 +6,7 @@
>  # To build this example as a standalone application with an already-installed
>  # DPDK instance, use 'make'
>  
> +allow_experimental_apis = true

Can you please add the experimental API as a comment.

>  sources = files(
>  	'main.c'
>  )
>