Re: [Xen-devel] [PATCH v8.1 15/27] xsplice, symbols: Implement fast symbol names -> virtual addresses lookup

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.04.16 at 17:21, <konrad.wilk@xxxxxxxxxx> wrote:
> Here is what I came up using your idea. Do you want me to add 'Suggested-by' 
> Jan on it?

I'll leave that up to you; it was really just a vague idea that I gave...

> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -74,6 +74,9 @@ efi-y := $(shell if [ ! -r $(BASEDIR)/include/xen/compile.h 
> -o \
>  
>  ifdef CONFIG_XSPLICE
>  all_symbols = --all-symbols
> +ifdef CONFIG_FAST_SYMBOL_LOOKUP
> +all_symbols = --all-symbols --add-extra-sorted
> +endif
>  else
>  all_symbols =
>  endif

This now even more calls for using the common list model, at least as
later cleanup.

> --- a/xen/common/symbols-dummy.c
> +++ b/xen/common/symbols-dummy.c
> @@ -5,6 +5,7 @@
>  
>  #include <xen/config.h>
>  #include <xen/types.h>
> +#include <xen/symbols.h>
>  
>  #ifdef SYMBOLS_ORIGIN
>  const unsigned int symbols_offsets[1];
> @@ -14,6 +15,10 @@ const unsigned long symbols_addresses[1];
>  const unsigned int symbols_num_syms;
>  const u8 symbols_names[1];
>  
> +#ifdef CONFIG_FAST_SYMBOL_LOOKUP
> +const struct symbol_offset symbols_sorted_offsets[1];
> +#endif

Oh, nice - you even do the sorting at build time!

> --- a/xen/common/symbols.c
> +++ b/xen/common/symbols.c
> @@ -31,6 +31,10 @@ extern const unsigned long symbols_addresses[];
>  extern const unsigned int symbols_num_syms;
>  extern const u8 symbols_names[];
>  
> +#ifdef CONFIG_FAST_SYMBOL_LOOKUP
> +extern const struct symbol_offset symbols_sorted_offsets[];
> +#endif

No need for an #ifdef around just a declaration.

> @@ -208,8 +212,45 @@ int xensyms_read(uint32_t *symnum, char *type,
>      return 0;
>  }
>  
> +#ifdef CONFIG_FAST_SYMBOL_LOOKUP
>  void *symbols_lookup_by_name(const char *symname)
>  {
> +    char namebuf[KSYM_NAME_LEN + 1];
> +    unsigned long low, high;
> +    static const char *filename_token = "#";

It's being used just once, so I don't see the value of this. But if you
want to retain it, please at least make it an array instead of a pointer.

> +    if ( *symname == '\0' )
> +        return NULL;
> +
> +    /* Unsupported search for filename in symbol right now. */
> +    if ( strpbrk(symname, filename_token) )
> +        return NULL;

That's unfortunate - why are you filtering these out? (Related to
the earlier comment - this could even be strchr(), with no need at
all for a string literal.)

> +    low = 0;
> +    high = symbols_num_syms;
> +    while ( low < high )
> +    {
> +        unsigned long mid = low + ((high - low) / 2);
> +        const struct symbol_offset *s;
> +        int rc;
> +
> +        s = &symbols_sorted_offsets[mid];
> +        (void)symbols_expand_symbol(s->stream, namebuf);
> +        /* Format is: [filename]#<symbol>. symbols_expand_symbol eats type.*/

[<filename>#]<symbol>

And what relevance does the type part of the comment have here?

> +        rc = strcmp(symname, namebuf);

So looking at this I really don't see why you filter filename prefixed
input names above.

> +        if ( rc < 0 )
> +            high = mid;
> +        else if ( rc > 0 )
> +            low = mid + 1;
> +        else
> +            return (void *)symbols_address(s->addr);
> +    }
> +    return NULL;
> +}
> +
> +#else
> +void *symbols_lookup_by_name(const char *symname)
> + {

Please put the #ifdef/#else/#endif inside the function body, to
avoid duplicating code as much as possible. Putting them outside
would be needed primarily if the function type varied between the
two instances.

> +/* Sort by original (non mangled) symbol name, then type. */
> +static int compare_name_orig(const void *p1, const void *p2)
> +{
> +     const struct sym_entry *sym1 = p1;
> +     const struct sym_entry *sym2 = p2;
> +     int rc;
> +
> +     rc = strcmp(sym1->orig_symbol, sym2->orig_symbol);
> +
> +     if (!rc) {
> +             if (sym1->type < sym2->type)
> +                     rc = -1;
> +             else if (sym1->type > sym2->type)
> +                     rc = 1;
> +             else
> +                     rc = 0;

How about just "rc = sym1->type - sym2->type"?

> @@ -350,6 +381,27 @@ static void write_src(void)
>       for (i = 0; i < 256; i++)
>               printf("\t.short\t%d\n", best_idx[i]);
>       printf("\n");
> +
> +     if (!extra_sorted) {
> +             free(markers);
> +             return;
> +     }
> +
> +     /* Sorted by original symbol names, filename, and lastly type. */
> +     qsort(table, table_cnt, sizeof(*table), compare_name_orig);
> +
> +     output_label("symbols_sorted_offsets");
> +     /* An fixed sized array with two entries: offset in the

"A fixed size ..."

> +      * compressed stream (for symbol name), and offset in
> +      * symbols_addresses (or symbols_offset). */
> +     for (i = 0; i < table_cnt; i++) {
> +             printf("\t.long %d", table[i].stream_offset);
> +             printf(", %d", table[i].addr_idx);
> +             printf("\n");

How about making this just a single printf()? Also both are unsigned,
so please at least %u, but even better %#x.

> @@ -561,6 +614,8 @@ int main(int argc, char **argv)
>                               input_format = fmt_sysv;
>                       else if (strcmp(argv[i], "--sort") == 0)
>                               unsorted = true;
> +                     else if (strcmp(argv[i], "--add-extra-sorted") == 0)
> +                             extra_sorted = 1;

s/extra/name/ ?

Or --sort-by-name?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel