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

Dynamically load/unload xdp dll #3770

Merged
merged 2 commits into from
Jul 21, 2023
Merged

Dynamically load/unload xdp dll #3770

merged 2 commits into from
Jul 21, 2023

Conversation

mtfriesen
Copy link
Contributor

Description

Use XdpLoadApi which uses run-time linking instead of XdpOpenApi which uses load-time linking.

Testing

CI/CD

Documentation

No.

@mtfriesen mtfriesen requested a review from a team as a code owner July 21, 2023 17:20
@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #3770 (700c12d) into main (69784e2) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3770      +/-   ##
==========================================
- Coverage   75.59%   75.56%   -0.04%     
==========================================
  Files          56       56              
  Lines       15554    15554              
==========================================
- Hits        11758    11753       -5     
- Misses       3796     3801       +5     

see 8 files with indirect coverage changes

csujedihy
csujedihy previously approved these changes Jul 21, 2023
@ProjectsByJackHe
Copy link
Contributor

Out of curiosity, why is run-time linking better than load time linking in our context? From some light research, with load time linking you can bundle everything into a self-contained executable and performance wise load time linking is a bit better (correct me if I am wrong). Happy to learn something new!

@csujedihy csujedihy dismissed their stale review July 21, 2023 17:49

wait, build jobs failed.

@mtfriesen
Copy link
Contributor Author

Out of curiosity, why is run-time linking better than load time linking in our context? From some light research, with load time linking you can bundle everything into a self-contained executable and performance wise load time linking is a bit better (correct me if I am wrong). Happy to learn something new!

Yup, absolutely, load time linking can perform more performance optimizations and some of them are quite valuable, especially with Spectre mitigations. The reason we want run-time linking is to (eventually) allow MsQuic to use XDP if it happens to be installed and available on the system, and otherwise use regular sockets. If the XDP DLL isn't present on the system, then load-time linking will prevent the MsQuic DLL from loading, which we do not want.

@ProjectsByJackHe
Copy link
Contributor

Out of curiosity, why is run-time linking better than load time linking in our context? From some light research, with load time linking you can bundle everything into a self-contained executable and performance wise load time linking is a bit better (correct me if I am wrong). Happy to learn something new!

Yup, absolutely, load time linking can perform more performance optimizations and some of them are quite valuable, especially with Spectre mitigations. The reason we want run-time linking is to (eventually) allow MsQuic to use XDP if it happens to be installed and available on the system, and otherwise use regular sockets. If the XDP DLL isn't present on the system, then load-time linking will prevent the MsQuic DLL from loading, which we do not want.

Ahh I see, thanks! So in the past, we just always assumed XDP was always available, otherwise MsQuic.dll wouldn't have been able to load.

@nibanks nibanks added the Area: Lola Related to low latency work label Jul 21, 2023
@mtfriesen mtfriesen merged commit 43c2931 into main Jul 21, 2023
416 of 423 checks passed
@mtfriesen mtfriesen deleted the mfriesen/xdp_load_api branch July 21, 2023 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Lola Related to low latency work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants