* [PATCH] drivers/perf: riscv: Disable PERF_SAMPLE_BRANCH_* while not supported @ 2024-03-05 15:52 Pu Lehui 2024-03-07 17:52 ` Conor Dooley 0 siblings, 1 reply; 5+ messages in thread From: Pu Lehui @ 2024-03-05 15:52 UTC (permalink / raw) To: linux-riscv, bpf Cc: Atish Patra, Anup Patel, Palmer Dabbelt, Björn Töpel, Pu Lehui, Pu Lehui From: Pu Lehui <pulehui@huawei.com> RISC-V perf does not yet support branch sampling. Two riscv bpf testcases get_branch_snapshot and perf_branches/perf_branches_hw failed due to not disabling such sampling. Signed-off-by: Pu Lehui <pulehui@huawei.com> --- drivers/perf/riscv_pmu.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/perf/riscv_pmu.c b/drivers/perf/riscv_pmu.c index c78a6fd6c57f..bc42cb95a97c 100644 --- a/drivers/perf/riscv_pmu.c +++ b/drivers/perf/riscv_pmu.c @@ -313,6 +313,10 @@ static int riscv_pmu_event_init(struct perf_event *event) u64 event_config = 0; uint64_t cmask; + /* does not support taken branch sampling */ + if (has_branch_stack(event)) + return -EOPNOTSUPP; + hwc->flags = 0; mapped_event = rvpmu->event_map(event, &event_config); if (mapped_event < 0) { -- 2.34.1 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drivers/perf: riscv: Disable PERF_SAMPLE_BRANCH_* while not supported 2024-03-05 15:52 [PATCH] drivers/perf: riscv: Disable PERF_SAMPLE_BRANCH_* while not supported Pu Lehui @ 2024-03-07 17:52 ` Conor Dooley 2024-03-09 6:48 ` Pu Lehui 0 siblings, 1 reply; 5+ messages in thread From: Conor Dooley @ 2024-03-07 17:52 UTC (permalink / raw) To: Pu Lehui Cc: linux-riscv, bpf, Atish Patra, Anup Patel, Palmer Dabbelt, Björn Töpel, Pu Lehui [-- Attachment #1.1: Type: text/plain, Size: 1526 bytes --] On Tue, Mar 05, 2024 at 03:52:23PM +0000, Pu Lehui wrote: > From: Pu Lehui <pulehui@huawei.com> > > RISC-V perf does not yet support branch sampling. Two riscv bpf > testcases get_branch_snapshot and perf_branches/perf_branches_hw failed > due to not disabling such sampling. > > Signed-off-by: Pu Lehui <pulehui@huawei.com> This seems worthy of a fixes tag. For what commit I do not know, but I figure it is a problem currently in mainline and a fix should be backported? > --- > drivers/perf/riscv_pmu.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/perf/riscv_pmu.c b/drivers/perf/riscv_pmu.c > index c78a6fd6c57f..bc42cb95a97c 100644 > --- a/drivers/perf/riscv_pmu.c > +++ b/drivers/perf/riscv_pmu.c > @@ -313,6 +313,10 @@ static int riscv_pmu_event_init(struct perf_event *event) > u64 event_config = 0; > uint64_t cmask; > > + /* does not support taken branch sampling */ Skimming patchwork, I found this comment confusing. Isolated from the commit message, which it will be once committed, it was not immediately clear that you were referring to the driver. I think it is just matter of making this a complete sentence that starts by mentioning /what/ does not support this feature. Is it the driver? Does the spec not allow it? etc Cheers, Conor. > + if (has_branch_stack(event)) > + return -EOPNOTSUPP; > + > hwc->flags = 0; > mapped_event = rvpmu->event_map(event, &event_config); > if (mapped_event < 0) { > -- > 2.34.1 > [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 161 bytes --] _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drivers/perf: riscv: Disable PERF_SAMPLE_BRANCH_* while not supported 2024-03-07 17:52 ` Conor Dooley @ 2024-03-09 6:48 ` Pu Lehui 2024-03-09 9:06 ` Atish Patra 0 siblings, 1 reply; 5+ messages in thread From: Pu Lehui @ 2024-03-09 6:48 UTC (permalink / raw) To: Conor Dooley Cc: linux-riscv, bpf, Atish Patra, Anup Patel, Palmer Dabbelt, Björn Töpel, Pu Lehui On 2024/3/8 1:52, Conor Dooley wrote: > On Tue, Mar 05, 2024 at 03:52:23PM +0000, Pu Lehui wrote: >> From: Pu Lehui <pulehui@huawei.com> >> >> RISC-V perf does not yet support branch sampling. Two riscv bpf >> testcases get_branch_snapshot and perf_branches/perf_branches_hw failed >> due to not disabling such sampling. >> >> Signed-off-by: Pu Lehui <pulehui@huawei.com> > > This seems worthy of a fixes tag. For what commit I do not know, but I > figure it is a problem currently in mainline and a fix should be > backported? Thanks for review. We need to ban this type of event, otherwise we will get unpredictable results. It should be with a fixes tag. > >> --- >> drivers/perf/riscv_pmu.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/perf/riscv_pmu.c b/drivers/perf/riscv_pmu.c >> index c78a6fd6c57f..bc42cb95a97c 100644 >> --- a/drivers/perf/riscv_pmu.c >> +++ b/drivers/perf/riscv_pmu.c >> @@ -313,6 +313,10 @@ static int riscv_pmu_event_init(struct perf_event *event) >> u64 event_config = 0; >> uint64_t cmask; >> >> + /* does not support taken branch sampling */ > > Skimming patchwork, I found this comment confusing. > Isolated from the commit message, which it will be once committed, it > was not immediately clear that you were referring to the driver. I think > it is just matter of making this a complete sentence that starts by > mentioning /what/ does not support this feature. Is it the driver? Does > the spec not allow it? etc I actually just found out that the current implementation of riscv perf driver doesn't support it. After reading the relevant spec, I did not find any description related to branch record. Maybe I may have missed something or it may be supported in the future, but the current driver does not support it, so I think it is more appropriate to limit this comment to the driver implementation. How about "riscv perf driver does not support branch stack sampling"? > > Cheers, > Conor. > >> + if (has_branch_stack(event)) >> + return -EOPNOTSUPP; >> + >> hwc->flags = 0; >> mapped_event = rvpmu->event_map(event, &event_config); >> if (mapped_event < 0) { >> -- >> 2.34.1 >> _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drivers/perf: riscv: Disable PERF_SAMPLE_BRANCH_* while not supported 2024-03-09 6:48 ` Pu Lehui @ 2024-03-09 9:06 ` Atish Patra 2024-03-12 1:18 ` Pu Lehui 0 siblings, 1 reply; 5+ messages in thread From: Atish Patra @ 2024-03-09 9:06 UTC (permalink / raw) To: Pu Lehui Cc: Conor Dooley, linux-riscv, bpf, Anup Patel, Palmer Dabbelt, Björn Töpel, Pu Lehui On Fri, Mar 8, 2024 at 10:48 PM Pu Lehui <pulehui@huaweicloud.com> wrote: > > > > On 2024/3/8 1:52, Conor Dooley wrote: > > On Tue, Mar 05, 2024 at 03:52:23PM +0000, Pu Lehui wrote: > >> From: Pu Lehui <pulehui@huawei.com> > >> > >> RISC-V perf does not yet support branch sampling. Two riscv bpf > >> testcases get_branch_snapshot and perf_branches/perf_branches_hw failed > >> due to not disabling such sampling. > >> > >> Signed-off-by: Pu Lehui <pulehui@huawei.com> > > > > This seems worthy of a fixes tag. For what commit I do not know, but I > > figure it is a problem currently in mainline and a fix should be > > backported? > > Thanks for review. We need to ban this type of event, otherwise we will > get unpredictable results. It should be with a fixes tag. > > > > >> --- > >> drivers/perf/riscv_pmu.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/drivers/perf/riscv_pmu.c b/drivers/perf/riscv_pmu.c > >> index c78a6fd6c57f..bc42cb95a97c 100644 > >> --- a/drivers/perf/riscv_pmu.c > >> +++ b/drivers/perf/riscv_pmu.c > >> @@ -313,6 +313,10 @@ static int riscv_pmu_event_init(struct perf_event *event) > >> u64 event_config = 0; > >> uint64_t cmask; > >> > >> + /* does not support taken branch sampling */ > > > > Skimming patchwork, I found this comment confusing. > > Isolated from the commit message, which it will be once committed, it > > was not immediately clear that you were referring to the driver. I think > > it is just matter of making this a complete sentence that starts by > > mentioning /what/ does not support this feature. Is it the driver? Does > > the spec not allow it? etc > > I actually just found out that the current implementation of riscv perf > driver doesn't support it. After reading the relevant spec, I did not > find any description related to branch record. Maybe I may have missed > something or it may be supported in the future, but the current driver > does not support it, so I think it is more appropriate to limit this Yes. There was no specification at the time this driver was upstreamed. The spec for branch record being is on its way for ratification and patches will be available soon. https://github.com/riscv/riscv-control-transfer-records The changes look good to me for now. I will be happy to send a RB tag once you add the Fixes tag. > comment to the driver implementation. How about "riscv perf driver does > not support branch stack sampling"? > > > > > Cheers, > > Conor. > > > >> + if (has_branch_stack(event)) > >> + return -EOPNOTSUPP; > >> + > >> hwc->flags = 0; > >> mapped_event = rvpmu->event_map(event, &event_config); > >> if (mapped_event < 0) { > >> -- > >> 2.34.1 > >> > -- Regards, Atish _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drivers/perf: riscv: Disable PERF_SAMPLE_BRANCH_* while not supported 2024-03-09 9:06 ` Atish Patra @ 2024-03-12 1:18 ` Pu Lehui 0 siblings, 0 replies; 5+ messages in thread From: Pu Lehui @ 2024-03-12 1:18 UTC (permalink / raw) To: Atish Patra Cc: Conor Dooley, linux-riscv, bpf, Anup Patel, Palmer Dabbelt, Björn Töpel, Pu Lehui On 2024/3/9 17:06, Atish Patra wrote: > On Fri, Mar 8, 2024 at 10:48 PM Pu Lehui <pulehui@huaweicloud.com> wrote: >> >> >> >> On 2024/3/8 1:52, Conor Dooley wrote: >>> On Tue, Mar 05, 2024 at 03:52:23PM +0000, Pu Lehui wrote: >>>> From: Pu Lehui <pulehui@huawei.com> >>>> >>>> RISC-V perf does not yet support branch sampling. Two riscv bpf >>>> testcases get_branch_snapshot and perf_branches/perf_branches_hw failed >>>> due to not disabling such sampling. >>>> >>>> Signed-off-by: Pu Lehui <pulehui@huawei.com> >>> >>> This seems worthy of a fixes tag. For what commit I do not know, but I >>> figure it is a problem currently in mainline and a fix should be >>> backported? >> >> Thanks for review. We need to ban this type of event, otherwise we will >> get unpredictable results. It should be with a fixes tag. >> >>> >>>> --- >>>> drivers/perf/riscv_pmu.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/perf/riscv_pmu.c b/drivers/perf/riscv_pmu.c >>>> index c78a6fd6c57f..bc42cb95a97c 100644 >>>> --- a/drivers/perf/riscv_pmu.c >>>> +++ b/drivers/perf/riscv_pmu.c >>>> @@ -313,6 +313,10 @@ static int riscv_pmu_event_init(struct perf_event *event) >>>> u64 event_config = 0; >>>> uint64_t cmask; >>>> >>>> + /* does not support taken branch sampling */ >>> >>> Skimming patchwork, I found this comment confusing. >>> Isolated from the commit message, which it will be once committed, it >>> was not immediately clear that you were referring to the driver. I think >>> it is just matter of making this a complete sentence that starts by >>> mentioning /what/ does not support this feature. Is it the driver? Does >>> the spec not allow it? etc >> >> I actually just found out that the current implementation of riscv perf >> driver doesn't support it. After reading the relevant spec, I did not >> find any description related to branch record. Maybe I may have missed >> something or it may be supported in the future, but the current driver >> does not support it, so I think it is more appropriate to limit this > > Yes. There was no specification at the time this driver was upstreamed. > The spec for branch record being is on its way for ratification and patches > will be available soon. > https://github.com/riscv/riscv-control-transfer-records > > The changes look good to me for now. I will be happy to send a RB tag once you > add the Fixes tag. > Thanks for your addition, looking forward to the upcoming implementation. Will send new version soon. >> comment to the driver implementation. How about "riscv perf driver does >> not support branch stack sampling"? >> >>> >>> Cheers, >>> Conor. >>> >>>> + if (has_branch_stack(event)) >>>> + return -EOPNOTSUPP; >>>> + >>>> hwc->flags = 0; >>>> mapped_event = rvpmu->event_map(event, &event_config); >>>> if (mapped_event < 0) { >>>> -- >>>> 2.34.1 >>>> >> > > _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-03-12 1:18 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-03-05 15:52 [PATCH] drivers/perf: riscv: Disable PERF_SAMPLE_BRANCH_* while not supported Pu Lehui 2024-03-07 17:52 ` Conor Dooley 2024-03-09 6:48 ` Pu Lehui 2024-03-09 9:06 ` Atish Patra 2024-03-12 1:18 ` Pu Lehui
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).