On Wed, Nov 8, 2017 at 7:57 AM, Jiri Olsa <jolsa@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
