Re: [PATCH v9 1/5] eal: add lcore info in telemetry

["Robin Jarry" <rjarry@redhat.com>]

Hi lihuisong,

lihuisong (C), Feb 08, 2023 at 03:24:
> >   static int
> >   lcore_dump_cb(unsigned int lcore_id, void *arg)
> >   {
> >   	struct rte_config *cfg = rte_eal_get_configuration();
> >   	char cpuset[RTE_CPU_AFFINITY_STR_LEN];
> > -	const char *role;
> >   	FILE *f = arg;
> >   	int ret;
> >   
> > -	switch (cfg->lcore_role[lcore_id]) {
> > -	case ROLE_RTE:
> > -		role = "RTE";
> > -		break;
> > -	case ROLE_SERVICE:
> > -		role = "SERVICE";
> > -		break;
> > -	case ROLE_NON_EAL:
> > -		role = "NON_EAL";
> > -		break;
> > -	default:
> > -		role = "UNKNOWN";
> > -		break;
> > -	}
> > -
> >   	ret = eal_thread_dump_affinity(&lcore_config[lcore_id].cpuset, cpuset,
> >   		sizeof(cpuset));
> >   	fprintf(f, "lcore %u, socket %u, role %s, cpuset %s%s\n", lcore_id,
> > -		rte_lcore_to_socket_id(lcore_id), role, cpuset,
> > -		ret == 0 ? "" : "...");
> > +		rte_lcore_to_socket_id(lcore_id),
> > +		lcore_role_str(cfg->lcore_role[lcore_id]),
> > +		cpuset, ret == 0 ? "" : "...");
> >   	return 0;
> >   }
> The above modification doesn't seem to be related to this patch.
> Suggest remove or delete it from this patch.

I was asked in an earlier review to factorize this into an helper to
avoid code duplication.

> > +	if (info->lcore_id != lcore_id)
>
> Suggest: info->lcore_id != lcore_id -> lcore_id != info->lcore_id
> Here, info->lcore_id is a target and lcore_id is the variable to be
> judged, right?

Yeah that looks better. I didn't pay too much attention since this
principle is not well respected in the current code base.