Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support detaching xdp_link-based attachments fix #361 #362

Merged
merged 2 commits into from
Sep 21, 2023

Conversation

msherif1234
Copy link
Contributor

@msherif1234 msherif1234 commented Sep 20, 2023

supporting force detach for link based attachments using xdp-loader unload -a <intf> fixes #361

@msherif1234
Copy link
Contributor Author

/assign @tohojo

@msherif1234 msherif1234 force-pushed the fix_xdp_loader branch 3 times, most recently from e1d2153 to 5a17836 Compare September 20, 2023 19:06
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.

The basic functionality is basically there, nice job! Just a whole bunch of nits :)

Also, it would be good to add a selftest for this; could you please write add a small test utility in libxdp/tests that does something like:

link = bpf_link_attach(ifindex);
mp = xdp_multiprog__get_from_ifindex(ifindex);
xdp_multiprog__detach(mp);
bpf_link_close(link);

You can look at some of the other examples in the tests/ directory for how to execute the tests from the test script :)

lib/libxdp/libxdp.c Outdated Show resolved Hide resolved
lib/libxdp/libxdp.c Outdated Show resolved Hide resolved
lib/libxdp/libxdp.c Outdated Show resolved Hide resolved
lib/libxdp/libxdp.c Show resolved Hide resolved
lib/libxdp/libxdp.c Show resolved Hide resolved
lib/libxdp/libxdp.c Outdated Show resolved Hide resolved
lib/libxdp/libxdp.c Outdated Show resolved Hide resolved
lib/libxdp/libxdp.c Outdated Show resolved Hide resolved
lib/libxdp/libxdp.c Outdated Show resolved Hide resolved
lib/libxdp/libxdp.c Outdated Show resolved Hide resolved
@tohojo tohojo self-assigned this Sep 21, 2023
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 more nits, but we're getting there :)

lib/libxdp/libxdp.c Show resolved Hide resolved
lib/libxdp/libxdp.c Outdated Show resolved Hide resolved
lib/libxdp/libxdp.c Outdated Show resolved Hide resolved
@msherif1234 msherif1234 force-pushed the fix_xdp_loader branch 6 times, most recently from 033563e to e3b6a88 Compare September 21, 2023 14:55
@msherif1234
Copy link
Contributor Author

msherif1234 commented Sep 21, 2023

make run
Executing tests in separate net- and mount namespaces
    Running tests from ./test-libxdp.sh
     [test_link_so]                PASS
     [test_link_a]                 PASS
     [test_old_dispatcher]         PASS
     [test_xdp_frags]              PASS
     [test_xsk_prog_refcnt_bpffs]  PASS
     [test_xsk_prog_refcnt_legacy] PASS
     [test_xsk_non_privileged]     SKIPPED
     [test_link_dispatch]          PASS

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.

Only a few style nits and some missing cleanup, otherwise the test LGTM.

lib/libxdp/tests/test_link_dispatch.c Outdated Show resolved Hide resolved
lib/libxdp/tests/test_link_dispatch.c Outdated Show resolved Hide resolved
lib/libxdp/tests/test-libxdp.sh Show resolved Hide resolved
exit(EXIT_FAILURE);
}

static int check_link_detach(int ifindex) {
Copy link
Member

Choose a reason for hiding this comment

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

The indentation is off in this and the main() function below. We use a single tab character per indentation level everywhere.

lib/libxdp/tests/test_link_dispatch.c Outdated Show resolved Hide resolved
lib/libxdp/tests/test_link_dispatch.c Outdated Show resolved Hide resolved
@msherif1234 msherif1234 requested a review from tohojo September 21, 2023 19:16
@msherif1234 msherif1234 force-pushed the fix_xdp_loader branch 4 times, most recently from d183e41 to 651960d Compare September 21, 2023 19:41
When explicitly detaching from an interface we should also handle bpf_link-based
attachments (by force-detaching the link).

Fixes xdp-project#361.

Signed-off-by: Mohamed Mahmoud <[email protected]>
Signed-off-by: Mohamed Mahmoud <[email protected]>
[ fixup whitespace, rename test from link_dispatch to link_detach ]
Signed-off-by: Toke Høiland-Jørgensen <[email protected]>
@tohojo tohojo merged commit e4ba50e into xdp-project:master Sep 21, 2023
5 checks passed
@msherif1234 msherif1234 deleted the fix_xdp_loader branch September 24, 2023 16:43
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.

Support force-detaching xdp_link-based attachments
2 participants