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: Monday, 3 April 2023 14.04
> 
> 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

Sounds good, Mattias. Looking forward to reviewing the next version. :-)