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

Improve kernel 4.4.x compatibility #436

Merged
merged 3 commits into from
Oct 29, 2021
Merged

Improve kernel 4.4.x compatibility #436

merged 3 commits into from
Oct 29, 2021

Conversation

nplanel
Copy link
Contributor

@nplanel nplanel commented Sep 23, 2021 •
edited

This PR contain

  • Improve kernel 4.4.x compatibility
  • map.Iterate() support for old kernel (< 4.4.132)

Here some implementation detail/comment about map.Iterate()

Safe map.Iterate() to use with old kernel by guessing an non existing key that will point the first element of the map/array.

Thanks to a trick (map.lookup(key, invalidPointer)) of iovisor/bcc we can quickly test if a key exist or not in the map.
If the key is unknown map.GetNextKey() will return the first key that we can iterate to.

The non existing guessing will test these keys in order [0x00..., 0xff..., 0x55..., random] to find an unknown key.
If after all these try we can't found a non existing key, error errFirstKeyNotFound will be returned.

iovisor/bcc trick : https://github.com/iovisor/bcc/blob/c73d7d8901fdac99510a94a3b9ff5ebf2d8f5956/src/cc/libbpf.c#L402

@nplanel nplanel force-pushed the nplanel/map-iterate-old-kernel branch 4 times, most recently from b82d40e to 592dbd0 Compare September 24, 2021 10:17
@lmb
Copy link
Collaborator

lmb commented Sep 29, 2021

Hi Nicholas, sorry that the first review took so long. Last week was busy with Linux Plumbers conference.

We don't support kernels < 4.9 at the moment, so this code will never be exercised on CI. So to be able to merge this we'd have to change this policy. We don't have to support kernels that old, so I don't want to shoulder that burden, tbh. Is that something DataDog is willing to invest time in? If yes, you would need to start by adding 4.4 kernels to CI and making sure tests are all working. Then we could tackle this PR.

lmb
lmb reviewed Sep 29, 2021
View reviewed changes
map.go Outdated Show resolved Hide resolved
map.go Outdated Show resolved Hide resolved
@nplanel nplanel force-pushed the nplanel/map-iterate-old-kernel branch 2 times, most recently from a008320 to 458e9c0 Compare September 29, 2021 17:09
@nplanel
Copy link
Contributor Author

nplanel commented Sep 29, 2021

Hi Nicholas, sorry that the first review took so long. Last week was busy with Linux Plumbers conference.

We don't support kernels < 4.9 at the moment, so this code will never be exercised on CI. So to be able to merge this we'd have to change this policy. We don't have to support kernels that old, so I don't want to shoulder that burden, tbh. Is that something DataDog is willing to invest time in? If yes, you would need to start by adding 4.4 kernels to CI and making sure tests are all working. Then we could tackle this PR.

No problem for the delay Linux Plumbers was nice.
I will add support to the CI for the 4.4 kernels and skip() tests that can't work with

@nplanel nplanel force-pushed the nplanel/map-iterate-old-kernel branch 2 times, most recently from 5221c83 to 6854a06 Compare September 29, 2021 17:47
@ti-mo ti-mo marked this pull request as draft October 8, 2021 12:17
@nplanel nplanel force-pushed the nplanel/map-iterate-old-kernel branch 2 times, most recently from 59f79da to 022202e Compare October 20, 2021 16:15
@nplanel nplanel requested a review from lmb October 20, 2021 16:16
@nplanel nplanel marked this pull request as ready for review October 20, 2021 16:16
@nplanel nplanel force-pushed the nplanel/map-iterate-old-kernel branch 7 times, most recently from ef453cf to 0ec2a91 Compare October 25, 2021 11:39
@ti-mo ti-mo changed the title [ebpf/map] add map.Iterate() support for old kernel (< 4.4.132) Support Map.Iterate() on kernels < 4.4.132 Oct 25, 2021
@nplanel nplanel mentioned this pull request Oct 27, 2021
Closed
@ti-mo ti-mo self-requested a review October 27, 2021 10:07
@nplanel nplanel changed the title Support Map.Iterate() on kernels < 4.4.132 Improve kernel 4.4.x compatibility Oct 27, 2021
lmb
lmb requested changes Oct 27, 2021
View reviewed changes
map.go Outdated Show resolved Hide resolved
map_test.go Show resolved Hide resolved
map.go Outdated Show resolved Hide resolved
map.go Show resolved Hide resolved
map.go Outdated Show resolved Hide resolved
@nplanel nplanel force-pushed the nplanel/map-iterate-old-kernel branch 2 times, most recently from 16e5cbf to d86194a Compare October 27, 2021 11:40
@nplanel nplanel force-pushed the nplanel/map-iterate-old-kernel branch 6 times, most recently from 2087703 to 89d1def Compare October 27, 2021 17:41
@nplanel nplanel requested a review from lmb October 27, 2021 17:42
@nplanel nplanel force-pushed the nplanel/map-iterate-old-kernel branch 2 times, most recently from 83b41ad to 2a1a987 Compare October 27, 2021 19:39
lmb
lmb requested changes Oct 28, 2021
View reviewed changes
map.go Outdated Show resolved Hide resolved
map.go Show resolved Hide resolved
map.go Outdated Show resolved Hide resolved
map_test.go Show resolved Hide resolved
map_test.go Outdated Show resolved Hide resolved
@nplanel nplanel force-pushed the nplanel/map-iterate-old-kernel branch 3 times, most recently from 65f574b to 8c499e4 Compare October 28, 2021 22:14
@nplanel nplanel requested a review from lmb October 28, 2021 22:21
lmb
lmb approved these changes Oct 29, 2021
View reviewed changes
Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

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

Can you adjust the commit message of the final commit to reflect the changes you have made? Otherwise lgtm, thanks for working this out!

@nplanel nplanel force-pushed the nplanel/map-iterate-old-kernel branch from 8c499e4 to 8465327 Compare October 29, 2021 14:08
@nplanel
Copy link
Contributor Author

nplanel commented Oct 29, 2021 •
edited

Can you adjust the commit message of the final commit to reflect the changes you have made? Otherwise lgtm, thanks for working this out!

Thanks, I updated the commit and PR comment

@lmb lmb force-pushed the nplanel/map-iterate-old-kernel branch from 8465327 to a9a17d9 Compare October 29, 2021 14:14
@nplanel
Copy link
Contributor Author

nplanel commented Oct 29, 2021

@lmb could you merge this PR as write access is need

nplanel and others added 3 commits October 29, 2021 18:06
@nplanel @ti-mo
map,prog_test: use qt.IsTrue instead of (qt.Equal, true)
cee6e98
@nplanel @ti-mo
CI: test against 4.4 kernel, add haveNoPreallocMaps() probe
31409d7
@nplanel @ti-mo
Add map.Iterate() support for kernels < 4.4.132
518c5fe 
Make map.Iterate() safe to use on older kernels by guessing a non-existent key
that will return a pointer to the first element of the map.

By specifying an invalid value pointer, we can test if a key exists without
having the kernel copy memory.

This approach takes inspiration from BCC:
https://github.com/iovisor/bcc/blob/c73d7d8901fdac99510a94a3b9ff5ebf2d8f5956/src/cc/libbpf.c#L402
@ti-mo ti-mo force-pushed the nplanel/map-iterate-old-kernel branch from a9a17d9 to 518c5fe Compare October 29, 2021 16:23
@ti-mo ti-mo merged commit 2f6552f into cilium:master Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmb lmb lmb approved these changes

@ti-mo ti-mo Awaiting requested review from ti-mo

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@nplanel @lmb @ti-mo