lkml.org 
[lkml]   [2017]   [Nov]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT
    On Wed, Nov 8, 2017 at 7:57 AM, Jiri Olsa <jolsa@redhat.com> 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@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?
    >
    > 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
    \
     
     \ /
      Last update: 2017-11-17 17:14    [W:2.450 / U:0.052 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site