* Re: [PATCH 2/3] perf parse-regs: Add generic support for non-gprs [not found] ` <1557844753-58223-2-git-send-email-kan.liang@linux.intel.com> @ 2019-05-14 18:19 ` Arnaldo Carvalho de Melo 2019-05-14 19:25 ` Liang, Kan 0 siblings, 1 reply; 3+ messages in thread From: Arnaldo Carvalho de Melo @ 2019-05-14 18:19 UTC (permalink / raw) To: kan.liang Cc: Jiri Olsa, Ingo Molnar, linux-kernel, Andi Kleen, linux-perf-users, Namhyung Kim, Adrian Hunter Em Tue, May 14, 2019 at 07:39:12AM -0700, kan.liang@linux.intel.com escreveu: > From: Kan Liang <kan.liang@linux.intel.com> > > Some non general purpose registers, e.g. XMM, can be collected on some > platforms, e.g. X86 Icelake. > > Add a weak function has_non_gprs_support() to check if the > kernel/hardware support non-gprs collection. > Add a weak function non_gprs_mask() to return non-gprs mask. > > By default, the non-gprs collection is not support. Specific platforms > should implement their own non-gprs functions if they can collect > non-gprs. > > Signed-off-by: Kan Liang <kan.liang@linux.intel.com> > --- > tools/perf/util/parse-regs-options.c | 10 +++++++++- > tools/perf/util/perf_regs.c | 10 ++++++++++ > tools/perf/util/perf_regs.h | 2 ++ > 3 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/tools/perf/util/parse-regs-options.c b/tools/perf/util/parse-regs-options.c > index b21617f..2f90f31 100644 > --- a/tools/perf/util/parse-regs-options.c > +++ b/tools/perf/util/parse-regs-options.c > @@ -12,6 +12,7 @@ __parse_regs(const struct option *opt, const char *str, int unset, bool intr) > const struct sample_reg *r; > char *s, *os = NULL, *p; > int ret = -1; > + bool has_non_gprs = has_non_gprs_support(intr); This is generic code, what does "non gprs" means for !Intel? Can we come up with a better, not architecture specific jargon? Or you mean "general purpose registers"? Perhaps we can ask for a register mask for use with intr? I.e.: For the -I/--intr-regs uint64_t mask = arch__intr_reg_mask(); And for --user-regs uint64_t mask = arch__user_reg_mask(); And then on that loop do: for (r = sample_reg_masks; r->name; r++) { if (r->mask & mask)) fprintf(stderr, "%s ", r->name); } ? > if (unset) > return 0; > @@ -37,6 +38,8 @@ __parse_regs(const struct option *opt, const char *str, int unset, bool intr) > if (!strcmp(s, "?")) { > fprintf(stderr, "available registers: "); > for (r = sample_reg_masks; r->name; r++) { > + if (!has_non_gprs && (r->mask & ~PERF_REGS_MASK)) > + break; > fprintf(stderr, "%s ", r->name); > } > fputc('\n', stderr); > @@ -44,6 +47,8 @@ __parse_regs(const struct option *opt, const char *str, int unset, bool intr) > return -1; > } > for (r = sample_reg_masks; r->name; r++) { > + if (!has_non_gprs && (r->mask & ~PERF_REGS_MASK)) > + continue; > if (!strcasecmp(s, r->name)) > break; > } > @@ -64,8 +69,11 @@ __parse_regs(const struct option *opt, const char *str, int unset, bool intr) > ret = 0; > > /* default to all possible regs */ > - if (*mode == 0) > + if (*mode == 0) { > *mode = PERF_REGS_MASK; > + if (has_non_gprs) > + *mode |= non_gprs_mask(intr); > + } > error: > free(os); > return ret; > diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c > index 2acfcc5..0d278b6 100644 > --- a/tools/perf/util/perf_regs.c > +++ b/tools/perf/util/perf_regs.c > @@ -13,6 +13,16 @@ int __weak arch_sdt_arg_parse_op(char *old_op __maybe_unused, > return SDT_ARG_SKIP; > } > > +bool __weak has_non_gprs_support(bool intr __maybe_unused) > +{ > + return false; > +} > + > +u64 __weak non_gprs_mask(bool intr __maybe_unused) > +{ > + return 0; > +} > + > #ifdef HAVE_PERF_REGS_SUPPORT > int perf_reg_value(u64 *valp, struct regs_dump *regs, int id) > { > diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h > index 1a15a4b..983b4e6 100644 > --- a/tools/perf/util/perf_regs.h > +++ b/tools/perf/util/perf_regs.h > @@ -23,6 +23,8 @@ enum { > }; > > int arch_sdt_arg_parse_op(char *old_op, char **new_op); > +bool has_non_gprs_support(bool intr); > +u64 non_gprs_mask(bool intr); > > #ifdef HAVE_PERF_REGS_SUPPORT > #include <perf_regs.h> > -- > 2.7.4 -- - Arnaldo ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 2/3] perf parse-regs: Add generic support for non-gprs 2019-05-14 18:19 ` [PATCH 2/3] perf parse-regs: Add generic support for non-gprs Arnaldo Carvalho de Melo @ 2019-05-14 19:25 ` Liang, Kan 2019-05-14 19:34 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 3+ messages in thread From: Liang, Kan @ 2019-05-14 19:25 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Jiri Olsa, Ingo Molnar, linux-kernel, Andi Kleen, linux-perf-users, Namhyung Kim, Adrian Hunter On 5/14/2019 2:19 PM, Arnaldo Carvalho de Melo wrote: > Em Tue, May 14, 2019 at 07:39:12AM -0700, kan.liang@linux.intel.com escreveu: >> From: Kan Liang <kan.liang@linux.intel.com> >> >> Some non general purpose registers, e.g. XMM, can be collected on some >> platforms, e.g. X86 Icelake. >> >> Add a weak function has_non_gprs_support() to check if the >> kernel/hardware support non-gprs collection. >> Add a weak function non_gprs_mask() to return non-gprs mask. >> >> By default, the non-gprs collection is not support. Specific platforms >> should implement their own non-gprs functions if they can collect >> non-gprs. >> >> Signed-off-by: Kan Liang <kan.liang@linux.intel.com> >> --- >> tools/perf/util/parse-regs-options.c | 10 +++++++++- >> tools/perf/util/perf_regs.c | 10 ++++++++++ >> tools/perf/util/perf_regs.h | 2 ++ >> 3 files changed, 21 insertions(+), 1 deletion(-) >> >> diff --git a/tools/perf/util/parse-regs-options.c b/tools/perf/util/parse-regs-options.c >> index b21617f..2f90f31 100644 >> --- a/tools/perf/util/parse-regs-options.c >> +++ b/tools/perf/util/parse-regs-options.c >> @@ -12,6 +12,7 @@ __parse_regs(const struct option *opt, const char *str, int unset, bool intr) >> const struct sample_reg *r; >> char *s, *os = NULL, *p; >> int ret = -1; >> + bool has_non_gprs = has_non_gprs_support(intr); > > This is generic code, what does "non gprs" means for !Intel? Can we come > up with a better, not architecture specific jargon? Or you mean "general > purpose registers"? I mean "general purpose registers". > > Perhaps we can ask for a register mask for use with intr? I.e.: > > For the -I/--intr-regs > > uint64_t mask = arch__intr_reg_mask(); > > And for --user-regs > > uint64_t mask = arch__user_reg_mask(); > > And then on that loop do: > > for (r = sample_reg_masks; r->name; r++) { > if (r->mask & mask)) > fprintf(stderr, "%s ", r->name); > } > > ? Yes, it looks like a little bit simpler than my implementation. I will send out V2. Thanks, Kan > >> if (unset) >> return 0; >> @@ -37,6 +38,8 @@ __parse_regs(const struct option *opt, const char *str, int unset, bool intr) >> if (!strcmp(s, "?")) { >> fprintf(stderr, "available registers: "); >> for (r = sample_reg_masks; r->name; r++) { >> + if (!has_non_gprs && (r->mask & ~PERF_REGS_MASK)) >> + break; >> fprintf(stderr, "%s ", r->name); >> } >> fputc('\n', stderr); >> @@ -44,6 +47,8 @@ __parse_regs(const struct option *opt, const char *str, int unset, bool intr) >> return -1; >> } >> for (r = sample_reg_masks; r->name; r++) { >> + if (!has_non_gprs && (r->mask & ~PERF_REGS_MASK)) >> + continue; >> if (!strcasecmp(s, r->name)) >> break; >> } >> @@ -64,8 +69,11 @@ __parse_regs(const struct option *opt, const char *str, int unset, bool intr) >> ret = 0; >> >> /* default to all possible regs */ >> - if (*mode == 0) >> + if (*mode == 0) { >> *mode = PERF_REGS_MASK; >> + if (has_non_gprs) >> + *mode |= non_gprs_mask(intr); >> + } >> error: >> free(os); >> return ret; >> diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c >> index 2acfcc5..0d278b6 100644 >> --- a/tools/perf/util/perf_regs.c >> +++ b/tools/perf/util/perf_regs.c >> @@ -13,6 +13,16 @@ int __weak arch_sdt_arg_parse_op(char *old_op __maybe_unused, >> return SDT_ARG_SKIP; >> } >> >> +bool __weak has_non_gprs_support(bool intr __maybe_unused) >> +{ >> + return false; >> +} >> + >> +u64 __weak non_gprs_mask(bool intr __maybe_unused) >> +{ >> + return 0; >> +} >> + >> #ifdef HAVE_PERF_REGS_SUPPORT >> int perf_reg_value(u64 *valp, struct regs_dump *regs, int id) >> { >> diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h >> index 1a15a4b..983b4e6 100644 >> --- a/tools/perf/util/perf_regs.h >> +++ b/tools/perf/util/perf_regs.h >> @@ -23,6 +23,8 @@ enum { >> }; >> >> int arch_sdt_arg_parse_op(char *old_op, char **new_op); >> +bool has_non_gprs_support(bool intr); >> +u64 non_gprs_mask(bool intr); >> >> #ifdef HAVE_PERF_REGS_SUPPORT >> #include <perf_regs.h> >> -- >> 2.7.4 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 2/3] perf parse-regs: Add generic support for non-gprs 2019-05-14 19:25 ` Liang, Kan @ 2019-05-14 19:34 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 3+ messages in thread From: Arnaldo Carvalho de Melo @ 2019-05-14 19:34 UTC (permalink / raw) To: Liang, Kan Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Ingo Molnar, linux-kernel, Andi Kleen, linux-perf-users, Namhyung Kim, Adrian Hunter Em Tue, May 14, 2019 at 03:25:51PM -0400, Liang, Kan escreveu: > > > On 5/14/2019 2:19 PM, Arnaldo Carvalho de Melo wrote: > > Em Tue, May 14, 2019 at 07:39:12AM -0700, kan.liang@linux.intel.com escreveu: > > > From: Kan Liang <kan.liang@linux.intel.com> > > > > > > Some non general purpose registers, e.g. XMM, can be collected on some > > > platforms, e.g. X86 Icelake. > > > > > > Add a weak function has_non_gprs_support() to check if the > > > kernel/hardware support non-gprs collection. > > > Add a weak function non_gprs_mask() to return non-gprs mask. > > > > > > By default, the non-gprs collection is not support. Specific platforms > > > should implement their own non-gprs functions if they can collect > > > non-gprs. > > > > > > Signed-off-by: Kan Liang <kan.liang@linux.intel.com> > > > --- > > > tools/perf/util/parse-regs-options.c | 10 +++++++++- > > > tools/perf/util/perf_regs.c | 10 ++++++++++ > > > tools/perf/util/perf_regs.h | 2 ++ > > > 3 files changed, 21 insertions(+), 1 deletion(-) > > > > > > diff --git a/tools/perf/util/parse-regs-options.c b/tools/perf/util/parse-regs-options.c > > > index b21617f..2f90f31 100644 > > > --- a/tools/perf/util/parse-regs-options.c > > > +++ b/tools/perf/util/parse-regs-options.c > > > @@ -12,6 +12,7 @@ __parse_regs(const struct option *opt, const char *str, int unset, bool intr) > > > const struct sample_reg *r; > > > char *s, *os = NULL, *p; > > > int ret = -1; > > > + bool has_non_gprs = has_non_gprs_support(intr); > > > > This is generic code, what does "non gprs" means for !Intel? Can we come > > up with a better, not architecture specific jargon? Or you mean "general > > purpose registers"? > > I mean "general purpose registers". Ok > > > > Perhaps we can ask for a register mask for use with intr? I.e.: > > > > For the -I/--intr-regs > > > > uint64_t mask = arch__intr_reg_mask(); > > > > And for --user-regs > > > > uint64_t mask = arch__user_reg_mask(); > > > > And then on that loop do: > > > > for (r = sample_reg_masks; r->name; r++) { > > if (r->mask & mask)) > > fprintf(stderr, "%s ", r->name); > > } > > > > ? > > Yes, it looks like a little bit simpler than my implementation. > I will send out V2. Thanks! > Thanks, > Kan > > > > if (unset) > > > return 0; > > > @@ -37,6 +38,8 @@ __parse_regs(const struct option *opt, const char *str, int unset, bool intr) > > > if (!strcmp(s, "?")) { > > > fprintf(stderr, "available registers: "); > > > for (r = sample_reg_masks; r->name; r++) { > > > + if (!has_non_gprs && (r->mask & ~PERF_REGS_MASK)) > > > + break; > > > fprintf(stderr, "%s ", r->name); > > > } > > > fputc('\n', stderr); > > > @@ -44,6 +47,8 @@ __parse_regs(const struct option *opt, const char *str, int unset, bool intr) > > > return -1; > > > } > > > for (r = sample_reg_masks; r->name; r++) { > > > + if (!has_non_gprs && (r->mask & ~PERF_REGS_MASK)) > > > + continue; > > > if (!strcasecmp(s, r->name)) > > > break; > > > } > > > @@ -64,8 +69,11 @@ __parse_regs(const struct option *opt, const char *str, int unset, bool intr) > > > ret = 0; > > > /* default to all possible regs */ > > > - if (*mode == 0) > > > + if (*mode == 0) { > > > *mode = PERF_REGS_MASK; > > > + if (has_non_gprs) > > > + *mode |= non_gprs_mask(intr); > > > + } > > > error: > > > free(os); > > > return ret; > > > diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c > > > index 2acfcc5..0d278b6 100644 > > > --- a/tools/perf/util/perf_regs.c > > > +++ b/tools/perf/util/perf_regs.c > > > @@ -13,6 +13,16 @@ int __weak arch_sdt_arg_parse_op(char *old_op __maybe_unused, > > > return SDT_ARG_SKIP; > > > } > > > +bool __weak has_non_gprs_support(bool intr __maybe_unused) > > > +{ > > > + return false; > > > +} > > > + > > > +u64 __weak non_gprs_mask(bool intr __maybe_unused) > > > +{ > > > + return 0; > > > +} > > > + > > > #ifdef HAVE_PERF_REGS_SUPPORT > > > int perf_reg_value(u64 *valp, struct regs_dump *regs, int id) > > > { > > > diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h > > > index 1a15a4b..983b4e6 100644 > > > --- a/tools/perf/util/perf_regs.h > > > +++ b/tools/perf/util/perf_regs.h > > > @@ -23,6 +23,8 @@ enum { > > > }; > > > int arch_sdt_arg_parse_op(char *old_op, char **new_op); > > > +bool has_non_gprs_support(bool intr); > > > +u64 non_gprs_mask(bool intr); > > > #ifdef HAVE_PERF_REGS_SUPPORT > > > #include <perf_regs.h> > > > -- > > > 2.7.4 > > -- - Arnaldo ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-05-14 19:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1557844753-58223-1-git-send-email-kan.liang@linux.intel.com>
[not found] ` <1557844753-58223-2-git-send-email-kan.liang@linux.intel.com>
2019-05-14 18:19 ` [PATCH 2/3] perf parse-regs: Add generic support for non-gprs Arnaldo Carvalho de Melo
2019-05-14 19:25 ` Liang, Kan
2019-05-14 19:34 ` Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).