On Thu, Dec 19, 2024 at 07:31:35PM +0000, Ihor Solodrai wrote:
SNIP
> >
> >
> > why '* 4' ?
>
> This is an arbitrary limit, described in comments.
>
> If we allow the workers to pick up next cu for decoding as soon as
> it's ready, then the memory usage may greatly increase, if the stealer
> can't keep up with incoming work.
>
> If we want to avoid this there needs to be a limit on how many
> decoded, but not yet stolen, CUs we allow to hold in memory. When
> this limit is reached the workers will wait for more CUs to get
> stolen.
>
> N x 4 is a number I picked after trying various values and looking at
> the resulting memory usage.
I think we can pick some number and add reasoning to the comment
>
> We could make it configurable, but this value doesn't look to me as a
> reasonable user-facing option. Maybe we could add "I don't care about
> memory usage" flag to pahole? wdyt?
--I-don-t-care-about-memory-usage sounds great :-) but I think constant with
some comment will be enough for now and we'll see if we need it in future
>
> >
> > > +
> > > + /* fill up the queue with nr_workers JOB_DECODE jobs */
> > > + for (int i = 0; i < nr_workers; i++) {
> > > + job = calloc(1, sizeof(*job));
> >
> >
> > missing job != NULL check
> >
> > > + job->type = JOB_DECODE;
> > > + /* no need for locks, workers were not started yet */
> > > + list_add(&job->node, &cus_processing_queue.jobs);
> > > }
> > >
> > > - for (i = 0; i < dcus->conf->nr_jobs; ++i) {
> > > - dthr[i].dcus = dcus;
> > > - dthr[i].data = thread_data[i];
> > > + if (dcus->error)
> > > + return dcus->error;
> > >
> > > - dcus->error = pthread_create(&threads[i], NULL,
> > > - dwarf_cus__process_cu_thread,
> > > - &dthr[i]);
> > > + for (int i = 0; i < nr_workers; ++i) {
> > > + dcus->error = pthread_create(&workers[i], NULL,
> > > + dwarf_loader__worker_thread,
> > > + dcus);
> > > if (dcus->error)
> > > goto out_join;
> > > }
> > > @@ -3596,54 +3766,19 @@ static int dwarf_cus__threaded_process_cus(struct dwarf_cus *dcus)
> > > dcus->error = 0;
> > >
> > > out_join:
> > > - while (--i >= 0) {
> > > + for (int i = 0; i < nr_workers; ++i) {
> >
> >
> > I think you should keep the original while loop to cleanup/wait only for
> > threads that we actually created
>
> Do you mean in case of an error from pthread_create? Ok.
yes, thanks
jirka
