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;
> +}


Reply via email to