From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933008AbbIDNpW (ORCPT ); Fri, 4 Sep 2015 09:45:22 -0400 Received: from mga11.intel.com ([192.55.52.93]:55754 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932771AbbIDNpS (ORCPT ); Fri, 4 Sep 2015 09:45:18 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,469,1437462000"; d="scan'208";a="782659126" Subject: Re: [PATCH] perf tools: Fix gaps propagating maps To: Arnaldo Carvalho de Melo References: <55E943B7.6000408@intel.com> <1441368954-25066-1-git-send-email-adrian.hunter@intel.com> <20150904132840.GC2570@redhat.com> Cc: Jiri Olsa , jolsa@kernel.org, mingo@kernel.org, kan.liang@intel.com, linux-kernel@vger.kernel.org From: Adrian Hunter Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki, Business Identity Code: 0357606 - 4, Domiciled in Helsinki Message-ID: <55E99FC6.5060807@intel.com> Date: Fri, 4 Sep 2015 16:42:30 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <20150904132840.GC2570@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/09/15 16:28, Arnaldo Carvalho de Melo wrote: > Em Fri, Sep 04, 2015 at 03:15:54PM +0300, Adrian Hunter escreveu: >> A perf_evsel is a selected event containing the perf_event_attr >> that is passed to perf_event_open(). A perf_evlist is a collection >> of perf_evsel's. A perf_evlist also has lists of cpus and threads >> (pids) on which to open the event. These lists are called 'maps' >> and this patch is about how those 'maps' are propagated from the >>> perf_evlist to the perf_evsels. > > Can't this be broken up in multiple patches, for instance this: Ok, might not be until next week though. > > int perf_evlist__create_maps(struct perf_evlist *evlist, struct > target *target) > { > + if (evlist->threads || evlist->cpus) > + return -1; > + Or you could just drop that chunk. > > Looks like a fix that could be separated. Also FOO__propagate(.., false) > to do the opposite of propagate seems confusing, how about > FOO__unpropagate() if that verb exists? :-) Ok > > > Also, when unpropagating, you do: > > if (evsel->cpus == evlist->cpus) { > cpu_map__put(evsel->cpus); > evsel->cpus = NULL; > } > > What if the PMU code _set_ it to the same cpus as in evlist->cpus, but > now we're unpropagating to set to another CPU, in this case we will be > changing the PMU setting with a new one. I.e. when a PMU sets it it > should be sticky, no? We are comparing the pointer, so that won't happen unless the PMU actually does evsel->cpus = evlist->cpus which seems unlikely. > > I.e. we would have to know, in the evsel, if evsel->cpus was set by the > PMU or any other future entity expecting this behaviour, so that we > don't touch it, i.e. testing (evsel->cpus != evlist->cpus) when > unpropagating doesn't seem to cut, right? I think the pointer comparison covers that. i.e. the pointers won't be the same even if the cpus are.