* [PATCH] perf/core: Fix the same task check in perf_event_set_output @ 2022-07-11 18:07 kan.liang 2023-03-22 10:59 ` Adrian Hunter 0 siblings, 1 reply; 5+ messages in thread From: kan.liang @ 2022-07-11 18:07 UTC (permalink / raw) To: peterz, mingo, linux-kernel Cc: alexander.shishkin, ak, adrian.hunter, nadav.amit, Kan Liang, Zhengjun Xing From: Kan Liang <kan.liang@linux.intel.com> With the --per-thread option, perf record errors out when sampling with a hardware event and a software event as below. $ perf record -e cycles,dummy --per-thread ls failed to mmap with 22 (Invalid argument) The same task is sampled with the two events. The IOC_OUTPUT is invoked to share the mmap memory of the task between the events. In the perf_event_set_output(), the event->ctx is used to check whether the two events are attached to the same task. However, a hardware event and a software event are from different task context. The check always fails. The task struct is stored in the event->hw.target for each per-thread event. It can be used to determine whether two events are attached to the same task. The patch can also fix another issue reported months ago. https://lore.kernel.org/all/92645262-D319-4068-9C44-2409EF44888E@gmail.com/ The event->ctx is not ready when the perf_event_set_output() is invoked in the perf_event_open(), while the event->hw.target has been assigned at the moment. The problem should be a long time issue since commit c3f00c70276d ("perf: Separate find_get_context() from event initialization"). The event->hw.target doesn't exist at that time. Here, the patch which introduces the event->hw.target is used by the Fixes tag. The problem should still exists between the broken patch and the event->hw.target patch. This patch does not intend to fix that case. Fixes: 50f16a8bf9d7 ("perf: Remove type specific target pointers") Reviewed-by: Zhengjun Xing <zhengjun.xing@linux.intel.com> Signed-off-by: Kan Liang <kan.liang@linux.intel.com> --- kernel/events/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/events/core.c b/kernel/events/core.c index b4d62210c3e5..22df79d3f19d 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -12080,7 +12080,7 @@ perf_event_set_output(struct perf_event *event, struct perf_event *output_event) /* * If its not a per-cpu rb, it must be the same task. */ - if (output_event->cpu == -1 && output_event->ctx != event->ctx) + if (output_event->cpu == -1 && output_event->hw.target != event->hw.target) goto out; /* -- 2.35.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] perf/core: Fix the same task check in perf_event_set_output 2022-07-11 18:07 [PATCH] perf/core: Fix the same task check in perf_event_set_output kan.liang @ 2023-03-22 10:59 ` Adrian Hunter 2023-03-22 13:42 ` Peter Zijlstra 0 siblings, 1 reply; 5+ messages in thread From: Adrian Hunter @ 2023-03-22 10:59 UTC (permalink / raw) To: peterz Cc: alexander.shishkin, ak, kan.liang, nadav.amit, Zhengjun Xing, linux-kernel, Arnaldo Carvalho de Melo, jolsa, Mark Rutland, mingo, Namhyung Kim, Ian Rogers, linux-perf-users On 11/07/22 21:07, kan.liang@linux.intel.com wrote: > From: Kan Liang <kan.liang@linux.intel.com> > > With the --per-thread option, perf record errors out when sampling with > a hardware event and a software event as below. > > $ perf record -e cycles,dummy --per-thread ls > failed to mmap with 22 (Invalid argument) > > The same task is sampled with the two events. The IOC_OUTPUT is invoked > to share the mmap memory of the task between the events. In the > perf_event_set_output(), the event->ctx is used to check whether the > two events are attached to the same task. However, a hardware event and > a software event are from different task context. The check always > fails. > > The task struct is stored in the event->hw.target for each per-thread > event. It can be used to determine whether two events are attached to > the same task. > > The patch can also fix another issue reported months ago. > https://lore.kernel.org/all/92645262-D319-4068-9C44-2409EF44888E@gmail.com/ > The event->ctx is not ready when the perf_event_set_output() is invoked > in the perf_event_open(), while the event->hw.target has been assigned > at the moment. > > The problem should be a long time issue since commit c3f00c70276d > ("perf: Separate find_get_context() from event initialization"). The > event->hw.target doesn't exist at that time. Here, the patch which > introduces the event->hw.target is used by the Fixes tag. > > The problem should still exists between the broken patch and the > event->hw.target patch. This patch does not intend to fix that case. > > Fixes: 50f16a8bf9d7 ("perf: Remove type specific target pointers") > Reviewed-by: Zhengjun Xing <zhengjun.xing@linux.intel.com> > Signed-off-by: Kan Liang <kan.liang@linux.intel.com> Did this slip through the cracks, or is there more complexity to this case than just sharing the rb? > --- > kernel/events/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index b4d62210c3e5..22df79d3f19d 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -12080,7 +12080,7 @@ perf_event_set_output(struct perf_event *event, struct perf_event *output_event) > /* > * If its not a per-cpu rb, it must be the same task. > */ > - if (output_event->cpu == -1 && output_event->ctx != event->ctx) > + if (output_event->cpu == -1 && output_event->hw.target != event->hw.target) > goto out; > > /* ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] perf/core: Fix the same task check in perf_event_set_output 2023-03-22 10:59 ` Adrian Hunter @ 2023-03-22 13:42 ` Peter Zijlstra 2023-03-22 14:15 ` Adrian Hunter 0 siblings, 1 reply; 5+ messages in thread From: Peter Zijlstra @ 2023-03-22 13:42 UTC (permalink / raw) To: Adrian Hunter Cc: alexander.shishkin, ak, kan.liang, nadav.amit, Zhengjun Xing, linux-kernel, Arnaldo Carvalho de Melo, jolsa, Mark Rutland, mingo, Namhyung Kim, Ian Rogers, linux-perf-users On Wed, Mar 22, 2023 at 12:59:28PM +0200, Adrian Hunter wrote: > On 11/07/22 21:07, kan.liang@linux.intel.com wrote: > > From: Kan Liang <kan.liang@linux.intel.com> > > > > With the --per-thread option, perf record errors out when sampling with > > a hardware event and a software event as below. > > > > $ perf record -e cycles,dummy --per-thread ls > > failed to mmap with 22 (Invalid argument) > > > > The same task is sampled with the two events. The IOC_OUTPUT is invoked > > to share the mmap memory of the task between the events. In the > > perf_event_set_output(), the event->ctx is used to check whether the > > two events are attached to the same task. However, a hardware event and > > a software event are from different task context. The check always > > fails. > > > > The task struct is stored in the event->hw.target for each per-thread > > event. It can be used to determine whether two events are attached to > > the same task. > > > > The patch can also fix another issue reported months ago. > > https://lore.kernel.org/all/92645262-D319-4068-9C44-2409EF44888E@gmail.com/ > > The event->ctx is not ready when the perf_event_set_output() is invoked > > in the perf_event_open(), while the event->hw.target has been assigned > > at the moment. > > > > The problem should be a long time issue since commit c3f00c70276d > > ("perf: Separate find_get_context() from event initialization"). The > > event->hw.target doesn't exist at that time. Here, the patch which > > introduces the event->hw.target is used by the Fixes tag. > > > > The problem should still exists between the broken patch and the > > event->hw.target patch. This patch does not intend to fix that case. > > > > Fixes: 50f16a8bf9d7 ("perf: Remove type specific target pointers") > > Reviewed-by: Zhengjun Xing <zhengjun.xing@linux.intel.com> > > Signed-off-by: Kan Liang <kan.liang@linux.intel.com> > > Did this slip through the cracks, or is there more complexity > to this case than just sharing the rb? Both; I very much missed it, but looking at it now, I'm not at all sure it is correct prior to the whole context rewrite we did recently. So after the rewrite every cpu/task only has a single perf_event_context, and your change below is actually an equivalence. But prior to that a task could have multiple contexts. Now they got co-scheduled most of the times and it will probably work, but I'm not entirely sure. So how about we change the Fixes tag to something like: Fixes: c3f00c70276d ("perf: Separate find_get_context() from event initialization") # >= v6.2 And anybody that wants to back-port this further gets to either do the full audit and/or keep the pieces. Hmm? > > --- > > kernel/events/core.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/events/core.c b/kernel/events/core.c > > index b4d62210c3e5..22df79d3f19d 100644 > > --- a/kernel/events/core.c > > +++ b/kernel/events/core.c > > @@ -12080,7 +12080,7 @@ perf_event_set_output(struct perf_event *event, struct perf_event *output_event) > > /* > > * If its not a per-cpu rb, it must be the same task. > > */ > > - if (output_event->cpu == -1 && output_event->ctx != event->ctx) > > + if (output_event->cpu == -1 && output_event->hw.target != event->hw.target) > > goto out; > > > > /* > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] perf/core: Fix the same task check in perf_event_set_output 2023-03-22 13:42 ` Peter Zijlstra @ 2023-03-22 14:15 ` Adrian Hunter 2023-03-22 19:17 ` Liang, Kan 0 siblings, 1 reply; 5+ messages in thread From: Adrian Hunter @ 2023-03-22 14:15 UTC (permalink / raw) To: Peter Zijlstra, kan.liang Cc: alexander.shishkin, ak, nadav.amit, Zhengjun Xing, linux-kernel, Arnaldo Carvalho de Melo, jolsa, Mark Rutland, mingo, Namhyung Kim, Ian Rogers, linux-perf-users On 22/03/23 15:42, Peter Zijlstra wrote: > On Wed, Mar 22, 2023 at 12:59:28PM +0200, Adrian Hunter wrote: >> On 11/07/22 21:07, kan.liang@linux.intel.com wrote: >>> From: Kan Liang <kan.liang@linux.intel.com> >>> >>> With the --per-thread option, perf record errors out when sampling with >>> a hardware event and a software event as below. >>> >>> $ perf record -e cycles,dummy --per-thread ls >>> failed to mmap with 22 (Invalid argument) >>> >>> The same task is sampled with the two events. The IOC_OUTPUT is invoked >>> to share the mmap memory of the task between the events. In the >>> perf_event_set_output(), the event->ctx is used to check whether the >>> two events are attached to the same task. However, a hardware event and >>> a software event are from different task context. The check always >>> fails. >>> >>> The task struct is stored in the event->hw.target for each per-thread >>> event. It can be used to determine whether two events are attached to >>> the same task. >>> >>> The patch can also fix another issue reported months ago. >>> https://lore.kernel.org/all/92645262-D319-4068-9C44-2409EF44888E@gmail.com/ >>> The event->ctx is not ready when the perf_event_set_output() is invoked >>> in the perf_event_open(), while the event->hw.target has been assigned >>> at the moment. >>> >>> The problem should be a long time issue since commit c3f00c70276d >>> ("perf: Separate find_get_context() from event initialization"). The >>> event->hw.target doesn't exist at that time. Here, the patch which >>> introduces the event->hw.target is used by the Fixes tag. >>> >>> The problem should still exists between the broken patch and the >>> event->hw.target patch. This patch does not intend to fix that case. >>> >>> Fixes: 50f16a8bf9d7 ("perf: Remove type specific target pointers") >>> Reviewed-by: Zhengjun Xing <zhengjun.xing@linux.intel.com> >>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com> >> >> Did this slip through the cracks, or is there more complexity >> to this case than just sharing the rb? > > Both; I very much missed it, but looking at it now, I'm not at all sure > it is correct prior to the whole context rewrite we did recently. > > So after the rewrite every cpu/task only has a single > perf_event_context, and your change below is actually an equivalence. > > But prior to that a task could have multiple contexts. Now they got > co-scheduled most of the times and it will probably work, but I'm not > entirely sure. > > So how about we change the Fixes tag to something like: > > Fixes: c3f00c70276d ("perf: Separate find_get_context() from event initialization") # >= v6.2 > > And anybody that wants to back-port this further gets to either do the > full audit and/or keep the pieces. > > Hmm? Seems reasonable to me. Kan? > >>> --- >>> kernel/events/core.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/kernel/events/core.c b/kernel/events/core.c >>> index b4d62210c3e5..22df79d3f19d 100644 >>> --- a/kernel/events/core.c >>> +++ b/kernel/events/core.c >>> @@ -12080,7 +12080,7 @@ perf_event_set_output(struct perf_event *event, struct perf_event *output_event) >>> /* >>> * If its not a per-cpu rb, it must be the same task. >>> */ >>> - if (output_event->cpu == -1 && output_event->ctx != event->ctx) >>> + if (output_event->cpu == -1 && output_event->hw.target != event->hw.target) >>> goto out; >>> >>> /* >> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] perf/core: Fix the same task check in perf_event_set_output 2023-03-22 14:15 ` Adrian Hunter @ 2023-03-22 19:17 ` Liang, Kan 0 siblings, 0 replies; 5+ messages in thread From: Liang, Kan @ 2023-03-22 19:17 UTC (permalink / raw) To: Adrian Hunter, Peter Zijlstra Cc: alexander.shishkin, ak, nadav.amit, Zhengjun Xing, linux-kernel, Arnaldo Carvalho de Melo, jolsa, Mark Rutland, mingo, Namhyung Kim, Ian Rogers, linux-perf-users On 2023-03-22 10:15 a.m., Adrian Hunter wrote: > On 22/03/23 15:42, Peter Zijlstra wrote: >> On Wed, Mar 22, 2023 at 12:59:28PM +0200, Adrian Hunter wrote: >>> On 11/07/22 21:07, kan.liang@linux.intel.com wrote: >>>> From: Kan Liang <kan.liang@linux.intel.com> >>>> >>>> With the --per-thread option, perf record errors out when sampling with >>>> a hardware event and a software event as below. >>>> >>>> $ perf record -e cycles,dummy --per-thread ls >>>> failed to mmap with 22 (Invalid argument) >>>> >>>> The same task is sampled with the two events. The IOC_OUTPUT is invoked >>>> to share the mmap memory of the task between the events. In the >>>> perf_event_set_output(), the event->ctx is used to check whether the >>>> two events are attached to the same task. However, a hardware event and >>>> a software event are from different task context. The check always >>>> fails. >>>> >>>> The task struct is stored in the event->hw.target for each per-thread >>>> event. It can be used to determine whether two events are attached to >>>> the same task. >>>> >>>> The patch can also fix another issue reported months ago. >>>> https://lore.kernel.org/all/92645262-D319-4068-9C44-2409EF44888E@gmail.com/ >>>> The event->ctx is not ready when the perf_event_set_output() is invoked >>>> in the perf_event_open(), while the event->hw.target has been assigned >>>> at the moment. >>>> >>>> The problem should be a long time issue since commit c3f00c70276d >>>> ("perf: Separate find_get_context() from event initialization"). The >>>> event->hw.target doesn't exist at that time. Here, the patch which >>>> introduces the event->hw.target is used by the Fixes tag. >>>> >>>> The problem should still exists between the broken patch and the >>>> event->hw.target patch. This patch does not intend to fix that case. >>>> >>>> Fixes: 50f16a8bf9d7 ("perf: Remove type specific target pointers") >>>> Reviewed-by: Zhengjun Xing <zhengjun.xing@linux.intel.com> >>>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com> >>> >>> Did this slip through the cracks, or is there more complexity >>> to this case than just sharing the rb? >> >> Both; I very much missed it, but looking at it now, I'm not at all sure >> it is correct prior to the whole context rewrite we did recently. >> >> So after the rewrite every cpu/task only has a single >> perf_event_context, and your change below is actually an equivalence. >> Right, they are equivalent after the rewrite. I cannot reproduce the "failed to mmap with 22 (Invalid argument)" issue with 6.2 and later anymore. But the issue reported by Nadav* can still be reproduced with the latest 6.3-rc.* https://lore.kernel.org/all/92645262-D319-4068-9C44-2409EF44888E@gmail.com/ Because the event->ctx is still not set when the perf_event_set_output() is invoked in the perf_event_open(). The rewrite doesn't change with it. >> But prior to that a task could have multiple contexts. Now they got >> co-scheduled most of the times and it will probably work, but I'm not >> entirely sure. >> >> So how about we change the Fixes tag to something like: >> >> Fixes: c3f00c70276d ("perf: Separate find_get_context() from event initialization") # >= v6.2 >> Not sure if we want the # >= v6.2. For v6.2 and later, the patch can fixes Nadav's issue. For v6.1 and earlier, the patch fixes both Nadav's issue and the issue described in the description. I think I can send a V2 patch to make the history clear in the description. Thanks, Kan >> And anybody that wants to back-port this further gets to either do the >> full audit and/or keep the pieces. >> >> Hmm? > > Seems reasonable to me. Kan? > >> >>>> --- >>>> kernel/events/core.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/kernel/events/core.c b/kernel/events/core.c >>>> index b4d62210c3e5..22df79d3f19d 100644 >>>> --- a/kernel/events/core.c >>>> +++ b/kernel/events/core.c >>>> @@ -12080,7 +12080,7 @@ perf_event_set_output(struct perf_event *event, struct perf_event *output_event) >>>> /* >>>> * If its not a per-cpu rb, it must be the same task. >>>> */ >>>> - if (output_event->cpu == -1 && output_event->ctx != event->ctx) >>>> + if (output_event->cpu == -1 && output_event->hw.target != event->hw.target) >>>> goto out; >>>> >>>> /* >>> > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-22 19:17 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-11 18:07 [PATCH] perf/core: Fix the same task check in perf_event_set_output kan.liang 2023-03-22 10:59 ` Adrian Hunter 2023-03-22 13:42 ` Peter Zijlstra 2023-03-22 14:15 ` Adrian Hunter 2023-03-22 19:17 ` Liang, Kan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).