> From: Mattias Rönnblom [mailto:mattias.ronnb...@ericsson.com] > Sent: Monday, 3 April 2023 14.04 > > 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
Sounds good, Mattias. Looking forward to reviewing the next version. :-)
