RE: [PATCH v14 1/4] lib: add generic support for reading PMU events

[Konstantin Ananyev <konstantin.ananyev@huawei.com>]

> >> +int
> >> +__rte_pmu_enable_group(void)
> >> +{
> >> +	struct rte_pmu_event_group *group = &RTE_PER_LCORE(_event_group);
> >> +	int ret;
> >> +
> >> +	if (rte_pmu.num_group_events == 0)
> >> +		return -ENODEV;
> >> +
> >> +	ret = open_events(group);
> >> +	if (ret)
> >> +		goto out;
> >> +
> >> +	ret = mmap_events(group);
> >> +	if (ret)
> >> +		goto out;
> >> +
> >> +	if (ioctl(group->fds[0], PERF_EVENT_IOC_RESET, PERF_IOC_FLAG_GROUP) == -1) {
> >> +		ret = -errno;
> >> +		goto out;
> >> +	}
> >> +
> >> +	if (ioctl(group->fds[0], PERF_EVENT_IOC_ENABLE, PERF_IOC_FLAG_GROUP) == -1) {
> >> +		ret = -errno;
> >> +		goto out;
> >> +	}
> >> +
> >> +	rte_spinlock_lock(&rte_pmu.lock);
> >> +	TAILQ_INSERT_TAIL(&rte_pmu.event_group_list, group, next);
> >> +	rte_spinlock_unlock(&rte_pmu.lock);
> >
> >I thought that after previous round of reviews, we got a consensus that it is a bad idea to insert
> >pointer of TLS variable into the global list:
> >https://urldefense.proofpoint.com/v2/url?u=https-
> >3A__patchwork.dpdk.org_project_dpdk_patch_20230216175502.3164820-2D2-2Dtduszynski-
> >40marvell.com_&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=PZNXgrbjdlXxVEEGYkxIxRndyEUwWU_ad5ce22YI6Is&m=oJ
> >-
> >eSnmJoK0r1zVFhKrkWMnfelOkxqpjtX2fCrXaG2RdWagOqAQ7vcFCJ0dOWrTt&s=TvGxqQmUz_U3xLOroMxOmsCiaxdqbNLi6GZ
> >pHIefniw&e=
> 
> I don't think there was any consensus. It was rather your point of view solely. 

Here is a mail where I highlighted the problem:
https://inbox.dpdk.org/dev/6bf789b7ba4e4a8e847431a130372a4b@huawei.com/

Here is a mail where Morten agreed that it needs to be addressed:
https://inbox.dpdk.org/dev/98CBD80474FA8B44BF855DF32C47DC35D87792@smartserver.smartshare.dk/

Here is a mail from David, where he summarizes the remaining work required for these series:
https://inbox.dpdk.org/dev/DM4PR18MB43684B889C50F20DDD1B0A29D20CA@DM4PR18MB4368.namprd18.prod.outlook.com/ 
"...
- Konstantin asked for better explanations in the implementation.
- He also pointed out at using this feature with non EAL lcores.
..."

Here is your reply:
https://inbox.dpdk.org/dev/DM4PR18MB43684B889C50F20DDD1B0A29D20CA@DM4PR18MB4368.namprd18.prod.outlook.com/

You didn't object, so  I interpreted that (probably wrongly) that we had a consensus here.  
 
> So I still believe
> that given currently that only runs on lcores and lcores do not terminate before the main
> one hence it's safe to access TLS from a main.

Could you clarify what you means by 'lcores' here?
Threads with valid 'lcore_id'?
There is an API within DPDK that allows user
to assign vacant lcore_id ((rte_thread_register) to some new thread, run it for some time,
then release lcore_id (rte_thread_unregister) and then terminate the thread.
 
Another thing - there is no checks within PMU API around  does this thread satisfy expected criteria's or not.
So if user will call PMU API from 'wrong' thread - it would succeed and for many cases  will keep working
as expected. But for other cases (thread lifetime is shorter then program lifetime) it might cause a crash or
silent memory corruption.

> Once support for running on anything other than lcore gets added current mechanics is likely to change.

Honestly, using TLS variables within the global list here - is just asking for trouble.
Please fix it, there are plenty of simple ways to avoid such issue once and for all. 
Without this problem addressed, my vote for that patch is NACK.

> And as you pointed out, there are plenty of options available.
> 
> >As I said before - I believe it is error prone, and needs to be addressed.
> >There are plenty of other ways to have one object per lcore (if that's really necessary):
> >array of RTE_MAX_LCORE indexed by lcore_id, or new rte_lcore_var.h API introduced by Mattias
> >recently, or even simple TLS pointer with malloc() behind would be better.