Re: [PATCH 2/3] perf tools: Allow to enable/disable events via control file

[Arnaldo Carvalho de Melo]

Em Thu, Dec 10, 2020 at 09:20:42PM +0300, Alexei Budankov escreveu:
> 
> On 10.12.2020 21:06, Jiri Olsa wrote:
> > On Thu, Dec 10, 2020 at 05:24:30PM +0100, Jiri Olsa wrote:
> >> On Mon, Dec 07, 2020 at 08:02:20PM +0300, Alexei Budankov wrote:
> >>> Hi,
> >>>
> >>> On 06.12.2020 20:05, Jiri Olsa wrote:
> >>>> Adding new control events to enable/disable specific event.
> >>>> The interface string for control file are:
> >>>>
> >>>>   'enable-<EVENT NAME>'
> >>>>   'disable-<EVENT NAME>'
> >>>
> >>> <SNIP>
> >>>
> >>>>
> >>>> when received the command, perf will scan the current evlist
> >>>> for <EVENT NAME> and if found it's enabled/disabled.
> >>>
> >>> <SNIP>
> >>>
> >>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> >>>> index 70aff26612a9..05723227bebf 100644
> >>>> --- a/tools/perf/util/evlist.c
> >>>> +++ b/tools/perf/util/evlist.c
> >>>> @@ -1915,7 +1915,13 @@ static int evlist__ctlfd_recv(struct evlist *evlist, enum evlist_ctl_cmd *cmd,
> >>>>  		 bytes_read == data_size ? "" : c == '\n' ? "\\n" : "\\0");
> >>>>  
> >>>>  	if (bytes_read > 0) {
> >>>> -		if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
> >>>> +		if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_EVSEL_TAG,
> >>>> +				    (sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG)-1))) {
> >>>> +			*cmd = EVLIST_CTL_CMD_ENABLE_EVSEL;
> >>>> +		} else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_EVSEL_TAG,
> >>>> +				    (sizeof(EVLIST_CTL_CMD_DISABLE_EVSEL_TAG)-1))) {
> >>>> +			*cmd = EVLIST_CTL_CMD_DISABLE_EVSEL;
> >>>> +		} else if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
> >>>>  			     (sizeof(EVLIST_CTL_CMD_ENABLE_TAG)-1))) {
> >>>>  			*cmd = EVLIST_CTL_CMD_ENABLE;
> >>>>  		} else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_TAG,
> >>>> @@ -1952,6 +1958,8 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
> >>>>  	char cmd_data[EVLIST_CTL_CMD_MAX_LEN];
> >>>>  	int ctlfd_pos = evlist->ctl_fd.pos;
> >>>>  	struct pollfd *entries = evlist->core.pollfd.entries;
> >>>> +	struct evsel *evsel;
> >>>> +	char *evsel_name;
> >>>>  
> >>>>  	if (!evlist__ctlfd_initialized(evlist) || !entries[ctlfd_pos].revents)
> >>>>  		return 0;
> >>>> @@ -1967,6 +1975,26 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
> >>>>  			case EVLIST_CTL_CMD_DISABLE:
> >>>>  				evlist__disable(evlist);
> >>>>  				break;
> >>>> +			case EVLIST_CTL_CMD_ENABLE_EVSEL:
> >>>> +				evsel_name = cmd_data + sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG) - 1;
> >>>
> >>> It makes sense to check that evsel_name still points
> >>> into cmd_data buffer after assigning to event name.
> >>
> >> right, will add that
> > 
> > actualy it's already checked in evlist__ctlfd_recv, evsel_name at
> > worst will be empty string so evlist__find_evsel_by_str will fail
> > 
> > I'll add '' around %s in the error output string:
> > 
> >   failed: can't find '%s' event
> > 
> > so it's obvious when it's empty
> 
> Looks good to me. Thanks!

I'm taking this as an Acked-by: you as per
Documentation/process/submitting-patches.rst:

<quoted>
Acked-by: is not as formal as Signed-off-by:.  It is a record that the acker
has at least reviewed the patch and has indicated acceptance.  Hence patch
mergers will sometimes manually convert an acker's "yep, looks good to me"
into an Acked-by: (but note that it is usually better to ask for an
explicit ack).
</>

Thanks,

- Arnaldo