Skip to content

bpf: Use nproc --all for __NR_CPUS__#12121

Merged
borkmann merged 1 commit intomasterfrom
pr/gandro/bpf-use-present-cpuset
Jun 17, 2020
Merged

bpf: Use nproc --all for __NR_CPUS__#12121
borkmann merged 1 commit intomasterfrom
pr/gandro/bpf-use-present-cpuset

Conversation

@gandro
Copy link
Copy Markdown
Member

@gandro gandro commented Jun 16, 2020

This uses -D__NR_CPUS__=$(nproc --all) (or GetNumPossibleCPUs when
invoked from Go) to compile the datapath.

This fixes an issue where cilium monitor fails to report any events
on AKS, due to the perf_event_array map duplicates being created
with different max_entries sizes, presumably causing the datapath
to write to the first one, while the agent is reading from the second
one.

This bug occurs for example on AKS due to the present/possible cpuset on
the VMs. The default Standard_D2s_v3 node size has 2 present CPUs, but
128 possible CPUs in /sys/devices/system/cpu.

Fixes: #12070

@gandro
bpf: Use nproc --all for __NR_CPUS__
6370441 
This uses `-D__NR_CPUS__=$(nproc --all)` (or `GetNumPossibleCPUs` when
invoked from Go) to compile the datapath.

This fixes an issue where cilium monitor fails to report any events
on AKS, due to the `perf_event_array` map duplicates being created
with different max_entries sizes, presumably causing the datapath
to write to the first one, while the agent is reading from the second
one.

This bug occurs for example on AKS due to the present/possible cpuset on
the VMs. The default Standard_D2s_v3 node size has 2 present CPUs, but
128 possible CPUs in /sys/devices/system/cpu.

Fixes: #12070

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
@gandro gandro added priority/release-blocker area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Jun 16, 2020
@gandro gandro requested a review from a team June 16, 2020 20:52
@gandro gandro requested review from a team as code owners June 16, 2020 20:52
@gandro gandro marked this pull request as draft June 16, 2020 20:52
@gandro
Copy link
Copy Markdown
Member Author

gandro commented Jun 16, 2020

test-me-please

@gandro gandro mentioned this pull request Jun 16, 2020
Closed
@gandro gandro requested review from borkmann and joestringer June 16, 2020 21:01
@gandro
Copy link
Copy Markdown
Member Author

gandro commented Jun 16, 2020 •
edited
Loading

This is an alternative to #12119 - I have just validated that this fixes the issue on AKS - marking ready for review.

@gandro gandro marked this pull request as ready for review June 16, 2020 21:02
@gandro
Copy link
Copy Markdown
Member Author

gandro commented Jun 16, 2020 •
edited
Loading

Please check out #12070 (comment) for @pchaigno's great explanation of why this was not causing more troubles beforehand.

borkmann
borkmann approved these changes Jun 16, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@borkmann borkmann left a comment

Choose a reason for hiding this comment

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

lgtm

@pchaigno
Copy link
Copy Markdown
Member

pchaigno commented Jun 16, 2020

There's two bugs fixed here in a way:

  1. We're not using the same map sizes everywhere. That bug was introduced in daemon: Create all global maps in cilium-agent #10626 (v1.8.0-rc1).
  2. We're using present CPUs in datapath. That bug has been there for a very long time (ex. cc74306, at least v0.10.1).

Do we want to backport this to v1.7 or even v1.6?

@borkmann
Copy link
Copy Markdown
Member

borkmann commented Jun 16, 2020

(Given this is only events and signals map, this shouldn't have upgrade implications.)

@joestringer
Copy link
Copy Markdown
Member

joestringer commented Jun 16, 2020

Do we want to backport this to v1.7 or even v1.6?

I believe that the core issue here where cilium doesn't report any flows is unique to v1.8 because v1.8 began opening (creating) this map prior to datapath provisioning.

However if someone were to hotplug CPUs on v1.7 or earlier, they could plausibly also hit this. The fix itself looks pretty harmless, v1.7 backport is reasonable to me.

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 16, 2020

Coverage Status

Coverage decreased (-0.01%) to 37.035% when pulling 6370441 on pr/gandro/bpf-use-present-cpuset into 496624b on master.

@pchaigno
Copy link
Copy Markdown
Member

pchaigno commented Jun 17, 2020

The K8s-1.17-Kernel-4.19 failure happened before on master. I filled #12127 for it. Since other required builds are passing, I think we're fine to merge.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 17, 2020
@borkmann borkmann merged commit 8191b16 into master Jun 17, 2020
@borkmann borkmann deleted the pr/gandro/bpf-use-present-cpuset branch June 17, 2020 07:16
@jrajahalme jrajahalme mentioned this pull request Jun 18, 2020
Merged
@pchaigno pchaigno mentioned this pull request Jun 26, 2020
Merged
@christarazi christarazi mentioned this pull request Jun 30, 2020
Merged
@gandro gandro mentioned this pull request Jul 1, 2020
Merged
@tklauser tklauser mentioned this pull request Aug 6, 2020
Closed
8 tasks
@gandro gandro mentioned this pull request Nov 11, 2024
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@borkmann borkmann borkmann approved these changes

@joestringer joestringer joestringer approved these changes

@pchaigno pchaigno pchaigno approved these changes

Assignees

No one assigned

Labels

area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cilium monitor broken on AKS 1.15 (engine 0.47)

6 participants

@gandro @pchaigno @borkmann @joestringer @coveralls @jrajahalme