On Wednesday, January 1st, 2025 at 8:56 AM, Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
>
> On Sat, Dec 21, 2024 at 01:23:38AM +0000, Ihor Solodrai wrote:
>
> SNIP
>
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index 90f1b9a..7e03ba4 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -1326,7 +1326,7 @@ static void btf_encoder__delete_saved_funcs(struct btf_encoder *encoder)
> > }
> > }
> >
> > -int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
> > +static int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
> > {
> > struct btf_encoder_func_state **saved_fns, *s;
> > struct btf_encoder *e = NULL;
> > @@ -2134,7 +2134,6 @@ int btf_encoder__encode(struct btf_encoder *encoder, struct conf_load *conf)
> > int err;
> > size_t shndx;
> >
> > - /* for single-threaded case, saved funcs are added here */
> > btf_encoder__add_saved_funcs(conf->skip_encoding_btf_inconsistent_proto);
>
>
> should we check the return value in here? now it's the only caller
Yes, thanks.
>
> SNIP
>
> > -struct dwarf_thread {
> > - struct dwarf_cus *dcus;
> > - void data;
> > +/ Multithreading is implemented using a job/worker model.
> > + * cus_processing_queue represents a collection of jobs to be
> > + * completed by workers.
> > + * dwarf_loader__worker_thread is the worker loop, taking jobs from
> > + * the queue, executing them and re-enqueuing new jobs as necessary.
> > + * Implementation of this queue ensures two constraints:
> > + * * JOB_STEAL jobs for a CU are executed in the order of cu->id, as
> > + * a consequence JOB_STEAL jobs always run one at a time.
> > + * * Initial number of JOB_DECODE jobs in the queue is effectively a
> > + * limit on how many decoded CUs can be held in memory.
> > + * See dwarf_loader__decoded_cus_limit()
> > + /
> > +static struct {
> > + pthread_mutex_t mutex;
> > + pthread_cond_t job_added;
> > + / next_cu_id determines the next CU ready to be stealed
> > + * This enforces the order of CU stealing.
> > + */
> > + uint32_t next_cu_id;
> > + struct list_head jobs;
> > +} cus_processing_queue;
> > +
> > +enum job_type {
> > + JOB_NONE = 0,
>
>
> nit, no need for JOB_NONE?
I find it useful for the default value to not be a valid "type". This
helps to avoid bugs when an object is initialized, but the type has
not been set explicitly (even though it should be).
>
> SNIP
>
> > +static struct cu_processing_job *cus_queue__try_dequeue(void)
> > +{
> > + struct cu_processing_job *job, *dequeued_job = NULL;
> > + struct list_head *pos, tmp;
> > +
> > + list_for_each_safe(pos, tmp, &cus_processing_queue.jobs) {
> > + job = list_entry(pos, struct cu_processing_job, node);
> > + if (job->type == JOB_STEAL && job->cu->id == cus_processing_queue.next_cu_id) {
> > + dequeued_job = job;
> > + break;
> > + }
> > + if (job->type == JOB_DECODE) {
> > + / all JOB_STEALs are added to the head, so no viable JOB_STEAL available /
> > + dequeued_job = job;
> > + break;
> > + }
> > + }
> > + / No jobs or only steals out of order */
> > + if (!dequeued_job)
> > + return NULL;
> > +
> > + list_del(&dequeued_job->node);
> > + return job;
>
>
> IIUC job == dequeued_job at this point, but I think we should return
> dequeued_job to be clear
Agree.
>
> SNIP
>
> > -static void *dwarf_cus__process_cu_thread(void *arg)
> > +static struct cu *dwarf_loader__decode_next_cu(struct dwarf_cus *dcus)
> > {
> > - struct dwarf_thread *dthr = arg;
> > - struct dwarf_cus *dcus = dthr->dcus;
> > uint8_t pointer_size, offset_size;
> > + struct dwarf_cu *dcu = NULL;
> > Dwarf_Die die_mem, *cu_die;
> > - struct dwarf_cu *dcu;
> > + int err;
> >
> > - while (dwarf_cus__nextcu(dcus, &dcu, &die_mem, &cu_die, &pointer_size, &offset_size) == 0) {
> > - if (cu_die == NULL)
> > + err = dwarf_cus__nextcu(dcus, &dcu, &die_mem, &cu_die, &pointer_size, &offset_size);
> > +
> > + if (err < 0)
> > + goto out_error;
> > + else if (err == 1) /* no more CUs */
> > + return NULL;
> > +
> > + err = die__process_and_recode(cu_die, dcu->cu, dcus->conf);
> > + if (err)
> > + goto out_error;
> > + if (cu_die == NULL)
> > + return NULL;
>
>
> should this check be placed before calling die__process_and_recode ?
Yes, but actually this check is redundant. If dwarf_cus__nextcu
returns 0, then the cu_die is valid. There are null checks within
dwarf_cus__nextcu:
if (ret == 0 && *cu_die != NULL) {
*dcu = dwarf_cus__create_cu(dcus, *cu_die, *pointer_size);
if (*dcu == NULL) {
dcus->error = ENOMEM;
ret = -1;
goto out_unlock;
}
// Do it here to keep all CUs in cus->cus in the same
// order as in the DWARF file being loaded (e.g. vmlinux)
__cus__add(dcus->cus, (*dcu)->cu);
}
>
>
> SNIP
>
> > -static int dwarf_cus__process_cus(struct dwarf_cus *dcus)
> > -{
> > - if (dcus->conf->nr_jobs > 1)
> > - return dwarf_cus__threaded_process_cus(dcus);
> > -
> > - return __dwarf_cus__process_cus(dcus);
> > -}
> > -
> > static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
> > Dwfl_Module *mod, Dwarf *dw, Elf *elf,
> > const char *filename,
> > @@ -3733,7 +3859,8 @@ static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
> > if (cu__resolve_func_ret_types_optimized(cu) != LSK__KEEPIT)
> > goto out_abort;
> >
> > - if (cus__finalize(cus, cu, conf, NULL) == LSK__STOP_LOADING)
> > + cu__finalize(cu, cus, conf);
> > + if (cus__steal_now(cus, cu, conf) == LSK__STOP_LOADING)
> > goto out_abort;
> >
> > return 0;
> > @@ -3765,7 +3892,8 @@ static int cus__load_module(struct cus *cus, struct conf_load *conf,
> > }
> >
> > if (type_cu != NULL) {
> > - type_lsk = cu__finalize(type_cu, cus, conf, NULL);
> > + cu__finalize(type_cu, cus, conf);
> > + type_lsk = cus__steal_now(cus, type_cu, conf);
> > if (type_lsk == LSK__DELETE) {
> > cus__remove(cus, type_cu);
> > }
> > @@ -3787,6 +3915,7 @@ static int cus__load_module(struct cus *cus, struct conf_load *conf,
> > .type_dcu = type_cu ? &type_dcu : NULL,
> > .build_id = build_id,
> > .build_id_len = build_id_len,
> > + .nr_cus_created = 0,
>
>
> should go to the previous patch? if needed at all..
Yeah. I think it's better to have an explicit assignment.
>
> thanks,
> jirka
>
> > };
> > res = dwarf_cus__process_cus(&dcus);
> > }
> > diff --git a/dwarves.c b/dwarves.c
> > index ae512b9..7c3e878 100644
> > --- a/dwarves.c
> > +++ b/dwarves.c
> > @@ -530,48 +530,6 @@ void cus__unlock(struct cus *cus)
> > pthread_mutex_unlock(&cus->mutex);
> > }
>
>
> SNIP
