RE: [RFC v2 2/2] eal: add high-performance timer facility

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

> From: Mattias Rönnblom [mailto:mattias.ronnblom@ericsson.com]
> Sent: Wednesday, 15 March 2023 18.04

> +++ b/lib/htimer/rte_htimer.h
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2023 Ericsson AB
> + */
> +
> +#ifndef _RTE_HTIMER_H_
> +#define _RTE_HTIMER_H_
> +
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <sys/queue.h>
> +
> +#include <rte_bitops.h>
> +
> +struct rte_htimer;
> +
> +typedef void (*rte_htimer_cb_t)(struct rte_htimer *, void *);
> +
> +struct rte_htimer {
> +	/**
> +	 * Absolute timer expiration time (in ticks).
> +	 */
> +	uint64_t expiration_time;
> +	/**
> +	 * Time between expirations (in ticks). Zero for one-shot timers.
> +	 */
> +	uint64_t period;
> +	/**
> +	 * Owning lcore. May safely be read from any thread.
> +	 */
> +	uint32_t owner_lcore_id;
> +	/**
> +	 * The current state of the timer.
> +	 */
> +	uint32_t state:4;
> +	/**
> +	 * Flags set on this timer.
> +	 */
> +	uint32_t flags:28;
> +	/**
> +	 * User-specified callback function pointer.
> +	 */
> +	rte_htimer_cb_t cb;
> +	/**
> +	 * Argument for user callback.
> +	 */
> +	void *cb_arg;
> +	/**
> +	 * Pointers used to add timer to various internal lists.
> +	 */
> +	LIST_ENTRY(rte_htimer) entry;
> +};

If the rte_htimer structure is supposed to be used in some other data structure, e.g. in a TCP/IP flow structure, it seems unnecessarily bloated.

Generally, if there is no significant performance benefit to the "period" feature, please remove it.

Let's say that this library is used for handling the timers of flows in an IP stack, then the vast majority of timers will be timers related to flows. I would prefer if this high-performance timer library is optimized for such high-volume use cases, rather than offering generic features for low-volume use cases.

And if one HTW instance is used for a single purpose (e.g. the IP stack state machine), both "cb" and "cb_arg" can be removed: The application can derive the pointer to the flow by the using container_of() with the pointer to the rte_htimer, and the cb_arg will effectively be a shadow variable of the flow's state anyway (if not just a pointer to the flow).

Here's an idea, which will offer both: For the high-volume single-purpose use cases you could provide a struct rte_htimer_core without the generic fields, and for the generic use cases, you could provide a struct rte_htimer containing a struct rte_htimer_core and the additional fields for generic use.

> +
> +#define RTE_HTIMER_FLAG_ABSOLUTE_TIME RTE_BIT32(0)
> +#define RTE_HTIMER_FLAG_PERIODICAL RTE_BIT32(1)
> +#define RTE_HTIMER_FLAG_TIME_TICK RTE_BIT32(2)
> +#define RTE_HTIMER_FLAG_TIME_TSC RTE_BIT32(3)
> +
> +#define RTE_HTIMER_STATE_PENDING 1
> +#define RTE_HTIMER_STATE_EXPIRED 2
> +#define RTE_HTIMER_STATE_CANCELED 3
> +
> +LIST_HEAD(rte_htimer_list, rte_htimer);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_HTIMER_H_ */