Re: [PATCH v6 bpf-next 1/2] bpf: Add bpf_read_branch_records() helper

["Daniel Xu" <dxu@dxuuu.xyz>]

On Mon Jan 27, 2020 at 1:26 PM, Daniel Borkmann wrote:
[...]
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index f1d74a2bd234..332aa433d045 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -2892,6 +2892,25 @@ union bpf_attr {
> >    *		Obtain the 64bit jiffies
> >    *	Return
> >    *		The 64 bit jiffies
> > + *
> > + * int bpf_read_branch_records(struct bpf_perf_event_data *ctx, void *buf, u32 buf_size, u64 flags)
>
> 
> Small nit: s/buf_size/size/, so that it matches with your BPF_CALL
> below.

Ok

> 
> +BPF_CALL_4(bpf_read_branch_records, struct bpf_perf_event_data_kern *,
> ctx,
> + void *, buf, u32, size, u64, flags)
>
> 
> > + *	Description
> > + *		For an eBPF program attached to a perf event, retrieve the
> > + *		branch records (struct perf_branch_entry) associated to *ctx*
> > + *		and store it in	the buffer pointed by *buf* up to size
> > + *		*buf_size* bytes.
> > + *
> > + *		The *flags* can be set to **BPF_F_GET_BRANCH_RECORDS_SIZE** to
> > + *		instead	return the number of bytes required to store all the
> > + *		branch entries. If this flag is set, *buf* may be NULL.
> > + *	Return
> > + *		On success, number of bytes written to *buf*. On error, a
> > + *		negative value.
>
> 
> Maybe pull the 2nd paragraph from above in here so that it reflects the
> description
> of the return value when flag is used also for this case in the 'Return'
> description.

Ok.

[...]
> >   
> > +BPF_CALL_4(bpf_read_branch_records, struct bpf_perf_event_data_kern *, ctx,
> > +	   void *, buf, u32, size, u64, flags)
> > +{
> > +#ifndef CONFIG_X86
> > +	return -ENOENT;
> > +#else
> > +	struct perf_branch_stack *br_stack = ctx->data->br_stack;
> > +	u32 br_entry_size = sizeof(struct perf_branch_entry);
>
> 
> 'static const u32 br_entry_size' if we use it as such below.

Ok

>
> 
> > +	u32 to_copy;
> > +
> > +	if (unlikely(flags & ~BPF_F_GET_BRANCH_RECORDS_SIZE))
> > +		return -EINVAL;
> > +
> > +	if (unlikely(!br_stack))
> > +		return -EINVAL;
>
> 
> Why the ifdef X86? In previous thread I meant to change it into since
> it's
> implicit:
>
> 
> if (unlikely(!br_stack))
> return -ENOENT;
>
> 
> Or is there any other additional rationale?

Yeah, so br_stack can be null if the perf_event is misconfigured (branch
record not enabled). So we need to differentiate that from arch not
supporting it.

[...]


Thanks,
Daniel