Re: [PATCH v2] perf/core: fast breakpoint modification via _IOC_MODIFY_ATTRIBUTES.

[Milind Chabbi]

On Tue, Nov 7, 2017 at 10:26 PM, Milind Chabbi <chabbi.milind@xxxxxxxxx> wrote:
> Problem and motivation: Once a breakpoint perf event (PERF_TYPE_BREAKPOINT)
> is created, there is no flexibility to change the breakpoint type
> (bp_type), breakpoint address (bp_addr), or breakpoint length (bp_len). The
> only option is to close the perf event and configure a new breakpoint
> event. This inflexibility has a significant performance overhead. For
> example, sampling-based, lightweight performance profilers (and also
> concurrency bug detection tools),  monitor different addresses for a short
> duration using PERF_TYPE_BREAKPOINT and change the address (bp_addr) to
> another address or change the kind of breakpoint (bp_type) from  "write" to
> a "read" or vice-versa or change the length (bp_len) of the address being
> monitored. The cost of these modifications is prohibitive since it involves
> unmapping the circular buffer associated with the perf event, closing the
> perf event, opening another perf event and mmaping another circular buffer.
>
> Solution: The new ioctl flag for perf events,
> PERF_EVENT_IOC_MODIFY_ATTRIBUTES, introduced in this patch takes a pointer
> to a struct perf_event_attr as an argument to update an old breakpoint
> event with new address, type, and size. This facility allows retaining a
> previous mmaped perf events ring buffer and avoids having to close and
> reopen another perf event.
>
> This patch supports only changing PERF_TYPE_BREAKPOINT event type; future
> implementations can extend this feature. The patch replicates some of its
> functionality of modify_user_hw_breakpoint() in
> kernel/events/hw_breakpoint.c. modify_user_hw_breakpoint cannot be called
> directly since perf_event_ctx_lock() is already held in _perf_ioctl().
>
> Evidence: Experiments show that the baseline (not able to modify an already
> created breakpoint) costs an order of magnitude (~10x) more than the
> suggested optimization (having the ability to dynamically modifying a
> configured breakpoint via ioctl). When the breakpoints typically do not
> trap, the speedup due to the suggested optimization is ~10x; even when the
> breakpoints always trap, the speedup is ~4x due to the suggested
> optimization.
>
> Testing: tests posted at
> https://github.com/linux-contrib/perf_event_modify_bp demonstrate the
> performance significance of this patch. Tests also check the functional
> correctness of the patch.
>
> Signed-off-by: Milind Chabbi <chabbi.milind@xxxxxxxxx>
> ---
>  include/uapi/linux/perf_event.h       |  2 ++
>  kernel/events/core.c                  | 61 +++++++++++++++++++++++++++++++++++
>  kernel/events/hw_breakpoint.c         |  2 +-
>  kernel/events/internal.h              |  1 +
>  tools/include/uapi/linux/perf_event.h |  2 ++
>  5 files changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index 362493a2f950..cd430821f022 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -433,6 +433,8 @@ struct perf_event_attr {
>  #define PERF_EVENT_IOC_ID              _IOR('$', 7, __u64 *)
>  #define PERF_EVENT_IOC_SET_BPF         _IOW('$', 8, __u32)
>  #define PERF_EVENT_IOC_PAUSE_OUTPUT    _IOW('$', 9, __u32)
> +#define PERF_EVENT_IOC_MODIFY_ATTRIBUTES       \
> +                       _IOW('$', 10, struct perf_event_attr *)
>
>  enum perf_event_ioc_flags {
>         PERF_IOC_FLAG_GROUP             = 1U << 0,
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 9d93db81fa36..2a01e94538d2 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -2746,6 +2746,55 @@ int perf_event_refresh(struct perf_event *event, int refresh)
>  }
>  EXPORT_SYMBOL_GPL(perf_event_refresh);
>
> +static int perf_event_modify_breakpoint(struct perf_event *bp,
> +                                        struct perf_event_attr *attr)
> +{
> +       u64 old_addr = bp->attr.bp_addr;
> +       u64 old_len = bp->attr.bp_len;
> +       int old_type = bp->attr.bp_type;
> +       int err = 0;
> +
> +       _perf_event_disable(bp);
> +
> +       bp->attr.bp_addr = attr->bp_addr;
> +       bp->attr.bp_type = attr->bp_type;
> +       bp->attr.bp_len = attr->bp_len;
> +
> +       if (attr->disabled)
> +               goto end;
> +
> +       err = validate_hw_breakpoint(bp);
> +       if (!err) {
> +               _perf_event_enable(bp);
> +       } else {
> +               bp->attr.bp_addr = old_addr;
> +               bp->attr.bp_type = old_type;
> +               bp->attr.bp_len = old_len;
> +               if (!bp->attr.disabled)
> +                       _perf_event_enable(bp);
> +
> +               return err;
> +       }
> +end:
> +       bp->attr.disabled = attr->disabled;
> +       return 0;
> +}
> +
> +static int perf_event_modify_attr(struct perf_event *event,
> +                                 struct perf_event_attr *attr)
> +{
> +       if (event->attr.type != attr->type)
> +               return -EINVAL;
> +
> +       switch (event->attr.type) {
> +       case PERF_TYPE_BREAKPOINT:
> +               return perf_event_modify_breakpoint(event, attr);
> +       default:
> +               /* Place holder for future additions. */
> +               return -EOPNOTSUPP;
> +       }
> +}
> +
>  static void ctx_sched_out(struct perf_event_context *ctx,
>                           struct perf_cpu_context *cpuctx,
>                           enum event_type_t event_type)
> @@ -4731,6 +4780,8 @@ static int perf_event_set_output(struct perf_event *event,
>                                  struct perf_event *output_event);
>  static int perf_event_set_filter(struct perf_event *event, void __user *arg);
>  static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd);
> +static int perf_copy_attr(struct perf_event_attr __user *uattr,
> +                         struct perf_event_attr *attr);
>
>  static long _perf_ioctl(struct perf_event *event, unsigned int cmd, unsigned long arg)
>  {
> @@ -4800,6 +4851,16 @@ static long _perf_ioctl(struct perf_event *event, unsigned int cmd, unsigned lon
>                 rcu_read_unlock();
>                 return 0;
>         }
> +       case PERF_EVENT_IOC_MODIFY_ATTRIBUTES: {
> +               struct perf_event_attr new_attr;
> +               int err = perf_copy_attr((struct perf_event_attr __user *)arg,
> +                                        &new_attr);
> +
> +               if (err)
> +                       return err;
> +
> +               return perf_event_modify_attr(event,  &new_attr);
> +       }
>         default:
>                 return -ENOTTY;
>         }
> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index 3f8cb1e14588..fde596cee420 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
> @@ -367,7 +367,7 @@ int dbg_release_bp_slot(struct perf_event *bp)
>         return 0;
>  }
>
> -static int validate_hw_breakpoint(struct perf_event *bp)
> +int validate_hw_breakpoint(struct perf_event *bp)
>  {
>         int ret;
>
> diff --git a/kernel/events/internal.h b/kernel/events/internal.h
> index 09b1537ae06c..acb2b8b01ba9 100644
> --- a/kernel/events/internal.h
> +++ b/kernel/events/internal.h
> @@ -82,6 +82,7 @@ extern int rb_alloc_aux(struct ring_buffer *rb, struct perf_event *event,
>  extern void rb_free_aux(struct ring_buffer *rb);
>  extern struct ring_buffer *ring_buffer_get(struct perf_event *event);
>  extern void ring_buffer_put(struct ring_buffer *rb);
> +extern int validate_hw_breakpoint(struct perf_event *bp);
>
>  static inline bool rb_has_aux(struct ring_buffer *rb)
>  {
> diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h
> index 140ae638cfd6..d1912309b7a5 100644
> --- a/tools/include/uapi/linux/perf_event.h
> +++ b/tools/include/uapi/linux/perf_event.h
> @@ -432,6 +432,8 @@ struct perf_event_attr {
>  #define PERF_EVENT_IOC_ID              _IOR('$', 7, __u64 *)
>  #define PERF_EVENT_IOC_SET_BPF         _IOW('$', 8, __u32)
>  #define PERF_EVENT_IOC_PAUSE_OUTPUT    _IOW('$', 9, __u32)
> +#define PERF_EVENT_IOC_MODIFY_ATTRIBUTES       \
> +                               _IOW('$', 10, struct perf_event_attr *)
>
>  enum perf_event_ioc_flags {
>         PERF_IOC_FLAG_GROUP             = 1U << 0,
> --
> 2.14.1
>



Peter and Andi,

Can you confirm whether this V2 patch addresses your "attribute
modification generality" concerns?

-Milind