On Mon, Jan 24, 2022 at 11:19 AM Kui-Feng Lee <kuifeng@xxxxxx> wrote:
>
> Create an instance of btf for each worker thread, and add type info to
> the local btf instance in the steal-function of pahole without mutex
> acquiring. Once finished with all worker threads, merge all
> per-thread btf instances to the primary btf instance.
>
> Signed-off-by: Kui-Feng Lee <kuifeng@xxxxxx>
> ---
BTW, I've already synced your btf__add_btf() optimization to Github,
so you can bump pahole libbpf submodule reference in the next
revision. You'll get a bunch of deprecation warnings, so it would be
great to fix those at the same time.
> btf_encoder.c | 5 +++
> btf_encoder.h | 2 +
> pahole.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++---
> 3 files changed, 116 insertions(+), 5 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 9d015f304e92..56a76f5d7275 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -1529,3 +1529,8 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu)
> out:
> return err;
> }
> +
> +struct btf *btf_encoder__btf(struct btf_encoder *encoder)
> +{
> + return encoder->btf;
> +}
> diff --git a/btf_encoder.h b/btf_encoder.h
> index f133b0d7674d..0f0eee84df74 100644
> --- a/btf_encoder.h
> +++ b/btf_encoder.h
> @@ -29,4 +29,6 @@ struct btf_encoder *btf_encoders__first(struct list_head *encoders);
>
> struct btf_encoder *btf_encoders__next(struct btf_encoder *encoder);
>
> +struct btf *btf_encoder__btf(struct btf_encoder *encoder);
> +
> #endif /* _BTF_ENCODER_H_ */
> diff --git a/pahole.c b/pahole.c
> index f3eeaaca4cdf..93b844f97c38 100644
> --- a/pahole.c
> +++ b/pahole.c
> @@ -2798,6 +2798,73 @@ out:
>
> static struct type_instance *header;
>
> +struct thread_data {
> + struct btf *btf;
> + struct btf_encoder *encoder;
> +};
> +
> +static int pahole_threads_prepare(struct conf_load *conf, int nr_threads, void **thr_data)
> +{
> + int i;
> + struct thread_data *threads = calloc(sizeof(struct thread_data), nr_threads);
> +
> + for (i = 0; i < nr_threads; i++)
> + thr_data[i] = threads + i;
> +
> + return 0;
> +}
> +
> +static int pahole_thread_exit(struct conf_load *conf, void *thr_data)
> +{
> + struct thread_data *thread = thr_data;
> +
> + if (thread == NULL)
> + return 0;
> +
> + /*
> + * Here we will call btf__dedup() here once we extend
> + * btf__dedup().
> + */
> +
> + return 0;
> +}
> +
> +static int pahole_threads_collect(struct conf_load *conf, int nr_threads, void **thr_data,
> + int error)
> +{
> + struct thread_data **threads = (struct thread_data **)thr_data;
> + int i;
> + int err = 0;
> +
> + if (error)
> + goto out;
> +
> + for (i = 0; i < nr_threads; i++) {
> + /*
> + * Merge content of the btf instances of worker
> + * threads to the btf instance of the primary
> + * btf_encoder.
> + */
> + if (!threads[i]->btf || threads[i]->encoder == btf_encoder)
> + continue; /* The primary btf_encoder */
> + err = btf__add_btf(btf_encoder__btf(btf_encoder), threads[i]->btf);
You've ignored my question and suggestion to move btf__add_btf() into
pahole_thread_exit. Can you please comment on why it's not a good
idea, if you insist on not doing that?
> + if (err < 0)
> + goto out;
> + btf_encoder__delete(threads[i]->encoder);
> + threads[i]->encoder = NULL;
> + }
> + err = 0;
> +
> +out:
> + for (i = 0; i < nr_threads; i++) {
> + if (threads[i]->encoder && threads[i]->encoder != btf_encoder)
> + btf_encoder__delete(threads[i]->encoder);
> + }
> + free(threads[0]);
> +
> + return err;
> +}
> +
> static enum load_steal_kind pahole_stealer(struct cu *cu,
> struct conf_load *conf_load,
> void *thr_data)
> @@ -2819,8 +2886,8 @@ static enum load_steal_kind pahole_stealer(struct cu *cu,
>
> if (btf_encode) {
> static pthread_mutex_t btf_lock = PTHREAD_MUTEX_INITIALIZER;
> + struct btf_encoder *encoder;
>
> - pthread_mutex_lock(&btf_lock);
> /*
> * FIXME:
> *
> @@ -2828,21 +2895,55 @@ static enum load_steal_kind pahole_stealer(struct cu *cu,
> * point we'll have cu->elf setup...
> */
> if (!btf_encoder) {
> - btf_encoder = btf_encoder__new(cu, detached_btf_filename, conf_load->base_btf, skip_encoding_btf_vars,
> - btf_encode_force, btf_gen_floats, global_verbose);
> + pthread_mutex_lock(&btf_lock);
> + if (!btf_encoder) {
are you trying to minimize lock contention here with !btf_encoder
check outside and inside locked area? Why? It just adds more code
nesting and this locking can't be a real bottleneck. We are talking
about a very few threads being initialized. Please keep it simple,
lock, check !btf_encode and goto unlock, if it's already set.
Otherwise proceed to initialization.
> + /*
> + * btf_encoder is the primary encoder.
> + * And, it is used by the thread
> + * create it.
> + */
> + btf_encoder = btf_encoder__new(cu, detached_btf_filename,
> + conf_load->base_btf,
> + skip_encoding_btf_vars,
> + btf_encode_force,
> + btf_gen_floats, global_verbose);
> + if (btf_encoder && thr_data) {
> + struct thread_data *thread = (struct thread_data *)thr_data;
> +
> + thread->encoder = btf_encoder;
> + thread->btf = btf_encoder__btf(btf_encoder);
Can you summarize the relationship between thr_data, btf_encode and
thread->encoder? I'm re-reading this code (including `if (thr_data)`
piece below) over and over and can't figure out why the initialization
pattern is so complicated.
> + }
> + }
> + pthread_mutex_unlock(&btf_lock);
> if (btf_encoder == NULL) {
> ret = LSK__STOP_LOADING;
> goto out_btf;
> }
> }
>
> - if (btf_encoder__encode_cu(btf_encoder, cu)) {
> + if (thr_data) {
> + struct thread_data *thread = (struct thread_data *)thr_data;
> +
> + if (thread->encoder == NULL) {
> + thread->encoder =
> + btf_encoder__new(cu, detached_btf_filename,
> + NULL,
> + skip_encoding_btf_vars,
> + btf_encode_force,
> + btf_gen_floats,
> + global_verbose);
> + thread->btf = btf_encoder__btf(thread->encoder);
> + }
> + encoder = thread->encoder;
> + } else
> + encoder = btf_encoder;
> +
> + if (btf_encoder__encode_cu(encoder, cu)) {
> fprintf(stderr, "Encountered error while encoding BTF.\n");
> exit(1);
> }
> ret = LSK__DELETE;
> out_btf:
> - pthread_mutex_unlock(&btf_lock);
> return ret;
> }
> #if 0
> @@ -3207,6 +3308,9 @@ int main(int argc, char *argv[])
> memset(tab, ' ', sizeof(tab) - 1);
>
> conf_load.steal = pahole_stealer;
> + conf_load.thread_exit = pahole_thread_exit;
> + conf_load.threads_prepare = pahole_threads_prepare;
> + conf_load.threads_collect = pahole_threads_collect;
>
> // Make 'pahole --header type < file' a shorter form of 'pahole -C type --count 1 < file'
> if (conf.header_type && !class_name && prettify_input) {
> --
> 2.30.2
>
