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

bpf: make setting fifo policy non-fatal when probing hz #11454

Merged
merged 3 commits into from
May 11, 2020
Merged

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 May 11, 2020
@borkmann borkmann requested review from pchaigno and a team May 11, 2020 07:20
@borkmann
Copy link
Member Author

test-me-please

@borkmann
Copy link
Member Author

test-me-please

@coveralls
Copy link

coveralls commented May 11, 2020

Coverage Status

Coverage decreased (-0.04%) to 37.855% when pulling bd2d30c on pr/fix-prio into fb5a886 on master.

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.

@borkmann Could you also add the executable to bpf/.gitignore?

bpf/cilium-probe-kernel-hz.c Outdated Show resolved Hide resolved
borkmann added 2 commits May 11, 2020 13:47
In case of an error in dump_kern_jiffies(), we need to propagate it so that
we exit with a non-success error code. Also move the macro output behind a
-m switch and add a non-macro output by default for cilium-probe-kernel-hz.

Also add cilium-probe-kernel-hz to gitignore.

Signed-off-by: Daniel Borkmann <[email protected]>
For cases where we don't care about a combined output, add a helper
to Cilium's exec library to only query stdout.

Signed-off-by: Daniel Borkmann <[email protected]>
@borkmann borkmann requested a review from a team May 11, 2020 11:55
@borkmann borkmann requested review from a team as code owners May 11, 2020 11:55
This work removes probing of kernel hz from init.sh and and moves it into
the agent. I'll upstream a small bpftool change which reads CONFIG_HZ from
the underlying kernel config which Cilium agent can then pick up via probe
manager automatically:

  diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
  index 88718ee6a438..946e9b89467d 100644
  --- a/tools/bpf/bpftool/feature.c
  +++ b/tools/bpf/bpftool/feature.c
  @@ -378,6 +378,9 @@ static void probe_kernel_image_config(void)

                  /* test_bpf module for BPF tests */
                  "CONFIG_TEST_BPF",
  +
  +               /* Miscellaneous, e.g. for jiffies conversion */
  +               "CONFIG_HZ",
          };
          char *values[ARRAY_SIZE(options)] = { };
          struct utsname utsn;

If it is not present on the system, we then fall-back to run the probe
(cilium-probe-kernel-hz) and hold the result in the agent config. Either
way, we then emit the KERNEL_HZ when generating the node_config.h from
the agent. If kernel config or probing failed, we disable checking for
jiffie helper since there is no point due to missing KERNEL_HZ value.

Fixes: 8d5654f ("bpf: probe and emit kernel hz to node_config")
Reported-by: Zang Li <[email protected]>
Reported-by: Benjamin Pineau <[email protected]>
Signed-off-by: Daniel Borkmann <[email protected]>
@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.

LGTM!

@borkmann
Copy link
Member Author

test-with-kernel

@nebril
Copy link
Member

nebril commented May 11, 2020

retest-4.19

@borkmann borkmann merged commit f518e88 into master May 11, 2020
@borkmann borkmann deleted the pr/fix-prio branch May 11, 2020 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
None yet
Development

Successfully merging this pull request may close these issues.

5 participants