From 3e9e0c7f5d1a54b7688924243cc14707906a14cf Mon Sep 17 00:00:00 2001 From: eho <2220386943@qq.com> Date: Wed, 25 Sep 2024 16:34:45 +0800 Subject: [PATCH 03/20] fix after pmuopen delete evtList exception if you delete the pointer pointed to by attr.evtList immediately after pmuuopen is successful, the event will be garbled. --- build/common.sh | 4 ++ pmu/pmu.cpp | 108 +++++++++++++++++++++++----------- pmu/pmu_list.cpp | 26 +++++--- pmu/pmu_list.h | 6 +- test/test_perf/test_count.cpp | 57 ++++++++++++++++++ 5 files changed, 155 insertions(+), 46 deletions(-) diff --git a/build/common.sh b/build/common.sh index 9cc16b0..f48b64e 100644 --- a/build/common.sh +++ b/build/common.sh @@ -15,6 +15,10 @@ set -e cpu_core_num=$(($(nproc)-1)) +if [ "$cpu_core_num" -eq 0 ];then + cpu_core_num=1 +fi + creat_dir(){ local target_dir="$1" if [ -d "${target_dir}" ];then diff --git a/pmu/pmu.cpp b/pmu/pmu.cpp index b7507ec..207d4fb 100644 --- a/pmu/pmu.cpp +++ b/pmu/pmu.cpp @@ -160,17 +160,29 @@ static int CheckAttr(enum PmuTaskType collectType, struct PmuAttr *attr) return LIBPERF_ERR_INVALID_SAMPLE_RATE; } - if ((collectType == SAMPLING || collectType == COUNTING) && attr->evtAttr == nullptr) { - struct EvtAttr *evtAttr = new struct EvtAttr[attr->numEvt]; + return SUCCESS; +} + +static void CopyAttrData(PmuAttr* newAttr, PmuAttr* inputAttr, enum PmuTaskType collectType) +{ + //Coping event data to prevent delete exceptions + if (inputAttr->numEvt > 0) { + char **newEvtList = new char *[inputAttr->numEvt]; + for (int i = 0; i < inputAttr->numEvt; ++i) { + newEvtList[i] = new char[strlen(inputAttr->evtList[i]) + 1]; + strcpy(newEvtList[i], inputAttr->evtList[i]); + } + newAttr->evtList = newEvtList; + } + + if ((collectType == SAMPLING || collectType == COUNTING) && inputAttr->evtAttr == nullptr) { + struct EvtAttr *evtAttr = new struct EvtAttr[inputAttr->numEvt]; // handle event group id. -1 means that it doesn't run event group feature. - for (int i = 0; i < attr->numEvt; ++i) { + for (int i = 0; i < inputAttr->numEvt; ++i) { evtAttr[i].group_id = -1; } - attr->evtAttr = evtAttr; - + newAttr->evtAttr = evtAttr; } - - return SUCCESS; } static bool FreeEvtAttr(struct PmuAttr *attr) @@ -196,6 +208,21 @@ static bool FreeEvtAttr(struct PmuAttr *attr) return SUCCESS; } +static void FreeEvtList(unsigned evtNum, char** evtList) +{ + if (!evtList) { + return; + } + for (int i = 0; i < evtNum; i++) { + if (evtList[i]) { + delete[] evtList[i]; + evtList[i] = nullptr; + } + } + delete[] evtList; + evtList = nullptr; +} + static bool AppendChildEvents(char* evt, unordered_map& eventSplitMap) { string strName(evt); @@ -304,51 +331,64 @@ static void PmuTaskAttrFree(PmuTaskAttr *taskAttr) int PmuOpen(enum PmuTaskType collectType, struct PmuAttr *attr) { SetWarn(SUCCESS); + PmuAttr copiedAttr = {*attr}; + pair previousEventList = {0, nullptr}; try { auto err = CheckAttr(collectType, attr); if (err != SUCCESS) { return -1; } + CopyAttrData(&copiedAttr, attr, collectType); + previousEventList = make_pair(copiedAttr.numEvt, copiedAttr.evtList); + int pd = -1; unordered_map eventSplitMap; - if (!SplitUncoreEvent(attr, eventSplitMap)) { - return -1; - } - auto previousEventList = make_pair(attr->numEvt, attr->evtList); - vector newEvtlist; - vector newEvtAttrList; - auto numEvt = GenerateSplitList(eventSplitMap, newEvtlist, attr, newEvtAttrList); - FreeEvtAttr(attr); - attr->numEvt = numEvt; - attr->evtList = newEvtlist.data(); - attr->evtAttr = newEvtAttrList.data(); - - auto pTaskAttr = AssignPmuTaskParam(collectType, attr); - if (pTaskAttr == nullptr) { - return -1; - } - unique_ptr taskAttr(pTaskAttr, PmuTaskAttrFree); + do { + if (!SplitUncoreEvent(&copiedAttr, eventSplitMap)) { + break; + } + vector newEvtlist; + vector newEvtAttrList; + auto numEvt = GenerateSplitList(eventSplitMap, newEvtlist, &copiedAttr, newEvtAttrList); + FreeEvtAttr(&copiedAttr); + copiedAttr.numEvt = numEvt; + copiedAttr.evtList = newEvtlist.data(); + copiedAttr.evtAttr = newEvtAttrList.data(); + + auto pTaskAttr = AssignPmuTaskParam(collectType, &copiedAttr); + if (pTaskAttr == nullptr) { + break; + } + unique_ptr taskAttr(pTaskAttr, PmuTaskAttrFree); + + pd = KUNPENG_PMU::PmuList::GetInstance()->NewPd(); + if (pd == -1) { + New(LIBPERF_ERR_NO_AVAIL_PD); + break; + } + + KUNPENG_PMU::PmuList::GetInstance()->SetSymbolMode(pd, attr->symbolMode); + err = KUNPENG_PMU::PmuList::GetInstance()->Register(pd, taskAttr.get()); + if (err != SUCCESS) { + PmuList::GetInstance()->Close(pd); + pd = -1; + } + New(err); + } while(false); - auto pd = KUNPENG_PMU::PmuList::GetInstance()->NewPd(); if (pd == -1) { - New(LIBPERF_ERR_NO_AVAIL_PD); + FreeEvtList(previousEventList.first, previousEventList.second); return -1; } - - KUNPENG_PMU::PmuList::GetInstance()->SetSymbolMode(pd, attr->symbolMode); - err = KUNPENG_PMU::PmuList::GetInstance()->Register(pd, taskAttr.get()); - if (err != SUCCESS) { - PmuList::GetInstance()->Close(pd); - pd = -1; - } // store eventList provided by user and the mapping relationship between the user eventList and the split // eventList into buff KUNPENG_PMU::PmuList::GetInstance()->StoreSplitData(pd, previousEventList, eventSplitMap); - New(err); return pd; } catch (std::bad_alloc&) { + FreeEvtList(previousEventList.first, previousEventList.second); New(COMMON_ERR_NOMEM); return -1; } catch (exception& ex) { + FreeEvtList(previousEventList.first, previousEventList.second); New(UNKNOWN_ERROR, ex.what()); return -1; } diff --git a/pmu/pmu_list.cpp b/pmu/pmu_list.cpp index 0caba7b..4f293a7 100644 --- a/pmu/pmu_list.cpp +++ b/pmu/pmu_list.cpp @@ -283,8 +283,8 @@ namespace KUNPENG_PMU { return SUCCESS; } - void PmuList::StoreSplitData(const unsigned pd, pair previousEventList, - unordered_map eventSplitMap) + void PmuList::StoreSplitData(const unsigned pd, pair& previousEventList, + unordered_map& eventSplitMap) { lock_guard lg(dataParentMtx); parentEventMap.emplace(pd, move(eventSplitMap)); @@ -303,7 +303,7 @@ namespace KUNPENG_PMU { RemoveEpollFd(pd); EraseSpeCpu(pd); EraseDummyEvent(pd); - EraseParentEventMap(); + EraseParentEventMap(pd); SymResolverDestroy(); PmuEventListFree(); PointerPasser::FreeRawFieldMap(); @@ -409,15 +409,23 @@ namespace KUNPENG_PMU { return dataEvtGroupList[pd]; } - void PmuList::EraseParentEventMap() + void PmuList::EraseParentEventMap(const unsigned pd) { lock_guard lg(dataParentMtx); - for (auto& pair: parentEventMap) { - auto& innerMap = pair.second; - innerMap.clear(); + auto iter = parentEventMap.find(pd); + if (iter != parentEventMap.end()) { + parentEventMap.at(pd).clear(); + parentEventMap.erase(iter); + } + auto preIter = previousEventMap.find(pd); + if (preIter != previousEventMap.end()) { + auto pair = previousEventMap.at(pd); + for (int i = 0; i < pair.first; i++) { + delete[] pair.second[i]; + } + delete[] pair.second; + previousEventMap.erase(preIter); } - parentEventMap.clear(); - previousEventMap.clear(); } PmuList::EventData& PmuList::GetDataList(const unsigned pd) diff --git a/pmu/pmu_list.h b/pmu/pmu_list.h index 80346eb..13724f8 100644 --- a/pmu/pmu_list.h +++ b/pmu/pmu_list.h @@ -71,8 +71,8 @@ public: int NewPd(); int GetHistoryData(const int pd, std::vector& pmuData); - void StoreSplitData(unsigned pd, std::pair previousEventList, - std::unordered_map eventSplitMap); + void StoreSplitData(unsigned pd, std::pair& previousEventList, + std::unordered_map& eventSplitMap); bool IsAllPidExit(const unsigned pd); private: @@ -95,7 +95,7 @@ private: void InsertEvtList(const unsigned pd, std::shared_ptr evtList); std::vector>& GetEvtList(const unsigned pd); void EraseEvtList(const unsigned pd); - void EraseParentEventMap(); + void EraseParentEventMap(const unsigned pd); int EvtInit(const bool groupEnable, const std::shared_ptr evtLeader, const int pd, const std::shared_ptr &evtList); int Init(const int pd); diff --git a/test/test_perf/test_count.cpp b/test/test_perf/test_count.cpp index 1fda697..1b64497 100644 --- a/test/test_perf/test_count.cpp +++ b/test/test_perf/test_count.cpp @@ -13,6 +13,7 @@ * Description: Unit test for counting. ******************************************************************************/ #include "test_common.h" +#include using namespace std; @@ -307,4 +308,60 @@ TEST_F(TestCount, SimdRatio) ASSERT_EQ(evtMap.size(), evts.size()); auto simdRatio = (double)evtMap[aseSpec]/evtMap[instSpec]; ASSERT_GT(simdRatio, 0.1); +} + +static std::vector GetHHADirs() { + vector hhaEvents; + unique_ptr dir(opendir("/sys/devices"), &closedir); + if(!dir) { + return hhaEvents; + } + + struct dirent* dt; + while((dt = readdir(dir.get())) != nullptr) { + std::string name = dt->d_name; + if(name == "." || name == "..") { + continue; + } + + if(dt->d_type == DT_DIR && strstr(name.c_str(), "hha") != nullptr) { + hhaEvents.push_back(name + "/"); + } + } + return hhaEvents; +} + + +TEST_F(TestCount, DeleteEvtAfterOpenPmuu) { + struct PmuAttr attr = {nullptr}; + vector hhaEvents = GetHHADirs(); + ASSERT_TRUE(hhaEvents.size() > 0); + set evtNames; + vector eventsStr = {"rx_outer", "rx_sccl", "rx_ops_num"}; + int evtNum = hhaEvents.size() * eventsStr.size(); + char **evtList = new char *[evtNum]; + for (int i = 0; i < hhaEvents.size(); ++i) { + for (int j = 0; j < eventsStr.size(); ++j) { + int evtLen = hhaEvents[i].size() + eventsStr[j].size() + 2; + evtList[i * eventsStr.size() + j] = new char[evtLen]; + snprintf(evtList[i * eventsStr.size() + j], evtLen, "%s%s/", hhaEvents[i].c_str(), eventsStr[j].c_str()); + evtNames.emplace(hhaEvents[i] + eventsStr[j] + "/"); + } + } + attr.evtList = evtList; + attr.numEvt = evtNum; + int pd = PmuOpen(COUNTING, &attr); + for (int i = 0; i < evtNum; i++) { + delete[] evtList[i]; + } + delete[] evtList; + PmuEnable(pd); + sleep(1); + PmuDisable(pd); + PmuData* pmuData = nullptr; + int len = PmuRead(pd, &pmuData); + for(int i = 0; i < len; i++) { + ASSERT_TRUE(evtNames.find(pmuData[i].evt) != evtNames.end()); + } + PmuClose(pd); } \ No newline at end of file -- 2.43.0