Re: [PATCH v2 02/11] perf evlist: implement control command handling functions

[Arnaldo Carvalho de Melo]

Em Thu, May 07, 2020 at 11:32:53AM +0300, Alexey Budankov escreveu:
> 
> On 06.05.2020 23:21, Arnaldo Carvalho de Melo wrote:
> > Em Wed, May 06, 2020 at 09:19:22PM +0300, Alexey Budankov escreveu:
> >>
> >> Implement functions of initialization, finalization and processing
> >> of control commands coming from control file descriptors.
> >>
> >> Signed-off-by: Alexey Budankov <alexey.budankov@xxxxxxxxxxxxxxx>
> >> ---
> >>  tools/perf/util/evlist.c | 100 +++++++++++++++++++++++++++++++++++++++
> >>  tools/perf/util/evlist.h |  12 +++++
> >>  2 files changed, 112 insertions(+)
> 
> <SNIP>
> 
> >> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> >> index 62f259d89b41..84386850c290 100644
> >> --- a/tools/perf/util/evlist.h
> >> +++ b/tools/perf/util/evlist.h
> >> @@ -358,4 +358,16 @@ void perf_evlist__force_leader(struct evlist *evlist);
> >>  struct evsel *perf_evlist__reset_weak_group(struct evlist *evlist,
> >>  						 struct evsel *evsel,
> >>  						bool close);
> >> +
> >> +enum evlist_ctl_cmd {
> >> +	CTL_CMD_UNSUPPORTED = 0,
> >> +	CTL_CMD_ENABLE = 'e',
> >> +	CTL_CMD_DISABLE = 'd',
> >> +	CTL_CMD_ACK = 'a'
> >> +};
> > 
> > Can we make this a string, I think we'll eventually ask for lots more
> 
> Like this?
> 
> #define EVLIST__CTL_CMD_ENABLE	"enable"
> #define EVLIST__CTL_CMD_DISABLE	"disable"
> #define EVLIST__CTL_CMD_ACK	"ack"

Yeah
 
> > stuff, like asking for a --switch-output snapshot with --overwrite,
> > reconfiguring events to increase/decrease frequency, etc, interfacing
> > with PERF_EVENT_IOC_MODIFY_ATTRIBUTES, PERF_EVENT_IOC_SET_FILTER, etc.
> > 
> > This will also allow us to have parameters, etc, wdyt?
> 
> Being a part of this patch the extension will implement configurability
> that potentially could never be used.
> 
> Switch from int to string commands of variable length belongs to
> the patches also implementing usage of that string commands.

Well, at that point we would have to support both, i.e. the way you're
doing now with integers, and as strings, otherwise 3rd party tooling
(vtune? :)) using this interface would break.

I.e. this is like the syscall interface.

So if we have "enable" now we can go ahead and forever understand that
as "please enable this evlist", but in the future we can extend it and
pass parameters to it, to control how that enablement will take place,
perhaps with a delay, etc.

> > Also please since these are events that deal with 'struct evlist', name
> > them with the evlist__ prefix, not the perf_evlist__ one, as those
> > should be used with 'struct perf_evlist', i.e. the one in libperf
> > (tools/lib/perf/).
> 
> Accepted in v3.
> 
> > 
> > Right now this is inconsistent, we did it that way to minimize
> > disruption of the codebase when moving things from tools/perf/ to
> > tools/lib/perf/, but this confuses things and I just did a
> > s/perf_evsel__/evsel__) for things dealing with 'struct evsel', so lets
> > not add new ones with the wrong prefix, eventually we'll have perf_ only
> > for things in libperf.
> > 
> >> +
> >> +int perf_evlist__initialize_ctlfd(struct evlist *evlist, int ctl_fd, int ctl_fd_ack);
> >> +int perf_evlist__finalize_ctlfd(struct evlist *evlist);
> >> +int perf_evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd);
> >> +
> >>  #endif /* __PERF_EVLIST_H */
> >> -- 
> >> 2.24.1
> >>
> >>
> > 

-- 

- Arnaldo