On 2023-03-22 13:18, Morten Brørup wrote: >> From: Mattias Rönnblom [mailto:mattias.ronnb...@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_ */
