Re: [PATCH bpf-next v2] bpf: Better build_id caching in stack_map_get_build_id_offset()

[Ihor Solodrai <ihor.solodrai@linux.dev>]

On 6/12/26 3:37 PM, bot+bpf-ci@kernel.org wrote:
>> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
>> index 77ba03216c09..822b5cb188ea 100644
>> --- a/kernel/bpf/stackmap.c
>> +++ b/kernel/bpf/stackmap.c
> 
> [ ... ]
> 
>> @@ -343,8 +410,8 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
>>  	struct mmap_unlock_irq_work *work = NULL;
>>  	bool irq_work_busy = bpf_mmap_unlock_get_irq_work(&work);
>>  	bool has_user_ctx = user && current && current->mm;
>> -	struct vm_area_struct *vma, *prev_vma = NULL;
>> -	const unsigned char *prev_build_id = NULL;
>> +	struct stack_map_build_id_cache cache = {};
>> +	struct vm_area_struct *vma;
>>  	int i;
>>
>>  	if (may_fault && has_user_ctx) {
> 
> [ ... ]
> 
>> @@ -365,27 +432,29 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
>>
>>  	for (i = 0; i < trace_nr; i++) {
>>  		u64 ip = READ_ONCE(id_offs[i].ip);
>> -		u64 offset;
>>
>> -		if (prev_build_id && range_in_vma(prev_vma, ip, ip)) {
>> -			vma = prev_vma;
>> -			offset = stack_map_build_id_offset(vma->vm_pgoff, vma->vm_start, ip);
>> -			stack_map_build_id_set_valid(&id_offs[i], offset, prev_build_id);
>> +		if (!stack_map_build_id_set_from_cache(&cache, &id_offs[i], ip))
>>  			continue;
>> -		}
>> +
>>  		vma = find_vma(current->mm, ip);
>>  		if (!vma || vma_is_anonymous(vma) ||
>>  		    fetch_build_id(vma, id_offs[i].build_id, may_fault)) {
>> -			/* per entry fall back to ips */
>> +			/* per entry fall back to ips; cache build-ID-less range */
>>  			stack_map_build_id_set_ip(&id_offs[i]);
>> -			prev_vma = vma;
>> -			prev_build_id = NULL;
>> +			if (vma)
>> +				stack_map_build_id_cache_set_unresolved(&cache,
>> +						vma->vm_start, vma->vm_end);
>>  			continue;
>>  		}
>> -		offset = stack_map_build_id_offset(vma->vm_pgoff, vma->vm_start, ip);
>> -		stack_map_build_id_set_valid(&id_offs[i], offset, id_offs[i].build_id);
>> -		prev_vma = vma;
>> -		prev_build_id = id_offs[i].build_id;
>> +		/* mmap_lock is held for the whole loop, so the cached VMA
>> +		 * fields stay valid; no file pinning is needed here.
>> +		 */
>                 ^^^^
> 
> This isn't a bug, but should the comment text start on the line after the
> opening /* to match the BPF multi-line comment style?

sigh... Looks like this also was present in v1, and I missed it.

I'll wait in case there is more feedback before making a v3.

> 
> The other multi-line comments added in this patch follow that convention,
> such as in the new stack_map_build_id_cache structure and the helper
> functions.
> 
>> +		stack_map_build_id_set_valid(&id_offs[i],
>> +			stack_map_build_id_offset(vma->vm_pgoff, vma->vm_start, ip),
>> +			id_offs[i].build_id);
>> +		stack_map_build_id_cache_set_resolved(&cache, NULL, id_offs[i].build_id,
>> +						      vma->vm_start, vma->vm_end,
>> +						      vma->vm_pgoff);
>>  	}
>>  	bpf_mmap_unlock_mm(work, current->mm);
>>  }
> 
> 
> ---
> 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/27446103592