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

net: replace the ethtool external program calls with the safchain/ethtool package #321

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

net: replace the ethtool external program calls with the safchain/ethtool package #321

wants to merge 2 commits into from

Conversation

ffromani
Copy link
Collaborator

@ffromani ffromani commented May 10, 2022 •
edited
Loading

replace the ethtool invocation and related output parsing with the ethtool package which can now provide the same information natively (e.g. the package will NOT call the binary bug it will query the kernel)

Fixes: #317

@ffromani
Copy link
Collaborator Author

test failures are intentional (!!!) to make sure we don't merge without explicit action!

@ffromani
Copy link
Collaborator Author

proposed PR: safchain/ethtool#49

@jak3kaj
Copy link
Contributor

jak3kaj commented May 15, 2023

I'm glad to see refactoring with the native Go ethtool library removed many package dependencies and even simplified the code!

If we decide to go this route, I can contribute a PR which refactors #338 #342 to also use github.com/safchain/ethtool.

@ffromani
Copy link
Collaborator Author

I'm glad to see refactoring with the native Go ethtool library removed many package dependencies and even simplified the code!

If we decide to go this route, I can contribute a PR which refactors #338 #342 to also use github.com/safchain/ethtool.

I'm happy there's interest in this approach! The main and only blocker here seems that the upstream project is a bit slowmoving. I'll look for other alternatives. I'm also considering a fork tailored for ghw purposes, but I'm still considering the maintainership costs.

@ffromani
Copy link
Collaborator Author

@jaypipes just wondering: would a minimal ethtool package (perhaps unexported, like in internal/?) tailored for the needs of the project be a welcome addition to ghw? I'd like to move forward here and I'm considering which options we have on the table.

@jaypipes
Copy link
Owner

@jaypipes just wondering: would a minimal ethtool package (perhaps unexported, like in internal/?) tailored for the needs of the project be a welcome addition to ghw? I'd like to move forward here and I'm considering which options we have on the table.

If there isn't a viable option out there in the open source world that is well-maintained and documented, I'd be perfectly fine doing an internal/ package, yes!

@ffromani
Copy link
Collaborator Author

@jaypipes just wondering: would a minimal ethtool package (perhaps unexported, like in internal/?) tailored for the needs of the project be a welcome addition to ghw? I'd like to move forward here and I'm considering which options we have on the table.

If there isn't a viable option out there in the open source world that is well-maintained and documented, I'd be perfectly fine doing an internal/ package, yes!

perfect, my thoughts exactly. Let me try harder not to have this internal package if we can help it.

@ffromani
Copy link
Collaborator Author

proposed PR: safchain/ethtool#49

merged! happy to resume the work on this PR!

@ffromani ffromani force-pushed the ethtool-package branch 2 times, most recently from 7133a50 to f1141d3 Compare June 11, 2024 16:25
@ffromani
Copy link
Collaborator Author

WIP just because I want to add more tests. Other than that the PR is fully reviewable

@ffromani ffromani changed the title WIP: net: replace "ethtool" with a package WIP: net: replace the ethtool external program calls with the safchain/ethtool package Jun 11, 2024
@ffromani ffromani changed the title WIP: net: replace the ethtool external program calls with the safchain/ethtool package net: replace the ethtool external program calls with the safchain/ethtool package Jun 14, 2024
@ffromani
Copy link
Collaborator Author

ready for review

jaypipes
jaypipes reviewed Jun 14, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
pkg/net/net_linux.go Show resolved Hide resolved
pkg/net/net_linux.go
Comment on lines -147 to -162
// Features for enp58s0f1:
// rx-checksumming: on
// tx-checksumming: off
// tx-checksum-ipv4: off
// tx-checksum-ip-generic: off [fixed]
// tx-checksum-ipv6: off
// tx-checksum-fcoe-crc: off [fixed]
// tx-checksum-sctp: off [fixed]
// scatter-gather: off
// tx-scatter-gather: off
// tx-scatter-gather-fraglist: off [fixed]
// tcp-segmentation-offload: off
// tx-tcp-segmentation: off
// tx-tcp-ecn-segmentation: off [fixed]
// tx-tcp-mangleid-segmentation: off
// tx-tcp6-segmentation: off
Copy link
Owner

Choose a reason for hiding this comment

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

Are the capability strings exactly the same with the ethtool library as they were with the raw parsing of the ethtool binary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right, pretty bad oversight on my side. Let me add a test which ensure this. On the code side, if this is not already the case, we can add a thin translation layer.

Copy link
Owner

Choose a reason for hiding this comment

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

@ffromani I was only making sure :) it may be that the ethtool library does use the exact same strings. but if not, we'll need to add a translation table to ensure backwards compat.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, so the ethtool program gets the identifiers from the kernel using the ioctl interface: https://kernel.googlesource.com/pub/scm/network/ethtool/ethtool/+/refs/heads/ethtool-3.4.y/ethtool.c#1310 https://docs.kernel.org/networking/ethtool-netlink.html#strset-get

And so it does the ethtool package: https://github.com/safchain/ethtool/blob/master/ethtool.go#L733

Hence the names should be exactly the same, unless it is the ethtool program which does some translation, which I haven't checked yet. On it.

@ffromani
Copy link
Collaborator Author

ffromani commented Jun 17, 2024 •
edited
Loading

ok, so the tool uses more than a single ioctl to dump all the information (this is ethtool -k , same flag ghw uses)

$ diff -Naur /tmp/prog.txt /tmp/pkg.txt 
--- /tmp/prog.txt	2024-06-17 08:24:23.342381113 +0200
+++ /tmp/pkg.txt	2024-06-17 08:24:31.884461500 +0200
@@ -2,8 +2,6 @@
 esp-tx-csum-hw-offload: off [fixed]
 fcoe-mtu: off [fixed]
 Features for lo:
-generic-receive-offload: on
-generic-segmentation-offload: on
 highdma: on [fixed]
 hsr-dup-offload: off [fixed]
 hsr-fwd-offload: off [fixed]
@@ -11,25 +9,24 @@
 hsr-tag-rm-offload: off [fixed]
 hw-tc-offload: off [fixed]
 l2-fwd-offload: off [fixed]
-large-receive-offload: off [fixed]
 loopback: on [fixed]
 macsec-hw-offload: off [fixed]
 netns-local: on [fixed]
-ntuple-filters: off [fixed]
-receive-hashing: off [fixed]
 rx-all: off [fixed]
-rx-checksumming: on [fixed]
+rx-checksum: on [fixed]
 rx-fcs: off [fixed]
 rx-gro-hw: off [fixed]
 rx-gro-list: off
+rx-gro: on
+rx-hashing: off [fixed]
+rx-lro: off [fixed]
+rx-ntuple-filter: off [fixed]
 rx-udp-gro-forwarding: off
 rx-udp_tunnel-port-offload: off [fixed]
 rx-vlan-filter: off [fixed]
-rx-vlan-offload: off [fixed]
+rx-vlan-hw-parse: off [fixed]
 rx-vlan-stag-filter: off [fixed]
 rx-vlan-stag-hw-parse: off [fixed]
-scatter-gather: on
-tcp-segmentation-offload: on
 tls-hw-record: off [fixed]
 tls-hw-rx-offload: off [fixed]
 tls-hw-tx-offload: off [fixed]
@@ -37,10 +34,10 @@
 tx-checksum-ip-generic: on [fixed]
 tx-checksum-ipv4: off [fixed]
 tx-checksum-ipv6: off [fixed]
-tx-checksumming: on
 tx-checksum-sctp: on [fixed]
 tx-esp-segmentation: off [fixed]
 tx-fcoe-segmentation: off [fixed]
+tx-generic-segmentation: on
 tx-gre-csum-segmentation: off [fixed]
 tx-gre-segmentation: off [fixed]
 tx-gso-list: on
@@ -61,7 +58,7 @@
 tx-udp-segmentation: on
 tx-udp_tnl-csum-segmentation: off [fixed]
 tx-udp_tnl-segmentation: off [fixed]
-tx-vlan-offload: off [fixed]
+tx-vlan-hw-insert: off [fixed]
 tx-vlan-stag-hw-insert: off [fixed]

@ffromani
net: replace "ethtool" with a package
90e212c 
Replace calls to the "ethtool" external binary to
the go package https://github.com/safchain/ethtool

No feature reduction is expected, internal change only.

Signed-off-by: Francesco Romani <[email protected]>
@ffromani
WIP
5988ac2 
Signed-off-by: Francesco Romani <[email protected]>
@ffromani
Copy link
Collaborator Author

I'm struggling to understand why the output is different once I force (recompile) ethtool to use ioctl, there are still some differences.
What if we keep and deprecate the ethtool binary support? we can add/change then environment variable to force the library to consume the tool, by default it will use the ioctl interface.

@jaypipes
Copy link
Owner

jaypipes commented Jul 3, 2024

I'm struggling to understand why the output is different once I force (recompile) ethtool to use ioctl, there are still some differences. What if we keep and deprecate the ethtool binary support? we can add/change then environment variable to force the library to consume the tool, by default it will use the ioctl interface.

@ffromani I'm open to any ideas you have :) If you want to try keeping the ethtool binary support and adding an environment variable switch for this new behaviour, good with me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaypipes jaypipes jaypipes left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

replace ethtool invocation with go packages
3 participants
@ffromani @jak3kaj @jaypipes