lib: cgen: group same-key maps using bitmasks

[pzmarzly]

Closes #381.

We're grouping all sets in a chain by key format. Then, sets of the same format will turn into a single BPF map, with bitmasks in its values that indicate which sets the element appears in. With the 64 maps per program limit, we should now be able to have 64 unique key formats per program, which is much less likely to hit than just 64 sets.

---

There is an exception for LPM tries though. BPF_FUNC_map_lookup_elem on an LPM trie performs longest-prefix-match. So for a ruleset like:

chain demo BF_HOOK_XDP ACCEPT
    set a (ip4.snet) in { 192.168.1.0/24 }
    set b (ip4.snet) in { 192.168.0.0/16 }
    rule (ip4.snet) in b counter DROP

If we grouped a and b together, and got packet from 192.168.1.1, BPF_FUNC_map_lookup_elem would return set a (key: 192.168.1.0/24, value: 0x01) and packet would be accepted.

Therefore, LPM-based sets are excluded from merging.

[pzmarzly]

@claude review

[github-actions]

Claude review of PR #520 (a7ebb96)

Must fix

• assert after dereference — src/libbpfilter/cgen/program.c:693 — fixed in v2: assert is now before any dereference
• missing NULL check on bf_vector_get — src/libbpfilter/cgen/program.c:868 — no longer applicable: code was restructured
• missing assert in cleanup function — src/libbpfilter/cgen/program.c:103 — fixed: _bf_set_group_free now has assert(group) at the top
• LPM trie read-modify-write correctness — src/libbpfilter/cgen/program.c:918 — addressed by design: LPM trie sets are excluded from grouping (each gets its own single-set group where the value is always 0x01)

Suggestions

• BPF_ALU64_IMM to BPF_ALU32_IMM — src/libbpfilter/cgen/matcher/set.c:85 — fixed in v2: now uses BPF_ALU32_IMM
• duplicated group computation — src/libbpfilter/cgen/matcher/set.c:35 — fixed in v2: grouping now centralized in _bf_program_build_set_groups
• consolidate parallel arrays — src/libbpfilter/cgen/program.c:700 — no longer applicable: restructured to use bf_set_group
• NULL set name in format strings — fixed: uses ?: operator throughout (set->name ?: "<anonymous>")
• total_elems overestimate — src/libbpfilter/cgen/program.c:925 — total_elems over-counts when keys overlap across sets; safe but wastes kernel map capacity
• group->map assignment ordering — src/libbpfilter/cgen/program.c:959 — non-owning alias stored before ownership transfer; fragile against future early returns
• e2e isolation test missing exit check — fixed: ping exit code is verified via || { echo ...; exit 1; }
• per-element syscall regression — src/libbpfilter/cgen/program.c:940 — the read-modify-write loop does 2 syscalls per element; for single-set groups (the common case) this is pure overhead vs. the old batch-update path
• silently swallowed lookup error — src/libbpfilter/cgen/program.c:945 — bf_bpf_map_lookup_elem return value is discarded; unexpected errors (not ENOENT) go unnoticed

Nits

• unnecessary calloc cast — src/libbpfilter/cgen/program.c:721 — no longer applicable
• %lu for size_t — src/libbpfilter/cgen/program.c:857 — pre-existing pattern throughout codebase; not a regression
• unnecessary bf_list_ops_free cast — no longer applicable: no cast present in current diff
• set_size param naming — src/libbpfilter/cgen/prog/map.h:74 — fixed: now uses n_elems and value_size
• @param set doc misleading — src/libbpfilter/cgen/prog/map.h:73 — doc says "Set to create the map from" but the set is now only used for key format, not elements

Workflow run

[qdeslandes]

Thanks for working on this, I think's it's a good first implementation, but there are a few things that could be improved to simplify it.

When grouping sets, using a dedicated structure would be helpful:

bf_set_group {
    key;
    sets;
};

When looping over sets, you can ignore the empty ones, then for non-empty ones find if a set group exists already and add it there. You don't necessary need a "leader".

Fixups can hold a pointer to a set directly, it will be easier to resolve them, instead of relying on a set index. This is possible as the fixup (and bf_program) aren't serialized, they're only temporary objects (which was not the case before).

[pzmarzly]

Fixups can hold a pointer to a set directly, it will be easier to resolve them, instead of relying on a set index. This is possible as the fixup (and bf_program) aren't serialized, they're only temporary objects (which was not the case before).

That's amazing. Trying this.

@claude review

[github-actions]

Claude: must-fix: For LPM trie maps, bf_bpf_map_lookup_elem performs longest-prefix matching, not exact-key matching. When inserting an element from set B (e.g. 192.168.1.0/24), the lookup may return the value for a shorter prefix from set A (e.g. 192.168.0.0/16) that was already inserted. The subsequent OR would incorrectly propagate set A's bit into set B's entry.

This means a packet matching 192.168.1.0/24 would appear to belong to both set A and set B, producing false positives. Consider either:

• Skipping the lookup for LPM trie groups and always writing only the current set's bit, or
• Preventing LPM trie sets from being grouped together when their prefixes can overlap.

[pzmarzly]

This means a packet matching 192.168.1.0/24 would appear to belong to both set A and set B

I mean, logically it does belong to both sets, so your example is not the best. However, thanks for bringing up LPM maps. They have footguns. Let's exclude them from grouping up.

[github-actions]

Claude: suggestion: The (void)bf_bpf_map_lookup_elem(...) discards the return value unconditionally. If the syscall fails for a reason other than ENOENT (e.g., EFAULT from a bad fd), the error is silently swallowed and the bitmask is built from zeroed data, which could cause silent incorrect behavior. Consider at least logging at debug level on unexpected failure:

int lr = bf_bpf_map_lookup_elem(new_map->fd, elem->data, value);
if (lr && lr != -ENOENT)
    bf_dbg("unexpected map lookup error: %d", lr);

[github-actions]

Claude: suggestion: The non-owning alias map_ref is stored before ownership transfer and assigned to group->map after the push. This is correct today, but fragile: if future code adds an early return between bf_list_push and group->map = map_ref, the group's map pointer will be left as NULL with no indication of what went wrong. Consider assigning group->map immediately after the successful push, or adding a brief comment noting the dependency.

[github-actions]

Claude: suggestion: total_elems sums all elements across all sets in the group, but keys shared across sets will occupy a single map entry. For groups with heavily overlapping sets this over-estimates the BPF map's max_entries, wasting kernel memory. Safe but worth a brief comment documenting the trade-off (or a cap to prevent requesting an excessively large map from the kernel).

[github-actions]

Claude: nit: The @param set documentation reads "Set to create the map from" but the set is now used only for its key format (elem_size, use_trie), not its elements. Consider updating to: "Set whose key format defines the map's key size and type."

[github-actions]

Claude: suggestion: The read-modify-write loop performs 2 syscalls (lookup + update) per element per set. For single-set groups (the common case and all LPM trie groups), the bitmask is always 0x01, so the lookup is pure overhead. Consider a fast path: when n_sets == 1, skip the lookup and unconditionally write a static 0x01 value, or use bf_bpf_map_update_batch as the old code did. This avoids a per-element syscall regression for chains that do not benefit from grouping.

[pzmarzly]

I think bf_bpf_map_update_batch is the right approach here, we can use bf_hashset to prepare final bf_map shape on userspace side. I'll try it later. However, it seems it's not a dealbreaker if we don't get it now - even with 2 syscalls per item, it only slows down load_maps 2-3x in microbenchmarks.