On 23/07/2025 09:39, Alexis Lothoré wrote:
> Hi Ihor,
>
> On Wed Jul 23, 2025 at 1:42 AM CEST, Ihor Solodrai wrote:
>> On 7/16/25 2:04 AM, Alexis Lothoré (eBPF Foundation) wrote:
>
> [...]
>
>>> cu__finalize(cu, cus, conf);
>>> - if (cus__steal_now(cus, cu, conf) == LSK__STOP_LOADING)
>>> + if (cus__steal_now(cus, cu, conf) == LSK__ABORT)
>>> goto out_abort;
>>
>> I think we should handle both LSK__STOP_LOADING and LSK__ABORT here,
>> otherwise the worker will keep loading DWARF (which is most of the
>> pahole's work) and passing cu-s to the stealer function.
>>
>> I guess in many cases it'll complete without errors, but it's just
>> unnecessary work.
>>
>> Maybe add a check for LSK__STOP_LOADING here with goto_ok?
>
> Hmmm, I am not sure how to follow you here. Even if cus__steal_now returns
> LSK__STOP_LOADING, I need to return DWARF_CB_OK from
> cus__merge_and_process_cu, otherwise it will be processed as an error
> again. Or are you suggesting just make sure to delete current cu but still
> return DWARF_CB_OK when getting LSK__STOP_LOADING, as done below ?
>
> --- a/dwarf_loader.c
> +++ b/dwarf_loader.c
> @@ -3796,6 +3796,7 @@ static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
> Dwarf_Off off = 0, noff;
> struct cu *cu = NULL;
> size_t cuhl;
> + int res;
>
> while (dwarf_nextcu(dw, off, &noff, &cuhl, NULL, &pointer_size,
> &offset_size) == 0) {
> @@ -3874,7 +3875,8 @@ static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
> goto out_abort;
>
> cu__finalize(cu, cus, conf);
> - if (cus__steal_now(cus, cu, conf) == LSK__ABORT)
> + res = cus__steal_now(cus, cu, conf);
> + if (res == LSK__STOP_LOADING || res == LSK__ABORT)
> goto out_abort;
>
> return 0;
> @@ -3882,7 +3884,7 @@ static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
> out_abort:
> dwarf_cu__delete(cu);
> cu__delete(cu);
> - return DWARF_CB_ABORT;
> + return res == LSK__STOP_LOADING ? DWARF_CB_OK : DWARF_CB_ABORT;
> }
>
>
> Thanks,
>
> Alexis
>
>
Good point; I think ideally we'd like to stop processing in either case
but still distinguish error from early exit. We could perhaps set the
appropriate LSK_ value in dcus (dcus->lsk_status?) to make the
distinction and use it in cus__load_module(), renaming out_error in
dwarf_cus__process_cu() to out_early or something to underline the fact
that it's an early exit not necessarily a failure. So we preserve the
return of CB_ABORT for both failure and early exit, but can still tell
these apart using dcus->lsk_status. Would that work?
