Re: [PATCH bpf-next v3 2/2] bpf: htab: Reduce elem_size by 8 bytes for small key sizes

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Multi-line comments in the BPF subsystem must have the opening `/*` on its own line.
--

--- Patch [2]: [PATCH bpf-next v3 2/2] bpf: htab: Reduce elem_size by 8 bytes for small key sizes ---
commit 9160b7ed8c4c16e32cb4fa06437bbe88f7eb2eea
Author: T.J. Mercier <[email protected]>

bpf: htab: Reduce elem_size by 8 bytes for small key sizes

This commit optimizes BPF hash tables by eliminating the cached hash value
for maps with small keys. This reduces the element size and key offset for
standard and PCPU maps by up to 8 bytes.

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index f54366da459f5..9967268d453d0 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c

[ ... ]

> @@ -618,6 +625,13 @@ static struct bpf_map *htab_map_alloc(union bpf_attr *attr)
>  
>  	bpf_map_init_from_attr(&htab->map, attr);
>  
> +	/* Avoid hash memory use and comparisons where unnecessary.
> +	 * u32 hash reads are always atomic. If we elide them, key comparisons must also be atomic
> +	 * to avoid false positive key matches due to torn key reads / writes. This is only possible
> +	 * when the key fits within a word, so check key_size.
> +	 */

[Severity: Low]
This isn't a bug, but should this multi-line comment have the opening /* on
its own line to match the BPF subsystem comment style?

> +	htab->has_hash = htab_is_lru(htab) || htab->map.key_size > sizeof(unsigned long);
> +
>  	if (percpu_lru) {

[ ... ]

> @@ -761,10 +777,22 @@ static struct htab_elem *lookup_elem_raw(struct bpf_htab *htab,

[ ... ]

> +	} else {
> +		/* When hash is omitted, key comparisons must be atomic. Zero extend
> +		 * the caller's key to the word size to support an atomic compare.
> +		 */

[Severity: Low]
This isn't a bug, but does this comment format match the preferred multi-line
style for the BPF subsystem? It typically expects the opening /* to be on a
separate line.

> +		unsigned long k = 0;
> +
> +		memcpy(&k, key, key_size);

[ ... ]

> @@ -1188,7 +1226,17 @@ static struct htab_elem *alloc_htab_elem(struct bpf_htab *htab, void *key,

[ ... ]

> +	} else {
> +		/* Zero-extend key into k for an atomic write to support
> +		 * lockless RCU readers.
> +		 */

[Severity: Low]
This isn't a bug, but should the opening /* be placed on its own line here as
well?

> +		unsigned long k = 0;
> +
> +		memcpy(&k, key, key_size);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2