Re: [PATCH v4 00/16] perf, persistent: Add persistent events

[Robert Richter <rric@kernel.org>]

On 06.05.14 20:58:26, Borislav Petkov wrote:
> On Tue, May 06, 2014 at 02:39:07PM +0200, Robert Richter wrote:
> > I rather would change the ioctl to
> > 
> >  id = ioctl(PERF_EVENT_IOC_SET_PERSIST, arg);
> > 
> >  arg != 0:	create persistent event (unclaim)
> >  arg == 0:	delete persistent event (claim)
> > 
> > This has the advantage that the naming is better and arg can be used
> > as parameter (e.g. event id to share a namespace).
> 
> Yep, this is better than the CLAIM/UNCLAIM thing.

Yeah, I am wondering why we didn't get this earlier. ;)

> > The unclaim ioctl *creates* a buffer with 512kB default size. The
> > reopening process must mmap with the same buffer size. This is a
> > problem as in this implementation the buffer size is fix and can not
> > be adjusted.
> 
> Why do we need it to be adjustable?

I am fine with a fixed size. But this design might not be changable in
the future without breaking the API. So its worth to think about this.
Maybe we allow the process also to create the buffer, there are 2
variants then:

 perf_event_open(...);
 mmap(..., fd, ...);
 ioctl(fd, set_persistent, 1);   /* Use existing buffer. */
 close();

 fd = perf_event_open(...);
 ioctl(fd, set_persistent, 1);   /* Create buffer with default size. */
 close();

This would solve the problem how we adjust buffer size.

This requires a way to detect buffer size. See my mail how this could
be done.

> 
> > We could let create the process the buffers and make the
> > event persistent including the current buffers.
> > 
> > Variable buffer size is a must,
> 
> Yeah, why? I'm just asking for my own understanding.

For general-purpose... Why 512k buffers are ok? Depending on the
event, smaller buffers are maybe good enough, esp. since they are
permanently enabled and use system resources. Or, at some point, 512k
is not sufficient anymore. You don't want 512k be carved in stone.

> > so the reopening process also must be
> > able to detect buffer size. Mmap buffer size could be detected by
> > mmap'ing only the header page, reading the buffer size from the header
> > and then remapping the buffer with adjusted size.
> > 
> > It would be good to have a perf tool option -P that puts events in
> > persistent state instead of starting a command:
> > 
> >  # perf record -e ... -a -P <namespace>		# create pers. events
> >  # perf record -e persistent/<namespace>/ -a	# read existing buffers
> >  # perf record -e persistent/<namespace>/ -a -k	# read and kill existing events
> > 
> > Note that no args for a command are given.
> 
> Yep, looks sane to me.

Ok.

Thanks for your commments.

-Robert