Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1761822AbbEERWp (ORCPT ); Tue, 5 May 2015 13:22:45 -0400 Received: from mga01.intel.com ([192.55.52.88]:32004 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1761805AbbEERWn convert rfc822-to-8bit (ORCPT ); Tue, 5 May 2015 13:22:43 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,373,1427785200"; d="scan'208";a="566677172" From: "Liang, Kan" To: Peter Zijlstra CC: "mingo@kernel.org" , "acme@infradead.org" , "eranian@google.com" , "andi@firstfloor.org" , "linux-kernel@vger.kernel.org" Subject: RE: [PATCH V7 3/6] perf, x86: handle multiple records in PEBS buffer Thread-Topic: [PATCH V7 3/6] perf, x86: handle multiple records in PEBS buffer Thread-Index: AQHQe310Bv0vIV2rhEqFiZATlJXnO51s7leAgACfL3D//6GRAIAAifhw Date: Tue, 5 May 2015 17:22:38 +0000 Message-ID: <37D7C6CF3E00A74B8858931C1DB2F0770180F3F5@SHSMSX103.ccr.corp.intel.com> References: <1429517270-8079-1-git-send-email-kan.liang@intel.com> <1429517270-8079-4-git-send-email-kan.liang@intel.com> <20150505131637.GO23123@twins.programming.kicks-ass.net> <37D7C6CF3E00A74B8858931C1DB2F0770180BB27@SHSMSX103.ccr.corp.intel.com> <20150505170822.GQ23123@twins.programming.kicks-ass.net> In-Reply-To: <20150505170822.GQ23123@twins.programming.kicks-ass.net> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2773 Lines: 74 > On Tue, May 05, 2015 at 04:30:25PM +0000, Liang, Kan wrote: > > > > + for (at = base; at < top; at += x86_pmu.pebs_record_size) { > > > > struct pebs_record_nhm *p = at; > > > > > > > > for_each_set_bit(bit, (unsigned long *)&p->status, > > > > x86_pmu.max_pebs_events) { > > > > event = cpuc->events[bit]; > > > > WARN_ON_ONCE(!event); > > > > > > > > + if (event->attr.precise_ip) > > > > + break; > > > > + } > > > > > > Would it make sense to delay looking for the event until you've > > > found there is a single bit set -- and already know which bit that is? > > > > > > > Yes, I think we can test cpuc->pebs_enabled here. > > It should be better than attr.precise_ip checking. > > > > - for (; at < top; at += x86_pmu.pebs_record_size) { > > + for (at = base; at < top; at += x86_pmu.pebs_record_size) { > > struct pebs_record_nhm *p = at; > > > > for_each_set_bit(bit, (unsigned long *)&p->status, > > x86_pmu.max_pebs_events) { > > - event = cpuc->events[bit]; > > - if (!test_bit(bit, cpuc->active_mask)) > > - continue; > > - > > - WARN_ON_ONCE(!event); > > > > - if (!event->attr.precise_ip) > > - continue; > > + if (test_bit(bit, cpuc->pebs_enabled)) > > + break; > > + } > > > > Can't we take that entire for_each_set_bit() loop out? > > It appears to me we effectively do that single test_bit() test you left in > there already with the & cpuc->pebs_enabled later on. > > Right, will use find_first_bit to replace. + for (at = base; at < top; at += x86_pmu.pebs_record_size) { struct pebs_record_nhm *p = at; - for_each_set_bit(bit, (unsigned long *)&p->status, - x86_pmu.max_pebs_events) { - event = cpuc->events[bit]; - if (!test_bit(bit, cpuc->active_mask)) - continue; - - WARN_ON_ONCE(!event); - - if (!event->attr.precise_ip) - continue; - - if (__test_and_set_bit(bit, (unsigned long *)&status)) + bit = find_first_bit((unsigned long *)&p->status, + x86_pmu.max_pebs_events); Thanks, Kan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/