On 15/1/26 02:44, Martin KaFai Lau wrote:
>
>
> On 1/7/26 7:14 AM, Leon Hwang wrote:
>> The hash field is not used by the LRU list itself.
>>
>> Setting hash while manipulating the LRU list also obscures the intent
>> of the code and makes it harder to follow.
>>
>> Tidy this up by moving the hash assignment to prealloc_lru_pop(),
>> where the element is prepared for insertion into the hash table.
>>
>> Signed-off-by: Leon Hwang <[email protected]>
>> ---
>> kernel/bpf/bpf_lru_list.c | 24 +++++++++---------------
>> kernel/bpf/bpf_lru_list.h | 5 ++---
>> kernel/bpf/hashtab.c | 5 ++---
>> 3 files changed, 13 insertions(+), 21 deletions(-)
>>
>> diff --git a/kernel/bpf/bpf_lru_list.c b/kernel/bpf/bpf_lru_list.c
>> index e7a2fc60523f..f4e183a9c28f 100644
>> --- a/kernel/bpf/bpf_lru_list.c
>> +++ b/kernel/bpf/bpf_lru_list.c
>> @@ -344,10 +344,8 @@ static void bpf_lru_list_pop_free_to_local(struct
>> bpf_lru *lru,
>> static void __local_list_add_pending(struct bpf_lru *lru,
>> struct bpf_lru_locallist *loc_l,
>> int cpu,
>> - struct bpf_lru_node *node,
>> - u32 hash)
>> + struct bpf_lru_node *node)
>> {
>> - *(u32 *)((void *)node + lru->hash_offset) = hash;
>> node->cpu = cpu;
>> node->type = BPF_LRU_LOCAL_LIST_T_PENDING;
>> bpf_lru_node_clear_ref(node);
>> @@ -393,8 +391,7 @@ __local_list_pop_pending(struct bpf_lru *lru,
>> struct bpf_lru_locallist *loc_l)
>> return NULL;
>> }
>> -static struct bpf_lru_node *bpf_percpu_lru_pop_free(struct bpf_lru
>> *lru,
>> - u32 hash)
>> +static struct bpf_lru_node *bpf_percpu_lru_pop_free(struct bpf_lru *lru)
>> {
>> struct list_head *free_list;
>> struct bpf_lru_node *node = NULL;
>> @@ -415,7 +412,6 @@ static struct bpf_lru_node
>> *bpf_percpu_lru_pop_free(struct bpf_lru *lru,
>> if (!list_empty(free_list)) {
>> node = list_first_entry(free_list, struct bpf_lru_node, list);
>> - *(u32 *)((void *)node + lru->hash_offset) = hash;
>> bpf_lru_node_clear_ref(node);
>> __bpf_lru_node_move(l, node, BPF_LRU_LIST_T_INACTIVE);
>
> init the hash value later (after releasing l->lock) is not correct. The
> node is in the inactive list. The inactive list is one of the rotate and
> _evict_ candidates, meaning tgt_l->hash will be used in
> htab_lru_map_delete_node(). In practice, it does not matter if
> htab_lru_map_delete_node() cannot find the node in an incorrect bucket.
> However, it still should not use an uninitialized value to begin with.
>
Thanks for the explanation — this is the part I missed earlier.
Without additional context or comments in the code, it was not obvious
why the hash needs to be set at that point.
I’ll drop this change as-is. If you have suggestions for a clearer or
better way to handle the hash assignment while preserving the required
ordering, I’d appreciate your guidance.
Thanks,
Leon
