RE: [PATCH] net/af_packet: add timestamp offloading support

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Stefan Laesser [mailto:stefan.laesser@omicronenergy.com]
> Sent: Tuesday, 3 September 2024 13.43
> 
> Add the packet timestamp from TPACKET_V2 to the mbuf
> dynamic rx timestamp register if offload RTE_ETH_RX_OFFLOAD_TIMESTAMP
> is enabled.
> 
> TPACKET_V2 provides the timestamp with nanosecond resolution.

Suggest adding: and UNIX origo, i.e. time since 1-JAN-1970 UTC.

> 
> Signed-off-by: Stefan Laesser <stefan.laesser@omicronenergy.com>
> ---


> +uint64_t af_packet_timestamp_dynflag;
> +int af_packet_timestamp_dynfield_offset = -1;

No need to expose these publicly, they should be static.
This also means that you can remove the af_packet_ prefix.


>  eth_dev_start(struct rte_eth_dev *dev)
>  {
>  	struct pmd_internals *internals = dev->data->dev_private;
> -	uint16_t i;
> +
> +	if (internals->timestamp_offloading) {
> +		/* Register mbuf field and flag for Rx timestamp */
> +		int rc =
> rte_mbuf_dyn_rx_timestamp_register(&af_packet_timestamp_dynfield_offset,
> +				&af_packet_timestamp_dynflag);
> +		if (rc) {
> +			PMD_LOG(ERR, "Cannot register mbuf field/flag for
> timestamp");
> +			return rc;
> +		}
> +	}
> 
>  	dev->data->dev_link.link_status = RTE_ETH_LINK_UP;
> +
> +	uint16_t i;

No need to move this down here. Please leave it where it was.

Alternatively, minimize its scope by moving it inside the for loop:
-	for (i = 0; i < internals->nb_queues; i++) {
+	for (uint16_t i = 0; i < internals->nb_queues; i++) {


With the two variables made local,

Acked-by: Morten Brørup <mb@smartsharesystems.com>