jaypipes / ghw

Go HardWare discovery/inspection library
Apache License 2.0
1.62k stars 174 forks source link

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

Open ffromani opened 2 years ago

ffromani commented 2 years ago

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: https://github.com/jaypipes/ghw/issues/317

ffromani commented 2 years ago

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

ffromani commented 2 years ago

proposed PR: https://github.com/safchain/ethtool/pull/49

jak3kaj commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 2 months ago

proposed PR: safchain/ethtool#49

merged! happy to resume the work on this PR!

ffromani commented 2 months ago

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

ffromani commented 2 months ago

ready for review

ffromani commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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!