Re: [RFC PATCH 2/3] perf: Add persistent event facilities

[Namhyung Kim <namhyung@kernel.org>]

On Fri, 15 Mar 2013 14:06:28 +0100, Borislav Petkov wrote:
> Add a barebones implementation for registering persistent events with
> perf. For that, we don't destroy the buffers when they're unmapped;
> also, we map them read-only so that multiple agents can access them.
>
> Also, we allocate the event buffers at event init time and not at mmap
> time so that we can log samples into them regardless of whether there
> are readers in userspace or not.
[SNIP]
> -static void ring_buffer_put(struct ring_buffer *rb)
> +void perf_ring_buffer_put(struct ring_buffer *rb)

Why did you rename this function?


>  {
>  	struct perf_event *event, *n;
>  	unsigned long flags;
[SNIP]
> +static struct perf_event *
> +add_persistent_event_on_cpu(unsigned int cpu, struct perf_event_attr *attr,
> +			    unsigned nr_pages)
> +{
> +	struct perf_event *event = ERR_PTR(-EINVAL);

Looks like -ENOMEM is more appropriate code for the below kzalloc.


> +	struct pers_event_desc *desc;
> +	struct ring_buffer *buf;
> +
> +	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> +	if (!desc)
> +		goto out;
> +
> +	event = perf_event_create_kernel_counter(attr, cpu, NULL, NULL, NULL);
> +	if (IS_ERR(event))
> +		goto err_event;
> +
> +	buf = rb_alloc(nr_pages, 0, cpu, 0);
> +	if (!buf)
> +		goto err_event_file;
> +
> +	rcu_assign_pointer(event->rb, buf);
> +
> +	desc->event = event;
> +	desc->attr  = attr;
> +
> +	INIT_LIST_HEAD(&desc->plist);
> +	list_add_tail(&desc->plist, &per_cpu(pers_events, cpu));
> +
> +	perf_event_enable(event);
> +
> +	goto out;
> +
> + err_event_file:
> +	perf_event_release_kernel(event);

It needs to reset event to ERR_PTR(-ENOMEM) ?

> +
> + err_event:
> +	kfree(desc);
> +
> + out:
> +	return event;
> +}
> +
> +static void rm_persistent_event(int cpu, struct perf_event_attr *attr)
> +{
> +	struct pers_event_desc *desc, *tmp;
> +	struct perf_event *event = NULL;
> +
> +	list_for_each_entry_safe(desc, tmp, &per_cpu(pers_events, cpu), plist) {
> +		if (desc->attr->config == attr->config) {
> +			event = desc->event;
> +			break;
> +		}
> +	}
> +
> +	if (!event)
> +		return;
> +
> +	__list_del(desc->plist.prev, desc->plist.next);

Why not using list_del(&desc->plist) ?

> +
> +	perf_event_disable(event);
> +	if (event->rb) {
> +		perf_ring_buffer_put(event->rb);
> +		rcu_assign_pointer(event->rb, NULL);
> +	}
> +
> +	perf_event_release_kernel(event);
> +	put_unused_fd(desc->fd);
> +	kfree(desc);
> +}
> +
> +int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)
> +{
> +	struct pers_event_desc *desc;
> +	struct file *event_file = NULL;
> +	int event_fd = -1;
> +
> +	list_for_each_entry(desc, &per_cpu(pers_events, cpu), plist) {
> +
> +		if (desc->attr->config != attr->config)
> +			continue;
> +
> +		event_fd = get_unused_fd();
> +		if (event_fd < 0)
> +			goto out;
> +
> +		event_file = anon_inode_getfile("[pers_event]", &perf_fops,
> +						desc->event, O_RDONLY);
> +		if (IS_ERR(event_file))
> +			goto err_event_file;
> +
> +		desc->fd = event_fd;
> +		fd_install(event_fd, event_file);
> +
> +		return event_fd;
> +	}
> +
> + err_event_file:
> +	put_unused_fd(event_fd);

Isn't it safe to have event_fd of -1 in case not found?  Anyway, if it's
returned to the user space directly, it's better having more meaningful
error code IMHO.

> +
> +out:
> +	return event_fd;
> +}

Thanks,
Namhyung