Re: [dpdk-dev] [PATCH v4 1/3] rte: add keep alive functionality

[Stephen Hemminger <stephen@networkplumber.org>]

In general, try not to introduce new thinks and avoid extra code.
Also Linux has more robust mechanism in the watchdog timers, why not use that?

Could you use RTE_PER_LCORE some how?




> +#ifdef KEEPALIVE_DEBUG_MSGS
Any #ifdef must have a config option to enable it.

> +static void
> +print_trace(const char *msg, struct rte_keepalive *keepcfg, int idx_core)
> +{
> +	printf("%sLast seen %" PRId64  "ms ago.\n",
> +		msg,
> +		((rte_rdtsc() - keepcfg->last_alive[idx_core])*1000)
> +		/ rte_get_tsc_hz()
> +	      );
> +}
> +#else
> +static void
> +print_trace(__attribute__((unused)) const char *msg,
> +	__attribute__((unused)) struct rte_keepalive *keepcfg,
> +	__attribute__((unused)) int idx_core)
> +{
> +}
> +#endif

Agree with others don't introduce not logging macros.


> +void
> +rte_keepalive_dispatch_pings(__attribute__((unused)) void *ptr_timer,

Use __rte_unused
> +	void *ptr_data)
> +{
> +	struct rte_keepalive *keepcfg = (struct rte_keepalive *)ptr_data;

Cast of void  * is unnecessary in C.

> +	int idx_core;
> +
> +	for (idx_core = 0; idx_core < RTE_KEEPALIVE_MAXCORES; idx_core++) {
> +		if (keepcfg->active_cores[idx_core] == 0)
> +			continue;
> +		switch (keepcfg->state_flags[idx_core]) {

My personal preference, prefer blank line after continue.

> +		case ALIVE: /* Alive */
> +			keepcfg->state_flags[idx_core] = 0;
> +			keepcfg->last_alive[idx_core] = rte_rdtsc();
> +			break;
> +		case MISSING: /* MIA */
> +			print_trace("Core MIA. ", keepcfg, idx_core);
> +			keepcfg->state_flags[idx_core] = 2;
> +			break;
> +		case DEAD: /* Dead */
> +			keepcfg->state_flags[idx_core] = 3;
> +			print_trace("Core died. ", keepcfg, idx_core);
> +			if (keepcfg->callback)
> +				keepcfg->callback(
> +					keepcfg->callback_data,
> +					idx_core
> +					);
> +			break;
> +		case GONE: /* Buried */
> +			break;
> +		}
> +	}
> +}
> +
> +
> +struct rte_keepalive *
> +rte_keepalive_create(rte_keepalive_failure_callback_t callback,
> +	void *data)
> +{
> +	int idx_core;
> +	struct rte_keepalive *keepcfg;
> +
> +	keepcfg = malloc(sizeof(struct rte_keepalive));

Why not use rte_zmalloc()?

> +	if (keepcfg != NULL) {
> +		for (idx_core = 0;
> +				idx_core < RTE_KEEPALIVE_MAXCORES;
> +				idx_core++) {
> +			keepcfg->state_flags[idx_core] = 0;
> +			keepcfg->active_cores[idx_core] = 0;
> +		}
> +		keepcfg->callback = callback;
> +		keepcfg->callback_data = data;
> +		keepcfg->tsc_initial = rte_rdtsc();
> +		keepcfg->tsc_mhz = rte_get_tsc_hz() / 1000;
> +	}
> +	return keepcfg;
> +}
> +
> +
> +void
> +rte_keepalive_register_core(struct rte_keepalive *keepcfg, const int id_core)
> +{
> +	if (id_core < RTE_KEEPALIVE_MAXCORES)
> +		keepcfg->active_cores[id_core] = 1;
> +}