Re: [PATCH V2 1/4] hw_breakpoint: Add step_needed event attribute
On Wed, Jul 26, 2017 at 11:12:03AM +0530, Pratyush Anand wrote: > I too had thought to put it under include/linux/perf_event.h : struct > perf_event. But, see hw_break_module_init() which does not have knowledge of > this structure, and we need to have some way so that none-perf kernel module > implementation can tell that it needs default arch step handler. > > Do you see any alternative? Fix the hw_breakpoint interface?
Re: [PATCH V2 1/4] hw_breakpoint: Add step_needed event attribute
On Tuesday 25 July 2017 06:57 PM, Will Deacon wrote:
On Fri, Jul 07, 2017 at 05:33:57PM +0530, Pratyush Anand wrote:
Architecture like ARM64 currently allows to use default hw breakpoint
single step handler only to perf. However, some other users like few
systemtap tests or kernel test in
samples/hw_breakpoint/data_breakpoint.c can also work with default step
handler implementation. At the same time, some other like GDB/ptrace may
implement their own step handler.
Therefore, this patch introduces a new perf_event_attr bit field, so
that arch specific code(specially on arm64) can make a decision to
enable single stepping.
Any architecture which is not using this field will not have any
side effect.
Signed-off-by: Pratyush Anand
---
include/linux/hw_breakpoint.h | 6 ++
include/uapi/linux/perf_event.h | 3 ++-
kernel/events/core.c | 2 ++
tools/include/uapi/linux/perf_event.h | 3 ++-
4 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/include/linux/hw_breakpoint.h b/include/linux/hw_breakpoint.h
index 0464c85e63fd..6173ae048cbc 100644
--- a/include/linux/hw_breakpoint.h
+++ b/include/linux/hw_breakpoint.h
@@ -38,6 +38,12 @@ static inline int hw_breakpoint_type(struct perf_event *bp)
return bp->attr.bp_type;
}
+static inline bool
+hw_breakpoint_needs_single_step(struct perf_event *bp)
+{
+ return bp->attr.step_needed;
+}
+
static inline unsigned long hw_breakpoint_len(struct perf_event *bp)
{
return bp->attr.bp_len;
diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index b1c0b187acfe..00935808de0d 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -345,7 +345,8 @@ struct perf_event_attr {
context_switch : 1, /* context switch data */
write_backward : 1, /* Write ring buffer from
end to beginning */
namespaces : 1, /* include namespaces data
*/
- __reserved_1 : 35;
+ step_needed: 1, /* Use arch step handler */
+ __reserved_1 : 34;
This needs documenting properly, as I really have no idea how userspace is
going to use it sensibley, especially as you silently overwrite it in some
cases below.
I too had thought to put it under include/linux/perf_event.h : struct
perf_event. But, see hw_break_module_init() which does not have knowledge of
this structure, and we need to have some way so that none-perf kernel module
implementation can tell that it needs default arch step handler.
Do you see any alternative?
--
Regards
Pratyush
Re: [PATCH V2 1/4] hw_breakpoint: Add step_needed event attribute
On Tue, Jul 25, 2017 at 04:14:23PM +0200, Peter Zijlstra wrote:
> On Tue, Jul 25, 2017 at 02:27:38PM +0100, Will Deacon wrote:
> > On Fri, Jul 07, 2017 at 05:33:57PM +0530, Pratyush Anand wrote:
> > > Architecture like ARM64 currently allows to use default hw breakpoint
> > > single step handler only to perf. However, some other users like few
> > > systemtap tests or kernel test in
> > > samples/hw_breakpoint/data_breakpoint.c can also work with default step
> > > handler implementation. At the same time, some other like GDB/ptrace may
> > > implement their own step handler.
> > >
> > > Therefore, this patch introduces a new perf_event_attr bit field, so
> > > that arch specific code(specially on arm64) can make a decision to
> > > enable single stepping.
> > >
> > > Any architecture which is not using this field will not have any
> > > side effect.
>
> > > diff --git a/include/uapi/linux/perf_event.h
> > > b/include/uapi/linux/perf_event.h
> > > index b1c0b187acfe..00935808de0d 100644
> > > --- a/include/uapi/linux/perf_event.h
> > > +++ b/include/uapi/linux/perf_event.h
> > > @@ -345,7 +345,8 @@ struct perf_event_attr {
> > > context_switch : 1, /* context switch data */
> > > write_backward : 1, /* Write ring buffer from
> > > end to beginning */
> > > namespaces : 1, /* include namespaces data
> > > */
> > > - __reserved_1 : 35;
> > > + step_needed: 1, /* Use arch step handler */
> > > + __reserved_1 : 34;
> >
> > This needs documenting properly, as I really have no idea how userspace is
> > going to use it sensibley, especially as you silently overwrite it in some
> > cases below.
>
> This is not something userspace _can_ use sensibly afaict. Therefore it
> should probably not live here.
Indeed. When I suggested this, I meant that it would be a
kernel-internal flag, and not UAPI.
Thanks,
Mark.
Re: [PATCH V2 1/4] hw_breakpoint: Add step_needed event attribute
On Tue, Jul 25, 2017 at 02:27:38PM +0100, Will Deacon wrote:
> On Fri, Jul 07, 2017 at 05:33:57PM +0530, Pratyush Anand wrote:
> > Architecture like ARM64 currently allows to use default hw breakpoint
> > single step handler only to perf. However, some other users like few
> > systemtap tests or kernel test in
> > samples/hw_breakpoint/data_breakpoint.c can also work with default step
> > handler implementation. At the same time, some other like GDB/ptrace may
> > implement their own step handler.
> >
> > Therefore, this patch introduces a new perf_event_attr bit field, so
> > that arch specific code(specially on arm64) can make a decision to
> > enable single stepping.
> >
> > Any architecture which is not using this field will not have any
> > side effect.
> > diff --git a/include/uapi/linux/perf_event.h
> > b/include/uapi/linux/perf_event.h
> > index b1c0b187acfe..00935808de0d 100644
> > --- a/include/uapi/linux/perf_event.h
> > +++ b/include/uapi/linux/perf_event.h
> > @@ -345,7 +345,8 @@ struct perf_event_attr {
> > context_switch : 1, /* context switch data */
> > write_backward : 1, /* Write ring buffer from
> > end to beginning */
> > namespaces : 1, /* include namespaces data
> > */
> > - __reserved_1 : 35;
> > + step_needed: 1, /* Use arch step handler */
> > + __reserved_1 : 34;
>
> This needs documenting properly, as I really have no idea how userspace is
> going to use it sensibley, especially as you silently overwrite it in some
> cases below.
This is not something userspace _can_ use sensibly afaict. Therefore it
should probably not live here.
Re: [PATCH V2 1/4] hw_breakpoint: Add step_needed event attribute
On Fri, Jul 07, 2017 at 05:33:57PM +0530, Pratyush Anand wrote:
> Architecture like ARM64 currently allows to use default hw breakpoint
> single step handler only to perf. However, some other users like few
> systemtap tests or kernel test in
> samples/hw_breakpoint/data_breakpoint.c can also work with default step
> handler implementation. At the same time, some other like GDB/ptrace may
> implement their own step handler.
>
> Therefore, this patch introduces a new perf_event_attr bit field, so
> that arch specific code(specially on arm64) can make a decision to
> enable single stepping.
>
> Any architecture which is not using this field will not have any
> side effect.
>
> Signed-off-by: Pratyush Anand
> ---
> include/linux/hw_breakpoint.h | 6 ++
> include/uapi/linux/perf_event.h | 3 ++-
> kernel/events/core.c | 2 ++
> tools/include/uapi/linux/perf_event.h | 3 ++-
> 4 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/hw_breakpoint.h b/include/linux/hw_breakpoint.h
> index 0464c85e63fd..6173ae048cbc 100644
> --- a/include/linux/hw_breakpoint.h
> +++ b/include/linux/hw_breakpoint.h
> @@ -38,6 +38,12 @@ static inline int hw_breakpoint_type(struct perf_event *bp)
> return bp->attr.bp_type;
> }
>
> +static inline bool
> +hw_breakpoint_needs_single_step(struct perf_event *bp)
> +{
> + return bp->attr.step_needed;
> +}
> +
> static inline unsigned long hw_breakpoint_len(struct perf_event *bp)
> {
> return bp->attr.bp_len;
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index b1c0b187acfe..00935808de0d 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -345,7 +345,8 @@ struct perf_event_attr {
> context_switch : 1, /* context switch data */
> write_backward : 1, /* Write ring buffer from
> end to beginning */
> namespaces : 1, /* include namespaces data
> */
> - __reserved_1 : 35;
> + step_needed: 1, /* Use arch step handler */
> + __reserved_1 : 34;
This needs documenting properly, as I really have no idea how userspace is
going to use it sensibley, especially as you silently overwrite it in some
cases below.
Will
>
> union {
> __u32 wakeup_events;/* wakeup every n events */
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 6c4e523dc1e2..220e26941475 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -9444,9 +9444,11 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
> } else if (is_write_backward(event)){
> event->overflow_handler = perf_event_output_backward;
> event->overflow_handler_context = NULL;
> + event->attr.step_needed = 1;
> } else {
> event->overflow_handler = perf_event_output_forward;
> event->overflow_handler_context = NULL;
> + event->attr.step_needed = 1;
> }
>
> perf_event__state_init(event);
> diff --git a/tools/include/uapi/linux/perf_event.h
> b/tools/include/uapi/linux/perf_event.h
> index b1c0b187acfe..00935808de0d 100644
> --- a/tools/include/uapi/linux/perf_event.h
> +++ b/tools/include/uapi/linux/perf_event.h
> @@ -345,7 +345,8 @@ struct perf_event_attr {
> context_switch : 1, /* context switch data */
> write_backward : 1, /* Write ring buffer from
> end to beginning */
> namespaces : 1, /* include namespaces data
> */
> - __reserved_1 : 35;
> + step_needed: 1, /* Use arch step handler */
> + __reserved_1 : 34;
>
> union {
> __u32 wakeup_events;/* wakeup every n events */
> --
> 2.9.3
>
[PATCH V2 1/4] hw_breakpoint: Add step_needed event attribute
Architecture like ARM64 currently allows to use default hw breakpoint
single step handler only to perf. However, some other users like few
systemtap tests or kernel test in
samples/hw_breakpoint/data_breakpoint.c can also work with default step
handler implementation. At the same time, some other like GDB/ptrace may
implement their own step handler.
Therefore, this patch introduces a new perf_event_attr bit field, so
that arch specific code(specially on arm64) can make a decision to
enable single stepping.
Any architecture which is not using this field will not have any
side effect.
Signed-off-by: Pratyush Anand
---
include/linux/hw_breakpoint.h | 6 ++
include/uapi/linux/perf_event.h | 3 ++-
kernel/events/core.c | 2 ++
tools/include/uapi/linux/perf_event.h | 3 ++-
4 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/include/linux/hw_breakpoint.h b/include/linux/hw_breakpoint.h
index 0464c85e63fd..6173ae048cbc 100644
--- a/include/linux/hw_breakpoint.h
+++ b/include/linux/hw_breakpoint.h
@@ -38,6 +38,12 @@ static inline int hw_breakpoint_type(struct perf_event *bp)
return bp->attr.bp_type;
}
+static inline bool
+hw_breakpoint_needs_single_step(struct perf_event *bp)
+{
+ return bp->attr.step_needed;
+}
+
static inline unsigned long hw_breakpoint_len(struct perf_event *bp)
{
return bp->attr.bp_len;
diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index b1c0b187acfe..00935808de0d 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -345,7 +345,8 @@ struct perf_event_attr {
context_switch : 1, /* context switch data */
write_backward : 1, /* Write ring buffer from
end to beginning */
namespaces : 1, /* include namespaces data
*/
- __reserved_1 : 35;
+ step_needed: 1, /* Use arch step handler */
+ __reserved_1 : 34;
union {
__u32 wakeup_events;/* wakeup every n events */
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 6c4e523dc1e2..220e26941475 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -9444,9 +9444,11 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
} else if (is_write_backward(event)){
event->overflow_handler = perf_event_output_backward;
event->overflow_handler_context = NULL;
+ event->attr.step_needed = 1;
} else {
event->overflow_handler = perf_event_output_forward;
event->overflow_handler_context = NULL;
+ event->attr.step_needed = 1;
}
perf_event__state_init(event);
diff --git a/tools/include/uapi/linux/perf_event.h
b/tools/include/uapi/linux/perf_event.h
index b1c0b187acfe..00935808de0d 100644
--- a/tools/include/uapi/linux/perf_event.h
+++ b/tools/include/uapi/linux/perf_event.h
@@ -345,7 +345,8 @@ struct perf_event_attr {
context_switch : 1, /* context switch data */
write_backward : 1, /* Write ring buffer from
end to beginning */
namespaces : 1, /* include namespaces data
*/
- __reserved_1 : 35;
+ step_needed: 1, /* Use arch step handler */
+ __reserved_1 : 34;
union {
__u32 wakeup_events;/* wakeup every n events */
--
2.9.3
