* Re: [PATCH v2] perf/core: fast breakpoint modification via _IOC_MODIFY_ATTRIBUTES.
2017-11-08 6:26 ` [PATCH v2] perf/core: fast breakpoint modification via _IOC_MODIFY_ATTRIBUTES Milind Chabbi
@ 2017-11-12 19:19 ` Milind Chabbi
0 siblings, 0 replies; 2+ messages in thread
From: Milind Chabbi @ 2017-11-12 19:19 UTC (permalink / raw)
To: Milind Chabbi, Peter Zijlstra, Andi Kleen
Cc: Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Michael Ellerman, Hari Bathini, Jin Yao,
Kan Liang, Sukadev Bhattiprolu, linux-kernel
On Tue, Nov 7, 2017 at 10:26 PM, Milind Chabbi <chabbi.milind@gmail.com> 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@gmail.com>
> ---
> 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
^ permalink raw reply [flat|nested] 2+ messages in thread