Add RecursionMisses to access ProgramInfo recursion misses counter by olsajiri · Pull Request #1465 · cilium/ebpf

olsajiri · 2024-05-11T17:45:33Z

Adding RecursionMisses to access ProgramInfo recursion misses counter.

It does not depend on whether collection of statistics is enabled, the value is always counted.

lmb · 2024-05-13T11:37:36Z

info.go

+// it keeps the return values pattern as the other stats accessors and returns
+// bool (allways true) together with the counter value.
+func (pi *ProgramInfo) RecursionMisses() (uint64, bool) {
+	return pi.stats.recursionMisses, true


Which kernel did this appear on though? The boolean is meant to indicate whether the kernel exposes this flag in general, not whether stats were enabled (IIRC).

it got in v5.11 9ed9e9ba2337 bpf: Count the number of times recursion was prevented

I'm actually not sure how is the availability detected, both Runtime and RunCount
return values if pi.stats != nil which seems to be the case always AFAICS

I can keep the same checks as for the Runtime and RunCount

I'm actually not sure how is the availability detected, both Runtime and RunCount
return values if pi.stats != nil which seems to be the case always AFAICS

Can't remember who contributed it, but iirc they were planning to add a probe, and the bool was added to not have to break API later. Looks like the probe was never implemented, and looking back it hardly seems worth it to load a prog, call prog_run and verify if the run counter increases just so we can return that bool correctly.

In the case of RecursionMisses, that one feels unrealistic to try to reproduce reliably at all. I'd just drop the bool and add a comment about it being 5.11+.

That breaks when using newProgramInfoFromProc. So unless we deprecate that we should just stick with the established pattern of returning a bool.

ok, keeping the bool then, thanks

That breaks when using newProgramInfoFromProc. So unless we deprecate that we should just stick with the established pattern of returning a bool.

@lmb Could you elaborate on what breaks exactly?

pi.stats is nil and therefore the line panics.

ti-mo

Thanks! Left some feedback. Let's just drop the bool, fix up the comment and call it a day.

info.go

ti-mo · 2024-05-14T08:45:34Z

info.go

+// it keeps the return values pattern as the other stats accessors and returns
+// bool (allways true) together with the counter value.
+func (pi *ProgramInfo) RecursionMisses() (uint64, bool) {
+	return pi.stats.recursionMisses, true


I'm actually not sure how is the availability detected, both Runtime and RunCount
return values if pi.stats != nil which seems to be the case always AFAICS

Can't remember who contributed it, but iirc they were planning to add a probe, and the bool was added to not have to break API later. Looks like the probe was never implemented, and looking back it hardly seems worth it to load a prog, call prog_run and verify if the run counter increases just so we can return that bool correctly.

In the case of RecursionMisses, that one feels unrealistic to try to reproduce reliably at all. I'd just drop the bool and add a comment about it being 5.11+.

lmb · 2024-05-17T12:50:18Z

Pushed a fixup, PTAL. Looking at the upstream commit it's not clear to me where the idea that this is related to program statistics comes from? Seems easier to me to just treat these things as completely independent.

Just returning pi.stats.recursionMisses, true doesn't work due to newProgramInfoFromProc. Reworked to just use a separate field which works around this problem.

olsajiri · 2024-05-23T07:10:42Z

hm, it's per program stats collected under same struct in kernel together with run count and run time

struct bpf_prog_stats {
        u64_stats_t cnt;
        u64_stats_t nsecs;
        u64_stats_t misses;
        struct u64_stats_sync syncp;
} __aligned(2 * sizeof(u64));

it's incremented through bpf_prog_inc_misses_counter call any time there's recursion detected on link and we can't run the program

it makes sense to me to place it under programStats

lmb · 2024-05-23T10:46:34Z

I can see your point, moving it out of the struct doesn't really add value. I've changed it back, fixed the panic and added a comment.

mmat11 · 2024-05-23T22:15:40Z

info.go

 	runtime time.Duration
 	// Total number of times the program was called.
 	runCount uint64
+	// Total number of timer the programm was NOT called.


Suggested change

// Total number of timer the programm was NOT called.

// Total number of times the program was NOT called.

Adding RecursionMisses to access ProgramInfo recursion misses counter. This value is always measured on supported kernels, regardless of statistics being enabled or not. Signed-off-by: Lorenz Bauer <[email protected]> Signed-off-by: Jiri Olsa <[email protected]>

lmb · 2024-05-28T09:47:47Z

CI failures are due to unrelated upstream changes.

Stale

olsajiri requested a review from a team as a code owner May 11, 2024 17:45

olsajiri marked this pull request as draft May 11, 2024 17:45

olsajiri marked this pull request as ready for review May 11, 2024 18:49

lmb reviewed May 13, 2024

View reviewed changes

ti-mo previously requested changes May 14, 2024

View reviewed changes

olsajiri force-pushed the prog_recursion_misses branch from c320be3 to 1210b92 Compare May 15, 2024 18:46

lmb force-pushed the prog_recursion_misses branch from 74e9661 to 08e672b Compare May 23, 2024 10:45

mmat11 reviewed May 23, 2024

View reviewed changes

olsajiri force-pushed the prog_recursion_misses branch from 08e672b to ddc6323 Compare May 28, 2024 07:16

olsajiri force-pushed the prog_recursion_misses branch from ddc6323 to b4bd7f6 Compare May 28, 2024 07:17

lmb approved these changes May 28, 2024

View reviewed changes

lmb merged commit b8dc0ee into cilium:main May 28, 2024

	// Total number of timer the programm was NOT called.
	// Total number of times the program was NOT called.

Comments

Conversation

olsajiri commented May 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ti-mo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmb commented May 17, 2024

Uh oh!

olsajiri commented May 23, 2024

Uh oh!

lmb commented May 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmb commented May 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants