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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Apr 25, 2016 at 02:38:57AM -0600, Jan Beulich wrote:
> >>> On 22.04.16 at 17:21, <konrad.wilk@oracle.com> 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.

OK, put it on the Wiki.
> 
> > --- 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!

:-)
..snip..
> > +#ifdef CONFIG_FAST_SYMBOL_LOOKUP
> >  void *symbols_lookup_by_name(const char *symname)
> >  {
..snip..
> > +    /* 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.)

This was an artificat of the earlier version. Removed it all now.

> 
> > +    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?

Artifact of the older version - Where I was searching for #.
.. snip.p
I removed the comment.

> > +	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.

I would rather have it as %u. As when debugging this I found it quite
useful for these values to be decimal as I could edit the .xen-sym.0.S file
and see in the editor where it would match up with the symbols_addresses
or the symbol_names in a quite easy way. Doing it in hex means doing extra
calculations.

> 
> > @@ -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@lists.xen.org
http://lists.xen.org/xen-devel