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.

Sign up for GitHub

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

Jump to bottom

T6231: Mellanox OFED #665

Merged
merged 5 commits into from
Jul 25, 2024
Merged

T6231: Mellanox OFED #665

merged 5 commits into from
Jul 25, 2024

Conversation

sempervictus
Copy link

Change Summary

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

T6231

Component(s) name

Mellanox (Nvidia) OFED kernel and userspace

Proposed changes

OFED stack: drivers, userspace utilities, libraries, and DPDK/VPP interfaces.

How to test

  1. run included script in build stack
  2. deploy the debs with the kernel built from the referenced sources
  3. boot on host with ConnectX card
  4. verify which module is loaded and features available

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

@sempervictus sempervictus requested a review from a team as a code owner June 21, 2024 02:58
@sever-sever
Copy link
Member

There are unrelated to this task commits
Why do you add them?

@sempervictus
Copy link
Author

sempervictus commented Jun 21, 2024 •
edited
Loading

Git fun it seems :-/ - I just checked out -b and cherry picked my commit on it then added the Jenkinsfile one... Not sure how GH turned that into this. Rebased

@sempervictus sempervictus force-pushed the feature/mellanox_ofed_drivers branch from 97a3ead to edd5236 Compare June 21, 2024 13:01
@sempervictus sempervictus mentioned this pull request Jun 21, 2024
Merged
1 task
@zdc
Copy link
Contributor

zdc commented Jun 21, 2024

Are there any examples of specific issues with upstream drivers fixed in OFED, or important for VyOS case benefits available in OFED and not available upstream?

@sempervictus
Copy link
Author

,

Are there any examples of specific issues with upstream drivers fixed in OFED, or important for VyOS case benefits available in OFED and not available upstream?

Addresses a few things:
Ever try to get line rate comms above connectx4, hw offload for complex functions, or vendor support when using upstream? First thing nv says in the support process is to use their drivers... We were struggling to route/nat more than 40gbit on upstream, were able to get nearly 200 going through inside lacp bond through outside lacp bond on OFED on cx6 and 7+ are a whole other ballgame. Its not as bad as difference between nouveau and actual GPU drivers from nv but well worth the change despite making kCFI that much more of a challenge (they hardcode GCC, for now) since this is a NOS.

@zdc
Copy link
Contributor

zdc commented Jun 21, 2024

We were struggling to route/nat more than 40gbit on upstream, were able to get nearly 200 going through inside lacp bond through outside lacp bond on OFED on cx6 and 7+

Can you confirm that in the described case a driver replacement was the only difference? The same device, same connections, same performance tester, same configuration, just another driver?

I have no objections against adding OFED drivers, but we need to understand the real profit from this.

@sempervictus
Copy link
Author

Yes, same device rebooted to an image containing ofed

T6231: Mellanox OFED Kernel and Userspace Packages
c0365df 
Build OFED drivers and userspace components against the kernel
source tree similar to Intel's NIC drivers.

OFED installers create Debian packages of their own tageting the
kernel version defined in the build invocation if DKMS is omitted.
Script builds with supporting components for VPP to permit handoff
of function to the underlying hardware as appropriate. Updating the
version is fairly trivial along with adding patching as needed to
handle kCFI and hardening measures as they are introduced.

Testing:
  Tested against GCC-built Linux Hardened kernel with the various
additions from PR 132 - sustained line-rate testing against 4x100g
links on a single machine at a hair below 200g for each LACP pair.
@sempervictus sempervictus force-pushed the feature/mellanox_ofed_drivers branch from edd5236 to c0365df Compare June 22, 2024 02:45
c-po
c-po requested changes Jun 23, 2024
View reviewed changes
Copy link
Member

@c-po c-po left a comment

Choose a reason for hiding this comment

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

Can both drivers co-exist or does the stock driver needs to be removed from the Kernel?

How does the system determine which to load? Or dies it go the usual way in detecting "there is a newer driver in /lib/modules/6.6.35/updates/dkms?

Issue: -amd64-vyos is missing from the installation path of the new modules, see KERNEL_SUFFIX environment variable also present in packages/linux-kernel/kernel-vers

@sempervictus
Copy link
Author

Can both drivers co-exist or does the stock driver needs to be removed from the Kernel?

both can coexist and you can insmod by path if you want to use the upstream ones.

How does the system determine which to load? Or dies it go the usual way in detecting "there is a newer driver in /lib/modules/6.6.35/updates/dkms?

the updates paths are how DKMS and such do this as well - upstream Linux semantic

Issue: -amd64-vyos is missing from the installation path of the new modules, see KERNEL_SUFFIX environment variable also present in packages/linux-kernel/kernel-vers

i havent found this to be an issue on any distribution over the ages and all of our kernels on all of our own OS forks and upstream ones utilize suffixes while DKMS and many out-of-tree drivers don't. Remedy would require changing the vendors source which is doable, but adds hassle/maintenance burden as their code changes rev to rev.

On that note, the suffix thing is rather constraining as implemented - we use different suffixes to denote specific kernel build types and tag our images with them (LLVM+hardened, GCC+hardened, GCC+grsec/pax, etc) because we cannot build an image with multiple kernels to let the user choose what to boot. IIRC this was was one of the big blockers to #132 - we couldn't get people to test anything to figure out what and how we should implement. I do have a much better "piecemeal it in" strategy for that code moving forward (actually been testing with this PR in our builders - linux-hardened and the xtables stuff drops in nicely now though i think we should just pull from linux-hardened for the source tree and adjust the version tag) but we still have to build dedicated images with those kernels as opposed to making a boot option (in part due to how the build scripts generate initramfs)

c-po added 4 commits July 25, 2024 15:30
@c-po
Kernel: T6231: update .gitignore for mellanox drivers
9cbc700
@c-po
Kernel: T6231: verify mellanox driver source by SHA1 hash
0ce12e4
@c-po
mellanox: T6231: add missing KERNEL_SUFFIX for module installation path
404fec7 
All VyOS kernel modules must live in the appropriate module directory,
example: /lib/modules/6.6.41-amd64-vyos/

In addition we do not abbreviate script options to make reading easier,
without call --help all the time.
@c-po
mellanox: T6231: we do not need to build all packages, delete all unu…
f33a22b 
…sed ones
@c-po
Copy link
Member

c-po commented Jul 25, 2024

Moved drivers to appropriate directory and added some other minor changes to this pr

-rw-r--r-- root/root 5218104 2012-12-31 13:38 ./lib/modules/6.6.41-amd64-vyos/updates/dkms/mlx5_core.ko

@github-actions GitHub Actions
Copy link

👍
No issues in PR Title / Commit Title

@c-po c-po enabled auto-merge July 25, 2024 13:37
@c-po c-po merged commit 02379d3 into vyos:current Jul 25, 2024
8 of 9 checks passed
@c-po c-po mentioned this pull request Jul 25, 2024
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c-po c-po c-po approved these changes

@zdc zdc zdc approved these changes

@dmbaturin dmbaturin Awaiting requested review from dmbaturin dmbaturin is a code owner automatically assigned from vyos/reviewers

@sarthurdev sarthurdev Awaiting requested review from sarthurdev sarthurdev is a code owner automatically assigned from vyos/reviewers

@jestabro jestabro Awaiting requested review from jestabro jestabro is a code owner automatically assigned from vyos/reviewers

@sever-sever sever-sever Awaiting requested review from sever-sever sever-sever is a code owner automatically assigned from vyos/reviewers

Assignees

@sempervictus sempervictus

Labels
current
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sempervictus @sever-sever @zdc @c-po