On Fri, Jan 27, 2023 at 11:43 PM Christophe Leroy
[off-list ref] wrote:
[...]

quoted

-struct module_layout {
-     /* The actual code + data. */
+enum mod_mem_type {
+     MOD_MEM_TYPE_TEXT,
+     MOD_MEM_TYPE_DATA,
+     MOD_MEM_TYPE_RODATA,
+     MOD_MEM_TYPE_RO_AFTER_INIT,
+     MOD_MEM_TYPE_INIT_TEXT,
+     MOD_MEM_TYPE_INIT_DATA,
+     MOD_MEM_TYPE_INIT_RODATA,
+
+     MOD_MEM_NUM_TYPES,
+     MOD_MEM_TYPE_INVALID = -1,
+};

Ok, so we agreed to keep it as a table with enums. Fair enough.

However, can we try to make it less ugly and more readable ?

I don't thing the enums needs to be prefixed by MOD_MEM_TYPE_
Would be enough with MOD_TEXT, MOD_DATA, MOD_RODATA, MOD_RO_AFTER_INIT,
MOD_INIT_TEXT, MOD_INIT_DATA, MOD_INIT_RODATA, MOD_INVALID.

[...]

quoted

-     /* Core layout: rbtree is accessed frequently, so keep together. */
-     struct module_layout core_layout __module_layout_align;
-     struct module_layout init_layout;
-#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
-     struct module_layout data_layout;
-#endif
+     /* rbtree is accessed frequently, so keep together. */
+     struct module_memory mod_mem[MOD_MEM_NUM_TYPES] __module_memory_align;

We are already in a struct called module, so the module_memory struct
could be called mem[MOD_MEM_NUM_TYPES]

quoted

      /* Arch-specific module values */
      struct mod_arch_specific arch;

@@ -573,23 +574,35 @@ bool __is_module_percpu_address(unsigned long addr, unsigned long *can_addr);
  bool is_module_percpu_address(unsigned long addr);
  bool is_module_text_address(unsigned long addr);

+static inline bool within_module_mem_type(unsigned long addr,
+                                       const struct module *mod,
+                                       enum mod_mem_type type)
+{
+     const struct module_memory *mod_mem;
+
+     if (WARN_ON_ONCE(type < MOD_MEM_TYPE_TEXT || type >= MOD_MEM_NUM_TYPES))

Here I would rather use 0 instead of  MOD_MEM_TYPE_TEXT because
MOD_MEM_TYPE_TEXT may change in the future.

quoted

+             return false;
+
+     mod_mem = &mod->mod_mem[type];

I can't see the added value of the mod_ prefix.

Would read better as

        mem = &mod->mem[type];

        return (unsigned long)mem->base <= addr && addr < (unsigned
long)mem->base + mem->size;

And could be even more readable as:

        unsigned long base, size;

        base = (unsigned long)mod->mod_mem[type].base;
        size = mod->mod_mem[type].size;

        return base <= addr && addr < base + size;

Yeah, the code does look better with shorter names.

If there is no objection from folks, I will send v4 with these
suggestions next week.

Thanks,
Song