From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752001AbcBZKhM (ORCPT ); Fri, 26 Feb 2016 05:37:12 -0500 Received: from bombadil.infradead.org ([198.137.202.9]:57376 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753846AbcBZKhI (ORCPT ); Fri, 26 Feb 2016 05:37:08 -0500 Date: Fri, 26 Feb 2016 11:36:58 +0100 From: Peter Zijlstra To: kan.liang@intel.com Cc: linux-kernel@vger.kernel.org, ak@linux.intel.com, eranian@google.com, vincent.weaver@maine.edu, tglx@linutronix.de, mingo@kernel.org, acme@redhat.com, jolsa@redhat.com, ying.huang@linux.intel.com, alexander.shishkin@linux.intel.com Subject: Re: [PATCH 1/1] perf/core: find auxiliary events in running pmus list Message-ID: <20160226103658.GL6344@twins.programming.kicks-ass.net> References: <1456348836-6163-1-git-send-email-kan.liang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1456348836-6163-1-git-send-email-kan.liang@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 > index 9> +static void account_running_pmu(struct perf_event *event) > +{ > + struct running_pmu *pmu; > + > + mutex_lock(&running_pmus_lock); > + > + list_for_each_entry(pmu, &running_pmus, entry) { > + if (pmu->pmu == event->pmu) { > + pmu->nr_event++; > + goto out; > + } > + } > + > + pmu = kzalloc(sizeof(struct running_pmu), GFP_KERNEL); > + if (pmu != NULL) { > + pmu->nr_event++; > + pmu->pmu = event->pmu; > + list_add_rcu(&pmu->entry, &running_pmus); > + } That kzalloc() doesn't make any sense, why not simply add a member to struct pmu? > +out: > + mutex_unlock(&running_pmus_lock); > +} In any case, can't we replace the whole perf_event_aux muck with a data structure for finding interested events? Because not only is iterating all PMUs silly, we then further iterate all events in whatever contexts we find from them. Even if none of the events in these contexts is interested in the side-band event. We have: mmap,comm,task,mmap_data,mmap2,comm_exec,context_switch Which I think we can reduce like: {mmap,mmap_data,mmap2} -> mmap {comm,comm_exec} -> comm mmap,comm,task,context_switch This would allow us to keep 4 per-cpu lists of events like: struct pmu_event_list { raw_spinlock_t lock; struct list_head list; }; enum event_sb_channel { sb_mmap = 0, sb_comm, sb_task, sb_switch, sb_nr, } static DEFINE_PER_CPU(struct pmu_event_list, pmu_sb_events[sb_nr]); static void attach_sb_event(struct perf_event *event, enum event_sb_channel sb) { struct pmu_event_list *pel = per_cpu_ptr(&pmu_sb_events[sb], event->cpu); raw_spin_lock(&pel->lock); list_add_rcu(&event->sb_list[sb], &pel->list); raw_spin_unlock(&pel->lock); } static void account_pmu_sb_event(struct perf_event *event) { if (event->parent) return; if (event->attach & ATTACH_TASK) return; if (event->attr.mmap || event->attr.mmap_data || event->attr.mmap2) attach_sb_event(event, sb_mmap); if (event->attr.comm || event->attr.comm_exec) attach_sb_event(event, sb_comm); if (event->attr.task) attach_sb_event(event, sb_task); if (event->attr.context_switch) attach_sb_event(event, sb_switch); } /* matching unaccount */ static void perf_event_sb_iterate(enum event_sb_channel sb, perf_event_sb_output_f output, void *data) { struct pmu_event_list *pel = __this_cpu_ptr(&pmu_sb_events[sb]); struct perf_event *event; list_for_each_entry_rcu(event, &pel->list, sb_list[sb]) { if (event->state < PERF_EVENT_STATE_INACTIVE) continue; if (!event_filter_match(event)) continue; output(event, data); } } static void perf_event_sb_mask(unsigned int sb_mask, perf_event_sb_output_f output, void *data) { int sb; for (sb = 0; sb < sb_nr; sb++) { if (!(sb_mask & (1 << sb))) continue; perf_event_sb_iterate(sb, output, data); } } Note: the mask is needed because a task event (as per perf_event_task) needs to go out to sb_comm, sb_mmap and sb_task, see perf_event_task_match(). And then you can replace the whole global part of perf_event_aux (which I would rename to perf_event_sb), with this. You still have to do something like: for_each_task_context_nr(ctxn) { ctx = rcu_dereference(current->perf_event_ctxp[ctxn]); if (!ctx) continue; perf_event_sb_ctx(ctx, output, data); } To get at the per task events, because I don't think we want to go update more global state on context switch, nor am I entirely sure its worth it to keep per sb ctx->event_list[]s.