lkml.org 
[lkml]   [2023]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v10] module: replace module_layout with module_memory
    Date


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

    \
     
     \ /
      Last update: 2023-03-27 00:15    [W:4.910 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site