Skip to content

Add collection of ethtool queue stats#12023

Merged
bkabrda merged 6 commits into
masterfrom
bonnefoa/queue_stats_2
Sep 12, 2022
Merged

Add collection of ethtool queue stats#12023
bkabrda merged 6 commits into
masterfrom
bonnefoa/queue_stats_2

Conversation

@bonnefoa

Copy link
Copy Markdown
Collaborator

What does this PR do?

This PR was previously applied in #11056
However, this triggered segfault as the buffer's size was incorrect as _u32 element was only counted as one byte.

1 + 32 + 32 + 32 + 32 + 32 + 12 + 5
178

While the correct size should have been

4 + 32 + 32 + 32 + 32 + 32 + 12 + 5 * 4
196

A sizeof confirms it

#include <linux/ethtool.h>
#include <stdio.h>

int main (int argc, char *argv[])
{
    printf("%zu\n", sizeof(struct ethtool_drvinfo));
    return 0;
}

gcc test.c && ./a.out 
196

Motivation

This will provide per queue stats.

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

@codecov

codecov Bot commented May 13, 2022

Copy link
Copy Markdown

Codecov Report

Merging #12023 (5d478cd) into master (54ef7cf) will increase coverage by 0.04%.
The diff coverage is 95.21%.

Flag Coverage Δ
network 84.63% <95.21%> (+6.74%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

truthbk
truthbk previously approved these changes May 13, 2022

@truthbk truthbk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fix looks good and should avoid the segfault, also, the fix is portable given that u32 is not ever going to have a greater width than 4.

@bonnefoa
bonnefoa force-pushed the bonnefoa/queue_stats_2 branch 2 times, most recently from 45a67fb to e4eb726 Compare May 30, 2022 11:37
Comment thread network/datadog_checks/network/network.py Outdated
Comment thread network/datadog_checks/network/network.py
bkabrda
bkabrda previously approved these changes Jun 14, 2022

@bkabrda bkabrda left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I reviewed the changes from #11056 and ensured that the size of the struct is correct now - everything seems fine, approved.

@github-actions

Copy link
Copy Markdown
Contributor

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@bonnefoa
bonnefoa force-pushed the bonnefoa/queue_stats_2 branch from 0dfe667 to d494615 Compare August 16, 2022 15:23
@bonnefoa
bonnefoa force-pushed the bonnefoa/queue_stats_2 branch from d494615 to 5d478cd Compare August 18, 2022 08:36
@bkabrda
bkabrda merged commit 6efebab into master Sep 12, 2022
@bkabrda
bkabrda deleted the bonnefoa/queue_stats_2 branch September 12, 2022 14:14
steveny91 pushed a commit that referenced this pull request Oct 27, 2022
* Revert "Revert "Add collection of ethtool queue stats (#11056)" (#12017)"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants