From: Mathieu Poirier <mathieu.poirier@linaro.org>
To: Leo Yan <leo.yan@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>,
Ian Rogers <irogers@google.com>, Andi Kleen <ak@linux.intel.com>,
Suzuki K Poulose <suzuki.poulose@arm.com>,
Peter Zijlstra <peterz@infradead.org>,
Adrian Hunter <adrian.hunter@intel.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Arnaldo Carvalho de Melo <acme@redhat.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Ingo Molnar <mingo@redhat.com>, Jiri Olsa <jolsa@kernel.org>,
Namhyung Kim <namhyung@kernel.org>,
Thomas Gleixner <tglx@linutronix.de>,
linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
Mike Leach <mike.leach@linaro.org>
Subject: Re: [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term
Date: Thu, 9 Jan 2020 09:34:24 -0700 [thread overview]
Message-ID: <20200109163424.GA5721@xps15> (raw)
In-Reply-To: <20200109050753.GA24741@leoy-ThinkPad-X240s>
On Thu, Jan 09, 2020 at 01:08:06PM +0800, Leo Yan wrote:
> Hi Mathieu,
>
> On Wed, Jan 08, 2020 at 10:58:31AM -0700, Mathieu Poirier wrote:
>
> [...]
>
> > > diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
> > > index 1f8d2fe0b66e..4e5b3ebf09cf 100644
> > > --- a/tools/perf/util/evsel_config.h
> > > +++ b/tools/perf/util/evsel_config.h
> > > @@ -33,21 +33,8 @@ struct perf_evsel_config_term {
> > > struct list_head list;
> > > enum evsel_term_type type;
> > > union {
> > > - u64 period;
> > > - u64 freq;
> > > - bool time;
> > > - char *callgraph;
> > > - char *drv_cfg;
> > > - u64 stack_user;
> > > - int max_stack;
> > > - bool inherit;
> > > - bool overwrite;
> > > - char *branch;
> > > - unsigned long max_events;
> > > - bool percore;
> > > - bool aux_output;
> > > - u32 aux_sample_size;
> > > - u64 cfg_chg;
> > > + u64 num;
> > > + char *str;
> >
> > That is a lot more than just dealing with the "char *" members. Given
> > the pervasiveness of the changes I would have been happy to leave
> > other members alone for the time being.
>
> I think actually you are suggesting like below which add general
> members and also keep the old members. If so, I prefer to add two
> general members 'num' and 'str'.
If we are to deal with all flields of the union, I think it should be as below:
union {
bool cfg_bool;
int cfg_int;
unsigned long cfg_ulong;
u32 cfg_u32;
char *cfg_str;
} val;
But just dealing with the "char *" as below would also be fine with me:
union {
u64 period;
u64 freq;
bool time;
u64 stack_user;
int max_stack;
bool inherit;
bool overwrite;
unsigned long max_events;
bool percore;
bool aux_output;
u32 aux_sample_size;
u64 cfg_chg;
u64 num;
char *str;
} val;
>
> struct perf_evsel_config_term {
> struct list_head list;
> enum evsel_term_type type;
> union {
> u64 period;
> u64 freq;
> bool time;
> char *callgraph;
> char *drv_cfg;
> u64 stack_user;
> int max_stack;
> bool inherit;
> bool overwrite;
> char *branch;
> unsigned long max_events;
> bool percore;
> bool aux_output;
> u32 aux_sample_size;
> u64 cfg_chg;
> + u64 num;
> + char *str;
> } val;
> bool weak;
> };
>
> > I will let Jiri make the
> > final call but if we are to proceed this way I think we should have a
> > member per type to avoid casting issues.
>
> Yeah, let's see what's Jiri thinking.
>
> Just note, with this change, I don't see any casting warning or errors
> when built perf on arm64/arm32.
At this time you may not, but they will happen and it will be very hard to
debug.
>
> Thanks,
> Leo Yan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-01-09 16:34 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-08 14:20 [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term Leo Yan
2020-01-08 14:20 ` [PATCH v4 2/2] perf parse: Copy string to perf_evsel_config_term Leo Yan
2020-01-08 17:58 ` [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term Mathieu Poirier
2020-01-09 5:08 ` Leo Yan
2020-01-09 16:34 ` Mathieu Poirier [this message]
2020-01-10 15:04 ` Jiri Olsa
2020-01-13 15:21 ` Leo Yan
2020-01-10 15:04 ` Jiri Olsa
2020-01-13 15:22 ` Leo Yan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200109163424.GA5721@xps15 \
--to=mathieu.poirier@linaro.org \
--cc=acme@redhat.com \
--cc=adrian.hunter@intel.com \
--cc=ak@linux.intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=irogers@google.com \
--cc=jolsa@kernel.org \
--cc=leo.yan@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mike.leach@linaro.org \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=suzuki.poulose@arm.com \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).