> On Feb 8, 2023, at 9:48 AM, Christophe Leroy <christophe.leroy@xxxxxxxxxx> 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)
