bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Daniel Borkmann <daniel@iogearbox.net>
To: Kumar Kartikeya Dwivedi <memxor@gmail.com>, bpf@vger.kernel.org
Cc: "Toke Høiland-Jørgensen" <toke@redhat.com>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Andrii Nakryiko" <andrii@kernel.org>,
	"Martin KaFai Lau" <kafai@fb.com>,
	"Song Liu" <songliubraving@fb.com>, "Yonghong Song" <yhs@fb.com>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"KP Singh" <kpsingh@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Jesper Dangaard Brouer" <brouer@redhat.com>,
	netdev@vger.kernel.org
Subject: Re: [PATCH bpf-next v4 2/3] libbpf: add low level TC-BPF API
Date: Tue, 27 Apr 2021 17:04:30 +0200	[thread overview]
Message-ID: <5811eb10-bc93-0b81-2ee4-10490388f238@iogearbox.net> (raw)
In-Reply-To: <20210423150600.498490-3-memxor@gmail.com>

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.

> +};
> +
> +/* 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.

> + */
> +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).

> +LIBBPF_API struct bpf_tc_ctx *bpf_tc_ctx_init(__u32 ifindex,

nit: in user space s/__u32 ifindex/int ifindex/

> +					      enum bpf_tc_attach_point parent,
> +					      struct bpf_tc_ctx_opts *opts);

Should we enforce opts being NULL or non-NULL here, or drop the arg from here
for now altogether? (And if later versions of the functions show up this could
be mapped to the right one?)

> +/*
> + * @ctx: Can be NULL, if not, must point to a valid object.
> + *	 If the qdisc was attached during ctx_init, it will be deleted if no
> + *	 filters are attached to it.
> + *	 When ctx == NULL, this is a no-op.
> + */
> +LIBBPF_API int bpf_tc_ctx_destroy(struct bpf_tc_ctx *ctx);
> +/*
> + * @ctx: Cannot be NULL.
> + * @fd: Must be >= 0.
> + * @opts: Cannot be NULL, prog_id must be unset, all other fields can be
> + *	  optionally set. All fields except replace  will be set as per created
> + *        filter's attributes. parent must only be set when attach_point of ctx is
> + *        BPF_TC_CUSTOM_PARENT, otherwise parent must be unset.
> + *
> + * Fills the following fields in opts:
> + *	handle
> + *	parent
> + *	priority
> + *	prog_id
> + */
> +LIBBPF_API int bpf_tc_attach(struct bpf_tc_ctx *ctx, int fd,
> +			     struct bpf_tc_opts *opts);
> +/*
> + * @ctx: Cannot be NULL.
> + * @opts: Cannot be NULL, replace and prog_id must be unset, all other fields
> + *	  must be set.
> + */
> +LIBBPF_API int bpf_tc_detach(struct bpf_tc_ctx *ctx,
> +			     const struct bpf_tc_opts *opts);

One thing that I find a bit odd from this API is that BPF_TC_INGRESS / BPF_TC_EGRESS
needs to be set each time via bpf_tc_ctx_init(). So whenever a specific program would
be attached to both we need to 're-init' in between just to change from hook a to b,
whereas when you have BPF_TC_CUSTOM_PARENT, you could just use a different opts->parent
without going this detour (unless the clsact wasn't loaded there in the first place).

Could we add a BPF_TC_UNSPEC to enum bpf_tc_attach_point, which the user would pass to
bpf_tc_ctx_init(), so that opts.direction = BPF_TC_INGRESS with subsequent bpf_tc_attach()
can be called, and same opts.direction = BPF_TC_EGRESS with bpf_tc_attach() for different
fd. The only thing we cared about in bpf_tc_ctx_init() resp. the ctx was that qdisc was
ready.

> +/*
> + * @ctx: Cannot be NULL.
> + * @opts: Cannot be NULL, replace and prog_id must be unset, all other fields
> + *	  must be set.
> + *
> + * Fills the following fields in opts:
> + *	handle
> + *	parent
> + *	priority
> + *	prog_id
> + */
> +LIBBPF_API int bpf_tc_query(struct bpf_tc_ctx *ctx,
> +			    struct bpf_tc_opts *opts);
> +
>   #ifdef __cplusplus
>   } /* extern "C" */
>   #endif

  reply	other threads:[~2021-04-27 15:04 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-23 15:05 [PATCH bpf-next v4 0/3] Add TC-BPF API Kumar Kartikeya Dwivedi
2021-04-23 15:05 ` [PATCH bpf-next v4 1/3] libbpf: add helpers for preparing netlink attributes Kumar Kartikeya Dwivedi
2021-04-23 15:05 ` [PATCH bpf-next v4 2/3] libbpf: add low level TC-BPF API Kumar Kartikeya Dwivedi
2021-04-27 15:04   ` Daniel Borkmann [this message]
2021-04-27 18:00     ` Toke Høiland-Jørgensen
2021-04-27 18:02     ` Kumar Kartikeya Dwivedi
2021-04-27 18:15       ` Toke Høiland-Jørgensen
2021-04-27 21:55       ` Daniel Borkmann
2021-04-27 22:05         ` Daniel Borkmann
2021-04-27 22:32           ` Daniel Borkmann
2021-04-27 22:36           ` Toke Høiland-Jørgensen
2021-04-27 22:40             ` Daniel Borkmann
2021-04-27 22:51               ` Toke Høiland-Jørgensen
2021-04-27 23:14                 ` Daniel Borkmann
2021-04-27 23:19                   ` Kumar Kartikeya Dwivedi
2021-04-27 20:36     ` Andrii Nakryiko
2021-04-23 15:06 ` [PATCH bpf-next v4 3/3] libbpf: add selftests for " Kumar Kartikeya Dwivedi
2021-04-27 21:50   ` Andrii Nakryiko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5811eb10-bc93-0b81-2ee4-10490388f238@iogearbox.net \
    --to=daniel@iogearbox.net \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=brouer@redhat.com \
    --cc=davem@davemloft.net \
    --cc=john.fastabend@gmail.com \
    --cc=kafai@fb.com \
    --cc=kpsingh@kernel.org \
    --cc=kuba@kernel.org \
    --cc=memxor@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=songliubraving@fb.com \
    --cc=toke@redhat.com \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).