Re: [PATCH 1/4] module: Add module_subinit{_noexit} and module_subeixt helper macros

[Youling Tang <youling.tang@linux.dev>]

Hi, Mika

On 23/07/2024 17:58, Mika Penttilä wrote:
> On 7/23/24 11:32, Youling Tang wrote:
>> From: Youling Tang <tangyouling@kylinos.cn>
>>
>> In theory init/exit should match their sequence, thus normally they should
>> look like this:
>> -------------------------+------------------------
>>      init_A();            |
>>      init_B();            |
>>      init_C();            |
>>                           |   exit_C();
>>                           |   exit_B();
>>                           |   exit_A();
>>
>> Providing module_subinit{_noexit} and module_subeixt helps macros ensure
>> that modules init/exit match their order, while also simplifying the code.
>>
>> The three macros are defined as follows:
>> - module_subinit(initfn, exitfn,rollback)
>> - module_subinit_noexit(initfn, rollback)
>> - module_subexit(rollback)
>>
>> `initfn` is the initialization function and `exitfn` is the corresponding
>> exit function.
>>
>> Signed-off-by: Youling Tang <tangyouling@kylinos.cn>
>> ---
>>   include/asm-generic/vmlinux.lds.h |  5 +++
>>   include/linux/init.h              | 62 ++++++++++++++++++++++++++++++-
>>   include/linux/module.h            | 22 +++++++++++
>>   3 files changed, 88 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>> index 677315e51e54..48ccac7c6448 100644
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -927,6 +927,10 @@
>>   		INIT_CALLS_LEVEL(7)					\
>>   		__initcall_end = .;
>>   
>> +#define SUBINIT_CALL							\
>> +		*(.subinitcall.init)					\
>> +		*(.subexitcall.exit)
>> +
>>   #define CON_INITCALL							\
>>   	BOUNDED_SECTION_POST_LABEL(.con_initcall.init, __con_initcall, _start, _end)
>>   
>> @@ -1155,6 +1159,7 @@
>>   		INIT_DATA						\
>>   		INIT_SETUP(initsetup_align)				\
>>   		INIT_CALLS						\
>> +		SUBINIT_CALL						\
>>   		CON_INITCALL						\
>>   		INIT_RAM_FS						\
>>   	}
>> diff --git a/include/linux/init.h b/include/linux/init.h
>> index ee1309473bc6..e8689ff2cb6c 100644
>> --- a/include/linux/init.h
>> +++ b/include/linux/init.h
>> @@ -55,6 +55,9 @@
>>   #define __exitdata	__section(".exit.data")
>>   #define __exit_call	__used __section(".exitcall.exit")
>>   
>> +#define __subinit_call	__used __section(".subinitcall.init")
>> +#define __subexit_call	__used __section(".subexitcall.exit")
>> +
>>   /*
>>    * modpost check for section mismatches during the kernel build.
>>    * A section mismatch happens when there are references from a
>> @@ -115,6 +118,9 @@
>>   typedef int (*initcall_t)(void);
>>   typedef void (*exitcall_t)(void);
>>   
>> +typedef int (*subinitcall_t)(void);
>> +typedef void (*subexitcall_t)(void);
>> +
>>   #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
>>   typedef int initcall_entry_t;
>>   
>> @@ -183,7 +189,61 @@ extern struct module __this_module;
>>   #endif
>>   
>>   #endif
>> -
>> +
>> +#ifndef __ASSEMBLY__
>> +struct subexitcall_rollback {
>> +	/*
>> +	 * Records the address of the first sub-initialization function in the
>> +	 * ".subexitcall.exit" section
>> +	 */
>> +	unsigned long first_addr;
>> +	int ncalls;
>> +};
>> +
>> +static inline void __subexitcall_rollback(struct subexitcall_rollback *r)
>> +{
>> +	unsigned long addr = r->first_addr - sizeof(r->first_addr) * (r->ncalls - 1);
>> +
>> +	for (; r->ncalls--; addr += sizeof(r->first_addr)) {
>> +		unsigned long *tmp = (void *)addr;
>> +		subexitcall_t fn = (subexitcall_t)*tmp;
>> +		fn();
>> +	}
>> +}
> How does this guarantee the exit calls match sequence? Are you assuming
> linker puts exit functions in reverse order?
Take btrfs for example:
Initialize the function sequentially in init_btrfs_fs() using
module_subinit{_noexit}, storing the corresponding function addresses
in the specified ".subinitcall.init" and ".subexitcall.exit" sections.

Using gcc to compile btrfs to.ko, the view section contains the following:
```
$ objdump -d -j ".subinitcall.init" fs/btrfs/super.o

fs/btrfs/super.o:     file format elf64-x86-64


Disassembly of section .subinitcall.init:

0000000000000000 <__subinitcall_register_btrfs.0>:
     ...

0000000000000008 <__subinitcall_btrfs_run_sanity_tests.2>:
     ...

0000000000000010 <__subinitcall_btrfs_print_mod_info.3>:
     ...

0000000000000018 <__subinitcall_btrfs_interface_init.4>:
     ...

0000000000000020 <__subinitcall_btrfs_prelim_ref_init.6>:
     ...

0000000000000028 <__subinitcall_btrfs_delayed_ref_init.8>:
     ...

0000000000000030 <__subinitcall_btrfs_auto_defrag_init.10>:
     ...

0000000000000038 <__subinitcall_btrfs_delayed_inode_init.12>:
     ...

0000000000000040 <__subinitcall_ordered_data_init.14>:
     ...

0000000000000048 <__subinitcall_extent_map_init.16>:
     ...

0000000000000050 <__subinitcall_btrfs_bioset_init.18>:
     ...

0000000000000058 <__subinitcall_extent_buffer_init_cachep.20>:
     ...

0000000000000060 <__subinitcall_extent_state_init_cachep.22>:
     ...

0000000000000068 <__subinitcall_btrfs_free_space_init.24>:
     ...

0000000000000070 <__subinitcall_btrfs_ctree_init.26>:
     ...

0000000000000078 <__subinitcall_btrfs_transaction_init.28>:
     ...

0000000000000080 <__subinitcall_btrfs_init_dio.30>:
     ...

0000000000000088 <__subinitcall_btrfs_init_cachep.32>:
     ...

0000000000000090 <__subinitcall_btrfs_init_compress.34>:
     ...

0000000000000098 <__subinitcall_btrfs_init_sysfs.36>:
     ...

00000000000000a0 <__subinitcall_btrfs_props_init.38>:
     ...

```

```
$ objdump -d -j ".subexitcall.exit" fs/btrfs/super.o

fs/btrfs/super.o:     file format elf64-x86-64


Disassembly of section .subexitcall.exit:

0000000000000000 <__subexitcall_unregister_btrfs.1>:
     ...

0000000000000008 <__subexitcall_btrfs_interface_exit.5>:
     ...

0000000000000010 <__subexitcall_btrfs_prelim_ref_exit.7>:
     ...

0000000000000018 <__subexitcall_btrfs_delayed_ref_exit.9>:
     ...

0000000000000020 <__subexitcall_btrfs_auto_defrag_exit.11>:
     ...

0000000000000028 <__subexitcall_btrfs_delayed_inode_exit.13>:
     ...

0000000000000030 <__subexitcall_ordered_data_exit.15>:
     ...

0000000000000038 <__subexitcall_extent_map_exit.17>:
     ...

0000000000000040 <__subexitcall_btrfs_bioset_exit.19>:
     ...

0000000000000048 <__subexitcall_extent_buffer_free_cachep.21>:
     ...

0000000000000050 <__subexitcall_extent_state_free_cachep.23>:
     ...

0000000000000058 <__subexitcall_btrfs_free_space_exit.25>:
     ...

0000000000000060 <__subexitcall_btrfs_ctree_exit.27>:
     ...

0000000000000068 <__subexitcall_btrfs_transaction_exit.29>:
     ...

0000000000000070 <__subexitcall_btrfs_destroy_dio.31>:
     ...

0000000000000078 <__subexitcall_btrfs_destroy_cachep.33>:
     ...

0000000000000080 <__subexitcall_btrfs_exit_compress.35>:
     ...

0000000000000088 <__subexitcall_btrfs_exit_sysfs.37>:
     ...


```

 From the above, we can see that the compiler stores the init/exit function
in reverse order.

Thanks,
Youling.