Re: [PATCH V3 1/2] bpf: control the trace data output on current cpu when perf sampling

[Alexei Starovoitov <ast@plumgrid.com>]

On 10/16/15 12:42 AM, Kaixu Xia wrote:
> This patch adds the flag dump_enable to control the trace data
> output process when perf sampling. By setting this flag and
> integrating with ebpf, we can control the data output process and
> get the samples we are most interested in.
>
> The bpf helper bpf_perf_event_dump_control() can control the
> perf_event on current cpu.
>
> Signed-off-by: Kaixu Xia <xiakaixu@huawei.com>
> ---
>   include/linux/perf_event.h      |  1 +
>   include/uapi/linux/bpf.h        |  5 +++++
>   include/uapi/linux/perf_event.h |  3 ++-
>   kernel/bpf/verifier.c           |  3 ++-
>   kernel/events/core.c            | 13 ++++++++++++
>   kernel/trace/bpf_trace.c        | 44 +++++++++++++++++++++++++++++++++++++++++
>   6 files changed, 67 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 092a0e8..2af527e 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -472,6 +472,7 @@ struct perf_event {
>   	struct irq_work			pending;
>
>   	atomic_t			event_limit;
> +	atomic_t			dump_enable;

The naming is the hardest...
How about calling it 'soft_enable' instead?

> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -287,6 +287,11 @@ enum bpf_func_id {
>   	 * Return: realm if != 0
>   	 */
>   	BPF_FUNC_get_route_realm,
> +
> +	/**
> +	 * u64 bpf_perf_event_dump_control(&map, index, flag)
> +	 */
> +	BPF_FUNC_perf_event_dump_control,

and this one is too long.
May be bpf_perf_event_control() ?

Daniel, any thoughts on naming?

> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -331,7 +331,8 @@ struct perf_event_attr {
>   				comm_exec      :  1, /* flag comm events that are due to an exec */
>   				use_clockid    :  1, /* use @clockid for time fields */
>   				context_switch :  1, /* context switch data */
> -				__reserved_1   : 37;
> +				dump_enable    :  1, /* don't output data on samples */

either comment or name is wrong.
how about calling this one 'soft_disable',
since you want zero to be default and the event should be on.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index b11756f..74a16af 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6337,6 +6337,9 @@ static int __perf_event_overflow(struct perf_event *event,
>   		irq_work_queue(&event->pending);
>   	}
>
> +	if (!atomic_read(&event->dump_enable))
> +		return ret;

I'm not an expert in this piece of perf, but should it be 'return 0'
instead ?
and may be moved to is_sampling_event() check?
Also please add unlikely().

> +static void perf_event_check_dump_flag(struct perf_event *event)
> +{
> +	if (event->attr.dump_enable == 1)
> +		atomic_set(&event->dump_enable, 1);
> +	else
> +		atomic_set(&event->dump_enable, 0);

that looks like it breaks perf, since default for bits is zero
and all events will be soft-disabled?
How did you test it?
Please add a test to samples/bpf/ for this feature.