Re: [PATCH v10] module: replace module_layout with module_memory

[Song Liu]

> 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)