From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751262AbdKLTTH (ORCPT ); Sun, 12 Nov 2017 14:19:07 -0500 Received: from mail-ot0-f193.google.com ([74.125.82.193]:57145 "EHLO mail-ot0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750802AbdKLTTG (ORCPT ); Sun, 12 Nov 2017 14:19:06 -0500 X-Google-Smtp-Source: AGs4zMYT/G7TI5yCisO/3mFTHqYihVzkMy16MaIK0bWe25xklbaPFknYiig0Q8nk3WfjmlpSS2ijgAZreiiL6WRCJ/s= MIME-Version: 1.0 In-Reply-To: <1510122418-25840-1-git-send-email-chabbi.milind@gmail.com> References: <1510004932-17864-1-git-send-email-chabbi.milind@gmail.com> <1510122418-25840-1-git-send-email-chabbi.milind@gmail.com> From: Milind Chabbi Date: Sun, 12 Nov 2017 11:19:04 -0800 X-Google-Sender-Auth: JiqpUZ9oPOwrn4SmRHkXjRhReQI Message-ID: Subject: Re: [PATCH v2] perf/core: fast breakpoint modification via _IOC_MODIFY_ATTRIBUTES. 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@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Nov 7, 2017 at 10:26 PM, Milind Chabbi 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 > --- > 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