| Message ID | 20240721202113.380750-5-leo.yan@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf auxtrace: Support multiple AUX events | expand |
On 21/07/24 23:21, Leo Yan wrote: > When finished to read AUX trace data from mmaped buffer, based on the > AUX buffer index the core layer needs to search the corresponding PMU > event and re-enable it to continue tracing. > > However, current code only searches the first AUX event. It misses to > search other enabled AUX events, thus, it returns failure if the buffer > index does not belong to the first AUX event. > > This patch extends the auxtrace_record__read_finish() function to > search for every enabled AUX events, so all the mmaped buffer indexes > can be covered. > > Signed-off-by: Leo Yan <leo.yan@arm.com> > --- > tools/perf/util/auxtrace.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c > index e2f317063eec..95be330d7e10 100644 > --- a/tools/perf/util/auxtrace.c > +++ b/tools/perf/util/auxtrace.c > @@ -670,18 +670,25 @@ static int evlist__enable_event_idx(struct evlist *evlist, struct evsel *evsel, > int auxtrace_record__read_finish(struct auxtrace_record *itr, int idx) > { > struct evsel *evsel; > + int ret = -EINVAL; > > if (!itr->evlist || !itr->pmu) > return -EINVAL; > > evlist__for_each_entry(itr->evlist, evsel) { > - if (evsel->core.attr.type == itr->pmu->type) { > + if (evsel__is_aux_event(evsel)) { If the type is the same, then there is no need to change the logic here? Otherwise, maybe that should be a separate patch > if (evsel->disabled) > - return 0; > - return evlist__enable_event_idx(itr->evlist, evsel, idx); > + continue; > + ret = evlist__enable_event_idx(itr->evlist, evsel, idx); > + if (ret >= 0) Should this be: if (ret < 0) > + return ret; And will need a common error path for the pr_err() below. > } > } > - return -EINVAL; > + > + if (ret < 0) > + pr_err("Failed to event enable event (idx=%d): %d\n", idx, ret); > + > + return ret; > } > > /*
On 7/22/24 12:13, Adrian Hunter wrote: [...] > On 21/07/24 23:21, Leo Yan wrote: >> When finished to read AUX trace data from mmaped buffer, based on the >> AUX buffer index the core layer needs to search the corresponding PMU >> event and re-enable it to continue tracing. >> >> However, current code only searches the first AUX event. It misses to >> search other enabled AUX events, thus, it returns failure if the buffer >> index does not belong to the first AUX event. >> >> This patch extends the auxtrace_record__read_finish() function to >> search for every enabled AUX events, so all the mmaped buffer indexes >> can be covered. >> >> Signed-off-by: Leo Yan <leo.yan@arm.com> >> --- >> tools/perf/util/auxtrace.c | 15 +++++++++++---- >> 1 file changed, 11 insertions(+), 4 deletions(-) >> >> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c >> index e2f317063eec..95be330d7e10 100644 >> --- a/tools/perf/util/auxtrace.c >> +++ b/tools/perf/util/auxtrace.c >> @@ -670,18 +670,25 @@ static int evlist__enable_event_idx(struct evlist *evlist, struct evsel *evsel, >> int auxtrace_record__read_finish(struct auxtrace_record *itr, int idx) >> { >> struct evsel *evsel; >> + int ret = -EINVAL; >> >> if (!itr->evlist || !itr->pmu) >> return -EINVAL; >> >> evlist__for_each_entry(itr->evlist, evsel) { >> - if (evsel->core.attr.type == itr->pmu->type) { >> + if (evsel__is_aux_event(evsel)) { > > If the type is the same, then there is no need to > change the logic here? No, the type is not same for AUX events. Every event has its own type value, this is likely related to recent refactoring. As a result, 'itr->pmu' only maintains the first registered AUX event, comparing to it the tool will find _only_ one AUX event. This is why here changes to use the evsel__is_aux_event() to detect AUX event. > Otherwise, maybe that should be a separate patch Could you explain what is a separate patch for? After this change, the field 'itr->pmu' will be redundant (at least this is the case for Arm SPE). I am preparing a refactoring patches for cleaning up and see if can totally remove the field 'itr->pmu' (if all AUX events have no issue. > >> if (evsel->disabled) >> - return 0; >> - return evlist__enable_event_idx(itr->evlist, evsel, idx); >> + continue; >> + ret = evlist__enable_event_idx(itr->evlist, evsel, idx); >> + if (ret >= 0) > > Should this be: > > if (ret < 0) Here the logic is to iterate all AUX events, even if an AUX event fails to find the buffer index, it will continue to next AUX event. So it directly bails out for success (as we have found the matched AUX event and enabled it). For the failure cause, it will continue for checking next event - until all events have been checked and no event is matched for buffer index, the failure will be handled at the end of the function. Thanks, Leo > >> + return ret; > > And will need a common error path for the pr_err() below. > >> } >> } >> - return -EINVAL; >> + >> + if (ret < 0) >> + pr_err("Failed to event enable event (idx=%d): %d\n", idx, ret); >> + >> + return ret; >> } >> >> /* >
On 22/07/24 18:09, Leo Yan wrote: > On 7/22/24 12:13, Adrian Hunter wrote: > > [...] > >> On 21/07/24 23:21, Leo Yan wrote: >>> When finished to read AUX trace data from mmaped buffer, based on the >>> AUX buffer index the core layer needs to search the corresponding PMU >>> event and re-enable it to continue tracing. >>> >>> However, current code only searches the first AUX event. It misses to >>> search other enabled AUX events, thus, it returns failure if the buffer >>> index does not belong to the first AUX event. >>> >>> This patch extends the auxtrace_record__read_finish() function to >>> search for every enabled AUX events, so all the mmaped buffer indexes >>> can be covered. >>> >>> Signed-off-by: Leo Yan <leo.yan@arm.com> >>> --- >>> tools/perf/util/auxtrace.c | 15 +++++++++++---- >>> 1 file changed, 11 insertions(+), 4 deletions(-) >>> >>> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c >>> index e2f317063eec..95be330d7e10 100644 >>> --- a/tools/perf/util/auxtrace.c >>> +++ b/tools/perf/util/auxtrace.c >>> @@ -670,18 +670,25 @@ static int evlist__enable_event_idx(struct evlist *evlist, struct evsel *evsel, >>> int auxtrace_record__read_finish(struct auxtrace_record *itr, int idx) >>> { >>> struct evsel *evsel; >>> + int ret = -EINVAL; >>> >>> if (!itr->evlist || !itr->pmu) >>> return -EINVAL; >>> >>> evlist__for_each_entry(itr->evlist, evsel) { >>> - if (evsel->core.attr.type == itr->pmu->type) { >>> + if (evsel__is_aux_event(evsel)) { >> >> If the type is the same, then there is no need to >> change the logic here? > > No, the type is not same for AUX events. Every event has its own type > value, this is likely related to recent refactoring. > > As a result, 'itr->pmu' only maintains the first registered AUX event, > comparing to it the tool will find _only_ one AUX event. This is why here > changes to use the evsel__is_aux_event() to detect AUX event. > >> Otherwise, maybe that should be a separate patch > > Could you explain what is a separate patch for? No need. > > After this change, the field 'itr->pmu' will be redundant (at least this > is the case for Arm SPE). I am preparing a refactoring patches for cleaning up > and see if can totally remove the field 'itr->pmu' (if all AUX events > have no issue. For this function, 'itr->pmu' could be removed in this patch since it is not used anymore. > >> >>> if (evsel->disabled) >>> - return 0; >>> - return evlist__enable_event_idx(itr->evlist, evsel, idx); >>> + continue; >>> + ret = evlist__enable_event_idx(itr->evlist, evsel, idx); >>> + if (ret >= 0) >> >> Should this be: >> >> if (ret < 0) > > Here the logic is to iterate all AUX events, even if an AUX event fails to > find the buffer index, it will continue to next AUX event. > > So it directly bails out for success (as we have found the matched AUX > event and enabled it). For the failure cause, it will continue for checking > next event - until all events have been checked and no event is matched > for buffer index, the failure will be handled at the end of the function. Thanks for the explanation. Could probably use a small comment. > > Thanks, > Leo > >> >>> + return ret; >> >> And will need a common error path for the pr_err() below. >> >>> } >>> } >>> - return -EINVAL; >>> + >>> + if (ret < 0) >>> + pr_err("Failed to event enable event (idx=%d): %d\n", idx, ret); >>> + >>> + return ret; >>> } >>> >>> /* >>
On 7/22/2024 4:59 PM, Adrian Hunter wrote: [...] >>>> @@ -670,18 +670,25 @@ static int evlist__enable_event_idx(struct evlist *evlist, struct evsel *evsel, >>>> int auxtrace_record__read_finish(struct auxtrace_record *itr, int idx) >>>> { >>>> struct evsel *evsel; >>>> + int ret = -EINVAL; >>>> >>>> if (!itr->evlist || !itr->pmu) >>>> return -EINVAL; >>>> >>>> evlist__for_each_entry(itr->evlist, evsel) { >>>> - if (evsel->core.attr.type == itr->pmu->type) { >>>> + if (evsel__is_aux_event(evsel)) { >>> >>> If the type is the same, then there is no need to >>> change the logic here? >> >> No, the type is not same for AUX events. Every event has its own type >> value, this is likely related to recent refactoring. >> >> As a result, 'itr->pmu' only maintains the first registered AUX event, >> comparing to it the tool will find _only_ one AUX event. This is why here >> changes to use the evsel__is_aux_event() to detect AUX event. >> >>> Otherwise, maybe that should be a separate patch >> >> Could you explain what is a separate patch for? > > No need. > >> >> After this change, the field 'itr->pmu' will be redundant (at least this >> is the case for Arm SPE). I am preparing a refactoring patches for cleaning up >> and see if can totally remove the field 'itr->pmu' (if all AUX events >> have no issue. > > For this function, 'itr->pmu' could be removed in this patch > since it is not used anymore. Thanks for confirmation. I will use a separate patch for removing 'itr-pmu' after it is not used anymore. >>>> if (evsel->disabled) >>>> - return 0; >>>> - return evlist__enable_event_idx(itr->evlist, evsel, idx); >>>> + continue; >>>> + ret = evlist__enable_event_idx(itr->evlist, evsel, idx); >>>> + if (ret >= 0) >>> >>> Should this be: >>> >>> if (ret < 0) >> >> Here the logic is to iterate all AUX events, even if an AUX event fails to >> find the buffer index, it will continue to next AUX event. >> >> So it directly bails out for success (as we have found the matched AUX >> event and enabled it). For the failure cause, it will continue for checking >> next event - until all events have been checked and no event is matched >> for buffer index, the failure will be handled at the end of the function. > > Thanks for the explanation. Could probably use a small comment. Will do. Thanks, Leo
Just a heads up. Arnaldo added this to tmp.perf-tools-next and it caused the intel-pt tests to start failing: ``` $ perf test 118 -v 118: Miscellaneous Intel PT testing: --- start --- test child forked, pid 148999 --- Test system-wide sideband --- Checking for CPU-wide recording on CPU 0 OK Checking for CPU-wide recording on CPU 1 OK Linux Failed to enable event (idx=0): -22 Failed to record MMAP events on CPU 1 when tracing CPU 0 ... ``` It's likely Adrian's comments already address this but you may also want to double check this test is passing with v2. Thanks, Ian
On Wed, Jul 31, 2024 at 06:38:59PM -0700, Ian Rogers wrote: > Just a heads up. Arnaldo added this to tmp.perf-tools-next and it > caused the intel-pt tests to start failing: My plan right now is just to remove that cset that Ian bisected since it is not on perf-tools-next, just on the scratch branch tmp.perf-tools-next. Trying to do that now as it will help us with bisection in the future. - Arnaldo > ``` > $ perf test 118 -v > 118: Miscellaneous Intel PT testing: > --- start --- > test child forked, pid 148999 > --- Test system-wide sideband --- > Checking for CPU-wide recording on CPU 0 > OK > Checking for CPU-wide recording on CPU 1 > OK > Linux > Failed to enable event (idx=0): -22 > Failed to record MMAP events on CPU 1 when tracing CPU 0 > ... > ``` > It's likely Adrian's comments already address this but you may also > want to double check this test is passing with v2. > > Thanks, > Ian
On 8/1/24 16:04, Arnaldo Carvalho de Melo wrote: > On Wed, Jul 31, 2024 at 06:38:59PM -0700, Ian Rogers wrote: >> Just a heads up. Arnaldo added this to tmp.perf-tools-next and it >> caused the intel-pt tests to start failing: Sorry for causing regression and thanks for reporting the issue. > My plan right now is just to remove that cset that Ian bisected since it > is not on perf-tools-next, just on the scratch branch > tmp.perf-tools-next. Please leave the patch 04 out. I have sent out v2 but Adrian pointed out a concern for per-thread mode, now I am working on this and after ready I will send new patches. I have verified the latest perf-tools-next branch, it works well on Arm SPE (thanks for picking up patches 05 and 06). > Trying to do that now as it will help us with bisection in the future. I am not clear this. Could you elaborate a bit what I should follow up? >> ``` >> $ perf test 118 -v >> 118: Miscellaneous Intel PT testing: >> --- start --- >> test child forked, pid 148999 >> --- Test system-wide sideband --- >> Checking for CPU-wide recording on CPU 0 >> OK >> Checking for CPU-wide recording on CPU 1 >> OK >> Linux >> Failed to enable event (idx=0): -22 >> Failed to record MMAP events on CPU 1 when tracing CPU 0 >> ... >> ``` >> It's likely Adrian's comments already address this but you may also >> want to double check this test is passing with v2. Sure. I will give a test for this test when I send new patches. Thanks, Leo
diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c index e2f317063eec..95be330d7e10 100644 --- a/tools/perf/util/auxtrace.c +++ b/tools/perf/util/auxtrace.c @@ -670,18 +670,25 @@ static int evlist__enable_event_idx(struct evlist *evlist, struct evsel *evsel, int auxtrace_record__read_finish(struct auxtrace_record *itr, int idx) { struct evsel *evsel; + int ret = -EINVAL; if (!itr->evlist || !itr->pmu) return -EINVAL; evlist__for_each_entry(itr->evlist, evsel) { - if (evsel->core.attr.type == itr->pmu->type) { + if (evsel__is_aux_event(evsel)) { if (evsel->disabled) - return 0; - return evlist__enable_event_idx(itr->evlist, evsel, idx); + continue; + ret = evlist__enable_event_idx(itr->evlist, evsel, idx); + if (ret >= 0) + return ret; } } - return -EINVAL; + + if (ret < 0) + pr_err("Failed to event enable event (idx=%d): %d\n", idx, ret); + + return ret; } /*
When finished to read AUX trace data from mmaped buffer, based on the AUX buffer index the core layer needs to search the corresponding PMU event and re-enable it to continue tracing. However, current code only searches the first AUX event. It misses to search other enabled AUX events, thus, it returns failure if the buffer index does not belong to the first AUX event. This patch extends the auxtrace_record__read_finish() function to search for every enabled AUX events, so all the mmaped buffer indexes can be covered. Signed-off-by: Leo Yan <leo.yan@arm.com> --- tools/perf/util/auxtrace.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)