Re: [PATCH bpf-next v4 1/2] bpf: htab: Split htab_elem_lru and htab_elem_pcpu off of htab_elem

[bot+bpf-ci]

> @@ -476,7 +522,7 @@ static void htab_mem_dtor(void *obj, void *ctx)
>  	if (IS_ERR_OR_NULL(hrec->record))
>  		return;
>
> -	map_value = htab_elem_value(elem, hrec->key_size);
> +	map_value = (void *)elem + sizeof(struct htab_elem) + round_up(hrec->key_size, 8);
>  	bpf_obj_free_fields(hrec->record, map_value);
>  }

This isn't a bug, but would it be worth storing the element header offset
in struct htab_btf_record (next to key_size) and keeping htab_mem_dtor on
the shared htab_elem_value() path, rather than open-coding
sizeof(struct htab_elem) here? Everywhere else in the file the header
size is now the per-map htab->key_offset, reached through
htab_elem_key()/htab_elem_value(). The hardcoded form happens to be
correct today because htab_mem_dtor is only installed for non-preallocated,
non-percpu, non-LRU BPF_MAP_TYPE_HASH maps (LRU requires prealloc, and
HASH_OF_MAPS uses map_check_no_btf), which is exactly the case where
key_offset == sizeof(struct htab_elem). The invariant is invisible at the
call site and there is no comment or assertion tying the two together, so
the open-coded copy silently diverges from the accessor if another element
layout ever registers this dtor.

> @@ -869,17 +929,17 @@ static bool htab_lru_map_delete_node(void *arg, struct bpf_lru_node *node)
>  	struct bucket *b;
>  	int ret;
>
> -	tgt_l = container_of(node, struct htab_elem, lru_node);
> -	b = __select_bucket(htab, tgt_l->hash);
> +	tgt_l = (struct htab_elem *)container_of(node, struct htab_elem_lru, lru_node);
> +	b = __select_bucket(htab, htab_elem_hash(htab, tgt_l));

This isn't a bug, but the commit adds htab_elem_lru_node() for the
htab_elem to bpf_lru_node direction, while the inverse conversion stays
open-coded and is now duplicated verbatim at two sites (line 358 in
prealloc_lru_pop() and line 932 here), each needing an explicit
(struct htab_elem *) cast to silence the incompatible-pointer warning.
Would a small counterpart helper, e.g. htab_elem_from_lru_node(), keep
both directions symmetric and confine the cast to one place?

Commit message issue:

>     bpf: htab: Split htab_elem_lru and htab_elem_pcpu off of htab_elem
>
>     The htab_elem struct is used as the per-element type for all BPF hash
>     map types and includes bpf_lru_node in a union with a ptr_to_pptr
>     pointer. For standard (non-LRU, non-PCPU) hash maps, the 24 byte union
>     allocated for every element is entirely unused. For non-preallocated
>     PCPU maps, ptr_to_pptr only requires 8 bytes, leaving 16 bytes of unused
>     overhead in the union. For preallocated PCPU maps ptr_to_pptr is unused
>     since elements are freed to the PCPU freelist.
>
>     Eliminate this per-element memory overhead by splitting htab_elem into
>     dedicated structures for each map type:
>        - struct htab_elem: Minimal structure for standard hash maps and
>          preallocated PCPU maps (saves 24 bytes per element).
>        - struct htab_elem_pcpu: Structure for non-preallocated PCPU maps
>          containing ptr_to_pptr (saves 16 bytes per element).
>        - struct htab_elem_lru: Retains struct bpf_lru_node for LRU maps.
>
>     Because element sizes now vary by map type, add key_offset to struct
>     bpf_htab to track the dynamic key offset. Update helper accessors and
>     lookups to compute key and value offsets using htab->key_offset.
>
>     Pointers to struct htab_elem in the existing code (e.g. htab_elem_hash)
>     serve as generic base element pointers. This is possible because
>     htab_elem, htab_elem_pcpu, and htab_elem_lru share a common initial
>     sequence, making pointer casts safe.

This isn't a bug, but could the changelog gain a paragraph on the
map_in_map.c side? The patch also touches kernel/bpf/map_in_map.c and
kernel/bpf/map_in_map.h: it adds bpf_map_type_is_htab(), grows
inner_map_meta to sizeof(struct bpf_htab) via the new
bpf_htab_map_meta_size(), and copies key_offset into the meta map with the
new bpf_htab_map_meta_init(). That part is the non-obvious
correctness-critical consequence of making the key offset a runtime field,
since the verifier's inlined lookups read the offset out of the
inner_map_meta. A reviewer looking at the diff would most benefit from an
explanation of why inner_map_meta must now be sizeof(struct bpf_htab) and
have key_offset copied. Also, the sentence about making pointer casts safe
mentions the common initial sequence but doesn't name which casts rely on
it (htab_elem_lru_node(), htab_elem_get_ptr_to_pptr(),
htab_elem_set_ptr_to_pptr()).


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31651309438