Skip to content

lib: cgen: add per-rule state map for log rate limiting#553

Merged
qdeslandes merged 2 commits into
facebook:mainfrom
qdeslandes:feature/log_ratelimit
May 27, 2026
Merged

lib: cgen: add per-rule state map for log rate limiting#553
qdeslandes merged 2 commits into
facebook:mainfrom
qdeslandes:feature/log_ratelimit

Conversation

@qdeslandes
Copy link
Copy Markdown
Contributor

Rules with a log action currently emit one entry per matching packet with no throttling. To support rate-limited logging, BPF programs need mutable per-rule state that persists across invocations.

Introduce bf_smap, a single-entry BPF_MAP_TYPE_ARRAY map per chain whose value is a flat array of bf_rule_state entries (one per rule). The map is looked up once in the program prologue and the base pointer stored in bf_runtime.state_map; per-rule access is then a constant offset, avoiding one bpf_map_lookup_elem call per rule at packet time.

For each rule with log_rate_ns set, the generated code reads last_log_ts, calls bpf_ktime_get_ns(), and skips the log elfstub if the elapsed time falls short of the configured interval. R9 (callee-saved) holds the state entry pointer across the ktime call. Writes to last_log_ts are best-effort: no CAS, but a naturally aligned 8-byte store is atomic on x86-64, so no torn writes occur.

Expose the rate in the DSL as 'log [] every {ns,us,ms,s}' and wire it to the new bf_rule.log_rate_ns field serialized alongside the existing rule fields.

@meta-cla meta-cla Bot added the cla signed label May 27, 2026
@qdeslandes
Copy link
Copy Markdown
Contributor Author

@claude review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 27, 2026

Claude review of PR #553 (f0f37fc)

Suggestions

  • Overflow in _parse_log_ratesrc/bfcli/parser.y:80strtod can return infinity for very large inputs; the subsequent cast to uint64_t is undefined behavior. Add a !isfinite(v) or upper-bound check before the cast.
  • Unreferenced BTF type entrysrc/libbpfilter/cgen/prog/map.c:116 — The BF_MAP_TYPE_STATE BTF case creates an extra u32 type that is never referenced. Remove the extraneous btf__add_int call.
  • R9 documentation outdatedsrc/libbpfilter/cgen/program.h:51 — The header documents r9: unused but R9 is now used as the per-rule state pointer during rate-limited log codegen.
  • Test dummy enables rate limit unconditionallytests/harness/fake.c:193 — Setting log_rate_ns on the default dummy rule means all tests using it exercise rate-limited codegen exclusively. Consider preserving coverage for both paths.

Nits

  • RST list formattingdoc/usage/bfcli.rst:440 — The blank line before the every paragraph may break the list grouping in rendered output.

Workflow run

Comment thread src/bfcli/parser.y
double v;
char *end;

v = strtod(s, &end);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude: suggestion: _parse_log_rate does not guard against overflow. If the user provides a very large number (e.g., 99999999999999999s), strtod returns HUGE_VAL/infinity which passes the v <= 0.0 check, and (uint64_t)(HUGE_VAL * multiplier + 0.5) is undefined behavior (casting a non-representable float to integer). Consider adding a check:

if (!isfinite(v) || v > (double)UINT64_MAX / multiplier)
    return -1;

btf__add_field(kbtf, "count", 1, 0, 0);
btf__add_field(kbtf, "size", 1, 64, 0);
break;
case BF_MAP_TYPE_STATE:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude: suggestion: The BTF construction for BF_MAP_TYPE_STATE creates an extra btf__add_int(kbtf, "u32", 4, 0) (the one before btf->key_type_id = ...) that produces an unreferenced BTF type entry. The BF_MAP_TYPE_COUNTERS case only pre-creates types it actually references. Consider removing the extraneous call to keep the BTF section clean.

Comment thread tests/harness/fake.c
Comment thread doc/usage/bfcli.rst
Rules with a log action currently emit one entry per matching packet
with no throttling. To support rate-limited logging, BPF programs need
mutable per-rule state that persists across invocations.

Introduce bf_smap, a single-entry BPF_MAP_TYPE_ARRAY map per chain
whose value is a flat array of bf_rule_state entries (one per rule).
The map is looked up once in the program prologue and the base pointer
stored in bf_runtime.state_map; per-rule access is then a constant
offset, avoiding one bpf_map_lookup_elem call per rule at packet time.

For each rule with log_rate_ns set, the generated code reads
last_log_ts, calls bpf_ktime_get_ns(), and skips the log elfstub if
the elapsed time falls short of the configured interval. R9
(callee-saved) holds the state entry pointer across the ktime call.
Writes to last_log_ts are best-effort: no CAS, but a naturally aligned
8-byte store is atomic on x86-64, so no torn writes occur.

Expose the rate in the DSL as 'log [<headers>] every <N>{ns,us,ms,s}'
and wire it to the new bf_rule.log_rate_ns field serialized alongside
the existing rule fields.
Stale bpffs pins from a previous benchmark run with a different binary
can cause deserialization failures when the new binary tries to reload
them (e.g. missing map keys added since the pin was written). Flush
/sys/fs/bpf/bpfilter before each run to start from a clean state.
@qdeslandes qdeslandes force-pushed the feature/log_ratelimit branch from 81e5c26 to bb247f3 Compare May 27, 2026 13:17
@qdeslandes qdeslandes enabled auto-merge (rebase) May 27, 2026 13:19
@qdeslandes qdeslandes merged commit ac7c90c into facebook:main May 27, 2026
38 checks passed
@qdeslandes qdeslandes deleted the feature/log_ratelimit branch May 27, 2026 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant