| Message ID | 1253626718-18887-4-git-send-email-alan-jenkins@tuffmail.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Alan, I really like what you're doing with this patch series; using a binary search for the symbol table has been something I've wanted to do in the kernel's module loader ever since I optimized Ksplice's symbol resolution code to use binary rather than linear searches. While Ksplice is not in-tree yet, Ksplice is a user of each_symbol (and in fact was the reason why each_symbol was originally exported). Is it easy to modify your patch series so that you don't have to remove each_symbol? Best regards, -Tim Abbott On Tue, 22 Sep 2009, Alan Jenkins wrote: > find_symbol() is about to be re-written to avoid traversing every single > exported symbol. each_symbol() has acquired no other in-tree user, so > let us remove it. > > Also struct symsearch is useless outside of module.c, so move it there > instead of cluttering up module.h. > > Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk> > --- > include/linux/module.h | 15 --------------- > kernel/module.c | 14 ++++++++++++-- > 2 files changed, 12 insertions(+), 17 deletions(-) > > diff --git a/include/linux/module.h b/include/linux/module.h > index 65b62e9..df25f99 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -348,17 +348,6 @@ static inline int within_module_init(unsigned long addr, struct module *mod) > /* Search for module by name: must hold module_mutex. */ > struct module *find_module(const char *name); > > -struct symsearch { > - const struct kernel_symbol *start, *stop; > - const unsigned long *crcs; > - enum { > - NOT_GPL_ONLY, > - GPL_ONLY, > - WILL_BE_GPL_ONLY, > - } licence; > - bool unused; > -}; > - > /* Search for an exported symbol by name. */ > const struct kernel_symbol *find_symbol(const char *name, > struct module **owner, > @@ -366,10 +355,6 @@ const struct kernel_symbol *find_symbol(const char *name, > bool gplok, > bool warn); > > -/* Walk the exported symbol table */ > -bool each_symbol(bool (*fn)(const struct symsearch *arr, struct module *owner, > - unsigned int symnum, void *data), void *data); > - > /* Returns 0 and fills in value, defined and namebuf, or -ERANGE if > symnum out of range. */ > int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type, > diff --git a/kernel/module.c b/kernel/module.c > index 2d53718..b24fc55 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -186,6 +186,17 @@ extern const unsigned long __start___kcrctab_unused[]; > extern const unsigned long __start___kcrctab_unused_gpl[]; > #endif > > +struct symsearch { > + const struct kernel_symbol *start, *stop; > + const unsigned long *crcs; > + enum { > + NOT_GPL_ONLY, > + GPL_ONLY, > + WILL_BE_GPL_ONLY, > + } licence; > + bool unused; > +}; > + > #ifndef CONFIG_MODVERSIONS > #define symversion(base, idx) NULL > #else > @@ -212,7 +223,7 @@ static bool each_symbol_in_section(const struct symsearch *arr, > } > > /* Returns true as soon as fn returns true, otherwise false. */ > -bool each_symbol(bool (*fn)(const struct symsearch *arr, struct module *owner, > +static bool each_symbol(bool (*fn)(const struct symsearch *arr, struct module *owner, > unsigned int symnum, void *data), void *data) > { > struct module *mod; > @@ -266,7 +277,6 @@ bool each_symbol(bool (*fn)(const struct symsearch *arr, struct module *owner, > } > return false; > } > -EXPORT_SYMBOL_GPL(each_symbol); > > struct find_symbol_arg { > /* Input */ > -- > 1.6.3.2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tim Abbott wrote: > Hi Alan, > > I really like what you're doing with this patch series; using a binary > search for the symbol table has been something I've wanted to do in the > kernel's module loader ever since I optimized Ksplice's symbol resolution > code to use binary rather than linear searches. > > While Ksplice is not in-tree yet, Ksplice is a user of each_symbol (and in > fact was the reason why each_symbol was originally exported). Is it easy > to modify your patch series so that you don't have to remove each_symbol? > I'll have to think harder about it, but that's my problem. I should have checked the changelog :-). Regards Alan -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 23, 2009 at 01:29:43PM -0400, Tim Abbott wrote: > While Ksplice is not in-tree yet, Ksplice is a user of each_symbol (and in > fact was the reason why each_symbol was originally exported). Is it easy > to modify your patch series so that you don't have to remove each_symbol? We don't keep symbols for out of tree junk around. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 24 Sep 2009 07:30:22 am Christoph Hellwig wrote: > On Wed, Sep 23, 2009 at 01:29:43PM -0400, Tim Abbott wrote: > > While Ksplice is not in-tree yet, Ksplice is a user of each_symbol (and in > > fact was the reason why each_symbol was originally exported). Is it easy > > to modify your patch series so that you don't have to remove each_symbol? > > We don't keep symbols for out of tree junk around. I expected ksplice to go in earlier, actually. I applied their export patch to ease the transition. I'd still like to see ksplice merged, but it's a big hunk of code. Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig wrote: > On Wed, Sep 23, 2009 at 01:29:43PM -0400, Tim Abbott wrote: > >> While Ksplice is not in-tree yet, Ksplice is a user of each_symbol (and in >> fact was the reason why each_symbol was originally exported). Is it easy >> to modify your patch series so that you don't have to remove each_symbol? >> > > We don't keep symbols for out of tree junk around. > We do alter them mercilessly though :-). I don't want to preserve the current implementation of each_symbol() because it duplicates too much of the modified find_symbol(). However, the duplicated code can be simplified if I changed the various "syms" fields in struct module with a single array (without increasing the size of struct module). I consider this a cleanup; it would benefit a couple of other sites in module.c as well. The change would make "struct symsearch" redundant - changing the interface of each_symbol(). If Ksplice fails to merge quickly enough it will still be easy to remove each_symbol(), and we'll still benefit from the cleanup in find_symbol() and elsewhere. Rusty, does that make sense to you? Thanks Alan -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/module.h b/include/linux/module.h index 65b62e9..df25f99 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -348,17 +348,6 @@ static inline int within_module_init(unsigned long addr, struct module *mod) /* Search for module by name: must hold module_mutex. */ struct module *find_module(const char *name); -struct symsearch { - const struct kernel_symbol *start, *stop; - const unsigned long *crcs; - enum { - NOT_GPL_ONLY, - GPL_ONLY, - WILL_BE_GPL_ONLY, - } licence; - bool unused; -}; - /* Search for an exported symbol by name. */ const struct kernel_symbol *find_symbol(const char *name, struct module **owner, @@ -366,10 +355,6 @@ const struct kernel_symbol *find_symbol(const char *name, bool gplok, bool warn); -/* Walk the exported symbol table */ -bool each_symbol(bool (*fn)(const struct symsearch *arr, struct module *owner, - unsigned int symnum, void *data), void *data); - /* Returns 0 and fills in value, defined and namebuf, or -ERANGE if symnum out of range. */ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type, diff --git a/kernel/module.c b/kernel/module.c index 2d53718..b24fc55 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -186,6 +186,17 @@ extern const unsigned long __start___kcrctab_unused[]; extern const unsigned long __start___kcrctab_unused_gpl[]; #endif +struct symsearch { + const struct kernel_symbol *start, *stop; + const unsigned long *crcs; + enum { + NOT_GPL_ONLY, + GPL_ONLY, + WILL_BE_GPL_ONLY, + } licence; + bool unused; +}; + #ifndef CONFIG_MODVERSIONS #define symversion(base, idx) NULL #else @@ -212,7 +223,7 @@ static bool each_symbol_in_section(const struct symsearch *arr, } /* Returns true as soon as fn returns true, otherwise false. */ -bool each_symbol(bool (*fn)(const struct symsearch *arr, struct module *owner, +static bool each_symbol(bool (*fn)(const struct symsearch *arr, struct module *owner, unsigned int symnum, void *data), void *data) { struct module *mod; @@ -266,7 +277,6 @@ bool each_symbol(bool (*fn)(const struct symsearch *arr, struct module *owner, } return false; } -EXPORT_SYMBOL_GPL(each_symbol); struct find_symbol_arg { /* Input */
find_symbol() is about to be re-written to avoid traversing every single exported symbol. each_symbol() has acquired no other in-tree user, so let us remove it. Also struct symsearch is useless outside of module.c, so move it there instead of cluttering up module.h. Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk> --- include/linux/module.h | 15 --------------- kernel/module.c | 14 ++++++++++++-- 2 files changed, 12 insertions(+), 17 deletions(-)