From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751338AbdKLTJ1 (ORCPT ); Sun, 12 Nov 2017 14:09:27 -0500 Received: from mail-ot0-f195.google.com ([74.125.82.195]:53481 "EHLO mail-ot0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750802AbdKLTJZ (ORCPT ); Sun, 12 Nov 2017 14:09:25 -0500 X-Google-Smtp-Source: AGs4zMa6+FXFyoWDwtP/PkCKSibl3s4XThkXKC9F+0xyUDZZiUSRwHonLIMYl0r4rOouWcMKEjtAyXQjQ2ESQSrhO9s= MIME-Version: 1.0 In-Reply-To: References: <20171106092305.GA16382@krava> <20171108141522.GA6320@krava> <20171108151218.GA11018@krava> <20171108155741.GA12627@krava> <20171109074658.GC14419@krava> <20171109131233.GA2942@krava> From: Milind Chabbi Date: Sun, 12 Nov 2017 11:09:23 -0800 X-Google-Sender-Auth: z2NlCWm-f27NRO5slzFPSHZf8-s Message-ID: Subject: Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT To: Milind Chabbi Cc: Jiri Olsa , Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Alexander Shishkin , Namhyung Kim , linux-kernel@vger.kernel.org, Michael Kerrisk-manpages , linux-man@vger.kernel.org, Michael Ellerman , Andi Kleen , Kan Liang , Hari Bathini , Sukadev Bhattiprolu , Jin Yao 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 Thu, Nov 9, 2017 at 10:59 AM, Milind Chabbi wrote: > SNIP > > On Thu, Nov 9, 2017 at 5:12 AM, Jiri Olsa 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