>-----Original Message-----
>From: Konstantin Ananyev <konstantin.anan...@huawei.com>
>Sent: Wednesday, October 16, 2024 10:50 AM
>To: Tomasz Duszynski <tduszyn...@marvell.com>; Thomas Monjalon
><tho...@monjalon.net>
>Cc: ruifeng.w...@arm.com; bruce.richard...@intel.com;
>david.march...@redhat.com; dev@dpdk.org;
>Jerin Jacob <jer...@marvell.com>; konstantin.v.anan...@yandex.ru;
>mattias.ronnb...@ericsson.com;
>m...@smartsharesystems.com; roret...@linux.microsoft.com; zhou...@loongson.cn;
>step...@networkplumber.org
>Subject: [EXTERNAL] RE: [PATCH v14 1/4] lib: add generic support for reading
>PMU events
>
>> >> +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
>
>
>
>> >> +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-
>> <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=PZNXgrbjdlXxVEEGYk
>> >xIxRndyEUwWU_ad5ce22YI6Is&m=oJ
>> >-
>> >eSnmJoK0r1zVFhKrkWMnfelOkxqpjtX2fCrXaG2RdWagOqAQ7vcFCJ0dOWrTt&s=TvGxq
>> >QmUz_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://urldefense.proofpoint.com/v2/url?u=https-
>3A__inbox.dpdk.org_dev_6bf789b7ba4e4a8e847431a130372a4b-
>40huawei.com_&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=PZNXgrbjdlXxVEEGYkxIxRndyEUwWU_ad5ce22YI6Is&m=wN1
>qnauDKLK8D51b9VKif_iUKl3vHRCvB4El-B965rNZHUr9xDs6GiBAERI6lHSM&s=J0zulN5Omv_77822arGCtQhab6oa74zF-
>tBWcdU7ZUI&e=
>
>Here is a mail where Morten agreed that it needs to be addressed:
>https://urldefense.proofpoint.com/v2/url?u=https-
>3A__inbox.dpdk.org_dev_98CBD80474FA8B44BF855DF32C47DC35D87792-
>40smartserver.smartshare.dk_&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=PZNXgrbjdlXxVEEGYkxIxRndyEUwWU_ad5
>ce22YI6Is&m=wN1qnauDKLK8D51b9VKif_iUKl3vHRCvB4El-
>B965rNZHUr9xDs6GiBAERI6lHSM&s=aUb6eYuLukWkofeLkMzDbBiafIeQXlKbxnw1OTauaMU&e=
>
>Here is a mail from David, where he summarizes the remaining work required for
>these series:
>https://urldefense.proofpoint.com/v2/url?u=https-
>3A__inbox.dpdk.org_dev_DM4PR18MB43684B889C50F20DDD1B0A29D20CA-
>40DM4PR18MB4368.namprd18.prod.outlook.com_&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=PZNXgrbjdlXxVEEGYkxI
>xRndyEUwWU_ad5ce22YI6Is&m=wN1qnauDKLK8D51b9VKif_iUKl3vHRCvB4El-
>B965rNZHUr9xDs6GiBAERI6lHSM&s=s8o4dWZJPBqtPTYSYB1jT7ufGGu-ADncO6u0ZRgOAuQ&e=
>"...
>- 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://urldefense.proofpoint.com/v2/url?u=https-
>3A__inbox.dpdk.org_dev_DM4PR18MB43684B889C50F20DDD1B0A29D20CA-
>40DM4PR18MB4368.namprd18.prod.outlook.com_&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=PZNXgrbjdlXxVEEGYkxI
>xRndyEUwWU_ad5ce22YI6Is&m=wN1qnauDKLK8D51b9VKif_iUKl3vHRCvB4El-
>B965rNZHUr9xDs6GiBAERI6lHSM&s=s8o4dWZJPBqtPTYSYB1jT7ufGGu-ADncO6u0ZRgOAuQ&e=
>
>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.
>
There are plenty of ways user can shoot shot show own foot and the more checks
gets added to APIs, especially the one that reads event, the more cycle hungry
it gets. So there needs to be some trade-off.
Currently I'm not aiming at supporting everything, rather worker lcores which
are spawned during eal init and say till the end.
As said once basic version gets merged I'll continue work on improving other
areas.
>> 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.
>
Good then. I'll address it earlier than anticipated then.
>> 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.
