Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933745AbbEERIf (ORCPT ); Tue, 5 May 2015 13:08:35 -0400 Received: from casper.infradead.org ([85.118.1.10]:42085 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932582AbbEERId (ORCPT ); Tue, 5 May 2015 13:08:33 -0400 Date: Tue, 5 May 2015 19:08:22 +0200 From: Peter Zijlstra To: "Liang, Kan" 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 Message-ID: <20150505170822.GQ23123@twins.programming.kicks-ass.net> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <37D7C6CF3E00A74B8858931C1DB2F0770180BB27@SHSMSX103.ccr.corp.intel.com> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2389 Lines: 66 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. > > + for (bit = 0; bit < x86_pmu.max_pebs_events; bit++) { > + if (counts[bit] == 0) > continue; > - > - __intel_pmu_pebs_event(event, iregs, at); > + event = cpuc->events[bit]; > + WARN_ON_ONCE(!event); > + WARN_ON_ONCE(!event->attr.precise_ip); > + __intel_pmu_pebs_event(event, iregs, base, > + top, bit, counts[bit]); > } Right bit of paranoia :-) -- 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/