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

bpf: add -nostdinc and a few more misc compilation options #11205

Merged
merged 6 commits into from May 4, 2020
Merged

bpf: add -nostdinc and a few more misc compilation options #11205

merged 6 commits into from May 4, 2020

Conversation

borkmann
Copy link
Member

See commit msg.

@borkmann borkmann added pending-review sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. labels Apr 28, 2020
@borkmann borkmann requested review from brb, joestringer and a team April 28, 2020 22:23
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Apr 28, 2020
@borkmann
Copy link
Member Author

test-me-please

@borkmann
Copy link
Member Author

test-me-please

@coveralls
Copy link

coveralls commented Apr 28, 2020 •
edited

Coverage Status

Coverage increased (+0.02%) to 44.446% when pulling ada0d34 on pr/bpf-misc into 6fcf0cd on master.

@borkmann
Copy link
Member Author

test-me-please

joestringer
joestringer approved these changes Apr 29, 2020
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.

Couple minor nits but changes LGTM.

bpf/lib/policy_log.h Outdated Show resolved Hide resolved
bpf/lib/trace.h Outdated Show resolved Hide resolved
@borkmann borkmann requested a review from a team as a code owner April 30, 2020 09:56
@borkmann
Copy link
Member Author

test-me-please

qmonnet
qmonnet reviewed Apr 30, 2020
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks like removal of get_hash_recalc() got split between the two patches.

Otherwise looks good, but second patch is huge. Did all of those fixes came out of the new warnings at compile time?

bpf/lib/dbg.h Show resolved Hide resolved
@borkmann
Copy link
Member Author

test-me-please

@borkmann
Copy link
Member Author

Otherwise looks good, but second patch is huge. Did all of those fixes came out of the new warnings at compile time?

Yeah not all of it, for example, the event output msg refactoring touches all other ones as well to have a consistent initialiser. Hmm, I can split if off from the 2nd one to shrink it further.

@qmonnet
Copy link
Member

qmonnet commented Apr 30, 2020

Hmm, I can split if off from the 2nd one to shrink it further.

If you don't mind too much, that would help with the review.

@borkmann
Copy link
Member Author

test-me-please

qmonnet
qmonnet approved these changes Apr 30, 2020
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks great!

bpf/lib/dbg.h Show resolved Hide resolved
bpf/lib/trace.h Outdated Show resolved Hide resolved
bpf/bpf_overlay.c Show resolved Hide resolved
@borkmann
Copy link
Member Author

test-me-please

@borkmann
Copy link
Member Author

restart-ginkgo

@borkmann
Copy link
Member Author 

13:53:00  echo "docker push cilium/docker-plugin:latest-amd64"
13:53:00  docker push cilium/docker-plugin:latest-amd64
13:53:00  build -f hubble-relay.Dockerfile -t "cilium/hubble-relay:latest" .
13:53:00  /bin/bash: build: command not found
13:53:00  Makefile:290: recipe for target 'docker-hubble-relay-image' failed
13:53:00  make: *** [docker-hubble-relay-image] Error 127
Post stage

@borkmann
Copy link
Member Author

test-me-please

@borkmann
Copy link
Member Author

test-focus K8sService.*

@borkmann
Copy link
Member Author 

22:53:35      runtime: already cached: 	docker.io/library/memcached:1.5.11
22:53:35      runtime: already cached: 	docker.io/tgraf/netperf:v1.0
22:53:35  Cancelling nested steps due to timeout
22:53:35  Sending interrupt signal to process
22:53:35      runtime: already cached: 	docker.io/wurstmeister/kafka:2.11-0.11.0.3
22:53:35      runtime: not cached in VM: 	quay.io/cilium/cilium-builder:2020-04-16
22:53:35      runtime: pulling: 		quay.io/cilium/cilium-builder:2020-04-16
22:53:36  script returned exit code 255

@borkmann
Copy link
Member Author

restart-ginkgo

1 similar comment
@qmonnet
Copy link
Member

qmonnet commented May 1, 2020

restart-ginkgo

@aanm
Copy link
Member

aanm commented May 1, 2020

@borkmann it seems Cilium was in CrashLoopback mode. Doesn't seem to be a flake

@borkmann
Copy link
Member Author

borkmann commented May 1, 2020

@borkmann it seems Cilium was in CrashLoopback mode. Doesn't seem to be a flake

Yep, saw it, will keep working on it in the evening today.

@maintainer-s-little-helper
Copy link

Commit 2097a89c2e990169b964fdf716b9fbc36f6ab3bd does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 2, 2020
@borkmann
Copy link
Member Author

borkmann commented May 2, 2020

test-focus K8sService.*

@borkmann
Copy link
Member Author

borkmann commented May 2, 2020

test-me-please

1 similar comment
@borkmann
Copy link
Member Author

borkmann commented May 3, 2020

test-me-please

@borkmann
Copy link
Member Author

borkmann commented May 3, 2020

provision error

@borkmann
Copy link
Member Author

borkmann commented May 3, 2020

test-me-please

@borkmann
Copy link
Member Author

borkmann commented May 3, 2020

4.19:

17:10:05  + python get-gh-comment-info.py test-me-please --retrieve=version
17:10:05  + sed s/^$/419/
17:10:05  + ./print-node-ip.sh
17:10:05  + KERNEL=419 CILIUM_REGISTRY=147.75.199.49 ./vagrant-ci-start.sh
17:10:05  destroying vms in case this is a retry
17:10:05  The provider 'virtualbox' that was requested to back the machine
17:10:05  'k8s1-1.17' is reporting that it isn't usable on this system. The
17:10:05  reason is shown below:
17:10:05  
17:10:05  VirtualBox is complaining that the kernel module is not loaded. Please
17:10:05  run `VBoxManage --version` or open the VirtualBox GUI to see the error
17:10:05  message which should contain instructions on how to fix this error.
[Pipeline] }

@borkmann
Copy link
Member Author

borkmann commented May 3, 2020

test-with-kernel

@pchaigno pchaigno mentioned this pull request May 4, 2020
Merged
@borkmann
bpf: avoid potentially expensive calls into get_hash_recalc
23485da 
Depending on the NICs hash, we might end up recalculating it
from the kernel flow dissector to get a higher quality L4 hash
on the skb instead of L3 one, for example.

We used to do this mainly for the skb->hash based service LB,
but nowadays all occurences in the code are for cilium monitor
debug output where it's used as a heuristic to see that the event
messages belong to the same flow.

Hubble side doesn't do anything with the hash.

Whether L3 or L4 hash doesn't really matter too much here, so
get rid of these calls.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann
bpf: refactor and improve perf event output message init
f81914e 
Prep work and general refactoring for enabling stricter compilation
options closer to the kernel. In order to avoid warnings from the
-Wdeclaration-after-statement, make use of compound literals where
adequate.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann
bpf: simplify session affinity api and reduce ifdefs in socklb
ac3e03c 
Fetch the get_netns_cookie() only once given it's an atomic op and
we already have it locally when we check for hostns. Also pass in
svc directly into session affinity APIs to simplify them a bit and
move the multiple defined(ENABLE_SESSION_AFFINITY) && defined(BPF_HAVE_NETNS_COOKIE)
out of the bpf_sock.c code by letting compiler do most of the work
to optimize away the constants - we add *_{netns,addr}() api variants
and for the former we can place the BPF_HAVE_NETNS_COOKIE ifdef into
lb.h header to keep the code flow more readable in bpf_sock.c. At the
same time this also fixes couple of -Wdeclaration-after-statement
warnings.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 4, 2020
@borkmann
Copy link
Member Author

borkmann commented May 4, 2020

test-focus K8sService.*

@borkmann
bpf, misc: fix shadow declarations and declarations after statements
8bfde9d 
Few random minor changes over the place to fix up warnings in order
to enable stricter compile checks in the subsequent patch. This also
fixes up some whitespace oddities in nodeport code that slipped into
our tree.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann
bpf: improve overlay build permutation coverage
9ee4c1c 
Given bpf_overlay is similarly serving BPF NodePort like bpf_netdev does,
improve coverage so we can easier spot any build issues.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann
bpf: enable -nostdinc and other stricter compilation options
ada0d34 
We want to have our code as close to usual kernel conventions as
possible to ease review, so add a few more switches for clang
where we emit useful warnings in our code if it's not the case,
that is, -std=gnu89 and -Wdeclaration-after-statement.

We also use quite a number of shadow declarations where they are
really useless; I'm not sure whether clang ends up allocating new
stack space for them; in any case lets add -Wshadow given the
current locations are fixed now.

Also, given our headers are all self-contained, add -nostdinc to
fully /guarantee/ that clang does not search the standard include
paths anymore.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann
Copy link
Member Author

borkmann commented May 4, 2020

test-me-please

@borkmann borkmann merged commit b6d1343 into master May 4, 2020
1.8.0 automation moved this from In progress to Merged May 4, 2020
@borkmann borkmann deleted the pr/bpf-misc branch May 4, 2020 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno left review comments

@joestringer joestringer joestringer approved these changes

@qmonnet qmonnet qmonnet approved these changes

@brb brb Awaiting requested review from brb

Assignees
No one assigned
Labels
release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@borkmann @coveralls @qmonnet @aanm @joestringer @pchaigno