Re: [PATCH 1/2] perf vendor events: Add stepping in CPUID string for x86

[Liang, Kan]

On 11/15/2018 9:26 AM, Liang, Kan wrote:
> On 11/15/2018 8:53 AM, Jiri Olsa wrote:
> > On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.liang@xxxxxxxxxxxxxxx 
> > wrote:
> >
> > SNIP
> >
> > > diff --git a/tools/perf/arch/x86/util/header.c 
> > > b/tools/perf/arch/x86/util/header.c
> > > index fb0d71afee8b..b428a4b00bf7 100644
> > > --- a/tools/perf/arch/x86/util/header.c
> > > +++ b/tools/perf/arch/x86/util/header.c
> > > @@ -4,6 +4,7 @@
> > > Â #include <stdio.h>
> > > Â #include <stdlib.h>
> > > Â #include <string.h>
> > > +#include <regex.h>
> > > Â #include "../../util/header.h"
> > > @@ -70,9 +71,73 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
> > > Â {
> > > ÂÂÂÂÂ char *buf = malloc(128);
> > > -ÂÂÂ if (buf && __get_cpuid(buf, 128, "%s-%u-%X$") < 0) {
> > > +ÂÂÂ if (buf && __get_cpuid(buf, 128, "%s-%u-%X-%X$") < 0) {
> > > ÂÂÂÂÂÂÂÂÂ free(buf);
> > > ÂÂÂÂÂÂÂÂÂ return NULL;
> > > ÂÂÂÂÂ }
> > > ÂÂÂÂÂ return buf;
> > > Â }
> > > +
> > > +/* Full CPUID format for x86 is vendor-family-model-stepping */
> > > +static bool is_full_cpuid(const char *cpuid)
> > > +{
> > > +ÂÂÂ const char *tmp = cpuid;
> > > +ÂÂÂ int count = 0;
> > > +
> > > +ÂÂÂ while ((tmp = strchr(tmp, '-')) != NULL) {
> > > +ÂÂÂÂÂÂÂ count++;
> > > +ÂÂÂÂÂÂÂ tmp++;
> > > +ÂÂÂ }
> > > +
> > > +ÂÂÂ if (count == 3)
> > > +ÂÂÂÂÂÂÂ return true;
> > > +
> > > +ÂÂÂ return false;
> > > +}
> > > +
> > > +int strcmp_cpuid_str(const char *mapcpuid, const char *cpuid)
> > > +{
> > > +ÂÂÂ regex_t re;
> > > +ÂÂÂ regmatch_t pmatch[1];
> > > +ÂÂÂ int match;
> > > +ÂÂÂ bool full_mapcpuid = is_full_cpuid(mapcpuid);
> > > +ÂÂÂ bool full_cpuid = is_full_cpuid(cpuid);
> >
> > cpuid will be always full from now right? why do we need to check it?
>
> User may set cpuid by environment string "PERF_CPUID", which may not be 
> full format.
>
> > also please move this to arch/x86/util/pmu.c
> > so it matches the weak function object
>
> Sure.

It looks like both pmu.c and header.c have cpuid related functions.
For example,
get_cpuid_str() has weak function in pmu.c. It's declaration in 
header.h. It's x86 specific function in x86/util/header.c
get_cpuid() has weak function in header.c. It's declaration in header.h. 
It's x86 specific function in x86/util/header.c

It looks like most of the cpuid related functions are in header.c/h.
I think it may be better to move all the cpuid related functions to 
header.c/h.

If it's OK for you, I will send a clean up patch later to move the weak 
functions strcmp_cpuid_str() and get_cpuid_str() to header.c

What do you think?

Thanks,
Kan