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

["Mattias Rönnblom" <mattias.ronnblom@ericsson.com>]

On 2023-03-22 13:18, Morten Brørup wrote:
>> 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.
> 
>> 

Good points.

I will look into:
a) making <rte_htw.h> public
b) split rte_htimer into two timer structs (where the now-public 
rte_htw_timer struct may be used from the rte_htimer_timer struct).
c) ...where the htw timer struct won't have any callbacks
d) merge rte_htimer_timer.h into rte_htimer.h.
e) remove the periodic feature, at least from the core timer wheel

+
>> +#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_ */