Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 27 May 2020 12:28:05 +0200
From:   Jiri Olsa <jolsa@redhat.com>
To:     "Jin, Yao" <yao.jin@linux.intel.com>
Cc:     acme@kernel.org, jolsa@kernel.org, peterz@infradead.org,
        mingo@redhat.com, alexander.shishkin@linux.intel.com,
        Linux-kernel@vger.kernel.org, ak@linux.intel.com,
        kan.liang@intel.com, yao.jin@intel.com
Subject: Re: [PATCH v2 1/2] perf evlist: Ensure grouped events with same cpu
 map
Message-ID: <20200527102805.GA420698@krava>
References: <20200525065559.6422-1-yao.jin@linux.intel.com>
 <20200526115155.GE333164@krava>
 <32c4663a-6934-2a2d-79e2-7a335e3629a2@linux.intel.com>
 <d6986a15-1e21-3414-9d68-c265e7db03f4@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d6986a15-1e21-3414-9d68-c265e7db03f4@linux.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, May 27, 2020 at 02:31:03PM +0800, Jin, Yao wrote:

SNIP

> > Thanks
> > Jin Yao
> 
> Issue is found!
> 
> It looks we can't set "pos->leader = pos" in either for_each_group_member()
> or in for_each_group_evsel() because it may exit the iteration immediately.
> 
> 	evlist__for_each_entry(evlist, evsel) {
> 		if (evsel->leader == evsel)
> 			continue;
> 
> 		if (cpu_maps_matched(evsel->leader, evsel))
> 			continue;
> 
> 		pr_warning("WARNING: event cpu maps are not fully matched, "
> 			   "disable group\n");
> 
> 		for_each_group_member(pos, evsel->leader) {
> 			pos->leader = pos;
> 			pos->core.nr_members = 0;
> 		}
> 
> Let me use the example of '{cycles,unc_cbo_cache_lookup.any_i}' again.
> 
> In evlist:
> cycles,
> unc_cbo_cache_lookup.any_i,
> unc_cbo_cache_lookup.any_i,
> unc_cbo_cache_lookup.any_i,
> unc_cbo_cache_lookup.any_i,
> 
> When we reach the for_each_group_member at first time, evsel is the first
> unc_cbo_cache_lookup.any_i and evsel->leader is cycles. pos is same as the
> evsel (the first unc_cbo_cache_lookup.any_i).
> 
> Once we execute "pos->leader = pos;", it's actually "evsel->leader = evsel".
> So now evsel->leader is changed to the first unc_cbo_cache_lookup.any_i.
> 
> In next iteration, pos is the second unc_cbo_cache_lookup.any_i. pos->leader
> is cycles but unfortunately evsel->leader has been changed to the first
> unc_cbo_cache_lookup.any_i. So iteration stops immediately.

hum, AFAICS the iteration will not break but continue to next evsel and
pass the 'continue' for another group member.. what do I miss?

jirka

> 
> I'm now thinking if we can solve this issue by an easy way.
> 
> Thanks
> Jin Yao
>