mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] perf parse-events: default data breakpoints to any size on RISC-V
@ 2026-11-09 12:03 Rui Qi
  2026-11-09 21:58 ` Ian Rogers
  0 siblings, 1 reply; 3+ messages in thread
From: Rui Qi @ 2026-11-09 12:03 UTC (permalink / raw)
  To: linux-perf-users
  Cc: linux-kernel, linux-riscv, peterz, mingo, acme, namhyung, Rui Qi

When the length of a data breakpoint is not specified on the command
line, perf defaults it to HW_BREAKPOINT_LEN_4:

  $ perf record -e mem:0x30020:w ./prog

That assumes the length describes a byte range that is compared
against the address of the access, which is how the x86 debug
registers and most other architectures work.

RISC-V debug triggers behave differently.  An Sdtrig mcontrol6
trigger encodes the width of the memory access it should match, and
it only fires for accesses of exactly that width.  Watching an 8-byte
variable with the implicit 4-byte length therefore never triggers:
the perf event is created and enabled, but no event is ever
delivered:

  $ perf stat -e mem:0x30020:w ./prog          # 8-byte stores
  0       mem:0x30020:w
  $ perf stat -e mem:0x30020/8:w ./prog
  10      mem:0x30020:w

The failure is silent and easily mistaken for a broken hardware
breakpoint implementation.  Tracing hw_breakpoint_exceptions_notify()
while the event is active confirms the trigger never fires.

Zero is a valid length for RISC-V data breakpoints: the kernel
encodes it as size 0, which means "match any size".  That is what a
user leaving out the length asks for, so use it as the default on
RISC-V and keep the existing 4-byte default elsewhere.

Tested on a riscv64 kernel with hardware breakpoint support.  With the
default length, watching an 8-byte and a 4-byte variable both count
10 stores, while explicit lengths keep their exact-width behaviour:
/8 only matches the 8-byte access and /4 only the 4-byte one.

Signed-off-by: Rui Qi <qirui.001@bytedance.com>
---
 tools/perf/util/parse-events.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index f3ba70d4f76b..b72e4b0da8a1 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -598,6 +598,27 @@ size_t default_breakpoint_len(void)
 #endif
 }
 
+/*
+ * Default length for data breakpoints when the user did not specify one.
+ *
+ * Most architectures treat the length as a byte range that is compared
+ * against the address of the access, so 4 bytes is a reasonable default
+ * that matches most accesses.  RISC-V debug triggers (Sdtrig mcontrol6)
+ * encode the width of the access in the trigger instead, and a trigger
+ * only fires when the access has exactly that width.  This holds for
+ * both RV32 and RV64.  With a fixed default, an access of a different
+ * width silently never matches, so pass 0 on RISC-V, which the kernel
+ * translates to "match any size".
+ */
+static size_t default_data_breakpoint_len(void)
+{
+#if defined(__riscv)
+	return 0;
+#else
+	return HW_BREAKPOINT_LEN_4;
+#endif
+}
+
 static int
 parse_breakpoint_type(const char *type, struct perf_event_attr *attr)
 {
@@ -658,7 +679,7 @@ int parse_events_add_breakpoint(struct parse_events_state *parse_state,
 		if (attr.bp_type == HW_BREAKPOINT_X)
 			len = default_breakpoint_len();
 		else
-			len = HW_BREAKPOINT_LEN_4;
+			len = default_data_breakpoint_len();
 	}
 
 	attr.bp_len = len;
-- 
2.52.0

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] perf parse-events: default data breakpoints to any size on RISC-V
  2026-11-09 12:03 [PATCH] perf parse-events: default data breakpoints to any size on RISC-V Rui Qi
@ 2026-11-09 21:58 ` Ian Rogers
  2026-11-11  6:23   ` Rui Qi
  0 siblings, 1 reply; 3+ messages in thread
From: Ian Rogers @ 2026-11-09 21:58 UTC (permalink / raw)
  To: Rui Qi
  Cc: linux-perf-users, linux-kernel, linux-riscv, peterz, mingo, acme,
	namhyung

On Mon, Nov 9, 2026 at 4:06 AM Rui Qi <qirui.001@bytedance.com> wrote:
>
> When the length of a data breakpoint is not specified on the command
> line, perf defaults it to HW_BREAKPOINT_LEN_4:
>
>   $ perf record -e mem:0x30020:w ./prog
>
> That assumes the length describes a byte range that is compared
> against the address of the access, which is how the x86 debug
> registers and most other architectures work.
>
> RISC-V debug triggers behave differently.  An Sdtrig mcontrol6
> trigger encodes the width of the memory access it should match, and
> it only fires for accesses of exactly that width.  Watching an 8-byte
> variable with the implicit 4-byte length therefore never triggers:
> the perf event is created and enabled, but no event is ever
> delivered:
>
>   $ perf stat -e mem:0x30020:w ./prog          # 8-byte stores
>   0       mem:0x30020:w
>   $ perf stat -e mem:0x30020/8:w ./prog
>   10      mem:0x30020:w
>
> The failure is silent and easily mistaken for a broken hardware
> breakpoint implementation.  Tracing hw_breakpoint_exceptions_notify()
> while the event is active confirms the trigger never fires.
>
> Zero is a valid length for RISC-V data breakpoints: the kernel
> encodes it as size 0, which means "match any size".  That is what a
> user leaving out the length asks for, so use it as the default on
> RISC-V and keep the existing 4-byte default elsewhere.
>
> Tested on a riscv64 kernel with hardware breakpoint support.  With the
> default length, watching an 8-byte and a 4-byte variable both count
> 10 stores, while explicit lengths keep their exact-width behaviour:
> /8 only matches the 8-byte access and /4 only the 4-byte one.
>
> Signed-off-by: Rui Qi <qirui.001@bytedance.com>

The change makes sense to me, and I may have been the last person to
tweak how this parsing runs. Could you address at least the test
failure issue with something like:

- TEST_ASSERT_EVSEL("wrong bp_len", HW_BREAKPOINT_LEN_4 ==
evsel->core.attr.bp_len, evsel);
+ TEST_ASSERT_EVSEL("wrong bp_len", default_data_breakpoint_len() ==
evsel->core.attr.bp_len, evsel);

and then I'll add my tag.

Thanks,
Ian

> ---
>  tools/perf/util/parse-events.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index f3ba70d4f76b..b72e4b0da8a1 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -598,6 +598,27 @@ size_t default_breakpoint_len(void)
>  #endif
>  }
>
> +/*
> + * Default length for data breakpoints when the user did not specify one.
> + *
> + * Most architectures treat the length as a byte range that is compared
> + * against the address of the access, so 4 bytes is a reasonable default
> + * that matches most accesses.  RISC-V debug triggers (Sdtrig mcontrol6)
> + * encode the width of the access in the trigger instead, and a trigger
> + * only fires when the access has exactly that width.  This holds for
> + * both RV32 and RV64.  With a fixed default, an access of a different
> + * width silently never matches, so pass 0 on RISC-V, which the kernel
> + * translates to "match any size".
> + */
> +static size_t default_data_breakpoint_len(void)
> +{
> +#if defined(__riscv)
> +       return 0;
> +#else
> +       return HW_BREAKPOINT_LEN_4;
> +#endif
> +}
> +
>  static int
>  parse_breakpoint_type(const char *type, struct perf_event_attr *attr)
>  {
> @@ -658,7 +679,7 @@ int parse_events_add_breakpoint(struct parse_events_state *parse_state,
>                 if (attr.bp_type == HW_BREAKPOINT_X)
>                         len = default_breakpoint_len();
>                 else
> -                       len = HW_BREAKPOINT_LEN_4;
> +                       len = default_data_breakpoint_len();
>         }
>
>         attr.bp_len = len;
> --
> 2.52.0
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] perf parse-events: default data breakpoints to any size on RISC-V
  2026-11-09 21:58 ` Ian Rogers
@ 2026-11-11  6:23   ` Rui Qi
  0 siblings, 0 replies; 3+ messages in thread
From: Rui Qi @ 2026-11-11  6:23 UTC (permalink / raw)
  To: Ian Rogers
  Cc: linux-perf-users, linux-kernel, linux-riscv, peterz, mingo, acme,
	namhyung

On 11/10/26 5:58 AM, Ian Rogers wrote:
> On Mon, Nov 9, 2026 at 4:06 AM Rui Qi <qirui.001@bytedance.com> wrote:
>>
>> When the length of a data breakpoint is not specified on the command
>> line, perf defaults it to HW_BREAKPOINT_LEN_4:
>>
>>   $ perf record -e mem:0x30020:w ./prog
>>
>> That assumes the length describes a byte range that is compared
>> against the address of the access, which is how the x86 debug
>> registers and most other architectures work.
>>
>> RISC-V debug triggers behave differently.  An Sdtrig mcontrol6
>> trigger encodes the width of the memory access it should match, and
>> it only fires for accesses of exactly that width.  Watching an 8-byte
>> variable with the implicit 4-byte length therefore never triggers:
>> the perf event is created and enabled, but no event is ever
>> delivered:
>>
>>   $ perf stat -e mem:0x30020:w ./prog          # 8-byte stores
>>   0       mem:0x30020:w
>>   $ perf stat -e mem:0x30020/8:w ./prog
>>   10      mem:0x30020:w
>>
>> The failure is silent and easily mistaken for a broken hardware
>> breakpoint implementation.  Tracing hw_breakpoint_exceptions_notify()
>> while the event is active confirms the trigger never fires.
>>
>> Zero is a valid length for RISC-V data breakpoints: the kernel
>> encodes it as size 0, which means "match any size".  That is what a
>> user leaving out the length asks for, so use it as the default on
>> RISC-V and keep the existing 4-byte default elsewhere.
>>
>> Tested on a riscv64 kernel with hardware breakpoint support.  With the
>> default length, watching an 8-byte and a 4-byte variable both count
>> 10 stores, while explicit lengths keep their exact-width behaviour:
>> /8 only matches the 8-byte access and /4 only the 4-byte one.
>>
>> Signed-off-by: Rui Qi <qirui.001@bytedance.com>
> 
> The change makes sense to me, and I may have been the last person to
> tweak how this parsing runs. Could you address at least the test
> failure issue with something like:
> 
> - TEST_ASSERT_EVSEL("wrong bp_len", HW_BREAKPOINT_LEN_4 ==
> evsel->core.attr.bp_len, evsel);
> + TEST_ASSERT_EVSEL("wrong bp_len", default_data_breakpoint_len() ==
> evsel->core.attr.bp_len, evsel);
> 
> and then I'll add my tag.
> 
> Thanks,
> Ian

Hi Ian,

Thanks — v2 posted as "[PATCH v2] perf parse-events: default data
breakpoints to any size on RISC-V".  It exports
default_data_breakpoint_len()
and switches the data-breakpoint assertions to it; the rw checker now takes
the expected length, so the explicit /4 cases still assert
HW_BREAKPOINT_LEN_4.

I ran 'perf test -v 5' on riscv64 with the patched tree: the bp_len
assertions pass.  The only failure left is the pre-existing "wrong config2"
in 5.3, which reproduces without my change as well.

Rui

> 
>> ---
>>  tools/perf/util/parse-events.c | 23 ++++++++++++++++++++++-
>>  1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index f3ba70d4f76b..b72e4b0da8a1 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -598,6 +598,27 @@ size_t default_breakpoint_len(void)
>>  #endif
>>  }
>>
>> +/*
>> + * Default length for data breakpoints when the user did not specify one.
>> + *
>> + * Most architectures treat the length as a byte range that is compared
>> + * against the address of the access, so 4 bytes is a reasonable default
>> + * that matches most accesses.  RISC-V debug triggers (Sdtrig mcontrol6)
>> + * encode the width of the access in the trigger instead, and a trigger
>> + * only fires when the access has exactly that width.  This holds for
>> + * both RV32 and RV64.  With a fixed default, an access of a different
>> + * width silently never matches, so pass 0 on RISC-V, which the kernel
>> + * translates to "match any size".
>> + */
>> +static size_t default_data_breakpoint_len(void)
>> +{
>> +#if defined(__riscv)
>> +       return 0;
>> +#else
>> +       return HW_BREAKPOINT_LEN_4;
>> +#endif
>> +}
>> +
>>  static int
>>  parse_breakpoint_type(const char *type, struct perf_event_attr *attr)
>>  {
>> @@ -658,7 +679,7 @@ int parse_events_add_breakpoint(struct parse_events_state *parse_state,
>>                 if (attr.bp_type == HW_BREAKPOINT_X)
>>                         len = default_breakpoint_len();
>>                 else
>> -                       len = HW_BREAKPOINT_LEN_4;
>> +                       len = default_data_breakpoint_len();
>>         }
>>
>>         attr.bp_len = len;
>> --
>> 2.52.0
>>

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-11-11  6:24 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-11-09 12:03 [PATCH] perf parse-events: default data breakpoints to any size on RISC-V Rui Qi
2026-11-09 21:58 ` Ian Rogers
2026-11-11  6:23   ` Rui Qi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®