Re: [PATCH 2/6] kallsyms: Emit symbol at the holes in the text

From: Masahiro Yamada
Date: Sat Jul 20 2024 - 10:15:45 EST

On Thu, Jul 18, 2024 at 12:45 PM Zheng Yejian <zhengyejian1@xxxxxxxxxx> wrote:
>
> On 2024/7/16 16:33, Masahiro Yamada wrote:
> > On Thu, Jun 13, 2024 at 10:36 PM Zheng Yejian <zhengyejian1@xxxxxxxxxx> 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@xxxxxxxxxx/
> >>
> >> 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@xxxxxxxxxxxxx>
> >> Signed-off-by: Zheng Yejian <zhengyejian1@xxxxxxxxxx>
> >
> >
> >
> > 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".


"__hole_symbol_XXXXX" is too much.

You can use the empty symbol type/name as a special case
to represent the hole.


--
Best Regards
Masahiro Yamada