[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v8.1 15/27] xsplice, symbols: Implement fast symbol names -> virtual addresses lookup
>>> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |