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

[Thomas Monjalon <thomas@monjalon.net>]

02/11/2020 16:39, Olivier Matz:
> 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
> > +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)

Yes we can have a function in rte_mbuf_dyn.h/c which is called
to register field and flags from drivers or apps.
Then no need to register at ethdev level.