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

[lihuisong (C)]

在 2023/2/9 1:04, Robin Jarry 写道:
> 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.
ok, this patch also use lcore_role_str function. please ignore this comment.
>
>>> +	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.
That's not a very good reason.
It's similar to "ret != 0" and "p != NULL" in DPDK coding style.
>
> .