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

Add Kernel Misc Probe #17541

Merged
merged 1 commit into from Oct 14, 2021
Merged

Add Kernel Misc Probe #17541

merged 1 commit into from Oct 14, 2021

Conversation

vincentmli
Copy link
Contributor

@vincentmli vincentmli commented Oct 5, 2021 •
edited

Add Kernel Misc Probe

Add kernel misc feature probe which include kernel BPF large instruction
support information. It allows Cilium features to probe kernel large instruction

Signed-off-by: Vincent Li vincent.mc.li@gmail.com

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!
Add Kernel Misc Probe

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 5, 2021
@vincentmli
Copy link
Contributor Author

@pchaigno @joestringer I created this PR for kernel large instruction probe to warn in the log if kernel not supporting large instructions

level=info msg="clang (10.0.0) and kernel (4.9.282) versions: OK!" subsys=linux-datapath
level=info msg="linking environment: OK!" subsys=linux-datapath
level=warning msg="BPF check: NOT OK" error="Kernel does not support large 1M instructions)" subsys=linux-datapath

@joestringer joestringer mentioned this pull request Oct 12, 2021
Merged
6 tasks
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Oct 12, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 12, 2021
@vincentmli vincentmli force-pushed the vli-kernellimit branch 3 times, most recently from 9e1c503 to 6b842a4 Compare October 12, 2021 22:30
joestringer
joestringer approved these changes Oct 12, 2021
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

LGTM.

I notice that this is marked as "draft" and you have not clicked the "ready for review" button. This is likely why folks are not looking at this PR, as we usually use draft status only for under-development PRs that do not need reviewer attention yet.

@vincentmli vincentmli marked this pull request as ready for review October 12, 2021 22:34
@vincentmli vincentmli requested a review from a team as a code owner October 12, 2021 22:34
@vincentmli vincentmli requested a review from ti-mo October 12, 2021 22:34
@vincentmli
Copy link
Contributor Author

LGTM.

I notice that this is marked as "draft" and you have not clicked the "ready for review" button. This is likely why folks are not looking at this PR, as we usually use draft status only for under-development PRs that do not need reviewer attention yet.

I see, I clicked the "ready for review" now

@vincentmli
Copy link
Contributor Author

fixed the checkpatch.pl missing Signed-off-by

@joestringer joestringer removed the request for review from ti-mo October 12, 2021 23:03
@joestringer
Copy link
Member

I added @pchaigno into the reviewers since we discussed this earlier, and to let him know in case he already prepared similar changes.

ti-mo
ti-mo requested changes Oct 13, 2021
View reviewed changes
pkg/datapath/linux/probes/probes.go Outdated Show resolved Hide resolved
pchaigno
pchaigno approved these changes Oct 13, 2021
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

LGTM. I agree with Timo's feedback, we should keep the same pattern as we have for helpers, maps and program types.

@vincentmli vincentmli force-pushed the vli-kernellimit branch 2 times, most recently from 88649d5 to 3029d91 Compare October 13, 2021 15:09
@pchaigno pchaigno requested a review from ti-mo October 13, 2021 15:10
@vincentmli vincentmli changed the title Add kernel large 1M instructions feature probe Add Kernel Misc Probe Oct 13, 2021
ti-mo
ti-mo reviewed Oct 14, 2021
View reviewed changes
pkg/datapath/linux/probes/probes.go Outdated
@@ -306,6 +312,11 @@ func (p *ProbeManager) GetMapTypes() *MapTypes {
return &p.features.MapTypes
}

// GetMisc returns information about kernel misc.
func (p *ProbeManager) GetMisc() *Misc {
return &p.features.Misc
Copy link
Contributor

Choose a reason for hiding this comment

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

Just returning p.features.Misc will do. Returning a pointer will unnecessarily allocate on the heap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, good to know that, thanks! done

@vincentmli
Add Kernel Misc Probe
9768b8c 
Add kernel misc feature probe which include kernel BPF large instruction
support information. It allows Cilium features to probe kernel large instruction
support.

Signed-off-by: Vincent Li <vincent.mc.li@gmail.com>
@pchaigno pchaigno requested a review from ti-mo October 14, 2021 16:05
ti-mo
ti-mo approved these changes Oct 14, 2021
View reviewed changes
Copy link
Contributor

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

Thanks!

@pchaigno
Copy link
Member

The new code is not used anywhere yet so I don't think it's worth running the full end-to-end testing suite. Several tests already checked the code compiles and smoke tests ran successfully. Team review requests are covered. Marking ready to merge.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 14, 2021
@joestringer joestringer merged commit 6cad004 into cilium:master Oct 14, 2021
@pchaigno pchaigno mentioned this pull request Oct 15, 2021
Closed
@vincentmli vincentmli deleted the vli-kernellimit branch April 13, 2022 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@pchaigno pchaigno pchaigno approved these changes

@ti-mo ti-mo ti-mo approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@vincentmli @joestringer @pchaigno @ti-mo