Re: [PATCH 1/2] btf_encoder: Move find_all_percpu_vars in generic collect_symbols

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Mon, Nov 02, 2020 at 10:29:22AM -0800, Hao Luo escreveu:
> This looks good to me. Thanks, Jiri.
> 
> Acked-by: Hao Luo <haoluo@google.com>

Thanks, applied, will wait for a v2 for the other.
 
> On Sat, Oct 31, 2020 at 3:31 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Moving find_all_percpu_vars under generic collect_symbols
> > function that walks over symbols and calls collect_percpu_var.
> >
> > We will add another collect function that needs to go through
> > all the symbols, so it's better we go through them just once.
> >
> > There's no functional change intended.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  btf_encoder.c | 124 +++++++++++++++++++++++++++-----------------------
> >  1 file changed, 67 insertions(+), 57 deletions(-)
> >
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index 4c92908beab2..1866bb16a8ba 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -250,75 +250,85 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name)
> >         return true;
> >  }
> >
> > -static int find_all_percpu_vars(struct btf_elf *btfe)
> > +static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
> >  {
> > -       uint32_t core_id;
> > -       GElf_Sym sym;
> > +       const char *sym_name;
> > +       uint64_t addr;
> > +       uint32_t size;
> >
> > -       /* cache variables' addresses, preparing for searching in symtab. */
> > -       percpu_var_cnt = 0;
> > +       /* compare a symbol's shndx to determine if it's a percpu variable */
> > +       if (elf_sym__section(sym) != btfe->percpu_shndx)
> > +               return 0;
> > +       if (elf_sym__type(sym) != STT_OBJECT)
> > +               return 0;
> >
> > -       /* search within symtab for percpu variables */
> > -       elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) {
> > -               const char *sym_name;
> > -               uint64_t addr;
> > -               uint32_t size;
> > +       addr = elf_sym__value(sym);
> > +       /*
> > +        * Store only those symbols that have allocated space in the percpu section.
> > +        * This excludes the following three types of symbols:
> > +        *
> > +        *  1. __ADDRESSABLE(sym), which are forcely emitted as symbols.
> > +        *  2. __UNIQUE_ID(prefix), which are introduced to generate unique ids.
> > +        *  3. __exitcall(fn), functions which are labeled as exit calls.
> > +        *
> > +        * In addition, the variables defined using DEFINE_PERCPU_FIRST are
> > +        * also not included, which currently includes:
> > +        *
> > +        *  1. fixed_percpu_data
> > +        */
> > +       if (!addr)
> > +               return 0;
> >
> > -               /* compare a symbol's shndx to determine if it's a percpu variable */
> > -               if (elf_sym__section(&sym) != btfe->percpu_shndx)
> > -                       continue;
> > -               if (elf_sym__type(&sym) != STT_OBJECT)
> > -                       continue;
> > +       size = elf_sym__size(sym);
> > +       if (!size)
> > +               return 0; /* ignore zero-sized symbols */
> >
> > -               addr = elf_sym__value(&sym);
> > -               /*
> > -                * Store only those symbols that have allocated space in the percpu section.
> > -                * This excludes the following three types of symbols:
> > -                *
> > -                *  1. __ADDRESSABLE(sym), which are forcely emitted as symbols.
> > -                *  2. __UNIQUE_ID(prefix), which are introduced to generate unique ids.
> > -                *  3. __exitcall(fn), functions which are labeled as exit calls.
> > -                *
> > -                * In addition, the variables defined using DEFINE_PERCPU_FIRST are
> > -                * also not included, which currently includes:
> > -                *
> > -                *  1. fixed_percpu_data
> > -                */
> > -               if (!addr)
> > -                       continue;
> > +       sym_name = elf_sym__name(sym, btfe->symtab);
> > +       if (!btf_name_valid(sym_name)) {
> > +               dump_invalid_symbol("Found symbol of invalid name when encoding btf",
> > +                                   sym_name, btf_elf__verbose, btf_elf__force);
> > +               if (btf_elf__force)
> > +                       return 0;
> > +               return -1;
> > +       }
> >
> > -               size = elf_sym__size(&sym);
> > -               if (!size)
> > -                       continue; /* ignore zero-sized symbols */
> > +       if (btf_elf__verbose)
> > +               printf("Found per-CPU symbol '%s' at address 0x%lx\n", sym_name, addr);
> >
> > -               sym_name = elf_sym__name(&sym, btfe->symtab);
> > -               if (!btf_name_valid(sym_name)) {
> > -                       dump_invalid_symbol("Found symbol of invalid name when encoding btf",
> > -                                           sym_name, btf_elf__verbose, btf_elf__force);
> > -                       if (btf_elf__force)
> > -                               continue;
> > -                       return -1;
> > -               }
> > +       if (percpu_var_cnt == MAX_PERCPU_VAR_CNT) {
> > +               fprintf(stderr, "Reached the limit of per-CPU variables: %d\n",
> > +                       MAX_PERCPU_VAR_CNT);
> > +               return -1;
> > +       }
> > +       percpu_vars[percpu_var_cnt].addr = addr;
> > +       percpu_vars[percpu_var_cnt].sz = size;
> > +       percpu_vars[percpu_var_cnt].name = sym_name;
> > +       percpu_var_cnt++;
> >
> > -               if (btf_elf__verbose)
> > -                       printf("Found per-CPU symbol '%s' at address 0x%lx\n", sym_name, addr);
> > +       return 0;
> > +}
> > +
> > +static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
> > +{
> > +       uint32_t core_id;
> > +       GElf_Sym sym;
> >
> > -               if (percpu_var_cnt == MAX_PERCPU_VAR_CNT) {
> > -                       fprintf(stderr, "Reached the limit of per-CPU variables: %d\n",
> > -                               MAX_PERCPU_VAR_CNT);
> > +       /* cache variables' addresses, preparing for searching in symtab. */
> > +       percpu_var_cnt = 0;
> > +
> > +       /* search within symtab for percpu variables */
> > +       elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) {
> > +               if (collect_percpu_vars && collect_percpu_var(btfe, &sym))
> >                         return -1;
> > -               }
> > -               percpu_vars[percpu_var_cnt].addr = addr;
> > -               percpu_vars[percpu_var_cnt].sz = size;
> > -               percpu_vars[percpu_var_cnt].name = sym_name;
> > -               percpu_var_cnt++;
> >         }
> >
> > -       if (percpu_var_cnt)
> > -               qsort(percpu_vars, percpu_var_cnt, sizeof(percpu_vars[0]), percpu_var_cmp);
> > +       if (collect_percpu_vars) {
> > +               if (percpu_var_cnt)
> > +                       qsort(percpu_vars, percpu_var_cnt, sizeof(percpu_vars[0]), percpu_var_cmp);
> >
> > -       if (btf_elf__verbose)
> > -               printf("Found %d per-CPU variables!\n", percpu_var_cnt);
> > +               if (btf_elf__verbose)
> > +                       printf("Found %d per-CPU variables!\n", percpu_var_cnt);
> > +       }
> >         return 0;
> >  }
> >
> > @@ -347,7 +357,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> >                 if (!btfe)
> >                         return -1;
> >
> > -               if (!skip_encoding_vars && find_all_percpu_vars(btfe))
> > +               if (collect_symbols(btfe, !skip_encoding_vars))
> >                         goto out;
> >
> >                 has_index_type = false;
> > --
> > 2.26.2
> >

-- 

- Arnaldo