Re: [PATCH bpf-next v4 2/3] libbpf: add low level TC-BPF API

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Kumar Kartikeya Dwivedi <memxor@gmail.com> writes:

> On Tue, Apr 27, 2021 at 08:34:30PM IST, Daniel Borkmann wrote:
>> On 4/23/21 5:05 PM, Kumar Kartikeya Dwivedi wrote:
>> [...]
>> >   tools/lib/bpf/libbpf.h   |  92 ++++++++
>> >   tools/lib/bpf/libbpf.map |   5 +
>> >   tools/lib/bpf/netlink.c  | 478 ++++++++++++++++++++++++++++++++++++++-
>> >   3 files changed, 574 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> > index bec4e6a6e31d..1c717c07b66e 100644
>> > --- a/tools/lib/bpf/libbpf.h
>> > +++ b/tools/lib/bpf/libbpf.h
>> > @@ -775,6 +775,98 @@ LIBBPF_API int bpf_linker__add_file(struct bpf_linker *linker, const char *filen
>> >   LIBBPF_API int bpf_linker__finalize(struct bpf_linker *linker);
>> >   LIBBPF_API void bpf_linker__free(struct bpf_linker *linker);
>> >
>> > +enum bpf_tc_attach_point {
>> > +	BPF_TC_INGRESS,
>> > +	BPF_TC_EGRESS,
>> > +	BPF_TC_CUSTOM_PARENT,
>> > +	_BPF_TC_PARENT_MAX,
>>
>> I don't think we need to expose _BPF_TC_PARENT_MAX as part of the API, I would drop
>> the latter.
>>
>
> Ok, will drop.
>
>> > +};
>> > +
>> > +/* The opts structure is also used to return the created filters attributes
>> > + * (e.g. in case the user left them unset). Some of the options that were left
>> > + * out default to a reasonable value, documented below.
>> > + *
>> > + *	protocol - ETH_P_ALL
>> > + *	chain index - 0
>> > + *	class_id - 0 (can be set by bpf program using skb->tc_classid)
>> > + *	bpf_flags - TCA_BPF_FLAG_ACT_DIRECT (direct action mode)
>> > + *	bpf_flags_gen - 0
>> > + *
>> > + *	The user must fulfill documented requirements for each function.
>>
>> Not sure if this is overly relevant as part of the bpf_tc_opts in here. For the
>> 2nd part, I would probably just mention that libbpf internally attaches the bpf
>> programs with direct action mode. The hw offload may be future todo, and the other
>> bits are little used anyway; mentioning them here, what value does it have to
>> libbpf users? I'd rather just drop the 2nd part and/or simplify this paragraph
>> just stating that the progs are attached in direct action mode.
>>
>
> The goal was to just document whatever attributes were set to by default, but I can see
> your point. I'll trim it.
>
>> > + */
>> > +struct bpf_tc_opts {
>> > +	size_t sz;
>> > +	__u32 handle;
>> > +	__u32 parent;
>> > +	__u16 priority;
>> > +	__u32 prog_id;
>> > +	bool replace;
>> > +	size_t :0;
>> > +};
>> > +
>> > +#define bpf_tc_opts__last_field replace
>> > +
>> > +struct bpf_tc_ctx;
>> > +
>> > +struct bpf_tc_ctx_opts {
>> > +	size_t sz;
>> > +};
>> > +
>> > +#define bpf_tc_ctx_opts__last_field sz
>> > +
>> > +/* Requirements */
>> > +/*
>> > + * @ifindex: Must be > 0.
>> > + * @parent: Must be one of the enum constants < _BPF_TC_PARENT_MAX
>> > + * @opts: Can be NULL, currently no options are supported.
>> > + */
>>
>> Up to Andrii, but we don't have such API doc in general inside libbpf.h, I
>> would drop it for the time being to be consistent with the rest (same for
>> others below).
>>
>
> I think we need to keep this somewhere. We dropped bpf_tc_info since it wasn't
> really serving any purpose, but that meant we would put the only extra thing it
> returned (prog_id) into bpf_tc_opts. That means we also need to take care that
> it is unset (along with replace) in functions where it isn't used, to allow for
> reuse for some future purpose. If we don't document that the user needs to unset
> them whenever working with bpf_tc_query and bpf_tc_detach, how are they supposed
> to know?
>
> Maybe a man page and/or a blog post would be better? Just putting it above the
> function seemed best for now.

You could document it together with the struct definition instead of for
each function? See the inline comments in bpf_object_open_opts for instance...

-Toke