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

[Alexey Budankov]

On 07.05.2020 20:01, Arnaldo Carvalho de Melo wrote:
> 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

Well, ok. Accepted in v3.
Command becomes of variable length of chars + \n, 
in comparison to current single char + \n.

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