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.

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

datapath: Use bpftool for generating BPF feature macros #10019

Merged
merged 5 commits into from Mar 17, 2020

Conversation

vadorovsky
Copy link
Member

@vadorovsky vadorovsky commented Feb 2, 2020

To adjust BPF programs to different kernels containing different BPF features, bpftool provides macros like i.e.:

# bpftool feature probe macros
/*** System call availability ***/
#define HAVE_BPF_SYSCALL
[...]
/*** eBPF map types ***/
#define HAVE_HASH_MAP_TYPE
[...]

Cilium implemented similar macros on its own, but this pull request removes them in favor of macros generated by bpftool.

Use bpftool for generating BPF feature macros

This change is Reviewable

@vadorovsky vadorovsky added wip sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Feb 2, 2020
@vadorovsky vadorovsky added this to the 1.8 milestone Feb 2, 2020
@vadorovsky vadorovsky requested a review from a team February 2, 2020 10:47
@vadorovsky vadorovsky requested a review from a team as a code owner February 2, 2020 10:47
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

1 similar comment
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.7.0 Feb 2, 2020
@vadorovsky vadorovsky removed this from In progress in 1.7.0 Feb 2, 2020
@vadorovsky
Copy link
Member Author

test-me-please

@coveralls
Copy link

coveralls commented Feb 2, 2020

Coverage Status

Coverage decreased (-0.02%) to 45.624% when pulling c3a7a5d on mrostecki:bpftool-feature-headers into 970a259 on cilium:master.

@vadorovsky
Copy link
Member Author

test-me-please

@aanm aanm added dont-merge/waiting-for-upstream Only merge once upstream library or kernel features have landed dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs and removed dont-merge/waiting-for-upstream Only merge once upstream library or kernel features have landed labels Feb 3, 2020
@aanm aanm modified the milestones: 1.8, 1.7 Feb 18, 2020
@aanm aanm added dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. and removed dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs labels Feb 18, 2020
@vadorovsky vadorovsky added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. and removed dont-merge/needs-release-note dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. labels Feb 28, 2020
@vadorovsky vadorovsky changed the title [WIP] datapath: Use bpftool for generating BPF feature macros datapath: Use bpftool for generating BPF feature macros Feb 28, 2020
@vadorovsky
Copy link
Member Author

test-me-please

1 similar comment
@vadorovsky
Copy link
Member Author

test-me-please

@vadorovsky vadorovsky removed the wip label Feb 28, 2020
Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
Previous changes remove Cilium BPF feature probes in favor of bpftool
probes, which are executed by daemon in Go code and errors are included
in the cilium-agent log. Additional .log file is not created anymore.

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
@vadorovsky
Copy link
Member Author

vadorovsky commented Mar 16, 2020

Again lots of failures on EKS: https://jenkins.cilium.io/job/Cilium-PR-K8s-EKS/104/consoleFull

And the flake #10565

@vadorovsky
Copy link
Member Author

test-me-please

@nebril
Copy link
Member

nebril commented Mar 17, 2020

test-eks was triggered here by mistake, this PR is good to merge as far as CI goes

@aanm aanm merged commit a244143 into cilium:master Mar 17, 2020
@aanm aanm added this to Merged in 1.8.0 via automation Mar 17, 2020
@vadorovsky vadorovsky deleted the bpftool-feature-headers branch March 17, 2020 11:07
borkmann added a commit that referenced this pull request Mar 19, 2020
Do not rely on clang-7/llvm-7 shipped by Ubuntu base image and
instead upgrade to clang-11/llvm-11 with a BPF-only backend.

This would help overcoming the blockade of [0] where we're hitting
the 1 mio instruction complexity limit which was bisected by Paul
to the kernel commit f7cf25b20 ("bpf: track spill/fill of constants").
As it is stated there:

  Newer clang generates better code by spilling less to the stack.
  Instead it keeps more constants in the registers which hurts
  state pruning since the verifier already tracks constants in the
  registers [...]. Tracking constants in the registers hurts state
  pruning already. Adding tracking of constants through stack hurts
  pruning even more. The later patch address this general constant
  tracking issue with coarse/precise logic.

Side-effect of going with a custom clang-11/llvm-11 build with a
BPF-only backend is that i) we can also shrink the image since x86
is not needed anymore, and ii) avoid shipping a generic compiler in
our image that can generate x86 executable code. This depends on [1]
where we first need to get rid of our custom runtime probes in Cilium
and instead rely on bpftool to take over that job.

Size shrinkage around 36.6M:

  clang-7 (90,777,392) -> clang-11/stripped   (75,617,520)
  llc-7   (51,453,072) -> llc-11/bpf/stripped (29,997,384)

  [0] #10517
  [1] #10019

Complexity comparison:

  https://user-images.githubusercontent.com/677393/76440789-aae08b80-63be-11ea-863f-37ab12106ad9.png

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Paul Chaignon <paul@cilium.io>
borkmann added a commit to borkmann/cilium that referenced this pull request Mar 20, 2020
Do not rely on clang-7/llvm-7 shipped by Ubuntu base image and
instead upgrade to clang-11/llvm-11 with a BPF-only backend.

This would help overcoming the blockade of [0] where we're hitting
the 1 mio instruction complexity limit which was bisected by Paul
to the kernel commit f7cf25b20 ("bpf: track spill/fill of constants").
As it is stated there:

  Newer clang generates better code by spilling less to the stack.
  Instead it keeps more constants in the registers which hurts
  state pruning since the verifier already tracks constants in the
  registers [...]. Tracking constants in the registers hurts state
  pruning already. Adding tracking of constants through stack hurts
  pruning even more. The later patch address this general constant
  tracking issue with coarse/precise logic.

Side-effect of going with a custom clang-11/llvm-11 build with a
BPF-only backend is that i) we can also shrink the image since x86
is not needed anymore, and ii) avoid shipping a generic compiler in
our image that can generate x86 executable code. This depends on [1]
where we first need to get rid of our custom runtime probes in Cilium
and instead rely on bpftool to take over that job.

Size shrinkage around 36.6M:

  clang-7 (90,777,392) -> clang-11/stripped   (75,617,520)
  llc-7   (51,453,072) -> llc-11/bpf/stripped (29,997,384)

  [0] cilium#10517
  [1] cilium#10019

Complexity comparison:

  https://user-images.githubusercontent.com/677393/76440789-aae08b80-63be-11ea-863f-37ab12106ad9.png

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Paul Chaignon <paul@cilium.io>
borkmann added a commit that referenced this pull request Mar 23, 2020
Do not rely on clang-7/llvm-7 shipped by Ubuntu base image and
instead upgrade to clang-11/llvm-11 with a BPF-only backend.

This would help overcoming the blockade of [0] where we're hitting
the 1 mio instruction complexity limit which was bisected by Paul
to the kernel commit f7cf25b20 ("bpf: track spill/fill of constants").
As it is stated there:

  Newer clang generates better code by spilling less to the stack.
  Instead it keeps more constants in the registers which hurts
  state pruning since the verifier already tracks constants in the
  registers [...]. Tracking constants in the registers hurts state
  pruning already. Adding tracking of constants through stack hurts
  pruning even more. The later patch address this general constant
  tracking issue with coarse/precise logic.

Side-effect of going with a custom clang-11/llvm-11 build with a
BPF-only backend is that i) we can also shrink the image since x86
is not needed anymore, and ii) avoid shipping a generic compiler in
our image that can generate x86 executable code. This depends on [1]
where we first need to get rid of our custom runtime probes in Cilium
and instead rely on bpftool to take over that job.

Size shrinkage around 36.6M:

  clang-7 (90,777,392) -> clang-11/stripped   (75,617,520)
  llc-7   (51,453,072) -> llc-11/bpf/stripped (29,997,384)

  [0] #10517
  [1] #10019

Complexity comparison:

  https://user-images.githubusercontent.com/677393/76440789-aae08b80-63be-11ea-863f-37ab12106ad9.png

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Paul Chaignon <paul@cilium.io>
borkmann added a commit that referenced this pull request Mar 24, 2020
Do not rely on clang-7/llvm-7 shipped by Ubuntu base image and
instead upgrade to clang-11/llvm-11 with a BPF-only backend.

This would help overcoming the blockade of [0] where we're hitting
the 1 mio instruction complexity limit which was bisected by Paul
to the kernel commit f7cf25b20 ("bpf: track spill/fill of constants").
As it is stated there:

  Newer clang generates better code by spilling less to the stack.
  Instead it keeps more constants in the registers which hurts
  state pruning since the verifier already tracks constants in the
  registers [...]. Tracking constants in the registers hurts state
  pruning already. Adding tracking of constants through stack hurts
  pruning even more. The later patch address this general constant
  tracking issue with coarse/precise logic.

Side-effect of going with a custom clang-11/llvm-11 build with a
BPF-only backend is that i) we can also shrink the image since x86
is not needed anymore, and ii) avoid shipping a generic compiler in
our image that can generate x86 executable code. This depends on [1]
where we first need to get rid of our custom runtime probes in Cilium
and instead rely on bpftool to take over that job.

Size shrinkage around 36.6M:

  clang-7 (90,777,392) -> clang-11/stripped   (75,617,520)
  llc-7   (51,453,072) -> llc-11/bpf/stripped (29,997,384)

  [0] #10517
  [1] #10019

Complexity comparison:

  https://user-images.githubusercontent.com/677393/76440789-aae08b80-63be-11ea-863f-37ab12106ad9.png

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Paul Chaignon <paul@cilium.io>
borkmann added a commit that referenced this pull request Mar 25, 2020
Do not rely on clang-7/llvm-7 shipped by Ubuntu base image and
instead upgrade to clang-11/llvm-11 with a BPF-only backend.

This would help overcoming the blockade of [0] where we're hitting
the 1 mio instruction complexity limit which was bisected by Paul
to the kernel commit f7cf25b20 ("bpf: track spill/fill of constants").
As it is stated there:

  Newer clang generates better code by spilling less to the stack.
  Instead it keeps more constants in the registers which hurts
  state pruning since the verifier already tracks constants in the
  registers [...]. Tracking constants in the registers hurts state
  pruning already. Adding tracking of constants through stack hurts
  pruning even more. The later patch address this general constant
  tracking issue with coarse/precise logic.

Side-effect of going with a custom clang-11/llvm-11 build with a
BPF-only backend is that i) we can also shrink the image since x86
is not needed anymore, and ii) avoid shipping a generic compiler in
our image that can generate x86 executable code. This depends on [1]
where we first need to get rid of our custom runtime probes in Cilium
and instead rely on bpftool to take over that job.

Size shrinkage around 36.6M:

  clang-7 (90,777,392) -> clang-11/stripped   (75,617,520)
  llc-7   (51,453,072) -> llc-11/bpf/stripped (29,997,384)

  [0] #10517
  [1] #10019

Complexity comparison:

  https://user-images.githubusercontent.com/677393/76440789-aae08b80-63be-11ea-863f-37ab12106ad9.png

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Paul Chaignon <paul@cilium.io>
borkmann added a commit that referenced this pull request Mar 25, 2020
Do not rely on clang-7/llvm-7 shipped by Ubuntu base image and
instead upgrade to clang-11/llvm-11 with a BPF-only backend.

This would help overcoming the blockade of [0] where we're hitting
the 1 mio instruction complexity limit which was bisected by Paul
to the kernel commit f7cf25b20 ("bpf: track spill/fill of constants").
As it is stated there:

  Newer clang generates better code by spilling less to the stack.
  Instead it keeps more constants in the registers which hurts
  state pruning since the verifier already tracks constants in the
  registers [...]. Tracking constants in the registers hurts state
  pruning already. Adding tracking of constants through stack hurts
  pruning even more. The later patch address this general constant
  tracking issue with coarse/precise logic.

Side-effect of going with a custom clang-11/llvm-11 build with a
BPF-only backend is that i) we can also shrink the image since x86
is not needed anymore, and ii) avoid shipping a generic compiler in
our image that can generate x86 executable code. This depends on [1]
where we first need to get rid of our custom runtime probes in Cilium
and instead rely on bpftool to take over that job.

Size shrinkage around 36.6M:

  clang-7 (90,777,392) -> clang-11/stripped   (75,617,520)
  llc-7   (51,453,072) -> llc-11/bpf/stripped (29,997,384)

  [0] #10517
  [1] #10019

Complexity comparison:

  https://user-images.githubusercontent.com/677393/76440789-aae08b80-63be-11ea-863f-37ab12106ad9.png

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Paul Chaignon <paul@cilium.io>
pchaigno added a commit that referenced this pull request Apr 6, 2020
When copying the output of bpftool into the bpf_features.h file, although
we wait for the bpftool command to finish, we don't wait for the consumer
goroutine. This issue can lead to corrupted bpf_features.h files. We
however do not need a goroutine to copy bpftool's output.

Fixes: #10857
Fixes: #10019
Signed-off-by: Paul Chaignon <paul@cilium.io>
pchaigno added a commit that referenced this pull request Apr 9, 2020
When copying the output of bpftool into the bpf_features.h file, although
we wait for the bpftool command to finish, we don't wait for the consumer
goroutine. This issue can lead to corrupted bpf_features.h files. We
however do not need a goroutine to copy bpftool's output.

Fixes: #10857
Fixes: #10019
Signed-off-by: Paul Chaignon <paul@cilium.io>
joestringer pushed a commit that referenced this pull request Apr 9, 2020
When copying the output of bpftool into the bpf_features.h file, although
we wait for the bpftool command to finish, we don't wait for the consumer
goroutine. This issue can lead to corrupted bpf_features.h files. We
however do not need a goroutine to copy bpftool's output.

Fixes: #10857
Fixes: #10019
Signed-off-by: Paul Chaignon <paul@cilium.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/high This is considered vital to an upcoming release. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

8 participants