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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <cbc522c3-d63d-e023-8bba-ce82c03ea16b@linux.intel.com>
Date:   Thu, 7 May 2020 20:51:10 +0300
From:   Alexey Budankov <alexey.budankov@...ux.intel.com>
To:     Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Cc:     Jiri Olsa <jolsa@...hat.com>, Namhyung Kim <namhyung@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Andi Kleen <ak@...ux.intel.com>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 02/11] perf evlist: implement control command handling
 functions


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@...ux.intel.com>
>>>> ---
>>>>  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
>>>>
>>>>
>>>
>