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

bwm: queue mapping & cong fixes #15964

Merged
merged 2 commits into from Apr 30, 2021
Merged

bwm: queue mapping & cong fixes #15964

merged 2 commits into from Apr 30, 2021

Conversation

borkmann
Copy link
Member

See commit msgs.

A Pod could potentially emit non-IPv4/v6 traffic and in that case we must
not set the aggregate since we also don't clear it in edt_sched_departure()
again. Hence, move the edt_set_aggregate() to the two supported protos.

Fixes: #15960
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann borkmann added 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. feature/bandwidth-manager Impacts BPF bandwidth manager. release-blocker/1.10 labels Apr 30, 2021
@borkmann borkmann requested review from pchaigno and a team April 30, 2021 13:00
@borkmann
Copy link
Member Author

test-me-please

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.

🚀

pkg/bandwidth/bandwidth.go Outdated Show resolved Hide resolved
@pchaigno pchaigno removed their assignment Apr 30, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.0-rc2 Apr 30, 2021
@borkmann
Copy link
Member Author

retest-1.20-4.19

Consider a socket which has SO_MAX_PACING_RATE of 4Gbit/s and the
socket being part of a Pod. This is currently broken given skb->tstamps
are cleared on BPF redirect as well as netns traversal even though
fq in hostns manages the socket's pacing. Rates would result being
unpredictable:

  root@apoc:~/go/src/github.com/cilium/cilium# netperf -H 10.217.1.19 -t TCP_STREAM -l40 -s2
  MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.217.1.19 () port 0 AF_INET : demo
  Recv   Send    Send
  Socket Socket  Message  Elapsed
  Size   Size    Size     Time     Throughput
  bytes  bytes   bytes    secs.    10^6bits/sec
   87380  16384  16384    40.04     655.52

  root@apoc:~/go/src/github.com/cilium/cilium# netperf -H 10.217.1.19 -t TCP_STREAM -l40 -s2
  MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.217.1.19 () port 0 AF_INET : demo
  Recv   Send    Send
  Socket Socket  Message  Elapsed
  Size   Size    Size     Time     Throughput
  bytes  bytes   bytes    secs.    10^6bits/sec
   87380  16384  16384    40.07    1274.70

  root@apoc:~/go/src/github.com/cilium/cilium# netperf -H 10.217.1.19 -t TCP_STREAM -l40 -s2
  MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.217.1.19 () port 0 AF_INET : demo
  Recv   Send    Send
  Socket Socket  Message  Elapsed
  Size   Size    Size     Time     Throughput
  bytes  bytes   bytes    secs.    10^6bits/sec
   87380  16384  16384    40.07    1519.32

  root@apoc:~/go/src/github.com/cilium/cilium# netperf -H 10.217.1.19 -t TCP_STREAM -l40 -s2
  MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.217.1.19 () port 0 AF_INET : demo
  Recv   Send    Send
  Socket Socket  Message  Elapsed
  Size   Size    Size     Time     Throughput
  bytes  bytes   bytes    secs.    10^6bits/sec
   87380  16384  16384    40.06     849.96

We are working on a kernel side solution to retain skb->tstamps which
would fix this issue and result in a stable 4Gbit/s rate for this
example. Once that is merged we can reenable BBR from BWM side for
those kernels (and fallback to cubic for those that do not have it).

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

(prior run green, only removed 1 comment line)

@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.7 Apr 30, 2021
@borkmann borkmann merged commit c233140 into master Apr 30, 2021
@borkmann borkmann deleted the pr/bwm-fixes branch April 30, 2021 18:24
@brb brb mentioned this pull request May 7, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.7 May 7, 2021
@brb brb mentioned this pull request May 7, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.0-rc2 May 7, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.7 May 10, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.0-rc2 May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/bandwidth-manager Impacts BPF bandwidth manager. 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.10.0-rc2
Backport done to v1.10
1.9.7
Backport done to v1.9
Development

Successfully merging this pull request may close these issues.

None yet

5 participants