| Message ID | 20240305155223.1823287-1-pulehui@huaweicloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drivers/perf: riscv: Disable PERF_SAMPLE_BRANCH_* while not supported | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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 >
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 >>
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 > >> >
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 >>>> >> > >
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) {