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

fix(driver): fixed build against 6.6 rc kernel. #1349

Merged
merged 3 commits into from
Sep 14, 2023
Merged

fix(driver): fixed build against 6.6 rc kernel. #1349

merged 3 commits into from
Sep 14, 2023

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Sep 13, 2023 •
edited
Loading

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area driver-kmod
/area driver-bpf
/area driver-modern-bpf

What this PR does / why we need it:

See torvalds/linux@13bc244

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix(driver): fixed build against linux v6.6.

@poiana poiana added the size/L label Sep 13, 2023
@FedeDP
Copy link
Contributor Author

FedeDP commented Sep 13, 2023

/milestone 6.0.1+driver

@poiana poiana added this to the 6.0.1+driver milestone Sep 13, 2023
@github-actions
Copy link

Please double check driver/API_VERSION file. See versioning.

/hold

@FedeDP
Copy link
Contributor Author

FedeDP commented Sep 13, 2023

/unhold

LucaGuerra
LucaGuerra reviewed Sep 13, 2023
View reviewed changes
driver/API_VERSION Outdated
@@ -1 +1 @@
5.0.0
5.0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Milestone is 6.0.1+driver but this reads 5.0.1 . Why is that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we were already at 6.0.0+driver (latest tagged), since we bump major when either SCHEMA or API have major bumps.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah that's the API version, sure! Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure we need an API bump here. This version regulates the API between userspace and drivers but here we don't change it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need it: we are releasing a new driver tag and it needs a minor bump in order to have different tag name (i think).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I double checked; it does not seem like we need to bump in this case.

@FedeDP
Copy link
Contributor Author

FedeDP commented Sep 13, 2023

/cc @Andreagit97

@poiana poiana requested a review from Andreagit97 September 13, 2023 08:25
@FedeDP
Copy link
Contributor Author

FedeDP commented Sep 13, 2023 •
edited
Loading

/hold
currently manually testing with driverkit.
This PR also improves latest-kernel workflow to be able to test against mainline kernels even from PRs (ie: when manually triggered).

@FedeDP
Copy link
Contributor Author

FedeDP commented Sep 13, 2023

level=INFO msg="kernel module available" path=mod.ko
level=INFO msg="eBPF probe available" path=probe.o

/unhold

nathan-b
nathan-b reviewed Sep 13, 2023
View reviewed changes
Copy link
Contributor

@nathan-b nathan-b left a comment

Choose a reason for hiding this comment

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

It seems like it might be worth it to create a helper function inside kernel_hacks.h (which is what that file is there for, after all).

@FedeDP
Copy link
Contributor Author

FedeDP commented Sep 13, 2023

We are not using that file since ages though :)
I really prefer to avoid "hiding" things, i mean it is much easier to follow kernel code if i can say "oh, here on kernel >= x.y.z we do this, else we do that", instead of "oh we call get_inode_ctime() here; is it our? is it kernel API? ...navigate in... oh on kernel >= x.y.z we do...".

But that's just my 2c and my taste. I know it gets much more "spaghetti" than it would need, but that's just what kernel allows us to do.

Andreagit97
Andreagit97 reviewed Sep 13, 2023
View reviewed changes
driver/API_VERSION Outdated
@@ -1 +1 @@
5.0.0
5.0.1
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we need an API bump here. This version regulates the API between userspace and drivers but here we don't change it

Andreagit97
Andreagit97 reviewed Sep 13, 2023
View reviewed changes
Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

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

The fact that we are breaking our drivers at every kernel release IMO is a clear signal that we are hooking in place in which we shouldn't hook. Maybe we should take some time to reflect on this :/

@poiana poiana added size/M and removed size/L labels Sep 13, 2023
FedeDP and others added 3 commits September 13, 2023 15:36
@FedeDP
fix(driver): fixed build against 6.6 rc kernel.
c4349e9 
Signed-off-by: Federico Di Pierro <[email protected]>
@FedeDP
chore(ci): improve latest-kernel workflow.
41756f8 
Signed-off-by: Federico Di Pierro <[email protected]>
@FedeDP @Andreagit97
fix(driver/modern_bpf): fixed modern bpf driver.
52da8ed 
Signed-off-by: Federico Di Pierro <[email protected]>

Co-authored-by: Andrea Terzolo <[email protected]>
Andreagit97
Andreagit97 approved these changes Sep 13, 2023
View reviewed changes
Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

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

/approve
Thank you!

@poiana poiana added the lgtm label Sep 13, 2023
@poiana
Copy link
Contributor

poiana commented Sep 14, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, leogr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [Andreagit97,FedeDP,leogr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana merged commit 027d40b into master Sep 14, 2023
@poiana poiana deleted the fix/6.6build branch September 14, 2023 07:46
@FedeDP FedeDP mentioned this pull request Sep 14, 2023
Merged
@Andreagit97 Andreagit97 mentioned this pull request Sep 14, 2023
Closed
25 tasks
@FedeDP FedeDP mentioned this pull request Oct 16, 2023
Merged
@ovalenti ovalenti mentioned this pull request Nov 2, 2023
Merged
@FedeDP FedeDP mentioned this pull request Dec 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathan-b nathan-b nathan-b left review comments

@LucaGuerra LucaGuerra LucaGuerra left review comments

@leogr leogr leogr approved these changes

@Andreagit97 Andreagit97 Andreagit97 approved these changes

@hbrueckner hbrueckner Awaiting requested review from hbrueckner

Assignees

@leogr leogr

@Andreagit97 Andreagit97

Labels
approved area/driver-bpf area/driver-kmod area/driver-modern-bpf dco-signoff: yes kind/bug Something isn't working lgtm release-note size/M
Projects
None yet
Milestone
6.0.1+driver
Development

Successfully merging this pull request may close these issues.
6 participants
@FedeDP @poiana @leogr @nathan-b @LucaGuerra @Andreagit97