Messages in this thread | ![/](/images/icornerl.gif) | | From | Namhyung Kim <> | Date | Thu, 29 Feb 2024 17:36:05 -0800 | Subject | Re: [PATCH v3 4/7] perf machine: Move machine's threads into its own abstraction |
| |
On Wed, Feb 28, 2024 at 10:33 PM Ian Rogers <irogers@google.com> wrote: > > Move thread_rb_node into the machine.c file. This hides the > implementation of threads from the rest of the code allowing for it to > be refactored. > > Locking discipline is tightened up in this change. As the lock is now > encapsulated in threads, the findnew function requires holding it (as > it already did in machine). Rather than do conditionals with locks > based on whether the thread should be created (which could potentially > be error prone with a read lock match with a write unlock), have a > separate threads__find that won't create the thread and only holds the > read lock. This effectively duplicates the findnew logic, with the > existing findnew logic only operating under a write lock assuming > creation is necessary as a previous find failed. The creation may > still fail with the write lock due to another thread. The duplication > is removed in a later next patch that delegates the implementation to > hashtable. > > Signed-off-by: Ian Rogers <irogers@google.com>
Thanks for doing this! A nit below..
> --- [SNIP] > @@ -3228,27 +3258,31 @@ int thread__resolve_callchain(struct thread *thread, > return ret; > } > > -int machine__for_each_thread(struct machine *machine, > - int (*fn)(struct thread *thread, void *p), > - void *priv) > +int threads__for_each_thread(struct threads *threads, > + int (*fn)(struct thread *thread, void *data), > + void *data) > { > - struct threads *threads; > - struct rb_node *nd; > - int rc = 0; > - int i; > + for (int i = 0; i < THREADS__TABLE_SIZE; i++) { > + struct threads_table_entry *table = &threads->table[i]; > + struct rb_node *nd; > > - for (i = 0; i < THREADS__TABLE_SIZE; i++) { > - threads = &machine->threads[i]; > - for (nd = rb_first_cached(&threads->entries); nd; > - nd = rb_next(nd)) { > + for (nd = rb_first_cached(&table->entries); nd; nd = rb_next(nd)) { > struct thread_rb_node *trb = rb_entry(nd, struct thread_rb_node, rb_node); > + int rc = fn(trb->thread, data); > > - rc = fn(trb->thread, priv); > if (rc != 0) > return rc; > } > } > - return rc; > + return 0;
Don't we need locking in this function?
Thanks, Namhyung
> + > +} > + > +int machine__for_each_thread(struct machine *machine, > + int (*fn)(struct thread *thread, void *p), > + void *priv) > +{ > + return threads__for_each_thread(&machine->threads, fn, priv); > } >
| ![\](/images/icornerr.gif) |