Re: Process-wide watchpoints

[Dmitry Vyukov]

On Thu, Feb 4, 2021 at 2:33 PM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> On Thu, Feb 04, 2021 at 01:53:59PM +0100, Dmitry Vyukov wrote:
> > On Thu, Feb 4, 2021 at 1:09 PM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> > > What do we do then? The advantage of IOC_REFRESH is that it disables the
> > > event until it gets explicitly re-armed, avoiding recursion issues etc.
> > > Do you want those semantics? If so, we'd need to have IOC_REFRESH find
> > > the actual event for the current task, which should be doable I suppose.
> >
> > Frankly, I don't know. I didn't use it in my prototype, nor I fully
> > understand what it's doing. Does it make sense for breakpoints?
> > I see IOC_REFRESH has a check for !attr.inherit, so it will fail for
> > my use case currently. I would say we just leave it as is for now.
>
> Well, the way it works is that currently you set event_limit > 0. Then
> each event will decrement, when we hit 0 we disable and raise a signal.
>
> REFRESH will increment event_limit and re-enable.
>
> This means you're guaranteed not to get another signal until you're
> ready for it. It allows leaving the signal handler context to handle the
> signal.
>
> I suppose you're looking for something like this, which goes in top of
> that thread_only thing.
>
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -389,7 +389,8 @@ struct perf_event_attr {
>                                 cgroup         :  1, /* include cgroup events */
>                                 text_poke      :  1, /* include text poke events */
>                                 thread_only    :  1, /* only inherit on threads */
> -                               __reserved_1   : 29;
> +                               sigtrap        :  1, /* foo */
> +                               __reserved_1   : 28;
>
>         union {
>                 __u32           wakeup_events;    /* wakeup every n events */
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6273,6 +6273,13 @@ static void perf_pending_event_disable(s
>
>         if (cpu == smp_processor_id()) {
>                 WRITE_ONCE(event->pending_disable, -1);
> +
> +               if (event->attr.sigtrap) {
> +                       atomic_inc(&event->event_limit); /* rearm */
> +                       send_sig_info(SIGTRAP, SEND_SIG_PRIV, current);
> +                       return;
> +               }
> +
>                 perf_event_disable_local(event);
>                 return;
>         }
> @@ -8936,6 +8943,7 @@ static int __perf_event_overflow(struct
>                                    int throttle, struct perf_sample_data *data,
>                                    struct pt_regs *regs)
>  {
> +       perf_overflow_handler_t ovf;
>         int events = atomic_read(&event->event_limit);
>         int ret = 0;
>
> @@ -8961,7 +8969,15 @@ static int __perf_event_overflow(struct
>                 perf_event_disable_inatomic(event);
>         }
>
> -       READ_ONCE(event->overflow_handler)(event, data, regs);
> +       ovf = READ_ONCE(event->overflow_handler);
> +#ifdef CONFIG_RETPOLINE
> +       if (ovf == perf_event_output_forward) {
> +               perf_event_output_forward(event, data, regs);
> +       } else if (ovf == perf_event_output_backward) {
> +               perf_event_output_backward(event, data, regs);
> +       } else
> +#endif
> +               ovf(event, data, regs);
>
>         if (*perf_event_fasync(event) && event->pending_kill) {
>                 event->pending_wakeup = 1;
> @@ -11281,6 +11297,9 @@ perf_event_alloc(struct perf_event_attr
>
>         event->state            = PERF_EVENT_STATE_INACTIVE;
>
> +       if (event->attr.sigtrap)
> +               event->event_limit = ATOMIC_INIT(1);
> +
>         if (task) {
>                 event->attach_state = PERF_ATTACH_TASK;
>                 /*
> @@ -11556,6 +11575,9 @@ static int perf_copy_attr(struct perf_ev
>         if (attr->thread_only && !attr->inherit)
>                 return -EINVAL;
>
> +       if (attr->sigtrap && attr->inherit && !attr->thread_only)
> +               return -EINVAL;
> +
>  out:
>         return ret;


Thanks. Let me see if this will work for us and test.