[v5,13/13] perf record: introduce --ctl-fd[-ack] options
diff mbox series

Message ID 8ffc9f9f-af58-deea-428b-f8a69004e3cb@linux.intel.com
State New
Headers show
Series
  • perf: support enable and disable commands in stat and record modes
Related show

Commit Message

Alexey Budankov June 1, 2020, 4:05 p.m. UTC
Introduce --ctl-fd[-ack] options to pass open file descriptors numbers
from command line. Extend perf-record.txt file with --ctl-fd[-ack]
options description. Document possible usage model introduced by
--ctl-fd[-ack] options by providing example bash shell script.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 tools/perf/builtin-record.c | 6 ++++++
 tools/perf/util/record.h    | 2 ++
 2 files changed, 8 insertions(+)

Comments

Adrian Hunter June 1, 2020, 4:30 p.m. UTC | #1
On 1/06/20 7:05 pm, Alexey Budankov wrote:
> 
> Introduce --ctl-fd[-ack] options to pass open file descriptors numbers
> from command line. Extend perf-record.txt file with --ctl-fd[-ack]
> options description. Document possible usage model introduced by
> --ctl-fd[-ack] options by providing example bash shell script.

Hi

I am interested in using this also for taking snapshots.

Did you consider using a single option, or allowing either a file descriptor
or a pathname, or including also the event default of "disabled".

e.g. add "--control" and support all of:

--control
--control 11
--control 11,15
--control 11,15,disabled
--control 11,,disabled
--control /tmp/my-perf.fifo
--control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo
--control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo,disabled
--control /tmp/my-perf.fifo,,disabled

Regards
Adrian
Alexey Budankov June 1, 2020, 5:11 p.m. UTC | #2
Hi Adrian,

On 01.06.2020 19:30, Adrian Hunter wrote:
> On 1/06/20 7:05 pm, Alexey Budankov wrote:
>>
>> Introduce --ctl-fd[-ack] options to pass open file descriptors numbers
>> from command line. Extend perf-record.txt file with --ctl-fd[-ack]
>> options description. Document possible usage model introduced by
>> --ctl-fd[-ack] options by providing example bash shell script.
> 
> Hi
> 
> I am interested in using this also for taking snapshots.

Good to hear from you.

> 
> Did you consider using a single option, or allowing either a file descriptor

Single option use case is already possible like --ctl-fd <NUM_1>.
Synchronization messages can be provided via --ctl-fd-ack <NUM_2>.

> or a pathname, or including also the event default of "disabled".

For my cases conversion of pathnames into open fds belongs to external
controlling process e.g. like in the examples provided in the patch set.
Not sure about "event default of 'disabled'"

> 
> e.g. add "--control" and support all of:
> 
> --control
> --control 11
> --control 11,15
> --control 11,15,disabled
> --control 11,,disabled
> --control /tmp/my-perf.fifo
> --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo
> --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo,disabled
> --control /tmp/my-perf.fifo,,disabled
> 
> Regards
> Adrian
> 

Regards,
Alexey
Andi Kleen June 1, 2020, 11:37 p.m. UTC | #3
> > or a pathname, or including also the event default of "disabled".
> 
> For my cases conversion of pathnames into open fds belongs to external
> controlling process e.g. like in the examples provided in the patch set.
> Not sure about "event default of 'disabled'"

It would be nicer for manual use cases if perf supported the path names
directly like in Adrian's example, not needing a complex wrapper script.

-Andi
> 
> > 
> > e.g. add "--control" and support all of:
> > 
> > --control
> > --control 11
> > --control 11,15
> > --control 11,15,disabled
> > --control 11,,disabled
> > --control /tmp/my-perf.fifo
> > --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo
> > --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo,disabled
> > --control /tmp/my-perf.fifo,,disabled
> > 
> > Regards
> > Adrian
> > 
> 
> Regards,
> Alexey
>
Alexey Budankov June 2, 2020, 8:32 a.m. UTC | #4
On 02.06.2020 2:37, Andi Kleen wrote:
>>> or a pathname, or including also the event default of "disabled".
>>
>> For my cases conversion of pathnames into open fds belongs to external
>> controlling process e.g. like in the examples provided in the patch set.
>> Not sure about "event default of 'disabled'"
> 
> It would be nicer for manual use cases if perf supported the path names
> directly like in Adrian's example, not needing a complex wrapper script.

fds interface is required for VTune integration since VTune wants control
over files creation aside of Perf tool process. The script demonstrates
just one possible use case.

Control files could easily be implemented on top of fds making open operations
for paths and then initializing fds. Interface below is vague and with explicit
options like below it could be more explicit:
--ctl-file /tmp/my-perf.fifo --ctl-file-ack /tmp/my-perf-ack.fifo

Make either fds and or files provided on the command line. Implement file
options handling callbacks that would open paths and setting fds. Close fds
if they were opened by Perf tool process.

Adrian, please share your mind and use case.

~Alexey

> 
> -Andi
>>
>>>
>>> e.g. add "--control" and support all of:
>>>
>>> --control
>>> --control 11
>>> --control 11,15
>>> --control 11,15,disabled
>>> --control 11,,disabled
>>> --control /tmp/my-perf.fifo
>>> --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo
>>> --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo,disabled
>>> --control /tmp/my-perf.fifo,,disabled
>>>
>>> Regards
>>> Adrian
>>>
>>
>> Regards,
>> Alexey
>>
Alexey Budankov June 2, 2020, 9:12 a.m. UTC | #5
On 02.06.2020 11:32, Alexey Budankov wrote:
> 
> On 02.06.2020 2:37, Andi Kleen wrote:
>>>> or a pathname, or including also the event default of "disabled".
>>>
>>> For my cases conversion of pathnames into open fds belongs to external
>>> controlling process e.g. like in the examples provided in the patch set.
>>> Not sure about "event default of 'disabled'"
>>
>> It would be nicer for manual use cases if perf supported the path names
>> directly like in Adrian's example, not needing a complex wrapper script.
> 
> fds interface is required for VTune integration since VTune wants control
> over files creation aside of Perf tool process. The script demonstrates
> just one possible use case.
> 
> Control files could easily be implemented on top of fds making open operations
> for paths and then initializing fds. Interface below is vague and with explicit
> options like below it could be more explicit:
> --ctl-file /tmp/my-perf.fifo --ctl-file-ack /tmp/my-perf-ack.fifo

Or even clearer:

--ctl-fifo /tmp/my-perf --ctl-fifo-ack /tmp/my-perf-ack

> 
> Make either fds and or files provided on the command line. Implement file
> options handling callbacks that would open paths and setting fds. Close fds
> if they were opened by Perf tool process.
> 
> Adrian, please share your mind and use case.
> 
> ~Alexey
> 
>>
>> -Andi
>>>
>>>>
>>>> e.g. add "--control" and support all of:
>>>>
>>>> --control
>>>> --control 11
>>>> --control 11,15
>>>> --control 11,15,disabled
>>>> --control 11,,disabled
>>>> --control /tmp/my-perf.fifo
>>>> --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo
>>>> --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo,disabled
>>>> --control /tmp/my-perf.fifo,,disabled
>>>>
>>>> Regards
>>>> Adrian
>>>>
>>>
>>> Regards,
>>> Alexey
>>>
Adrian Hunter June 2, 2020, 1:43 p.m. UTC | #6
On 2/06/20 12:12 pm, Alexey Budankov wrote:
> 
> On 02.06.2020 11:32, Alexey Budankov wrote:
>>
>> On 02.06.2020 2:37, Andi Kleen wrote:
>>>>> or a pathname, or including also the event default of "disabled".
>>>>
>>>> For my cases conversion of pathnames into open fds belongs to external
>>>> controlling process e.g. like in the examples provided in the patch set.
>>>> Not sure about "event default of 'disabled'"
>>>
>>> It would be nicer for manual use cases if perf supported the path names
>>> directly like in Adrian's example, not needing a complex wrapper script.
>>
>> fds interface is required for VTune integration since VTune wants control
>> over files creation aside of Perf tool process. The script demonstrates
>> just one possible use case.
>>
>> Control files could easily be implemented on top of fds making open operations
>> for paths and then initializing fds. Interface below is vague and with explicit
>> options like below it could be more explicit:
>> --ctl-file /tmp/my-perf.fifo --ctl-file-ack /tmp/my-perf-ack.fifo
> 
> Or even clearer:
> 
> --ctl-fifo /tmp/my-perf --ctl-fifo-ack /tmp/my-perf-ack

If people are OK with having so many options, then that is fine by me.

> 
>>
>> Make either fds and or files provided on the command line. Implement file
>> options handling callbacks that would open paths and setting fds. Close fds
>> if they were opened by Perf tool process.
>>
>> Adrian, please share your mind and use case.
>>
>> ~Alexey
>>
>>>
>>> -Andi
>>>>
>>>>>
>>>>> e.g. add "--control" and support all of:
>>>>>
>>>>> --control
>>>>> --control 11
>>>>> --control 11,15
>>>>> --control 11,15,disabled
>>>>> --control 11,,disabled
>>>>> --control /tmp/my-perf.fifo
>>>>> --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo
>>>>> --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo,disabled
>>>>> --control /tmp/my-perf.fifo,,disabled
>>>>>
>>>>> Regards
>>>>> Adrian
>>>>>
>>>>
>>>> Regards,
>>>> Alexey
>>>>
Jiri Olsa June 5, 2020, 10:51 a.m. UTC | #7
On Tue, Jun 02, 2020 at 04:43:58PM +0300, Adrian Hunter wrote:
> On 2/06/20 12:12 pm, Alexey Budankov wrote:
> > 
> > On 02.06.2020 11:32, Alexey Budankov wrote:
> >>
> >> On 02.06.2020 2:37, Andi Kleen wrote:
> >>>>> or a pathname, or including also the event default of "disabled".
> >>>>
> >>>> For my cases conversion of pathnames into open fds belongs to external
> >>>> controlling process e.g. like in the examples provided in the patch set.
> >>>> Not sure about "event default of 'disabled'"
> >>>
> >>> It would be nicer for manual use cases if perf supported the path names
> >>> directly like in Adrian's example, not needing a complex wrapper script.
> >>
> >> fds interface is required for VTune integration since VTune wants control
> >> over files creation aside of Perf tool process. The script demonstrates
> >> just one possible use case.
> >>
> >> Control files could easily be implemented on top of fds making open operations
> >> for paths and then initializing fds. Interface below is vague and with explicit
> >> options like below it could be more explicit:
> >> --ctl-file /tmp/my-perf.fifo --ctl-file-ack /tmp/my-perf-ack.fifo
> > 
> > Or even clearer:
> > 
> > --ctl-fifo /tmp/my-perf --ctl-fifo-ack /tmp/my-perf-ack
> 
> If people are OK with having so many options, then that is fine by me.

the single option Adrian suggested seems better to me:

 --control
 --control 11
 --control 11,15
 --control 11,15,disabled
 --control 11,,disabled
 --control /tmp/my-perf.fifo
 --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo
 --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo,disabled
 --control /tmp/my-perf.fifo,,disabled

we already support this kind of options arguments, like for --call-graph

jirka
Alexey Budankov June 5, 2020, 1:15 p.m. UTC | #8
On 05.06.2020 13:51, Jiri Olsa wrote:
> On Tue, Jun 02, 2020 at 04:43:58PM +0300, Adrian Hunter wrote:
>> On 2/06/20 12:12 pm, Alexey Budankov wrote:
>>>
>>> On 02.06.2020 11:32, Alexey Budankov wrote:
>>>>
>>>> On 02.06.2020 2:37, Andi Kleen wrote:
>>>>>>> or a pathname, or including also the event default of "disabled".
>>>>>>
>>>>>> For my cases conversion of pathnames into open fds belongs to external
>>>>>> controlling process e.g. like in the examples provided in the patch set.
>>>>>> Not sure about "event default of 'disabled'"
>>>>>
>>>>> It would be nicer for manual use cases if perf supported the path names
>>>>> directly like in Adrian's example, not needing a complex wrapper script.
>>>>
>>>> fds interface is required for VTune integration since VTune wants control
>>>> over files creation aside of Perf tool process. The script demonstrates
>>>> just one possible use case.
>>>>
>>>> Control files could easily be implemented on top of fds making open operations
>>>> for paths and then initializing fds. Interface below is vague and with explicit
>>>> options like below it could be more explicit:
>>>> --ctl-file /tmp/my-perf.fifo --ctl-file-ack /tmp/my-perf-ack.fifo
>>>
>>> Or even clearer:
>>>
>>> --ctl-fifo /tmp/my-perf --ctl-fifo-ack /tmp/my-perf-ack
>>
>> If people are OK with having so many options, then that is fine by me.
> 
> the single option Adrian suggested seems better to me:
> 
>  --control
>  --control 11
>  --control 11,15

What if a user specifies fifos named like this above, not fds?

>  --control 11,15,disabled
>  --control 11,,disabled
>  --control /tmp/my-perf.fifo
>  --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo

What if a user wants not fifos but other type of comm channels?

>  --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo,disabled
>  --control /tmp/my-perf.fifo,,disabled
> 
> we already support this kind of options arguments, like for --call-graph
> 
> jirka
> 

IMHO,
this interface, of course, looks more compact (in amount of options) however
the other side it is less user friendly. One simple option for one simple
purpose is more convenient as for users as for developers. Also complex
option syntax tends to have limitations and there are probably more
non-obvious ones.

Please speak up. I might have missed something meaningful.

~Alexey
Jiri Olsa June 5, 2020, 1:57 p.m. UTC | #9
On Fri, Jun 05, 2020 at 04:15:52PM +0300, Alexey Budankov wrote:
> 
> On 05.06.2020 13:51, Jiri Olsa wrote:
> > On Tue, Jun 02, 2020 at 04:43:58PM +0300, Adrian Hunter wrote:
> >> On 2/06/20 12:12 pm, Alexey Budankov wrote:
> >>>
> >>> On 02.06.2020 11:32, Alexey Budankov wrote:
> >>>>
> >>>> On 02.06.2020 2:37, Andi Kleen wrote:
> >>>>>>> or a pathname, or including also the event default of "disabled".
> >>>>>>
> >>>>>> For my cases conversion of pathnames into open fds belongs to external
> >>>>>> controlling process e.g. like in the examples provided in the patch set.
> >>>>>> Not sure about "event default of 'disabled'"
> >>>>>
> >>>>> It would be nicer for manual use cases if perf supported the path names
> >>>>> directly like in Adrian's example, not needing a complex wrapper script.
> >>>>
> >>>> fds interface is required for VTune integration since VTune wants control
> >>>> over files creation aside of Perf tool process. The script demonstrates
> >>>> just one possible use case.
> >>>>
> >>>> Control files could easily be implemented on top of fds making open operations
> >>>> for paths and then initializing fds. Interface below is vague and with explicit
> >>>> options like below it could be more explicit:
> >>>> --ctl-file /tmp/my-perf.fifo --ctl-file-ack /tmp/my-perf-ack.fifo
> >>>
> >>> Or even clearer:
> >>>
> >>> --ctl-fifo /tmp/my-perf --ctl-fifo-ack /tmp/my-perf-ack
> >>
> >> If people are OK with having so many options, then that is fine by me.
> > 
> > the single option Adrian suggested seems better to me:
> > 
> >  --control
> >  --control 11
> >  --control 11,15
> 
> What if a user specifies fifos named like this above, not fds?
> 
> >  --control 11,15,disabled
> >  --control 11,,disabled
> >  --control /tmp/my-perf.fifo
> >  --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo
> 
> What if a user wants not fifos but other type of comm channels?
> 
> >  --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo,disabled
> >  --control /tmp/my-perf.fifo,,disabled
> > 
> > we already support this kind of options arguments, like for --call-graph
> > 
> > jirka
> > 
> 
> IMHO,
> this interface, of course, looks more compact (in amount of options) however
> the other side it is less user friendly. One simple option for one simple
> purpose is more convenient as for users as for developers. Also complex
> option syntax tends to have limitations and there are probably more
> non-obvious ones.
> 
> Please speak up. I might have missed something meaningful.

how about specify the type like:

--control fd:1,2,...
--control fifo:/tmp/fifo1,/tmp/fifo2
--control xxx:....

this way we can extend the functionality in the future
and stay backward compatible, while keeping single option

jirka
Alexey Budankov June 5, 2020, 2:47 p.m. UTC | #10
On 05.06.2020 16:57, Jiri Olsa wrote:
> On Fri, Jun 05, 2020 at 04:15:52PM +0300, Alexey Budankov wrote:
>>
>> On 05.06.2020 13:51, Jiri Olsa wrote:
>>> On Tue, Jun 02, 2020 at 04:43:58PM +0300, Adrian Hunter wrote:
>>>> On 2/06/20 12:12 pm, Alexey Budankov wrote:
>>>>>
>>>>> On 02.06.2020 11:32, Alexey Budankov wrote:
>>>>>>
>>>>>> On 02.06.2020 2:37, Andi Kleen wrote:
>>>>>>>>> or a pathname, or including also the event default of "disabled".
>>>>>>>>
>>>>>>>> For my cases conversion of pathnames into open fds belongs to external
>>>>>>>> controlling process e.g. like in the examples provided in the patch set.
>>>>>>>> Not sure about "event default of 'disabled'"
>>>>>>>
>>>>>>> It would be nicer for manual use cases if perf supported the path names
>>>>>>> directly like in Adrian's example, not needing a complex wrapper script.
>>>>>>
>>>>>> fds interface is required for VTune integration since VTune wants control
>>>>>> over files creation aside of Perf tool process. The script demonstrates
>>>>>> just one possible use case.
>>>>>>
>>>>>> Control files could easily be implemented on top of fds making open operations
>>>>>> for paths and then initializing fds. Interface below is vague and with explicit
>>>>>> options like below it could be more explicit:
>>>>>> --ctl-file /tmp/my-perf.fifo --ctl-file-ack /tmp/my-perf-ack.fifo
>>>>>
>>>>> Or even clearer:
>>>>>
>>>>> --ctl-fifo /tmp/my-perf --ctl-fifo-ack /tmp/my-perf-ack
>>>>
>>>> If people are OK with having so many options, then that is fine by me.
>>>
>>> the single option Adrian suggested seems better to me:
>>>
>>>  --control
>>>  --control 11
>>>  --control 11,15
>>
>> What if a user specifies fifos named like this above, not fds?
>>
>>>  --control 11,15,disabled
>>>  --control 11,,disabled
>>>  --control /tmp/my-perf.fifo
>>>  --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo
>>
>> What if a user wants not fifos but other type of comm channels?
>>
>>>  --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo,disabled
>>>  --control /tmp/my-perf.fifo,,disabled
>>>
>>> we already support this kind of options arguments, like for --call-graph
>>>
>>> jirka
>>>
>>
>> IMHO,
>> this interface, of course, looks more compact (in amount of options) however
>> the other side it is less user friendly. One simple option for one simple
>> purpose is more convenient as for users as for developers. Also complex
>> option syntax tends to have limitations and there are probably more
>> non-obvious ones.
>>
>> Please speak up. I might have missed something meaningful.
> 
> how about specify the type like:
> 
> --control fd:1,2,...

What do these ... mean?

> --control fifo:/tmp/fifo1,/tmp/fifo2
> --control xxx:....
> 
> this way we can extend the functionality in the future
> and stay backward compatible, while keeping single option

Well, it clarifies more. However it still implicitly assumes
and requires proper ordering e.g. 1 is ctl-fd and 2 is ack-fd
and if there are some more positions there will be gaps like
--control fd:10,,something,,something ...

Why is one single option with complex syntax more preferable
than several simple options? Also it would still consume almost
equal amount of command line space in shell.

Thanks,
Alexey

> 
> jirka
>
Alexey Budankov June 5, 2020, 3:23 p.m. UTC | #11
On 05.06.2020 17:47, Alexey Budankov wrote:
> 
> On 05.06.2020 16:57, Jiri Olsa wrote:
>> On Fri, Jun 05, 2020 at 04:15:52PM +0300, Alexey Budankov wrote:
>>>
>>> On 05.06.2020 13:51, Jiri Olsa wrote:
>>>> On Tue, Jun 02, 2020 at 04:43:58PM +0300, Adrian Hunter wrote:
>>>>> On 2/06/20 12:12 pm, Alexey Budankov wrote:
>>>>>>
>>>>>> On 02.06.2020 11:32, Alexey Budankov wrote:
>>>>>>>
>>>>>>> On 02.06.2020 2:37, Andi Kleen wrote:
>>>>>>>>>> or a pathname, or including also the event default of "disabled".
>>>>>>>>>
>>>>>>>>> For my cases conversion of pathnames into open fds belongs to external
>>>>>>>>> controlling process e.g. like in the examples provided in the patch set.
>>>>>>>>> Not sure about "event default of 'disabled'"
>>>>>>>>
>>>>>>>> It would be nicer for manual use cases if perf supported the path names
>>>>>>>> directly like in Adrian's example, not needing a complex wrapper script.
>>>>>>>
>>>>>>> fds interface is required for VTune integration since VTune wants control
>>>>>>> over files creation aside of Perf tool process. The script demonstrates
>>>>>>> just one possible use case.
>>>>>>>
>>>>>>> Control files could easily be implemented on top of fds making open operations
>>>>>>> for paths and then initializing fds. Interface below is vague and with explicit
>>>>>>> options like below it could be more explicit:
>>>>>>> --ctl-file /tmp/my-perf.fifo --ctl-file-ack /tmp/my-perf-ack.fifo
>>>>>>
>>>>>> Or even clearer:
>>>>>>
>>>>>> --ctl-fifo /tmp/my-perf --ctl-fifo-ack /tmp/my-perf-ack
>>>>>
>>>>> If people are OK with having so many options, then that is fine by me.
>>>>
>>>> the single option Adrian suggested seems better to me:
>>>>
>>>>  --control
>>>>  --control 11
>>>>  --control 11,15
>>>
>>> What if a user specifies fifos named like this above, not fds?
>>>
>>>>  --control 11,15,disabled
>>>>  --control 11,,disabled
>>>>  --control /tmp/my-perf.fifo
>>>>  --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo
>>>
>>> What if a user wants not fifos but other type of comm channels?
>>>
>>>>  --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo,disabled
>>>>  --control /tmp/my-perf.fifo,,disabled
>>>>
>>>> we already support this kind of options arguments, like for --call-graph
>>>>
>>>> jirka
>>>>
>>>
>>> IMHO,
>>> this interface, of course, looks more compact (in amount of options) however
>>> the other side it is less user friendly. One simple option for one simple
>>> purpose is more convenient as for users as for developers. Also complex
>>> option syntax tends to have limitations and there are probably more
>>> non-obvious ones.
>>>
>>> Please speak up. I might have missed something meaningful.
>>
>> how about specify the type like:
>>
>> --control fd:1,2,...
> 
> What do these ... mean?

After all,
if you want it this way and it now also fits my needs I could convert
--ctl-fd[-ack] to --control fd:<ctl-fd>,<ack-fd> with use cases like
--control fd:<ctl-fd> and --control fd:<ctl-fd>,<ack-fd>. Accepted?

~Alexey

> 
>> --control fifo:/tmp/fifo1,/tmp/fifo2
>> --control xxx:....
>>
>> this way we can extend the functionality in the future
>> and stay backward compatible, while keeping single option
> 
> Well, it clarifies more. However it still implicitly assumes
> and requires proper ordering e.g. 1 is ctl-fd and 2 is ack-fd
> and if there are some more positions there will be gaps like
> --control fd:10,,something,,something ...
> 
> Why is one single option with complex syntax more preferable
> than several simple options? Also it would still consume almost
> equal amount of command line space in shell.
> 
> Thanks,
> Alexey
> 
>>
>> jirka
>>
Jiri Olsa June 6, 2020, 8:27 a.m. UTC | #12
On Fri, Jun 05, 2020 at 05:47:28PM +0300, Alexey Budankov wrote:
> 
> On 05.06.2020 16:57, Jiri Olsa wrote:
> > On Fri, Jun 05, 2020 at 04:15:52PM +0300, Alexey Budankov wrote:
> >>
> >> On 05.06.2020 13:51, Jiri Olsa wrote:
> >>> On Tue, Jun 02, 2020 at 04:43:58PM +0300, Adrian Hunter wrote:
> >>>> On 2/06/20 12:12 pm, Alexey Budankov wrote:
> >>>>>
> >>>>> On 02.06.2020 11:32, Alexey Budankov wrote:
> >>>>>>
> >>>>>> On 02.06.2020 2:37, Andi Kleen wrote:
> >>>>>>>>> or a pathname, or including also the event default of "disabled".
> >>>>>>>>
> >>>>>>>> For my cases conversion of pathnames into open fds belongs to external
> >>>>>>>> controlling process e.g. like in the examples provided in the patch set.
> >>>>>>>> Not sure about "event default of 'disabled'"
> >>>>>>>
> >>>>>>> It would be nicer for manual use cases if perf supported the path names
> >>>>>>> directly like in Adrian's example, not needing a complex wrapper script.
> >>>>>>
> >>>>>> fds interface is required for VTune integration since VTune wants control
> >>>>>> over files creation aside of Perf tool process. The script demonstrates
> >>>>>> just one possible use case.
> >>>>>>
> >>>>>> Control files could easily be implemented on top of fds making open operations
> >>>>>> for paths and then initializing fds. Interface below is vague and with explicit
> >>>>>> options like below it could be more explicit:
> >>>>>> --ctl-file /tmp/my-perf.fifo --ctl-file-ack /tmp/my-perf-ack.fifo
> >>>>>
> >>>>> Or even clearer:
> >>>>>
> >>>>> --ctl-fifo /tmp/my-perf --ctl-fifo-ack /tmp/my-perf-ack
> >>>>
> >>>> If people are OK with having so many options, then that is fine by me.
> >>>
> >>> the single option Adrian suggested seems better to me:
> >>>
> >>>  --control
> >>>  --control 11
> >>>  --control 11,15
> >>
> >> What if a user specifies fifos named like this above, not fds?
> >>
> >>>  --control 11,15,disabled
> >>>  --control 11,,disabled
> >>>  --control /tmp/my-perf.fifo
> >>>  --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo
> >>
> >> What if a user wants not fifos but other type of comm channels?
> >>
> >>>  --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo,disabled
> >>>  --control /tmp/my-perf.fifo,,disabled
> >>>
> >>> we already support this kind of options arguments, like for --call-graph
> >>>
> >>> jirka
> >>>
> >>
> >> IMHO,
> >> this interface, of course, looks more compact (in amount of options) however
> >> the other side it is less user friendly. One simple option for one simple
> >> purpose is more convenient as for users as for developers. Also complex
> >> option syntax tends to have limitations and there are probably more
> >> non-obvious ones.
> >>
> >> Please speak up. I might have missed something meaningful.
> > 
> > how about specify the type like:
> > 
> > --control fd:1,2,...
> 
> What do these ... mean?

other possible options

> 
> > --control fifo:/tmp/fifo1,/tmp/fifo2
> > --control xxx:....
> > 
> > this way we can extend the functionality in the future
> > and stay backward compatible, while keeping single option
> 
> Well, it clarifies more. However it still implicitly assumes
> and requires proper ordering e.g. 1 is ctl-fd and 2 is ack-fd
> and if there are some more positions there will be gaps like
> --control fd:10,,something,,something ...

right, that's what we do for other options

> 
> Why is one single option with complex syntax more preferable
> than several simple options? Also it would still consume almost
> equal amount of command line space in shell.

I think it's better for future.. say if there's going to be support
for passing file paths you'll need to add something like --ctl-fifo
and --ctl-fifo-ack no?  with single option we'd just add something
like:

  --control fifo:/tmp/my-perf.fifo,/tmp/my-perf-ack.fifo

jirka
Alexey Budankov June 8, 2020, 8:04 a.m. UTC | #13
On 05.06.2020 18:23, Alexey Budankov wrote:
> 
> On 05.06.2020 17:47, Alexey Budankov wrote:
>>
>> On 05.06.2020 16:57, Jiri Olsa wrote:
>>> On Fri, Jun 05, 2020 at 04:15:52PM +0300, Alexey Budankov wrote:
>>>>
>>>> On 05.06.2020 13:51, Jiri Olsa wrote:
>>>>> On Tue, Jun 02, 2020 at 04:43:58PM +0300, Adrian Hunter wrote:
>>>>>> On 2/06/20 12:12 pm, Alexey Budankov wrote:
>>>>>>>
>>>>>>> On 02.06.2020 11:32, Alexey Budankov wrote:
>>>>>>>>
>>>>>>>> On 02.06.2020 2:37, Andi Kleen wrote:
>>>>>>>>>>> or a pathname, or including also the event default of "disabled".
>>>>>>>>>>
>>>>>>>>>> For my cases conversion of pathnames into open fds belongs to external
>>>>>>>>>> controlling process e.g. like in the examples provided in the patch set.
>>>>>>>>>> Not sure about "event default of 'disabled'"
>>>>>>>>>
>>>>>>>>> It would be nicer for manual use cases if perf supported the path names
>>>>>>>>> directly like in Adrian's example, not needing a complex wrapper script.
>>>>>>>>
>>>>>>>> fds interface is required for VTune integration since VTune wants control
>>>>>>>> over files creation aside of Perf tool process. The script demonstrates
>>>>>>>> just one possible use case.
>>>>>>>>
>>>>>>>> Control files could easily be implemented on top of fds making open operations
>>>>>>>> for paths and then initializing fds. Interface below is vague and with explicit
>>>>>>>> options like below it could be more explicit:
>>>>>>>> --ctl-file /tmp/my-perf.fifo --ctl-file-ack /tmp/my-perf-ack.fifo
>>>>>>>
>>>>>>> Or even clearer:
>>>>>>>
>>>>>>> --ctl-fifo /tmp/my-perf --ctl-fifo-ack /tmp/my-perf-ack
>>>>>>
>>>>>> If people are OK with having so many options, then that is fine by me.
>>>>>
>>>>> the single option Adrian suggested seems better to me:
>>>>>
>>>>>  --control
>>>>>  --control 11
>>>>>  --control 11,15
>>>>
>>>> What if a user specifies fifos named like this above, not fds?
>>>>
>>>>>  --control 11,15,disabled
>>>>>  --control 11,,disabled
>>>>>  --control /tmp/my-perf.fifo
>>>>>  --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo
>>>>
>>>> What if a user wants not fifos but other type of comm channels?
>>>>
>>>>>  --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo,disabled
>>>>>  --control /tmp/my-perf.fifo,,disabled
>>>>>
>>>>> we already support this kind of options arguments, like for --call-graph
>>>>>
>>>>> jirka
>>>>>
>>>>
>>>> IMHO,
>>>> this interface, of course, looks more compact (in amount of options) however
>>>> the other side it is less user friendly. One simple option for one simple
>>>> purpose is more convenient as for users as for developers. Also complex
>>>> option syntax tends to have limitations and there are probably more
>>>> non-obvious ones.
>>>>
>>>> Please speak up. I might have missed something meaningful.
>>>
>>> how about specify the type like:
>>>
>>> --control fd:1,2,...
>>
>> What do these ... mean?
> 
> After all,
> if you want it this way and it now also fits my needs I could convert
> --ctl-fd[-ack] to --control fd:<ctl-fd>,<ack-fd> with use cases like
> --control fd:<ctl-fd> and --control fd:<ctl-fd>,<ack-fd>. Accepted?

So, do we implement fds options like this?

~Alexey
Jiri Olsa June 8, 2020, 8:45 a.m. UTC | #14
On Fri, Jun 05, 2020 at 06:23:17PM +0300, Alexey Budankov wrote:

SNIP

> >>>>>> Or even clearer:
> >>>>>>
> >>>>>> --ctl-fifo /tmp/my-perf --ctl-fifo-ack /tmp/my-perf-ack
> >>>>>
> >>>>> If people are OK with having so many options, then that is fine by me.
> >>>>
> >>>> the single option Adrian suggested seems better to me:
> >>>>
> >>>>  --control
> >>>>  --control 11
> >>>>  --control 11,15
> >>>
> >>> What if a user specifies fifos named like this above, not fds?
> >>>
> >>>>  --control 11,15,disabled
> >>>>  --control 11,,disabled
> >>>>  --control /tmp/my-perf.fifo
> >>>>  --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo
> >>>
> >>> What if a user wants not fifos but other type of comm channels?
> >>>
> >>>>  --control /tmp/my-perf.fifo,/tmp/my-perf-ack.fifo,disabled
> >>>>  --control /tmp/my-perf.fifo,,disabled
> >>>>
> >>>> we already support this kind of options arguments, like for --call-graph
> >>>>
> >>>> jirka
> >>>>
> >>>
> >>> IMHO,
> >>> this interface, of course, looks more compact (in amount of options) however
> >>> the other side it is less user friendly. One simple option for one simple
> >>> purpose is more convenient as for users as for developers. Also complex
> >>> option syntax tends to have limitations and there are probably more
> >>> non-obvious ones.
> >>>
> >>> Please speak up. I might have missed something meaningful.
> >>
> >> how about specify the type like:
> >>
> >> --control fd:1,2,...
> > 
> > What do these ... mean?
> 
> After all,
> if you want it this way and it now also fits my needs I could convert
> --ctl-fd[-ack] to --control fd:<ctl-fd>,<ack-fd> with use cases like
> --control fd:<ctl-fd> and --control fd:<ctl-fd>,<ack-fd>. Accepted?

looks good

jirka

Patch
diff mbox series

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 0394e068dde8..fbe5069eb5d7 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1749,6 +1749,9 @@  static int __cmd_record(struct record *rec, int argc, const char **argv)
 		perf_evlist__start_workload(rec->evlist);
 	}
 
+	if (evlist__initialize_ctlfd(rec->evlist, opts->ctl_fd, opts->ctl_fd_ack))
+		goto out_child;
+
 	if (opts->initial_delay) {
 		pr_info(EVLIST_DISABLED_MSG);
 		if (opts->initial_delay > 0) {
@@ -1895,6 +1898,7 @@  static int __cmd_record(struct record *rec, int argc, const char **argv)
 		record__synthesize_workload(rec, true);
 
 out_child:
+	evlist__finalize_ctlfd(rec->evlist);
 	record__mmap_read_all(rec, true);
 	record__aio_mmap_read_sync(rec);
 
@@ -2380,6 +2384,8 @@  static struct record record = {
 		},
 		.mmap_flush          = MMAP_FLUSH_DEFAULT,
 		.nr_threads_synthesize = 1,
+		.ctl_fd              = -1,
+		.ctl_fd_ack          = -1,
 	},
 	.tool = {
 		.sample		= process_sample_event,
diff --git a/tools/perf/util/record.h b/tools/perf/util/record.h
index da138dcb4d34..4cb72a478af1 100644
--- a/tools/perf/util/record.h
+++ b/tools/perf/util/record.h
@@ -70,6 +70,8 @@  struct record_opts {
 	int	      mmap_flush;
 	unsigned int  comp_level;
 	unsigned int  nr_threads_synthesize;
+	int	      ctl_fd;
+	int	      ctl_fd_ack;
 };
 
 extern const char * const *record_usage;