Message-ID: <c87eeb9c-5f54-480c-17c2-01339416b1b9@huawei.com>
Date: Thu, 18 Jul 2024 11:45:42 +0800
From: Zheng Yejian <zhengyejian1@...wei.com>
To: Masahiro Yamada <masahiroy@...nel.org>
CC: <rostedt@...dmis.org>, <mhiramat@...nel.org>, <mark.rutland@....com>,
	<mpe@...erman.id.au>, <npiggin@...il.com>, <christophe.leroy@...roup.eu>,
	<naveen.n.rao@...ux.ibm.com>, <tglx@...utronix.de>, <mingo@...hat.com>,
	<bp@...en8.de>, <dave.hansen@...ux.intel.com>, <x86@...nel.org>,
	<hpa@...or.com>, <mcgrof@...nel.org>, <mathieu.desnoyers@...icios.com>,
	<nathan@...nel.org>, <nicolas@...sle.eu>, <kees@...nel.org>,
	<james.clark@....com>, <kent.overstreet@...ux.dev>, <yhs@...com>,
	<jpoimboe@...nel.org>, <peterz@...radead.org>,
	<linux-kernel@...r.kernel.org>, <linux-trace-kernel@...r.kernel.org>,
	<linuxppc-dev@...ts.ozlabs.org>, <linux-modules@...r.kernel.org>,
	<linux-kbuild@...r.kernel.org>, <bpf@...r.kernel.org>
Subject: Re: [PATCH 2/6] kallsyms: Emit symbol at the holes in the text

On 2024/7/16 16:33, Masahiro Yamada wrote:
> On Thu, Jun 13, 2024 at 10:36 PM Zheng Yejian <zhengyejian1@...wei.com> wrote:
>>
>> When a weak type function is overridden, its symbol will be removed
>> from the symbol table, but its code will not be removed. Besides,
>> due to lacking of size for kallsyms, kernel compute function size by
>> substracting its symbol address from its next symbol address (see
>> kallsyms_lookup_size_offset()). These will cause that size of some
>> function is computed to be larger than it actually is, just because
>> symbol of its following weak function is removed.
>>
>> This issue also causes multiple __fentry__ locations to be counted in
>> the some function scope, and eventually causes ftrace_location() to find
>> wrong __fentry__ location. It was reported in
>> Link: https://lore.kernel.org/all/20240607115211.734845-1-zhengyejian1@huawei.com/
>>
>> Peter suggested to change scipts/kallsyms.c to emit readily
>> identifiable symbol names for all the weak junk, eg:
>>
>>    __weak_junk_NNNNN
>>
>> The name of this kind symbol needs some discussion, but it's temporarily
>> called "__hole_symbol_XXXXX" in this patch:
>> 1. Pass size info to scripts/kallsyms  (see mksysmap());
>> 2. Traverse sorted function symbols, if one function address plus its
>>     size less than next function address, it means there's a hole, then
>>     emit a symbol "__hole_symbol_XXXXX" there which type is 't'.
>>
>> After this patch, the effect is as follows:
>>
>>    $ cat /proc/kallsyms | grep -A 3 do_one_initcall
>>    ffffffff810021e0 T do_one_initcall
>>    ffffffff8100245e t __hole_symbol_XXXXX
>>    ffffffff810024a0 t __pfx_rootfs_init_fs_context
>>
>> Suggested-by: Peter Zijlstra <peterz@...radead.org>
>> Signed-off-by: Zheng Yejian <zhengyejian1@...wei.com>
> 
> 
> 
> With my quick test, "t__hole_symbol_XXXXX" was encoded
> into the following 10-byte stream.
> 
> .byte 0x09, 0x94, 0xbf, 0x18, 0xf3, 0x3d, 0xce, 0xd1, 0xd1, 0x58
> 
> 
> 
> Now "t__hole_symbol_XXXXX" is the most common symbol name.
> However, 10 byte is consumed for every instance of
> "t__hole_symbol_XXXXX".
> 
> This is much less efficient thanI had expected,
> although I did not analyze the logic of this inefficiency.
>
Hi, Masahiro!

In my local test, "t__hole_symbol_XXXXX" was finally encoded
into just one byte. See "kallsyms_token_table" in the .tmp_vmlinux.kallsyms2.S:

   kallsyms_token_table:
         [...]
         .asciz  "t__hole_symbol_XXXXX"
         .asciz  "hole_symbol_XXXXX"
         .asciz  "e_symbol_XXXXX"
         .asciz  "XXXXX"
         .asciz  "XXX"
         .asciz  "e_symbol_"
         .asciz  "ymbol_"
         .asciz  "ymb"
         .asciz  "hol"
         .asciz  "ol_"
         .asciz  "pfx"
         .asciz  "pf"
         .asciz  "e_s"
         .asciz  "ym"
         .asciz  "t__"
         .asciz  "_s"
         .asciz  "ol"
         .asciz  "__"
         .asciz  "XX"

But it would still takes up several tokens due to substrings of
"t__hole_symbol_XXXXX" would also become the most common ones.
After this patch, the number of "t__hole_symbol_XXXXX" will be ~30% of the total.

> 
> 
> 
> 
> 
> 
>> ---
>>   scripts/kallsyms.c      | 101 +++++++++++++++++++++++++++++++++++++++-
>>   scripts/link-vmlinux.sh |   4 +-
>>   scripts/mksysmap        |   2 +-
>>   3 files changed, 102 insertions(+), 5 deletions(-)
>>
>> diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
>> index 6559a9802f6e..5c4cde864a04 100644
>> --- a/scripts/kallsyms.c
>> +++ b/scripts/kallsyms.c
>> @@ -35,6 +35,7 @@
>>   struct sym_entry {
>>          struct sym_entry *next;
>>          unsigned long long addr;
>> +       unsigned long long size;
>>          unsigned int len;
>>          unsigned int seq;
>>          unsigned int start_pos;
>> @@ -74,6 +75,7 @@ static int token_profit[0x10000];
>>   static unsigned char best_table[256][2];
>>   static unsigned char best_table_len[256];
>>
>> +static const char hole_symbol[] = "__hole_symbol_XXXXX";
>>
>>   static void usage(void)
>>   {
>> @@ -130,8 +132,16 @@ static struct sym_entry *read_symbol(FILE *in, char **buf, size_t *buf_len)
>>          size_t len;
>>          ssize_t readlen;
>>          struct sym_entry *sym;
>> +       unsigned long long size = 0;
>>
>>          errno = 0;
>> +       /*
>> +        * Example of expected symbol format:
>> +        * 1. symbol with size info:
>> +        *    ffffffff81000070 00000000000001d7 T __startup_64
>> +        * 2. symbol without size info:
>> +        *    0000000002a00000 A text_size
>> +        */
>>          readlen = getline(buf, buf_len, in);
>>          if (readlen < 0) {
>>                  if (errno) {
>> @@ -145,9 +155,24 @@ static struct sym_entry *read_symbol(FILE *in, char **buf, size_t *buf_len)
>>                  (*buf)[readlen - 1] = 0;
>>
>>          addr = strtoull(*buf, &p, 16);
>> +       if (*buf == p || *p++ != ' ') {
>> +               fprintf(stderr, "line format error: unable to parse address\n");
>> +               exit(EXIT_FAILURE);
>> +       }
>> +
>> +       if (*p == '0') {
>> +               char *str = p;
>>
>> -       if (*buf == p || *p++ != ' ' || !isascii((type = *p++)) || *p++ != ' ') {
>> -               fprintf(stderr, "line format error\n");
>> +               size = strtoull(str, &p, 16);
>> +               if (str == p || *p++ != ' ') {
>> +                       fprintf(stderr, "line format error: unable to parse size\n");
>> +                       exit(EXIT_FAILURE);
>> +               }
>> +       }
>> +
>> +       type = *p++;
>> +       if (!isascii(type) || *p++ != ' ') {
>> +               fprintf(stderr, "line format error: unable to parse type\n");
>>                  exit(EXIT_FAILURE);
>>          }
>>
>> @@ -182,6 +207,7 @@ static struct sym_entry *read_symbol(FILE *in, char **buf, size_t *buf_len)
>>                  exit(EXIT_FAILURE);
>>          }
>>          sym->addr = addr;
>> +       sym->size = size;
>>          sym->len = len;
>>          sym->sym[0] = type;
>>          strcpy(sym_name(sym), name);
>> @@ -795,6 +821,76 @@ static void sort_symbols(void)
>>          qsort(table, table_cnt, sizeof(table[0]), compare_symbols);
>>   }
>>
>> +static int may_exist_hole_after_symbol(const struct sym_entry *se)
> 
> 
> The return type should be bool.
> 

Yes!

> 
> 
>> +{
>> +       char type = se->sym[0];
>> +
>> +       /* Only check text symbol or weak symbol */
>> +       if (type != 't' && type != 'T' &&
>> +           type != 'w' && type != 'W')
>> +               return 0;
>> +       /* Symbol without size has no hole */
>> +       return se->size != 0;
>> +}
>> +
>> +static struct sym_entry *gen_hole_symbol(unsigned long long addr)
>> +{
>> +       struct sym_entry *sym;
>> +       static size_t len = sizeof(hole_symbol);
>> +
>> +       /* include type field */
>> +       sym = malloc(sizeof(*sym) + len + 1);
>> +       if (!sym) {
>> +               fprintf(stderr, "unable to allocate memory for hole symbol\n");
>> +               exit(EXIT_FAILURE);
>> +       }
>> +       sym->addr = addr;
>> +       sym->size = 0;
>> +       sym->len = len;
>> +       sym->sym[0] = 't';
>> +       strcpy(sym_name(sym), hole_symbol);
>> +       sym->percpu_absolute = 0;
>> +       return sym;
>> +}
>> +
>> +static void emit_hole_symbols(void)
>> +{
>> +       unsigned int i, pos, nr_emit;
>> +       struct sym_entry **new_table;
>> +       unsigned int new_cnt;
>> +
>> +       nr_emit = 0;
>> +       for (i = 0; i < table_cnt - 1; i++) {
>> +               if (may_exist_hole_after_symbol(table[i]) &&
>> +                   table[i]->addr + table[i]->size < table[i+1]->addr)
>> +                       nr_emit++;
>> +       }
>> +       if (!nr_emit)
>> +               return;
>> +
>> +       new_cnt = table_cnt + nr_emit;
>> +       new_table = malloc(sizeof(*new_table) * new_cnt);
> 
> 
> Do you need to allocate another huge table?
> 
> You can use realloc() to append the room for nr_emit
> if you iterate the table in the reverse order.
> 

Yes, it would be much better. If it turns out to be the
"emit hole symbol" solution, I'll change it to that in the next version,
actually, I forgot to mark this series as "RFC".

> 
> 
> 
> 
> 
> 
>> +       if (!new_table) {
>> +               fprintf(stderr, "unable to allocate memory for new table\n");
>> +               exit(EXIT_FAILURE);
>> +       }
>> +
>> +       pos = 0;
>> +       for (i = 0; i < table_cnt; i++) {
>> +               unsigned long long addr;
>> +
>> +               new_table[pos++] = table[i];
>> +               if ((i == table_cnt - 1) || !may_exist_hole_after_symbol(table[i]))
>> +                       continue;
>> +               addr = table[i]->addr + table[i]->size;
>> +               if (addr < table[i+1]->addr)
>> +                       new_table[pos++] = gen_hole_symbol(addr);
>> +       }
>> +       free(table);
>> +       table = new_table;
>> +       table_cnt = new_cnt;
>> +}
>> +
>>   static void make_percpus_absolute(void)
>>   {
>>          unsigned int i;
>> @@ -854,6 +950,7 @@ int main(int argc, char **argv)
>>          if (absolute_percpu)
>>                  make_percpus_absolute();
>>          sort_symbols();
>> +       emit_hole_symbols();
>>          if (base_relative)
>>                  record_relative_base();
>>          optimize_token_table();
>> diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
>> index 518c70b8db50..8e1373902bfe 100755
>> --- a/scripts/link-vmlinux.sh
>> +++ b/scripts/link-vmlinux.sh
>> @@ -189,11 +189,11 @@ kallsyms_step()
>>   }
>>
>>   # Create map file with all symbols from ${1}
>> -# See mksymap for additional details
>> +# See mksysmap for additional details
>>   mksysmap()
>>   {
>>          info NM ${2}
>> -       ${NM} -n "${1}" | sed -f "${srctree}/scripts/mksysmap" > "${2}"
>> +       ${NM} -nS "${1}" | sed -f "${srctree}/scripts/mksysmap" > "${2}"
>>   }
>>
>>   sorttable()
>> diff --git a/scripts/mksysmap b/scripts/mksysmap
>> index c12723a04655..7a4415f21143 100755
>> --- a/scripts/mksysmap
>> +++ b/scripts/mksysmap
>> @@ -2,7 +2,7 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   #
>>   # sed script to filter out symbols that are not needed for System.map,
>> -# or not suitable for kallsyms. The input should be 'nm -n <file>'.
>> +# or not suitable for kallsyms. The input should be 'nm -nS <file>'.
>>   #
>>   # System.map is used by module-init tools and some debugging
>>   # tools to retrieve the actual addresses of symbols in the kernel.
>> --
>> 2.25.1
>>
>>
> 
> 

-- 
Thanks,
Zheng Yejian

Powered by blists - more mailing lists