Xsk migration with multiprog support #92

magnus-karlsson · 2021-02-15T10:37:57Z

Pushed a whole new branch. Rewrote the handling of loading and manipulating the default xsk program using your xdp_program__* APIs and fixed all your other comments. Also updated with the latest changes from the Linux repo version.

Make the print functions in libxdp.c visible to other files by putting them in a new header file called libxdp_internal.h. These print functions can then be used by the new AF_XDP support that will be added in the next commit. Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>

Add AF_XDP functionality to libxdp. This is the same functionality as in libbpf in the kernel tree at the moment. The plan is to remove this from libbpf before the 1.0 release and for libxdp to be its new home. Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>

Add documentation for the AF_XDP socket support in libxdp. Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>

tohojo

A few comments; biggest issue is probably the race condition / locking thing.

Also, did you have a chance to test if using the XDP dispatcher has any measurable performance overhead? :)

tohojo · 2021-02-15T13:10:00Z

lib/libxdp/Makefile

 PC_FILE := $(OBJDIR)/libxdp.pc
 TEMPLATED_SOURCES := xdp-dispatcher.c

-CFLAGS += -I$(HEADER_DIR) -I$(LIB_DIR)/util
+CFLAGS += -I$(HEADER_DIR) -I$(LIB_DIR)/util -O0 -g


Why is this needed?

It is not ;-). Just garbage left after a debug session.

tohojo · 2021-02-15T13:14:51Z

lib/libxdp/xsk.c

+	int ifindex;
+	struct list_head list;
+	struct xdp_program *xdp_prog;
+	int prog_fd;


Why have both? It's possible to build an xdp_program from an fd (xdp_program__from_fd()), so just keeping the xdp_prog pointer saves branching (see comments below).

tohojo · 2021-02-15T13:21:06Z

lib/libxdp/xsk.c

+	close(ctx->xsks_map_fd);
+}
+
+static int xsk_lookup_bpf_map(struct xsk_socket *xsk)


Looking at this function I wonder if it would make sense to have libxdp parse the list of maps in xdp_program__fill_from_fd() so they are always available to callers? A bit orthogonal to this patch, but may be generally useful? WDYT?

Might make sense, but if this is never performance critical it might not make sense to waste the memory. In my case, performance is not an issue.

tohojo · 2021-02-15T13:21:53Z

lib/libxdp/xsk.c

+			continue;
+		}
+
+		if (!strcmp(map_info.name, "xsks_map")) {


I am a little worried about forward compatibility here; what if we need to change the layout of xsks_map in the future? I think we should at least checking that the key and value size matches what we expect...

Good point. Will fix.

tohojo · 2021-02-15T13:29:27Z

lib/libxdp/xsk.c

+
+	if (!prog)
+		return -1;
+	return xdp_program__fd(prog);


This leaks the multi_prog reference... Given what I said above re: just keeping the xdp_program reference, maybe the right thing here is to have this function return an xdp_program pointer, and expose xdp_program__clone() so it can still close the multiprog...

I will make a stab at just using the program pointer and make sure to destroy it when necessary.

tohojo · 2021-02-15T13:35:08Z

lib/libxdp/xsk.c

+	xdp_program__detach(ctx->xdp_prog, ctx->ifindex,
+			    xsk_convert_xdp_flags(xsk->config.xdp_flags), 0);
+err_prog_load:
+	xdp_program__close(ctx->xdp_prog);


This should clear the pointer as well.

tohojo · 2021-02-15T13:36:12Z

lib/libxdp/xsk.c

+
+	ctx->prog_fd = -1;
+
+	if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) {


This flag is not mentioned in the documentation :)

Oops! Will add.

tohojo · 2021-02-15T13:37:03Z

lib/libxdp/xsk.c

+		if (ctx->xdp_prog)
+			xdp_program__close(ctx->xdp_prog);
+		else
+			close(ctx->prog_fd);


If we just always store ctx->xdp_prog this disambiguation is not needed anymore...

tohojo · 2021-02-15T13:40:45Z

lib/libxdp/xsk.c

+		return -1;
+
+	while ((prog = xdp_multiprog__next_prog(prog, multi_prog)))
+		if (!strcmp(xdp_program__name(prog), "xsk_def_prog"))


This is also a potential forward-compatibility concern. If we ever need to change the default program in an incompatible way, we'd get subtle breakage when mixing library versions. I put a version check into the dispatcher program for just this reason - see check_dispatcher_version(), and feel free to just copy that (or better, refactor it to be reusable for this :))

Good point. Will steal your code ;-).

tohojo · 2021-02-15T13:49:39Z

lib/libxdp/xsk.c

+	return xsk_add_map_entry(xsk);
+}
+
+int xsk_setup_xdp_prog(int ifindex, int *xsks_map_fd)


How is the application supposed to tear down the XDP prog again? Or rather, how is an application supposed to do this without racing against another program trying to install itself? Since we abstract away the creation and attachment of the XDP program, I think we need to handle the race condition as well...

We do already have a locking mechanism (in xdp_lock_acquire/release()) we could use, but a bit of care is needed to prevent deadlocking with outselves (xdp_program__attach() will grab the lock). So may need a bit more knowledge of the internals of the rest of the library... WDYT?

I have kept all locking outside the library by design. I think this is best left up to the user to fix with a synchronization primitive of its choosing which might even be none with certain designs.

For synchronisation inside an application I agree that we shouldn't impose any particular locking. But this also has to do with synchronisation between applications, that don't necessarily have any way of coordinating, other than libxdp. So I think solving this in a race-free way is in scope; it's basically the same issue as Maciej's patch[0] is addressing in the libbpf version of this code.

[0] https://lore.kernel.org/bpf/3a625c8b-6dc0-3933-25e5-f747197ae1f4@intel.com/T/#t

magnus-karlsson · 2021-02-15T14:11:48Z

A few comments; biggest issue is probably the race condition / locking thing.

I think we should not implement any locking as discussed above. Let the user do this if it needs it.

Also, did you have a chance to test if using the XDP dispatcher has any measurable performance overhead? :)

No, I have not done this yet. I take an action to do it.

magnus-karlsson · 2021-02-16T07:10:56Z

On Mon, Feb 15, 2021 at 6:02 PM Toke Høiland-Jørgensen < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/libxdp/xsk.c <#92 (comment)>: > + return ctx; +} + +static void xsk_destroy_xsk_struct(struct xsk_socket *xsk) +{ + free(xsk->ctx); + free(xsk); +} + +int xsk_socket__update_xskmap(struct xsk_socket *xsk, int fd) +{ + xsk->ctx->xsks_map_fd = fd; + return xsk_add_map_entry(xsk); +} + +int xsk_setup_xdp_prog(int ifindex, int *xsks_map_fd) For synchronisation inside an application I agree that we shouldn't impose any particular locking. But this also has to do with synchronisation *between* applications, that don't necessarily have any way of coordinating, other than libxdp. So I think solving this in a race-free way is in scope; it's basically the same issue as Maciej's patch[0] is addressing in the libbpf version of this code. [0] ***@***.***/T/#t

This was never in scope for the libbpf implementation, but I agree that we should provide this with libxdp if it is successfully going to serve as this coordination point that the original code just did not offer. But I consider this outside of this patch set. It is something we should add as a follow up IMHO. /Magnus

…

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#92 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASGUEM3JRB2GNM5NNZLWMDS7FHT7ANCNFSM4XUOHZPQ> .

tohojo · 2021-02-16T10:02:07Z

Magnus Karlsson <notifications@github.com> writes:

This was never in scope for the libbpf implementation, but I agree that we should provide this with libxdp if it is successfully going to serve as this coordination point that the original code just did not offer. But I consider this outside of this patch set. It is something we should add as a follow up IMHO.

OK, fair enough, we can defer this to after merge. I opened an issue so we don't forget: #93

magnus-karlsson · 2021-03-23T10:25:48Z

Pushed a new version/branch

tohojo · 2021-03-23T10:42:29Z

Magnus Karlsson ***@***.***> writes:

Pushed a new version/branch

You know you can just force-push to the same source branch and github will update the PR, right? :)

magnus-karlsson · 2021-03-23T12:24:02Z

On Tue, Mar 23, 2021 at 11:42 AM Toke Høiland-Jørgensen ***@***.***> wrote: Magnus Karlsson ***@***.***> writes: > Pushed a new version/branch You know you can just force-push to the same source branch and github will update the PR, right? :)

Yes :-). Just did not know what you preferred. In this case, a force push would have been warranted as it is largely a rewrite from scratch. Threw away my changes and started with a clean libbpf, since I was not happy with the result last time. Will force push new versions on top of this one.

…

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or unsubscribe.

magnus-karlsson added 3 commits February 15, 2021 10:34

libxdp: add documentation for AF_XDP functions

fab4d78

Add documentation for the AF_XDP socket support in libxdp. Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>

tohojo mentioned this pull request Feb 15, 2021

Xsk migration to libxdp #90

Closed

tohojo requested changes Feb 15, 2021

View reviewed changes

magnus-karlsson closed this Mar 23, 2021

tohojo deleted the xsk_migration3 branch February 18, 2022 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Xsk migration with multiprog support #92

Xsk migration with multiprog support #92

magnus-karlsson commented Feb 15, 2021

tohojo left a comment

tohojo Feb 15, 2021

magnus-karlsson Feb 15, 2021

tohojo Feb 15, 2021

tohojo Feb 15, 2021

magnus-karlsson Feb 15, 2021

tohojo Feb 15, 2021

magnus-karlsson Feb 15, 2021

tohojo Feb 15, 2021

magnus-karlsson Feb 15, 2021

tohojo Feb 15, 2021

magnus-karlsson Feb 15, 2021

tohojo Feb 15, 2021

magnus-karlsson Feb 15, 2021

tohojo Feb 15, 2021

tohojo Feb 15, 2021

magnus-karlsson Feb 15, 2021

tohojo Feb 15, 2021

magnus-karlsson Feb 15, 2021

tohojo Feb 15, 2021

magnus-karlsson commented Feb 15, 2021

magnus-karlsson commented Feb 16, 2021 via email

tohojo commented Feb 16, 2021 via email

magnus-karlsson commented Mar 23, 2021

tohojo commented Mar 23, 2021 via email

magnus-karlsson commented Mar 23, 2021 via email


		ctx->prog_fd = -1;

		if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) {

Xsk migration with multiprog support #92

Xsk migration with multiprog support #92

Conversation

magnus-karlsson commented Feb 15, 2021

tohojo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magnus-karlsson commented Feb 15, 2021

magnus-karlsson commented Feb 16, 2021 via email

tohojo commented Feb 16, 2021 via email

magnus-karlsson commented Mar 23, 2021

tohojo commented Mar 23, 2021 via email

magnus-karlsson commented Mar 23, 2021 via email