diff mbox series

drivers/perf: riscv: Disable PERF_SAMPLE_BRANCH_* while not supported

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

Checks

Context Check Description
conchuod/vmtest-for-next-PR success PR summary
conchuod/patch-1-test-1 success .github/scripts/patches/tests/build_rv32_defconfig.sh
conchuod/patch-1-test-2 success .github/scripts/patches/tests/build_rv64_clang_allmodconfig.sh
conchuod/patch-1-test-3 success .github/scripts/patches/tests/build_rv64_gcc_allmodconfig.sh
conchuod/patch-1-test-4 success .github/scripts/patches/tests/build_rv64_nommu_k210_defconfig.sh
conchuod/patch-1-test-5 success .github/scripts/patches/tests/build_rv64_nommu_virt_defconfig.sh
conchuod/patch-1-test-6 success .github/scripts/patches/tests/checkpatch.sh
conchuod/patch-1-test-7 success .github/scripts/patches/tests/dtb_warn_rv64.sh
conchuod/patch-1-test-8 success .github/scripts/patches/tests/header_inline.sh
conchuod/patch-1-test-9 success .github/scripts/patches/tests/kdoc.sh
conchuod/patch-1-test-10 success .github/scripts/patches/tests/module_param.sh
conchuod/patch-1-test-11 success .github/scripts/patches/tests/verify_fixes.sh
conchuod/patch-1-test-12 success .github/scripts/patches/tests/verify_signedoff.sh

Commit Message

Pu Lehui March 5, 2024, 3:52 p.m. UTC 
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(+)

Comments

Conor Dooley March 7, 2024, 5:52 p.m. UTC | #1 
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
>
Pu Lehui March 9, 2024, 6:48 a.m. UTC | #2 
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
>>
Atish Patra March 9, 2024, 9:06 a.m. UTC | #3 
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
> >>
>
Pu Lehui March 12, 2024, 1:18 a.m. UTC | #4 
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 mbox series

Patch

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) {