Basic04 lesson code brush up #463

noguxun · 2025-09-20T09:08:39Z

Implemented TODO: unloading & unpinning
Reimplemented the assignment 2: map use, according to the readme

tohojo · 2025-09-22T15:30:18Z

basic-solutions/xdp_loader.c

+	*map_reused = false;
+
+	/* 1) First, try to reuse maps by opening object and reusing before libxdp creates it */
+	struct bpf_object *obj = NULL;


Please keep all variable declarations at the top of the function

basic-solutions/xdp_loader.c

tohojo · 2025-09-26T09:44:18Z

basic-solutions/xdp_loader.c

 		}
 	}

 	return EXIT_OK;


Even though the program exits, it is good practice to explicitly clean up resources before exiting. So please add xdp_program__close() and bpf_object__close() calls before returning :)

(And yeah, I realise this bug was already there for xdp_program, but let's use this opportunity to fix it)

Good suggestion, added the close of resources in the new push.

Cool! Except this is not hit in the error path of the !map_reused branch. That return needs to be replaced with a goto out, and the final return should be using the value of err.

tohojo · 2025-09-26T09:44:42Z

basic04-pinning-maps/xdp_loader.c

 		return err;
 	}

 	return EXIT_OK;


...and same thing here wrt returning without an explicit close.

Did the same in the new push, Thanks!

Yup, same comment about the error branch, though :)

tohojo

A few error branches that are not handled correctly; and a comment on restructuring the loading code that would make things simpler...

tohojo · 2025-09-29T10:20:54Z

basic-solutions/xdp_loader.c

+	err = xdp_program__attach(prog, cfg->ifindex, cfg->attach_mode, 0);
+	if (err) {
+		fprintf(stderr, "ERR: xdp_program__attach: %s\n", strerror(-err));
+		xdp_program__close(prog);


This doesn't close the bpf_object.

Thinking about this some more, separating out the opening of the BPF object seems to just be unneeded complexity; we might as well just open the object through libxdp, then access it through the xdp_program pointer afterwards. That simplifies the state handling because the xdp_program will own the bpf_object reference, and we only need to keep track of one thing.

So something like:

DECLARE_LIBXDP_OPTS(xdp_program_opts, xdp_opts, .prog_name = cfg->progname, .open_filename = cfg->filename); prog = xdp_program__create(&xdp_opts); if (!prog) return -errno; map = bpf_object__find_map_by_name(xdp_program__bpf_obj(prog), map_name)); if (!map) { fprintf(stderr, "Map %s not found!\n", map_name); xdp_program__close(prog); return -ENOENT; } // reuse map err = xdp_program__attach(...). // etc

When the line below is executed, my understanding is that xdp has created the BPF object and it's map.

DECLARE_LIBXDP_OPTS(xdp_program_opts, xdp_opts, .prog_name = cfg->progname, .open_filename = cfg->filename); prog = xdp_program__create(&xdp_opts);

Then we try to reuse the pinned map. What will happen to the newly created map (with the BPF object)?

or my understanding is wrong. bpf's map creation actually happens when xdp_program__attach is called?

There are some comments at

xdp-tutorial/common/common_user_bpf_xdp.c

Lines 117 to 121 in 004feab

/* At this point: BPF-progs are (only) loaded by the kernel, and prog_fd

* is our select file-descriptor handle. Next step is attaching this FD

* to a kernel hook point, in this case XDP net_device link-level hook.

*/

err = xdp_program__attach(prog, cfg->ifindex, cfg->attach_mode, 0);

It sounds like the BPF object is already loaded before the XDP attach is called.
But I checked the libxdp code, it seems BPF load is called in the XDP attach, not the create.

If your suggestion works (which means the BPF object is loaded in the attachment), this comment needs to be fixed.

Yeah, looks like that comment is left-over from before the conversion to libxdp. At that point it was accurate, today it's wrong (i.e., libxdp doesn't load the bpf_object into the kernel until xdp_program__attach() is called)

@tohojo , Thanks for the review.
I have a new push to the PR, according to your review.

tohojo · 2025-09-29T10:22:17Z

basic-solutions/xdp_loader.c

 		}
 	}

 	return EXIT_OK;


Cool! Except this is not hit in the error path of the !map_reused branch. That return needs to be replaced with a goto out, and the final return should be using the value of err.

tohojo · 2025-09-29T10:22:41Z

basic04-pinning-maps/xdp_loader.c

 		return err;
 	}

 	return EXIT_OK;


Yup, same comment about the error branch, though :)

tohojo · 2025-10-02T09:36:10Z

basic-solutions/xdp_loader.c

-	/* Initialize the pin_dir configuration */
-	len = snprintf(cfg.pin_dir, 512, "%s/%s", pin_basedir, cfg.ifname);
+	/* Try to reuse existing pinned maps before loading */
+	len = snprintf(path, PATH_MAX, "%s/%s", pin_basedir, cfg.ifname);


Why change this? You're adding a bunch of code duplication by building the path multiple times instead of just reusing cfg->pin_dir everywhere as the code was doing before...

It is because I want to implement the solution based on basic04 xdp_loader.c , in which cfg.pin_dir is not implemented. Also it looks like there are a lot of changes in pin_maps_in_bpf_object function in the solution folder. But actually, it is the same as what is in basic04.

But I do think implementation in basic-solution is better than basic04.
Maybe I can merge basic-solution's implementation to basic04.

They don't have to be completely identical, just enough that one can go look at the solution if one gets stuck on completing the assignment. So something like the use of the config struct is fine to divert on...

this should be fixed in the new push.

tohojo · 2025-10-02T09:36:33Z

basic-solutions/xdp_loader.c

-	 "Unload XDP program instead of loading"},
-
-	{{"reuse-maps",  no_argument,		NULL, 'M' },
-	 "Reuse pinned maps"},


Why remove the reuse-maps option?

Current version's reuse-map is used for map pinning.
This does not look like what is needed for the assignment 2.
So I delete it completely as the assignment does not mention to create a on/off switch.

if (!cfg.reuse_maps) { err = pin_maps_in_bpf_object(xdp_program__bpf_obj(program), &cfg); if (err) { fprintf(stderr, "ERR: pinning maps\n"); return err; } }

If a flag is needed, I can add it back and put it around map-reuse code

I think the intention of the option is: If the option set, try to reuse maps if they already exist, and if not, pin them. If the option is not set, do neither (no reuse, no pinning)...

That make sense!
I just had a new push to implement above logic. Thanks for the explanation.

…rame unloading Signed-off-by: Xun Gu <[email protected]>

tohojo requested changes Sep 22, 2025

View reviewed changes

tohojo mentioned this pull request Sep 22, 2025

Question about the solution of basic-04 assignment 2 #464

Open

noguxun force-pushed the basic04 branch from a26a87b to 1d8357d Compare September 25, 2025 05:15

tohojo requested changes Sep 26, 2025

View reviewed changes

noguxun force-pushed the basic04 branch from 1d8357d to 31ebcce Compare September 27, 2025 02:11

tohojo requested changes Sep 29, 2025

View reviewed changes

noguxun force-pushed the basic04 branch 3 times, most recently from b3c40d8 to 1e57b42 Compare October 2, 2025 05:29

tohojo requested changes Oct 2, 2025

View reviewed changes

noguxun force-pushed the basic04 branch from 1e57b42 to 0c3db62 Compare October 5, 2025 01:43

basic04-pinning-maps: Implementation Assignment2, map unpinning, prog…

299b6d4

…rame unloading Signed-off-by: Xun Gu <[email protected]>

noguxun force-pushed the basic04 branch from 0c3db62 to 299b6d4 Compare October 5, 2025 01:48

	/* At this point: BPF-progs are (only) loaded by the kernel, and prog_fd
	* is our select file-descriptor handle. Next step is attaching this FD
	* to a kernel hook point, in this case XDP net_device link-level hook.
	*/
	err = xdp_program__attach(prog, cfg->ifindex, cfg->attach_mode, 0);

Basic04 lesson code brush up #463

Are you sure you want to change the base?

Basic04 lesson code brush up #463

Uh oh!

Conversation

noguxun commented Sep 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tohojo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!