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: various datapath follow-up optimisations and fixes #11924

Merged
merged 5 commits into from Jun 8, 2020
Merged

bpf: various datapath follow-up optimisations and fixes #11924

merged 5 commits into from Jun 8, 2020

Conversation

borkmann
Copy link
Member

@borkmann borkmann commented Jun 5, 2020 •
edited

See commit msgs for details. Needed in 1.8.0.

@borkmann
bpf: be more efficient when setting v4 dsr option
2d70a9d 
The way set_dsr_opt4() inserts the IP option is rather inefficient
as we have 2 csum_diff() and 2 ctx_store_bytes() calls. These can be
avoided and reduced to once each which will shave off expensive calls
in particular for the csum calculation.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann
bpf: inline csum_diff calls for small sizes
7d225ba 
Calling a helper for small-sized csum_diff() doesn't make sense and is
expensive. Just do it inline in BPF instead. This helps performance on
the load-balancer side in particular where we DNAT/SNAT IPv4.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@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 Jun 5, 2020
@borkmann borkmann requested review from brb, joestringer and a team June 5, 2020 14:15
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Jun 5, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.0 Jun 5, 2020
@borkmann
Copy link
Member Author

borkmann commented Jun 5, 2020

test-me-please

@borkmann
Copy link
Member Author

borkmann commented Jun 5, 2020

test-me-please

@coveralls
Copy link

coveralls commented Jun 5, 2020 •
edited

Coverage Status

Coverage increased (+0.008%) to 37.004% when pulling 0935675 on pr/bpf-follow-ups into 961da39 on master.

@borkmann
Copy link
Member Author

borkmann commented Jun 5, 2020

retest-4.9

2 similar comments
@borkmann
Copy link
Member Author

borkmann commented Jun 5, 2020

retest-4.9

@borkmann
Copy link
Member Author

borkmann commented Jun 6, 2020

retest-4.9

joestringer
joestringer approved these changes Jun 6, 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.

Diff looks big but almost 400LoC is just Linux bpf.h header updates, then another 50 is moving the same csum header code from one place to another.

I looked at the individual changes, they all look like pretty minor shuffling of code to reduce duplicate cases and reduce unnecessary initializations & calls from paths that wouldn't necessarily make use of the code being moved.

LGTM.

@joestringer
Copy link
Member

@borkmann why did 4.9 tests require restarts multiple times?

@borkmann
Copy link
Member Author

borkmann commented Jun 7, 2020 •
edited

@borkmann why did 4.9 tests require restarts multiple times?

Looks like in each of the cases it fails with FAIL: Kubernetes DNS did not become ready in time on the 4.9 k8s test suite only even though Cilium seems to come up just fine there. Given Runtime-4.9 tests are green, I'm not sure yet if it's related.

@borkmann
Copy link
Member Author

borkmann commented Jun 7, 2020 •
edited

retest-4.9
(all green except K8s-1.18-kernel-4.9)

brb
brb approved these changes Jun 8, 2020
View reviewed changes
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

LGTM! Just one question.

bpf/include/bpf/csum.h Show resolved Hide resolved
pchaigno
pchaigno approved these changes Jun 8, 2020
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

One nit (not worth it if the PR is ready to go). LGTM otherwise!

I couldn't find anything that would explain the 4.9 failure, but that error doesn't seem to happen on master.

bpf/lib/lb.h Outdated Show resolved Hide resolved
bpf/lib/lb.h Show resolved Hide resolved
@borkmann
bpf: extend csum_diff for non-zero seed and compile out loopback hand…
722a455 
…ling

Extend csum_diff() to handle the case where we have a prior seed, and
compile out the SNAT to loopback entirely since we only ever need it in
the bpf_lxc lb4_xlate() case (netdev lb is compiled with DISABLE_LOOPBACK_LB).

Also, small change to reorder the tests on lb{4,6}_svc_is_nodeport() to test
the more commonly used cases first.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann
bpf: upstream header resync
0261c2d 
Resync in order to get UAPI bits for [0] included.

  [0] https://lore.kernel.org/bpf/cover.1591108731.git.daniel@iogearbox.net/

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann
bpf: do not reset csum on adjust room for newer kernels
0935675 
See also [0]. Fall back to passing in 0 flags for older kernels. We
do not push/pop protocol headers but rather keep the same layers
instead, therefore there is also no point in changing to csum_none.
We can probe on csum_level() helper for this since both are needed
if this gets ever backported somewhere.

  [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=836e66c218f355ec01ba57671c85abf32961dcea
  [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7cdec54f9713256bb170873a1fc5c75c9127c9d2

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

borkmann commented Jun 8, 2020

test-me-please

@borkmann borkmann merged commit 1915b73 into master Jun 8, 2020
1.8.0 automation moved this from In progress to Merged Jun 8, 2020
@borkmann borkmann deleted the pr/bpf-follow-ups branch June 8, 2020 17:48
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.0 Jun 9, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.0 Jun 9, 2020
@aanm aanm mentioned this pull request Jun 9, 2020
Merged
@pchaigno
Copy link
Member

pchaigno commented Jun 9, 2020

@borkmann Did you find and fix the 4.9 issue with Kubernetes DNS did not become ready in time you were having?

@borkmann
Copy link
Member Author

borkmann commented Jun 9, 2020 •
edited

@borkmann Did you find and fix the 4.9 issue with Kubernetes DNS did not become ready in time you were having?

I bisected a change in my code and I had to undo one optimization, that is, to merge both csum updates into a single once since in one case it was called with pseduo-hdr flag and in the other case it was not (the port updates). With that fixed, CI went green here.

@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.0 Jun 12, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.0 Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brb brb brb approved these changes

@joestringer joestringer joestringer approved these changes

@pchaigno pchaigno pchaigno approved these changes

@qmonnet qmonnet qmonnet approved these changes

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
1.8.0
Backport done to v1.8
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@borkmann @coveralls @joestringer @pchaigno @brb @qmonnet @aanm