-
Notifications
You must be signed in to change notification settings - Fork 169
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
ebpf: reducing ebpf call overhead by using sampling instead of tracing every calls #823
Conversation
depend on #824 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, I only concerned about the sample rate.
@@ -44,6 +44,9 @@ BPF_ARRAY(cache_miss, u64, NUM_CPUS); | |||
// cpu freq counters | |||
BPF_ARRAY(cpu_freq_array, u32, NUM_CPUS); | |||
|
|||
int sample_rate = 1000; | |||
int counter = 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be:
int sample_rate = SAMPLE_RATE;
int counter = SAMPLE_RATE;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libbpf is pre-compiled, we cannot use compilation flags. In this case, we have to global variable to set it. I opened #824 so I can use InitGlobalVar
function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it!
If we need to hard code now, let's use a smaller value.
bpfassets/perf_event/perf_event.c
Outdated
@@ -71,6 +71,11 @@ BPF_ARRAY(cache_miss, u64, NUM_CPUS); | |||
// cpu freq counters | |||
BPF_ARRAY(cpu_freq_array, u32, NUM_CPUS); | |||
|
|||
#ifndef SAMPLE_RATE | |||
#define SAMPLE_RATE 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't skipping 1000 sample to extreme?
Did you try 10 and 100?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, 10 or 100 does have some reduction from 1000ns to 300ns.
pkg/config/config.go
Outdated
@@ -80,6 +80,7 @@ var ( | |||
BindAddressKey = "BIND_ADDRESS" | |||
CPUArchOverride = getConfig("CPU_ARCH_OVERRIDE", "") | |||
MaxLookupRetry = getIntConfig("MAX_LOOKUP_RETRY", defaultMaxLookupRetry) | |||
BPFSampleRate = getIntConfig("BPF_SAMPLE_RATE", 1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use smaller values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, let's have some stats first so users know what to choose and start from small values as default.
test environment
kepler commandBPF_SAMPLE_RATE=1000 _output/bin/linux_amd64/kepler ebpf calculation
result
@marceloamaral @sunya-ch what's your recommended default sample rate? |
Thanks for sharing it. This is great! |
@rootfs, for now, let's set the default value to 10 since we're uncertain about the consequences of skipping samples. Moreover, using 10 seems to bring significant improvements. Once we conduct further analysis, we can increasing this value. |
@marceloamaral sure, let default to 10. One more question, when we sample the ebpf calls, should we also extrapolate the metrics (cpu time, cpu instructions, etc) as well? |
Just created a discussion |
@eklee15 @marceloamaral @sunya-ch Let's disable sampling for now until we have a resolution on #836 |
if (sample_counter_value > 0) { | ||
if (*sample_counter_value > 0) { | ||
(*sample_counter_value)--; | ||
return 0; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New to this so please excuse if this suggestion looks stupid . Would this work?
if (sample_counter_value > 0) { | |
if (*sample_counter_value > 0) { | |
(*sample_counter_value)--; | |
return 0; | |
} | |
} | |
if (sample_counter_value && *sample_counter_value > 0) { | |
(*sample_counter_value)--; | |
return 0; | |
} |
if c == nil { | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be done we must expect the receiver to be initialised at all time. If it is not the case, then it is a programming error and must panic. By returning we are only shadowing a logical error.
@@ -74,6 +74,10 @@ func getProcessResUsage(process *collector_metric.ProcessMetrics, usageMetric st | |||
// UpdateProcessComponentEnergyByRatioPowerModel calculates the process energy consumption based on the energy consumption of the container that contains all the processes | |||
func UpdateProcessComponentEnergyByRatioPowerModel(processMetrics map[uint64]*collector_metric.ProcessMetrics, containerMetrics *collector_metric.ContainerMetrics, component, usageMetric string, wg *sync.WaitGroup) { | |||
defer wg.Done() | |||
if containerMetrics == nil || processMetrics == nil { | |||
klog.V(5).Infoln("containerMetrics or processMetrics is nil") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we return an error if the arguments aren't initialised properly?
bpfassets/libbpf/src/kepler.bpf.c
Outdated
u32 next_pid = ctx->next_pid; // the new pid that is to be scheduled | ||
======= | ||
>>>>>>> 2c38bc2dc7b9aca85374c16c96db555f16784169 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge left over
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the libbpf module conflict is hard to merge, now my git log gets quite messy. Will open a different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rootfs, could you also modify the approach for dropping samples? Instead of calculating the percentage, could we implement a method that aggregates a counter and drops samples accordingly?
For instance, after collecting 99 samples, we could skip the next 1 sample, effectively skipping 1% of the total. Then, if we decide to skip 10 samples, it would translate to a 10% reduction. To achieve this, we would need a mechanism to skip 'y' samples after gathering 'x' samples. This would provide us with the flexibility to adjust the dropout rate as needed.
the rebase was not successful, will reopen another PR |
fix #668
this reduces the bpf call overhead from 1352ns to 99ns
without this fix
with this fix
InitilizeGlobalVar