Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT

Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT

[Arnaldo Carvalho de Melo]

Em Mon, Nov 06, 2017 at 07:00:29AM -0800, Milind Chabbi escreveu:
> Hi Jirka,
> 
> I see the tabs in my sent email, do you have suggestions on how best to
> send this patch so that the tabs are preserved by the email client?

Documentation/process/email-clients.rst

- Arnaldo
 
> Can anybody else also check if they received with/without tabs?
> 
> release_bp_slot/reserve_bp_slot majic is not necessary since
> _IOC_MODIFY_BREAKPOINT ioctl modifies an already registered breakpoint
> without affecting the count of breakpoints active.
> 
> -Milind
> 
> On Mon, Nov 6, 2017 at 1:23 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> 
> > On Sun, Nov 05, 2017 at 02:35:34PM -0800, Milind Chabbi wrote:
> >
> > SNIP
> >
> > > +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);
> >
> > thre patch is mangled.. seems like you've lost all your tabs somehow
> >
> > anyway, should you also do the release_bp_slot/reserve_bp_slot
> > magic to keep the slot accounting corrent?
> >
> > jirka
> >

Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT

[Jiri Olsa]

On Sun, Nov 05, 2017 at 02:35:34PM -0800, Milind Chabbi wrote:

SNIP

> +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);

thre patch is mangled.. seems like you've lost all your tabs somehow

anyway, should you also do the release_bp_slot/reserve_bp_slot
magic to keep the slot accounting corrent?

jirka

Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT

[Jiri Olsa]

On Mon, Nov 06, 2017 at 07:04:40AM -0800, Milind Chabbi wrote:
> Hi Jirka,
> 
> I see the tabs in my sent email, do you have suggestions on how best to
> send this patch so that the tabs are preserved by the email client?
> Can anybody else also check if they received with/without tabs?
> 
> release_bp_slot/reserve_bp_slot majic is not necessary since
> _IOC_MODIFY_BREAKPOINT ioctl modifies an already registered breakpoint
> without affecting the count of breakpoints active.

but AFAICS you allow to change the breakpoint type (bp_type)
and slot counts are based on the breakpoint type

jirka

Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT

[Milind Chabbi]

On Wed, Nov 8, 2017 at 6:15 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Mon, Nov 06, 2017 at 07:04:40AM -0800, Milind Chabbi wrote:
>> Hi Jirka,
>>
>> I see the tabs in my sent email, do you have suggestions on how best to
>> send this patch so that the tabs are preserved by the email client?
>> Can anybody else also check if they received with/without tabs?
>>
>> release_bp_slot/reserve_bp_slot majic is not necessary since
>> _IOC_MODIFY_BREAKPOINT ioctl modifies an already registered breakpoint
>> without affecting the count of breakpoints active.
>
> but AFAICS you allow to change the breakpoint type (bp_type)
> and slot counts are based on the breakpoint type
>
> jirka

Jirka,
I am not able to fully understand your concern.
Can you point to a code file and line related to your observation?
The patch is modeled after the existing modify_user_hw_breakpoint() function
present in events/hw_breakpoint.c; don't you see this problem in that code?

-Milind

Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT

[Jiri Olsa]

On Wed, Nov 08, 2017 at 07:02:22AM -0800, Milind Chabbi wrote:
> On Wed, Nov 8, 2017 at 6:15 AM, Jiri Olsa <jolsa-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Mon, Nov 06, 2017 at 07:04:40AM -0800, Milind Chabbi wrote:
> >> Hi Jirka,
> >>
> >> I see the tabs in my sent email, do you have suggestions on how best to
> >> send this patch so that the tabs are preserved by the email client?
> >> Can anybody else also check if they received with/without tabs?
> >>
> >> release_bp_slot/reserve_bp_slot majic is not necessary since
> >> _IOC_MODIFY_BREAKPOINT ioctl modifies an already registered breakpoint
> >> without affecting the count of breakpoints active.
> >
> > but AFAICS you allow to change the breakpoint type (bp_type)
> > and slot counts are based on the breakpoint type
> >
> > jirka
> 
> Jirka,
> I am not able to fully understand your concern.
> Can you point to a code file and line related to your observation?
> The patch is modeled after the existing modify_user_hw_breakpoint() function
> present in events/hw_breakpoint.c; don't you see this problem in that code?

the reserve_bp_slot/release_bp_slot functions manage
counts for current breakpoints based on its type

those counts are cumulated in here:
  static DEFINE_PER_CPU(struct bp_cpuinfo, bp_cpuinfo[TYPE_MAX]);

you allow to change the breakpoint type, so I'd expect
to see some code that release slot count for old type
and take new one (if it's available)

jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT

[Milind Chabbi]

On Wed, Nov 8, 2017 at 7:12 AM, Jiri Olsa <jolsa@redhat.com> wrote:

> > I am not able to fully understand your concern.
> > Can you point to a code file and line related to your observation?
> > The patch is modeled after the existing modify_user_hw_breakpoint() function
> > present in events/hw_breakpoint.c; don't you see this problem in that code?
>
> the reserve_bp_slot/release_bp_slot functions manage
> counts for current breakpoints based on its type
>
> those counts are cumulated in here:
>   static DEFINE_PER_CPU(struct bp_cpuinfo, bp_cpuinfo[TYPE_MAX]);
>
> you allow to change the breakpoint type, so I'd expect
> to see some code that release slot count for old type
> and take new one (if it's available)
>
> jirka


Why is this not a concern for modify_user_hw_breakpoint() function?

Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT

[Jiri Olsa]

On Wed, Nov 08, 2017 at 07:51:10AM -0800, Milind Chabbi wrote:
> On Wed, Nov 8, 2017 at 7:12 AM, Jiri Olsa <jolsa-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > > I am not able to fully understand your concern.
> > > Can you point to a code file and line related to your observation?
> > > The patch is modeled after the existing modify_user_hw_breakpoint() function
> > > present in events/hw_breakpoint.c; don't you see this problem in that code?
> >
> > the reserve_bp_slot/release_bp_slot functions manage
> > counts for current breakpoints based on its type
> >
> > those counts are cumulated in here:
> >   static DEFINE_PER_CPU(struct bp_cpuinfo, bp_cpuinfo[TYPE_MAX]);
> >
> > you allow to change the breakpoint type, so I'd expect
> > to see some code that release slot count for old type
> > and take new one (if it's available)
> >
> > jirka
> 
> 
> Why is this not a concern for modify_user_hw_breakpoint() function?

I don't know ;-)

jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT

[Milind Chabbi]

On Wed, Nov 8, 2017 at 7:57 AM, Jiri Olsa <jolsa-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Wed, Nov 08, 2017 at 07:51:10AM -0800, Milind Chabbi wrote:
>> On Wed, Nov 8, 2017 at 7:12 AM, Jiri Olsa <jolsa-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>
>> > > I am not able to fully understand your concern.
>> > > Can you point to a code file and line related to your observation?
>> > > The patch is modeled after the existing modify_user_hw_breakpoint() function
>> > > present in events/hw_breakpoint.c; don't you see this problem in that code?
>> >
>> > the reserve_bp_slot/release_bp_slot functions manage
>> > counts for current breakpoints based on its type
>> >
>> > those counts are cumulated in here:
>> >   static DEFINE_PER_CPU(struct bp_cpuinfo, bp_cpuinfo[TYPE_MAX]);
>> >
>> > you allow to change the breakpoint type, so I'd expect
>> > to see some code that release slot count for old type
>> > and take new one (if it's available)
>> >
>> > jirka
>>
>>
>> Why is this not a concern for modify_user_hw_breakpoint() function?
>
> I don't know ;-)
>
> jirka


Jirka,

I carefully looked at bp_cpuinfo[] and nr_slots[] data structures.
nr_slots[] is an array of length two (one slot of TYPE_INST and
another for TYPE_DATA).
The accounting "thinks" that there is one limit on the number of
instruction breakpoints and another limit on the number of data
breakpoints.
The assumption is clearly broken; for example, on x86 there exists a
limit on the *total* number of all breakpoints disregarding their kind
and the code has failed to capture this aspect.

As such, modify_user_hw_breakpoint() makes no attempt to keep the
counts correct. Instead, it simply tries to change and install a new
breakpoint and fails if the hardware disallows.
This can lead to a situation where, say on x86, someone creates 4
TYPE_DATA breakpoints, then changes one of them to TYPE_INS via
modify_user_hw_breakpoint() and then releases the TYPE_INS breakpoint.
Since the accounting still thinks that there are four TYPE_DATA
breakpoints, it will disallow creating a new TYPE_DATA breakpoint,
although there is place for one TYPE_DATA breakpoint.

This convinces me that the problem and the solution are outside of
this current patch.
Do you agree?

-Milind
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT

[Jiri Olsa]

On Wed, Nov 08, 2017 at 08:59:22AM -0800, Milind Chabbi wrote:
> On Wed, Nov 8, 2017 at 7:57 AM, Jiri Olsa <jolsa-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Wed, Nov 08, 2017 at 07:51:10AM -0800, Milind Chabbi wrote:
> >> On Wed, Nov 8, 2017 at 7:12 AM, Jiri Olsa <jolsa-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >>
> >> > > I am not able to fully understand your concern.
> >> > > Can you point to a code file and line related to your observation?
> >> > > The patch is modeled after the existing modify_user_hw_breakpoint() function
> >> > > present in events/hw_breakpoint.c; don't you see this problem in that code?
> >> >
> >> > the reserve_bp_slot/release_bp_slot functions manage
> >> > counts for current breakpoints based on its type
> >> >
> >> > those counts are cumulated in here:
> >> >   static DEFINE_PER_CPU(struct bp_cpuinfo, bp_cpuinfo[TYPE_MAX]);
> >> >
> >> > you allow to change the breakpoint type, so I'd expect
> >> > to see some code that release slot count for old type
> >> > and take new one (if it's available)
> >> >
> >> > jirka
> >>
> >>
> >> Why is this not a concern for modify_user_hw_breakpoint() function?
> >
> > I don't know ;-)
> >
> > jirka
> 
> 
> Jirka,
> 
> I carefully looked at bp_cpuinfo[] and nr_slots[] data structures.
> nr_slots[] is an array of length two (one slot of TYPE_INST and
> another for TYPE_DATA).
> The accounting "thinks" that there is one limit on the number of
> instruction breakpoints and another limit on the number of data
> breakpoints.
> The assumption is clearly broken; for example, on x86 there exists a
> limit on the *total* number of all breakpoints disregarding their kind
> and the code has failed to capture this aspect.

there's the CONFIG_HAVE_MIXED_BREAKPOINTS_REGS that puts DATA and INST
under one count on x86.. but that seems to be the enabled only for:

	arch/sh/Kconfig:        select HAVE_MIXED_BREAKPOINTS_REGS
	arch/x86/Kconfig:       select HAVE_MIXED_BREAKPOINTS_REGS

> 
> As such, modify_user_hw_breakpoint() makes no attempt to keep the
> counts correct. Instead, it simply tries to change and install a new
> breakpoint and fails if the hardware disallows.
> This can lead to a situation where, say on x86, someone creates 4
> TYPE_DATA breakpoints, then changes one of them to TYPE_INS via
> modify_user_hw_breakpoint() and then releases the TYPE_INS breakpoint.
> Since the accounting still thinks that there are four TYPE_DATA
> breakpoints, it will disallow creating a new TYPE_DATA breakpoint,
> although there is place for one TYPE_DATA breakpoint.
> 
> This convinces me that the problem and the solution are outside of
> this current patch.
> Do you agree?

I'll leave this decision to maintainer ;-) but seems better to fix
the interface before we add any new dependent function calls

jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT

[Jiri Olsa]

On Thu, Nov 09, 2017 at 08:46:58AM +0100, Jiri Olsa wrote:

SNIP

> > Jirka,
> > 
> > I carefully looked at bp_cpuinfo[] and nr_slots[] data structures.
> > nr_slots[] is an array of length two (one slot of TYPE_INST and
> > another for TYPE_DATA).
> > The accounting "thinks" that there is one limit on the number of
> > instruction breakpoints and another limit on the number of data
> > breakpoints.
> > The assumption is clearly broken; for example, on x86 there exists a
> > limit on the *total* number of all breakpoints disregarding their kind
> > and the code has failed to capture this aspect.
> 
> there's the CONFIG_HAVE_MIXED_BREAKPOINTS_REGS that puts DATA and INST
> under one count on x86.. but that seems to be the enabled only for:
> 
> 	arch/sh/Kconfig:        select HAVE_MIXED_BREAKPOINTS_REGS
> 	arch/x86/Kconfig:       select HAVE_MIXED_BREAKPOINTS_REGS
> 
> > 
> > As such, modify_user_hw_breakpoint() makes no attempt to keep the
> > counts correct. Instead, it simply tries to change and install a new
> > breakpoint and fails if the hardware disallows.
> > This can lead to a situation where, say on x86, someone creates 4
> > TYPE_DATA breakpoints, then changes one of them to TYPE_INS via
> > modify_user_hw_breakpoint() and then releases the TYPE_INS breakpoint.
> > Since the accounting still thinks that there are four TYPE_DATA
> > breakpoints, it will disallow creating a new TYPE_DATA breakpoint,
> > although there is place for one TYPE_DATA breakpoint.
> > 
> > This convinces me that the problem and the solution are outside of
> > this current patch.
> > Do you agree?
> 
> I'll leave this decision to maintainer ;-) but seems better to fix
> the interface before we add any new dependent function calls

how about something like below (untested)

looks like there's no irq caller for modify_user_hw_breakpoint,
so we should be fine with locking nr_bp_mutex

jirka


---
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 3f8cb1e14588..f062b68399ea 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -448,6 +448,8 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
 	else
 		perf_event_disable(bp);
 
+	release_bp_slot(bp);
+
 	bp->attr.bp_addr = attr->bp_addr;
 	bp->attr.bp_type = attr->bp_type;
 	bp->attr.bp_len = attr->bp_len;
@@ -455,9 +457,9 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
 	if (attr->disabled)
 		goto end;
 
-	err = validate_hw_breakpoint(bp);
+	err = reserve_bp_slot(bp);
 	if (!err)
-		perf_event_enable(bp);
+		err = validate_hw_breakpoint(bp);
 
 	if (err) {
 		bp->attr.bp_addr = old_addr;
@@ -469,6 +471,7 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
 		return err;
 	}
 
+	perf_event_enable(bp);
 end:
 	bp->attr.disabled = attr->disabled;
 
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT

[Milind Chabbi]

SNIP

On Thu, Nov 9, 2017 at 5:12 AM, Jiri Olsa <jolsa-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
>
> how about something like below (untested)
>
> looks like there's no irq caller for modify_user_hw_breakpoint,
> so we should be fine with locking nr_bp_mutex
>
> jirka
>
>
> ---
> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index 3f8cb1e14588..f062b68399ea 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
> @@ -448,6 +448,8 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
>         else
>                 perf_event_disable(bp);
>
> +       release_bp_slot(bp);
> +
>         bp->attr.bp_addr = attr->bp_addr;
>         bp->attr.bp_type = attr->bp_type;
>         bp->attr.bp_len = attr->bp_len;
> @@ -455,9 +457,9 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
>         if (attr->disabled)
>                 goto end;
>
> -       err = validate_hw_breakpoint(bp);
> +       err = reserve_bp_slot(bp);
>         if (!err)
> -               perf_event_enable(bp);
> +               err = validate_hw_breakpoint(bp);
>
>         if (err) {
>                 bp->attr.bp_addr = old_addr;
> @@ -469,6 +471,7 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
>                 return err;
>         }
>
> +       perf_event_enable(bp);
>  end:
>         bp->attr.disabled = attr->disabled;
>

We can do this accounting only if bp->attr.bp_type != attr->bp_type.

-Milind
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT

[Milind Chabbi]

 ,

On Thu, Nov 9, 2017 at 10:59 AM, Milind Chabbi <chabbi.milind-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> SNIP
>
> On Thu, Nov 9, 2017 at 5:12 AM, Jiri Olsa <jolsa-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>
>>
>> how about something like below (untested)
>>
>> looks like there's no irq caller for modify_user_hw_breakpoint,
>> so we should be fine with locking nr_bp_mutex
>>
>> jirka
>>
>>
>> ---
>> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
>> index 3f8cb1e14588..f062b68399ea 100644
>> --- a/kernel/events/hw_breakpoint.c
>> +++ b/kernel/events/hw_breakpoint.c
>> @@ -448,6 +448,8 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
>>         else
>>                 perf_event_disable(bp);
>>
>> +       release_bp_slot(bp);
>> +
>>         bp->attr.bp_addr = attr->bp_addr;
>>         bp->attr.bp_type = attr->bp_type;
>>         bp->attr.bp_len = attr->bp_len;
>> @@ -455,9 +457,9 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
>>         if (attr->disabled)
>>                 goto end;
>>
>> -       err = validate_hw_breakpoint(bp);
>> +       err = reserve_bp_slot(bp);
>>         if (!err)
>> -               perf_event_enable(bp);
>> +               err = validate_hw_breakpoint(bp);
>>
>>         if (err) {
>>                 bp->attr.bp_addr = old_addr;
>> @@ -469,6 +471,7 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
>>                 return err;
>>         }
>>
>> +       perf_event_enable(bp);
>>  end:
>>         bp->attr.disabled = attr->disabled;
>>
>
> We can do this accounting only if bp->attr.bp_type != attr->bp_type.
>
> -Milind


Jirka,

Neither of us seems to fully understand the convoluted logic used in
breakpoint counting.

I tested the following sequence on an x86 machine, which has four
debug registers (without your suggested patch for counting
correction).

fd1 = perf_event_open(...); //BP_TYPE= HW_BREAKPOINT_RW @ ADDR1
fd2 = perf_event_open(...); //BP_TYPE= HW_BREAKPOINT_RW @ ADDR2
fd3 = perf_event_open(...); //BP_TYPE= HW_BREAKPOINT_RW @ ADDR3
fd4 = perf_event_open(...); //BP_TYPE= HW_BREAKPOINT_RW @ ADDR4
ioctl(fd4, MODIFY, ...); // change fd4 to BP_TYPE= HW_BREAKPOINT_X @ ADDR5
close(fd4);
fd5 = perf_event_open(); //BP_TYPE=RW @ ADDR6

We expected fd5 to fail because four BP_TYPE=TYPE_DATA are in use as
per the accounting, but in reality, fd5 was successfully opened.

Is the accounting accidentally working on x86?
Is there another architecture where TYPE_DATA and TYPE_INS are counted
differently?

-Milind
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT

[Jiri Olsa]

On Sun, Nov 12, 2017 at 11:09:23AM -0800, Milind Chabbi wrote:
>  ,
> 
> On Thu, Nov 9, 2017 at 10:59 AM, Milind Chabbi <chabbi.milind-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > SNIP
> >
> > On Thu, Nov 9, 2017 at 5:12 AM, Jiri Olsa <jolsa-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >>
> >>
> >> how about something like below (untested)
> >>
> >> looks like there's no irq caller for modify_user_hw_breakpoint,
> >> so we should be fine with locking nr_bp_mutex
> >>
> >> jirka
> >>
> >>
> >> ---
> >> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> >> index 3f8cb1e14588..f062b68399ea 100644
> >> --- a/kernel/events/hw_breakpoint.c
> >> +++ b/kernel/events/hw_breakpoint.c
> >> @@ -448,6 +448,8 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
> >>         else
> >>                 perf_event_disable(bp);
> >>
> >> +       release_bp_slot(bp);
> >> +
> >>         bp->attr.bp_addr = attr->bp_addr;
> >>         bp->attr.bp_type = attr->bp_type;
> >>         bp->attr.bp_len = attr->bp_len;
> >> @@ -455,9 +457,9 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
> >>         if (attr->disabled)
> >>                 goto end;
> >>
> >> -       err = validate_hw_breakpoint(bp);
> >> +       err = reserve_bp_slot(bp);
> >>         if (!err)
> >> -               perf_event_enable(bp);
> >> +               err = validate_hw_breakpoint(bp);
> >>
> >>         if (err) {
> >>                 bp->attr.bp_addr = old_addr;
> >> @@ -469,6 +471,7 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
> >>                 return err;
> >>         }
> >>
> >> +       perf_event_enable(bp);
> >>  end:
> >>         bp->attr.disabled = attr->disabled;
> >>
> >
> > We can do this accounting only if bp->attr.bp_type != attr->bp_type.
> >
> > -Milind
> 
> 
> Jirka,
> 
> Neither of us seems to fully understand the convoluted logic used in
> breakpoint counting.

yea, I was hoping some of the guys would take over ;-)

the problem I have with the patch above is that we could
fail to reserve the slot at the end, which is not what
the caller might expect

> 
> I tested the following sequence on an x86 machine, which has four
> debug registers (without your suggested patch for counting
> correction).
> 
> fd1 = perf_event_open(...); //BP_TYPE= HW_BREAKPOINT_RW @ ADDR1
> fd2 = perf_event_open(...); //BP_TYPE= HW_BREAKPOINT_RW @ ADDR2
> fd3 = perf_event_open(...); //BP_TYPE= HW_BREAKPOINT_RW @ ADDR3
> fd4 = perf_event_open(...); //BP_TYPE= HW_BREAKPOINT_RW @ ADDR4
> ioctl(fd4, MODIFY, ...); // change fd4 to BP_TYPE= HW_BREAKPOINT_X @ ADDR5
> close(fd4);
> fd5 = perf_event_open(); //BP_TYPE=RW @ ADDR6
> 
> We expected fd5 to fail because four BP_TYPE=TYPE_DATA are in use as
> per the accounting, but in reality, fd5 was successfully opened.

but you closed fd4 before openning fd5..?

> 
> Is the accounting accidentally working on x86?
> Is there another architecture where TYPE_DATA and TYPE_INS are counted
> differently?

[jolsa@krava linux-perf]$ grep -r HAVE_MIXED_BREAKPOINTS_REGS arch/*
arch/Kconfig:config HAVE_MIXED_BREAKPOINTS_REGS
arch/sh/Kconfig:        select HAVE_MIXED_BREAKPOINTS_REGS
arch/x86/Kconfig:       select HAVE_MIXED_BREAKPOINTS_REGS

I'll try to check on it this week

jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT

[Milind Chabbi]

SNIP

On Sun, Nov 12, 2017 at 11:46 PM, Jiri Olsa <jolsa@redhat.com> wrote:

> but you closed fd4 before openning fd5..?

Yes, that is correct. I closed fd4. The reason is by closing fd4, we
are having a total of 3 hardware breakpoints active, but we are making
the software counting in the kernel think that four TYPE_DATA
breakpoints active. The counting should have disallowed us from
creating fd5 as per the following logic in the kernel:

static int __reserve_bp_slot(struct perf_event *bp)

{
 ....

        /* Flexible counters need to keep at least one slot */
        if (slots.pinned + (!!slots.flexible) > nr_slots[type])
                return -ENOSPC;
....
}

Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT

[Jiri Olsa]

On Mon, Nov 13, 2017 at 12:02:56AM -0800, Milind Chabbi wrote:
> SNIP
> 
> On Sun, Nov 12, 2017 at 11:46 PM, Jiri Olsa <jolsa-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > but you closed fd4 before openning fd5..?
> 
> Yes, that is correct. I closed fd4. The reason is by closing fd4, we
> are having a total of 3 hardware breakpoints active, but we are making
> the software counting in the kernel think that four TYPE_DATA
> breakpoints active. The counting should have disallowed us from
> creating fd5 as per the following logic in the kernel:
> 
> static int __reserve_bp_slot(struct perf_event *bp)
> 
> {
>  ....
> 
>         /* Flexible counters need to keep at least one slot */
>         if (slots.pinned + (!!slots.flexible) > nr_slots[type])
>                 return -ENOSPC;
> ....
> }

So the issue is with the cpu pinned breakpoints, because we keep
their slot counts for both breakpoint types. For task breakpoints
we dont keep the slot count, we just count it every time we need it.

The issue will not expose on x86, because both breakpoint types
share same slot count (CONFIG_HAVE_MIXED_BREAKPOINTS_REGS).

I'm seeing the issue on arm machine (with 4 watchpoints and 6 breakpoints)

creating 4 watchpoints:
	2028  perf_event_open(0xffffdb232bd0, -1, 0, -1, PERF_FLAG_FD_CLOEXEC) = 3
	2028  perf_event_open(0xffffdb232c40, -1, 0, -1, PERF_FLAG_FD_CLOEXEC) = 4
	2028  perf_event_open(0xffffdb232cb0, -1, 0, -1, PERF_FLAG_FD_CLOEXEC) = 5
	2028  perf_event_open(0xffffdb232d20, -1, 0, -1, PERF_FLAG_FD_CLOEXEC) = 6

changing last one to breakpoint:
	2028  ioctl(6, _IOC(_IOC_WRITE, 0x24, 0x0a, 0x08), 0xffffdb232e08) = 0

and trying to create one more watchpoint:
	2028  perf_event_open(0xffffdb232d90, -1, 0, -1, PERF_FLAG_FD_CLOEXEC) = -1 ENOSPC (No space left on device)

after this, we have slot counts:
	get_bp_info(0, TYPE_DATA)->cpu_pinned = 4
	get_bp_info(0, TYPE_INST)->cpu_pinned = 0

now when we close all of it:
	close(3)
	close(4)
	close(5)
	close(6)

we get the slot counts messed up, because fd 6 has different type now:
	get_bp_info(0, TYPE_DATA)->cpu_pinned = 1
	get_bp_info(0, TYPE_INST)->cpu_pinned = -1


I put together some fix and put it in here:
  https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
  perf/bp

if you could please run your tests on it, and if it's all
good I'll post it

thanks,
jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html