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

[Christophe Leroy]

Le 08/02/2023 à 22:39, Song Liu a écrit :
> 
> 
>> 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?

Ah, OK.
It should be OK then.


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

An if/else with IS_ENABLED(CONFIG_SOMETHING) is folded at build time, so 
it makes no difference with an #ifdef in terms of performance.

However it has the advantage of making the entire code visible to the 
compiler at all times, which means that you don't have to build it 
several times with difference value for CONFIG_SOMETHING in order to see 
whether your code is valid or not.

As an exemple, if you had written mod_mem_use_vmalloc() with 
IS_ENABLED() instead of an #ifdef, you would have detected the problem 
in v8 even without building with CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC

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