Re: [PATCH bpf-next v3 1/5] bpf: lru: Tidy hash handling in LRU code

[Leon Hwang <leon.hwang@linux.dev>]

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 <leon.hwang@linux.dev>
>> ---
>>   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