Re: [dpdk-dev] [PATCH v2 03/14] ethdev: register mbuf field and flags for timestamp

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

On Sun, Nov 01, 2020 at 07:06:15PM +0100, Thomas Monjalon wrote:
> During port configure or queue setup, the offload flags
> DEV_RX_OFFLOAD_TIMESTAMP and DEV_TX_OFFLOAD_SEND_ON_TIMESTAMP
> trigger the registration of the related mbuf field and flags.
> 
> Previously, the Tx timestamp field and flag were registered in testpmd,
> as described in mlx5 guide.
> For the general usage of Rx and Tx timestamps,
> managing registrations inside ethdev is simpler and properly documented.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

<...>

> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -31,6 +31,7 @@
>  #include <rte_mempool.h>
>  #include <rte_malloc.h>
>  #include <rte_mbuf.h>
> +#include <rte_mbuf_dyn.h>
>  #include <rte_errno.h>
>  #include <rte_spinlock.h>
>  #include <rte_string_fns.h>
> @@ -1232,6 +1233,54 @@ eth_dev_check_lro_pkt_size(uint16_t port_id, uint32_t config_size,
>  	return ret;
>  }
>  
> +static inline int
> +eth_dev_timestamp_mbuf_register(uint64_t rx_offloads, uint64_t tx_offloads)
> +{
> +	static const struct rte_mbuf_dynfield field_desc = {
> +		.name = RTE_MBUF_DYNFIELD_TIMESTAMP_NAME,
> +		.size = sizeof(rte_mbuf_timestamp_t),
> +		.align = __alignof__(rte_mbuf_timestamp_t),
> +	};
> +	static const struct rte_mbuf_dynflag rx_flag_desc = {
> +		.name = RTE_MBUF_DYNFLAG_RX_TIMESTAMP_NAME,
> +	};
> +	static const struct rte_mbuf_dynflag tx_flag_desc = {
> +		.name = RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME,
> +	};
> +	bool todo_rx, todo_tx;
> +	int offset;
> +
> +	todo_rx = (rx_offloads & DEV_RX_OFFLOAD_TIMESTAMP) != 0;
> +	todo_tx = (tx_offloads & DEV_TX_OFFLOAD_SEND_ON_TIMESTAMP) != 0;
> +
> +	if (todo_rx || todo_tx) {
> +		offset = rte_mbuf_dynfield_register(&field_desc);
> +		if (offset < 0) {
> +			RTE_ETHDEV_LOG(ERR,
> +					"Failed to register mbuf field for timestamp\n");
> +			return -rte_errno;
> +		}
> +	}
> +	if (todo_rx) {
> +		offset = rte_mbuf_dynflag_register(&rx_flag_desc);
> +		if (offset < 0) {
> +			RTE_ETHDEV_LOG(ERR,
> +					"Failed to register mbuf flag for Rx timestamp\n");
> +			return -rte_errno;
> +		}
> +	}
> +	if (todo_tx) {
> +		offset = rte_mbuf_dynflag_register(&tx_flag_desc);
> +		if (offset < 0) {
> +			RTE_ETHDEV_LOG(ERR,
> +					"Failed to register mbuf flag for Tx timestamp\n");
> +			return -rte_errno;
> +		}
> +	}
> +
> +	return 0;
> +}

The code that registers the dynamic fields and flags for timestamp is
more or less duplicated several times in the patchset. As discussed
privately, it would make sense to have helpers to register them in one
operation, without the need to redeclare the structures:

  int rte_mbuf_dyn_rx_timestamp_register(int *offset, uint64_t *rx_bitnum)
  int rte_mbuf_dyn_tx_timestamp_register(int *offset, uint64_t *tx_bitnum)