lkml.org 
[lkml]   [2009]   [Nov]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 06/10] module: refactor symbol tables and try to reduce code size of each_symbol()
Date
On Tue, 3 Nov 2009 08:36:18 pm Alan Jenkins wrote:
> find_symbol() will use bsearch() instead of each_symbol(). each_symbol()
> is still desired by Ksplice (although it is not in-tree yet). Let's try
> to minimize the code which will be duplicated between these two
> functions, by changing how the symbol tables are declared.

Alan, this is a particularly neat cleanup. Thanks!

> +typedef bool each_symbol_fn_t (enum export_type type,
> + const struct kernel_symbol *sym,
> + const unsigned long *crc,
> + struct module *owner,
> + void *data);
> +
> /* Walk the exported symbol table */
> -bool each_symbol(bool (*fn)(const struct symsearch *arr, struct module *owner,
> - unsigned int symnum, void *data), void *data);
> +bool each_symbol(each_symbol_fn_t *fn, void *data);

I really hate throwaway typedefs like this. But it's used in two other
places, so I'll suppress my distaste :)

> +static struct ksymtab ksymtab[EXPORT_TYPE_MAX];
> +
> +static int __init init_ksymtab(void)
> +{
> + struct ksymtab tmp[EXPORT_TYPE_MAX] = {
> + [EXPORT_TYPE_PLAIN] = {
> + __start___ksymtab, __start___kcrctab,
> + __stop___ksymtab - __start___ksymtab,
> + },
> + [EXPORT_TYPE_GPL] = {
> + __start___ksymtab_gpl, __start___kcrctab_gpl,
> + __stop___ksymtab_gpl - __start___ksymtab_gpl,
> + },
> +#ifdef CONFIG_UNUSED_EXPORTS
> + [EXPORT_TYPE_UNUSED] = {
> + __start___ksymtab_unused, __start___kcrctab_unused,
> + __stop___ksymtab_unused - __start___ksymtab_unused,
> + },
> + [EXPORT_TYPE_UNUSED_GPL] = {
> + __start___ksymtab_unused_gpl,
> + __start___kcrctab_unused_gpl,
> + __stop___ksymtab_unused_gpl -
> + __start___ksymtab_unused_gpl,
> + },
> +#endif
> + [EXPORT_TYPE_GPL_FUTURE] = {
> + __start___ksymtab_gpl_future,
> + __start___kcrctab_gpl_future,
> + __stop___ksymtab_gpl_future -
> + __start___ksymtab_gpl_future,
> + },
> + };
> +
> + memcpy(ksymtab, tmp, sizeof(ksymtab));

This works, but I'd prefer you to open-code the assignments. Simpler and
marginally more efficient.

> @@ -322,9 +322,9 @@ static bool find_symbol_in_section(const struct symsearch *syms,
> }
> #endif
>
> + fsa->sym = sym;
> + fsa->crc = crc;
> fsa->owner = owner;
> - fsa->crc = symversion(syms->crcs, symnum);
> - fsa->sym = &syms->start[symnum];
> return true;

Strange gratuitous reorder here?

> +static const char *export_section_names[EXPORT_TYPE_MAX] = {
> + [EXPORT_TYPE_PLAIN] = "__ksymtab",
> + [EXPORT_TYPE_GPL] = "__ksymtab_gpl",
> +#ifdef CONFIG_UNUSED_SYMBOLS
> + [EXPORT_TYPE_UNUSED] = "__ksymtab_unused",
> + [EXPORT_TYPE_UNUSED_GPL] = "__ksymtab_unused_gpl",
> +#endif
> + [EXPORT_TYPE_GPL_FUTURE] = "__ksymtab_gpl_future",
> +};
> +
> +static const char *crc_section_names[EXPORT_TYPE_MAX] = {
> + [EXPORT_TYPE_PLAIN] = "__kcrctab",
> + [EXPORT_TYPE_GPL] = "__kcrctab_gpl",
> +#ifdef CONFIG_UNUSED_SYMBOLS
> + [EXPORT_TYPE_UNUSED] = "__kcrctab_unused",
> + [EXPORT_TYPE_UNUSED_GPL] = "__kcrctab_unused_gpl",
> +#endif
> + [EXPORT_TYPE_GPL_FUTURE] = "__kcrctab_gpl_future",
> +};

You can use [] here for size instead of explicit EXPORT_TYPE_MAX. We should
have named these sections better :(

> + for (export_type = 0; export_type < EXPORT_TYPE_MAX; export_type++) {

Then use ARRAY_SIZE(export_section_names) here.

> + for (export_type = 0; export_type < EXPORT_TYPE_MAX; export_type++) {
> + if (mod->syms[export_type].syms &&

Similar ARRAY_SIZE(mod->syms). It's less indirect, IMHO.

But all minor nitpicks; code is excellent!

Thanks,
Rusty.


\
 
 \ /
  Last update: 2009-11-04 09:31    [W:0.121 / U:0.268 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site