Messages in this thread | | | From | Song Liu <> | Subject | Re: [PATCH v10] module: replace module_layout with module_memory | Date | Wed, 8 Feb 2023 21:39:54 +0000 |
| |
> On Feb 8, 2023, at 9:48 AM, Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
[...]
>> diff --git a/arch/arc/kernel/unwind.c b/arch/arc/kernel/unwind.c >> index 200270a94558..933451f4494f 100644 >> --- a/arch/arc/kernel/unwind.c >> +++ b/arch/arc/kernel/unwind.c >> @@ -369,6 +369,8 @@ void *unwind_add_table(struct module *module, const void *table_start, >> unsigned long table_size) >> { >> struct unwind_table *table; >> + struct module_memory *mod_mem_core_text; >> + struct module_memory *mod_mem_init_text; > > This function is small (35 lines) so no need to have so big names for > local functions, see > https://docs.kernel.org/process/coding-style.html#naming > > struct module_memory *core_text; > struct module_memory *init_text;
Will fix.
[...]
>> >> >> /* >> - * Bounds of module text, for speeding up __module_address. >> + * Bounds of module memory, for speeding up __module_address. >> * Protected by module_mutex. >> */ >> -static void __mod_update_bounds(void *base, unsigned int size, struct mod_tree_root *tree) >> +static void __mod_update_bounds(enum mod_mem_type type __maybe_unused, void *base, >> + unsigned int size, struct mod_tree_root *tree) >> { >> unsigned long min = (unsigned long)base; >> unsigned long max = min + size; >> >> +#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC > > A #ifdef shouldn't be required. You can use IS_ENABLED() instead:
Will fix.
> > > >> + if (mod_mem_type_is_core_data(type)) { > > if (IS_ENABLED(CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC) && > mod_mem_type_is_core_data(type))
[...]
>> - switch (m) { >> - case 0: /* executable */ >> - mod->core_layout.size = strict_align(mod->core_layout.size); > > Where is the strict alignment done now ?
AFAICT, each of these memory regions are allocated separately, so they are always page aligned, no?
> >> - mod->core_layout.text_size = mod->core_layout.size; >> - break; >> - case 1: /* RO: text and ro-data */ >> - mod->data_layout.size = strict_align(mod->data_layout.size); >> - mod->data_layout.ro_size = mod->data_layout.size; >> - break; >> - case 2: /* RO after init */ >> - mod->data_layout.size = strict_align(mod->data_layout.size); >> - mod->data_layout.ro_after_init_size = mod->data_layout.size; >> - break; >> - case 4: /* whole core */ >> - mod->data_layout.size = strict_align(mod->data_layout.size); >> - break; >> - } >> - }
[...]
> >> >> if (shdr->sh_type != SHT_NOBITS) >> memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size); > >> @@ -3060,20 +3091,21 @@ bool is_module_address(unsigned long addr) >> struct module *__module_address(unsigned long addr) >> { >> struct module *mod; >> - struct mod_tree_root *tree; >> >> if (addr >= mod_tree.addr_min && addr <= mod_tree.addr_max) >> - tree = &mod_tree; >> + goto lookup; >> + >> #ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC > > Can we try to avoid that #ifdef ? > I know that means getting data_addr_min and data_addr_max alwyas > existing, maybe through an unnamed union or a macro or a static inline > helper ?
IIUC, we want __module_address() to be as fast as possible. So #ifdef is probably the best solution here?
Thanks, Song
> >> - else if (addr >= mod_data_tree.addr_min && addr <= mod_data_tree.addr_max) >> - tree = &mod_data_tree; >> + if (addr >= mod_tree.data_addr_min && addr <= mod_tree.data_addr_max) >> + goto lookup; >> #endif >> - else >> - return NULL; >> >> + return NULL; >> + >> +lookup: >> module_assert_mutex_or_preempt(); >> >> - mod = mod_find(addr, tree); >> + mod = mod_find(addr, &mod_tree); >> if (mod) { >> BUG_ON(!within_module(addr, mod)); >> if (mod->state == MODULE_STATE_UNFORMED)
| |