* 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).