eventsmap: Use present cpuset instead of possible for max_entries#12119
eventsmap: Use present cpuset instead of possible for max_entries#12119
present cpuset instead of possible for max_entries#12119Conversation
This fixes an issue where `cilium monitor` fails to report any events on AKS, due to the `perf_event_array` duplicates being created with max_entries=2 and max_entries=128, presumably causing the datapath to write to the first one, while the agent is reading from the second one. This bug occurs for example on AKS due to the present/possible cpuset on the VMs. The default `Standard_D2s_v3` node size has 2 present CPUs, but 128 possible CPUs in /sys/devices/system/cpu. Fixes #12070. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
|
@gandro do you know which commit is this fixing? |
|
While I have validated that this seems to fix the linked issue, I still don't understand why.
It's also not clear to me if we want to use |
|
An alternative fix could be to switch the datapath to use |
| &Value{}, | ||
| int(unsafe.Sizeof(Value{})), | ||
| common.GetNumPossibleCPUs(log), | ||
| common.GetNumPresentCPUs(log), |
There was a problem hiding this comment.
I don't know if we should use present or possible or what the role of 1256a13 is, but we want to use the same value in datapath and Golang for all maps. So we should also fix this for signalmap.
There was a problem hiding this comment.
We discussed this on previous PRs and deliberately chose possible so if there is any hotplugging, we would handle that correctly (ie not just drop events on newly hotplugged CPUs).
Do we actually need to be fixing up the BPF side to ensure that it opens using possible instead?
There was a problem hiding this comment.
@pchaigno @gandro @joestringer It sounds like -D__NR_CPUS__=$(nproc) is broken then in bpf_compile() from bpf/init.sh. Does echo $(nproc) return 2 or 128?
There was a problem hiding this comment.
Agree on hotplug, if suddenly CPU 4 would start generating events these would otherwise not be visible if we use present instead of possible. All of BPF kernel side uses possible.
There was a problem hiding this comment.
@gandro reported that nproc returns 2. I think we want nproc --all. runtime.NumCPU() also returns 2.
There was a problem hiding this comment.
Yes, such that agent and datapath both has the max_entries=128.
There was a problem hiding this comment.
That sounds reasonable to me as well. I can try to whip up a fix and validate it. Are there cases where __NR_CPUS__ should not be equal nproc --all?
There was a problem hiding this comment.
Alternative PR to have the datapath always use possible: #12121
|
Closing in favor of #12121 |
This fixes an issue where
cilium monitorfails to report any eventson AKS, due to the
perf_event_arrayduplicates being createdwith
max_entries=2andmax_entries=128, presumably causing the datapathto write to the first one, while the agent is reading from the second
one.
This bug occurs for example on AKS due to the present/possible cpuset on
the VMs. The default
Standard_D2s_v3node size has 2 present CPUs, but128 possible CPUs in
/sys/devices/system/cpu.Fixes #12070.