Re: [PATCH net-next v2 1/2] bpf/tracing: allow user space to query prog array on the same tp

On 12/6/17 5:16 AM, Peter Zijlstra wrote:

On Wed, Dec 06, 2017 at 12:56:36PM +0100, Peter Zijlstra wrote:

quoted

On Tue, Dec 05, 2017 at 10:31:28PM -0800, Yonghong Song wrote:

quoted

Commit e87c6bc3852b ("bpf: permit multiple bpf attachments
for a single perf event") added support to attach multiple
bpf programs to a single perf event.
Commit 2541517c32be ("tracing, perf: Implement BPF programs
attached to kprobes") utilized the existing perf ioctl
interface and added the command PERF_EVENT_IOC_SET_BPF
to attach a bpf program to a tracepoint.

This patch adds a new ioctl
command, given a perf event fd, to query the bpf program array
attached to the same perf tracepoint event.

The new uapi ioctl command:
   PERF_EVENT_IOC_QUERY_BPF

The new uapi/linux/perf_event.h structure:
   struct perf_event_query_bpf {
        __u64	prog_ids;
        __u32	prog_cnt;
   };

The usage:
   struct perf_event_query_bpf query;
   query.prog_ids = (__u64)usr_prog_ids_buf;
   query.prog_cnt = usr_prog_ids_buf_len;
   err = ioctl(pmu_efd, PERF_EVENT_IOC_QUERY_BPF, &query);

Signed-off-by: Yonghong Song <redacted>
Acked-by: Alexei Starovoitov <ast@kernel.org>

Can you please fix that example to make it clear that prog_ids is in
fact a pointer to an array of size prog_cnt. Ideally also describing
what the type of array is.

Right. Will address this with more descriptions in the commit message
and also add some comments in the perf_event.h.

quoted

In fact, would not something like:

	struct perf_event_query_bpf {
		__u32 len;
		__u32 __reserved;

I suppose we could use this field to store the number of entries
returned, retaining the len to indicate how large the structure is.

quoted

		__u64 ids[0];
	};

be a much clearer interface?

Yes, this is clearer and may be consistent with perf interface.

FYI, my old interface is similar to the BPF query interface below:
        struct { /* anonymous struct used by BPF_PROG_QUERY command */
                __u32           target_fd;      /* container object to query */
                __u32           attach_type;
                __u32           query_flags;
                __u32           attach_flags;
                __aligned_u64   prog_ids;
                __u32           prog_cnt;
        } query;

quoted

Also, you forgot to tell us why we need this interface at all.

Right. Will add some descriptions for this.