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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux