On 9/5/25 10:08, Masami Hiramatsu (Google) wrote:
On Thu, 4 Sep 2025 14:35:33 +0800
Jinchao Wang <wangjinchao...@gmail.com> wrote:

On 9/2/25 22:02, Masami Hiramatsu (Google) wrote:
(Adding Jinchao)

On Mon,  1 Sep 2025 23:44:35 +0900
"Masami Hiramatsu (Google)" <mhira...@kernel.org> wrote:

Hi,

Here is an RFC series for adding new wprobe (watch probe) which
provides memory access tracing event. Moreover, this can be used via
event trigger. Thus it can trace memory access on a dynamically
allocated objects too.

BTW, this series is on the top of probes/for-next branch in the
linux-trace tree.


Hi, Masami

Thanks for including me. I only received the cover letter, so I'm
providing my feedback here:

Thanks for your feedback! I'll send v2.


- trigger_data issue:
    it appears that the instance being removed is not the same as
    the one that was originally set.

Did you mean it will free invalid struct event_trigger_data by
wprobe_trigger_free(), or hw_breakpoint instance?

When trace_event_enable_disable(wprobe_data->file, 1, 1) at
wprobe_trigger_cmd_parse(), the trace_wprobe is enabled == call
enable_trace_wprobe(), which eventually call register_wide_hw_breakpoint()
with dummy address. IOW, the (wide)hw_breakpoint is allocated here.

When the trigger is hit, modify_wide_hw_breakpoint_local() is called
for updating local target address on the same hw_breakpoint.
So latter instance should be the same.
In wprobe_trigger_cmd_parse(), consider removing:
  echo '!set_wprobe:watch:dentry' >> events/fprobes/truncate/trigger
The trigger_data passed to event_trigger_unregister() is freshly allocated.


   - the function call issue:
     `trace_wprobe_update_local()` is called twice, once in the trigger
     callback and again in `wprobe_work_func`.

Yes, I found it can happen. Maybe I need a cpumask to skip the first one.

I also noticed that the Watchpoint probe and KStackWatch implementations
share very similar logic for managing hardware breakpoints/watchpoints
(`hwbp/watch`):
- `watch_init(unsigned long &place_holder)`
- `watch_on(struct perf_event_attr *attr)`
- `watch_off()` (or reset to the `place_holder` value)
- `watch_uninit()`

Hmm, I agree that we are using hwbp in a similar way but
I think current interface is enough.


Their primary difference lies in their handler functions, specifically
the `perf_overflow_handler_t triggered` callback.

I believe we could work together to unify this logic. I am open to
either approach: I can refactor my watch.c, or you can introduce new
helpers. This would help us save duplicated work and review time.

I'm not sure what you mean. Is there any concern to use the same
(current hwbp) interfaces?

It is not the _current_ HWBP interface, it needs review. We will write
the same logic except for the HWBP interface regarding atomic context
watch switch (addr/type/len) and the process to sync with other CPUs.

The only difference is the HWBP handler.

I am not familiar with the process of how we usually do roughly the
same thing. Maybe I am thinking about it the wrong way; I will look
at it later. > Thank you,


Thanks,


In this version, I reuse Jinchao's arch_reinstall_hw_breakpoint()
patch[1].

[1] 
https://lore.kernel.org/all/20250828073311.1116593-6-wangjinchao...@gmail.com/

The basic usage of this wprobe is similar to other probes;

    w:[GRP/][EVENT] [r|w|rw]@<ADDRESS|SYMBOL[+OFFS]> [FETCHARGS]

This defines a new wprobe event. For example, to trace jiffies update,
you can do;

   echo 'w:my_jiffies w@jiffies:8 value=+0($addr)' >> dynamic_events
   echo 1 > events/wprobes/my_jiffies/enable

Moreover, this can be combined with event trigger to trace the memory
accecss on slab objects. The trigger syntax is;

    set_wprobe:WPROBE_EVENT:FIELD[+ADJUST]
    clear_wprobe:WPROBE_EVENT

For example, trace the first 8 byte of the dentry data structure passed
to do_truncate() until it is deleted by __dentry_kill().
(Note: all tracefs setup uses '>>' so that it does not kick do_truncate())

    # echo 'w:watch rw@0:8 address=$addr value=+0($addr)' > dynamic_events

    # echo 'f:truncate do_truncate dentry=$arg2' >> dynamic_events
    # echo 'set_wprobe:watch:dentry' >> events/fprobes/truncate/trigger

    # echo 'f:dentry_kill __dentry_kill dentry=$arg1' >> dynamic_events
    # echo 'clear_wprobe:watch' >> events/fprobes/dentry_kill/trigger

    # echo 1 >> events/fprobes/truncate/enable
    # echo 1 >> events/fprobes/dentry_kill/enable

    # echo aaa > /tmp/hoge
    # echo bbb > /tmp/hoge
    # echo ccc > /tmp/hoge
    # rm /tmp/hoge

Then, the trace data will show;

# tracer: nop
#
# entries-in-buffer/entries-written: 16/16   #P:8
#
#                                _-----=> irqs-off/BH-disabled
#                               / _----=> need-resched
#                              | / _---=> hardirq/softirq
#                              || / _--=> preempt-depth
#                              ||| / _-=> migrate-disable
#                              |||| /     delay
#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#    [    7.026136] sh (113) used greatest stack depth: 12912 bytes left
            | |         |   |||||     |         |
                sh-113     [002] .....     7.024402: truncate: 
(do_truncate+0x4/0x120) dentry=0xffff8880069194b8
                sh-113     [002] ..Zff     7.024822: watch: 
(lookup_fast+0xaa/0x150) address=0xffff8880069194b8 value=0x200008
                sh-113     [002] ..Zff     7.024830: watch: 
(step_into+0x82/0x360) address=0xffff8880069194b8 value=0x200008
                sh-113     [002] ..Zff     7.024834: watch: 
(step_into+0x9f/0x360) address=0xffff8880069194b8 value=0x200008
                sh-113     [002] ..Zff     7.024839: watch: 
(path_openat+0xb3a/0xe70) address=0xffff8880069194b8 value=0x200008
                sh-113     [002] ..Zff     7.024843: watch: 
(path_openat+0xb9a/0xe70) address=0xffff8880069194b8 value=0x200008
                sh-113     [002] .....     7.024847: truncate: 
(do_truncate+0x4/0x120) dentry=0xffff8880069194b8
                sh-113     [002] ...1.     7.025364: dentry_kill: 
(__dentry_kill+0x0/0x220) dentry=0xffff888006919380
                sh-113     [002] ...1.     7.025511: dentry_kill: 
(__dentry_kill+0x0/0x220) dentry=0xffff8880069195f0
                rm-118     [003] ...1.     7.027543: dentry_kill: 
(__dentry_kill+0x0/0x220) dentry=0xffff8880069194b8
                sh-113     [002] ...2.     7.027825: dentry_kill: 
(__dentry_kill+0x0/0x220) dentry=0xffff8880044429c0
                sh-113     [002] ...2.     7.027833: dentry_kill: 
(__dentry_kill+0x0/0x220) dentry=0xffff888004442270


Thank you,

---

Jinchao Wang (1):
        x86/HWBP: introduce arch_reinstall_hw_breakpoint() for atomic context

Masami Hiramatsu (Google) (5):
        tracing: wprobe: Add watchpoint probe event based on hardware breakpoint
        HWBP: Add modify_wide_hw_breakpoint_local() API
        tracing: wprobe: Add wprobe event trigger
        selftests: tracing: Add a basic testcase for wprobe
        selftests: tracing: Add syntax testcase for wprobe


   Documentation/trace/index.rst                      |    1
   Documentation/trace/wprobetrace.rst                |  129 ++
   arch/Kconfig                                       |   10
   arch/x86/Kconfig                                   |    1
   arch/x86/include/asm/hw_breakpoint.h               |    3
   arch/x86/kernel/hw_breakpoint.c                    |   61 +
   include/linux/hw_breakpoint.h                      |    6
   include/linux/trace_events.h                       |    3
   kernel/events/hw_breakpoint.c                      |   36 +
   kernel/trace/Kconfig                               |   24
   kernel/trace/Makefile                              |    1
   kernel/trace/trace.c                               |    9
   kernel/trace/trace.h                               |    5
   kernel/trace/trace_probe.c                         |   20
   kernel/trace/trace_probe.h                         |    8
   kernel/trace/trace_wprobe.c                        | 1111 
++++++++++++++++++++
   .../ftrace/test.d/dynevent/add_remove_wprobe.tc    |   68 +
   .../test.d/dynevent/wprobes_syntax_errors.tc       |   20
   18 files changed, 1513 insertions(+), 3 deletions(-)
   create mode 100644 Documentation/trace/wprobetrace.rst
   create mode 100644 kernel/trace/trace_wprobe.c
   create mode 100644 
tools/testing/selftests/ftrace/test.d/dynevent/add_remove_wprobe.tc
   create mode 100644 
tools/testing/selftests/ftrace/test.d/dynevent/wprobes_syntax_errors.tc

--
Masami Hiramatsu (Google) <mhira...@kernel.org>




--
Best regards,
Jinchao




--
Best regards,
Jinchao

Reply via email to