Re: [PATCH V2 3/3] perf tools: fix perf stat with large socket IDs

[Jiri Olsa <jolsa@redhat.com>]

On Thu, Nov 12, 2020 at 09:42:42PM +0200, James Clark wrote:
> 
> 
> On 08/11/2020 14:44, Jiri Olsa wrote:
> > On Wed, Oct 28, 2020 at 09:43:11AM +0000, James Clark wrote:
> > 
> > SNIP
> > 
> >> -		 * Encode socket in bit range 31:24
> >> -		 * encode die id in bit range 23:16
> >>  		 * core_id is relative to socket and die,
> >>  		 * we need a global id. So we combine
> >>  		 * socket + die id + core id
> >>  		 */
> >> -		if (WARN_ONCE(env->cpu[cpu].socket_id >> 8, "The socket id number is too big.\n"))
> >> -			return cpu_map__empty_aggr_cpu_id();
> >> -
> >> -		if (WARN_ONCE(env->cpu[cpu].die_id >> 8, "The die id number is too big.\n"))
> >> -			return cpu_map__empty_aggr_cpu_id();
> >> -
> >> -		if (WARN_ONCE(env->cpu[cpu].core_id >> 16, "The core id number is too big.\n"))
> >> -			return cpu_map__empty_aggr_cpu_id();
> >> -
> >> -		core.id = (env->cpu[cpu].socket_id << 24) |
> >> -		       (env->cpu[cpu].die_id << 16) |
> >> -		       (env->cpu[cpu].core_id & 0xffff);
> >> +		core.socket_id = env->cpu[cpu].socket_id;
> >> +		core.die_id = env->cpu[cpu].die_id;
> >> +		core.core_id = env->cpu[cpu].core_id;
> > 
> > all 3 functions above look like they could be united just
> > to single one, because they are no longer steping on ech
> > other data
> > 
> > perhaps best as separate patch, so the change is clean
> > 
> > there are also the 'system retrieval counter' functions,
> > where it's possible better to keep it separated from
> > performance reasons, but here it's just value assigment
> 
> Hi Jiri,
> 
> I have a new patchset ready without this change, and I started working on this, but I came
> across some issues:
> 
> For example if all of the members are filled in regardless of the aggregation type,
> then the comparison function that actually decides whether cores should be aggregated or
> not will need to be modified to accept the current aggregation mode.

ah, because we compare all the values regardless and normaly there'd be -1

> 
> This would be possible, but would have a knock on effect when the values are read
> from the system if the code is to be shared.
> 
> Also there is a second issue where some of the current functions have different failure
> conditions. For example if socket can't be read, then that mode will fail, but other
> aggregation modes will still work. If we make the change to unify all the functions then
> that could get a bit more complicated.
> 
> I just wanted to get your opinion whether to submit the broken up set without this extra
> change or carry on trying to work out a solution?

ok forget it then, seems like possible follow up changes if it's even worthy

thanks for checking on that

jirka