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

[Alexey Budankov]

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"

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

> 
> 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
>>
>>
>