Skip to content

Conversation

@dtatulea
Copy link
Contributor

As requested by Jesper, added a egress mode for xdp-bench in redirect-map and redirect-multimap modes. This action can be forward (as before) or drop (new).

Haven't found any guidelines for contribution so please excuse me if I omitted something.

Copy link
Member

@tohojo tohojo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits, and as Jesper already pointed out on the kernel list, please update the man page as well.

I also have one reservation about the option: --egress-action will silently fail if --load-egress is not set. We should error out here instead, to avoid having silent errors that are hard to debug. Probably the easiest way to do this is to have a DEVMAP_EGRESS_NONE option in the enum that is not exposed as a visible option value, and checking that --load-egress was supplied if the option egress_action value is different from NONE...

@dtatulea dtatulea force-pushed the xdp-bench-redir-map-action-drop branch from ac80f7f to f4d3368 Compare December 7, 2025 14:33
@dtatulea
Copy link
Contributor Author

dtatulea commented Dec 7, 2025

Thanks for your review!

A few nits, and as Jesper already pointed out on the kernel list, please update the man page as well.

man page updated. LMK if you spot any issues.

I also have one reservation about the option: --egress-action will silently fail if --load-egress is not set. We should error out here instead, to avoid having silent errors that are hard to debug. Probably the easiest way to do this is to have a DEVMAP_EGRESS_NONE option in the enum that is not exposed as a visible option value, and checking that --load-egress was supplied if the option egress_action value is different from NONE...

Fixed using your suggestion. Couldn't think of a better way. For now NONE and FORWARD are equivalent after the initial check. LMK if this is confusing.

Copy link
Member

@tohojo tohojo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option handling looks good now!

But please don't edit the man page directly; edit the README.org file, then recompile the man page (should update on make, assuming you have Emacs installed).

Also, totally forgot the first time around: we'll need a selftest for this. Just a simple test to see that the new options work correctly should be fine; you can just copy and adjust one of the existing tests in test-xdp-bench.sh :)

@dtatulea
Copy link
Contributor Author

dtatulea commented Dec 8, 2025

Option handling looks good now!

Great!

But please don't edit the man page directly; edit the README.org file, then recompile the man page (should update on make, assuming you have Emacs installed).

Oh, didn't realize that the man page is generated. Will do.

Also, totally forgot the first time around: we'll need a selftest for this. Just a simple test to see that the new options work correctly should be fine; you can just copy and adjust one of the existing tests in test-xdp-bench.sh :)
Ack, will do.

@dtatulea dtatulea force-pushed the xdp-bench-redir-map-action-drop branch from f4d3368 to e0469f4 Compare December 14, 2025 12:34
Add a new `--egress-action` option for devmap and devmap-multi that
allows selecting different actions for egress.

This change only adds the existing forward action.

egress-action is initialized to DEVMAP_EGRESS_NONE to allow
for checking if the user selected `--egress-action` without
`--load-egress`.

Signed-off-by: Dragos Tatulea <[email protected]>
Simple XDP_DROP program as egress action.

Signed-off-by: Dragos Tatulea <[email protected]>
@dtatulea dtatulea force-pushed the xdp-bench-redir-map-action-drop branch from e0469f4 to c3801dd Compare December 14, 2025 12:34
Copy link
Member

@tohojo tohojo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - thanks!

@tohojo tohojo merged commit a28950b into xdp-project:main Dec 15, 2025
138 of 145 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants