The Kconfig currently controlling compilation of this code is:
menuconfig MODULES
bool "Enable loadable module support"
...meaning that it currently is not being built as a module by anyone.
No surprise here, since modular support being a module would be an
interesting chicken before the egg problem.
Lets remove the use of module_init in this code so that when reading
the file, there is less doubt that it is builtin-only.
Since module_init translates to device_initcall in the non-modular
case, the init ordering remains unchanged with this commit. However
one could argue that fs_initcall makes more sense for proc stuff,
and we can change the initcall order later and watch for fallout
if so desired.
We can't of course delete the module.h include in this case since it
is used all through the rest of the file.
Cc: Rusty Russell <[email protected]>
Signed-off-by: Paul Gortmaker <[email protected]>
---
[I was undecided as to whether we should do this in one step
or two, i.e. instead just make the change to fs_initcall here
and now, and so went with the more cautious/granular approach.]
kernel/module.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/module.c b/kernel/module.c
index 8f051a106676..7750bdcb12fc 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3947,7 +3947,7 @@ static int __init proc_modules_init(void)
proc_create("modules", 0, NULL, &proc_modules_operations);
return 0;
}
-module_init(proc_modules_init);
+device_initcall(proc_modules_init);
#endif
/* Given an address, look for it in the module exception tables. */
--
2.5.0
Paul Gortmaker <[email protected]> writes:
> The Kconfig currently controlling compilation of this code is:
>
> menuconfig MODULES
> bool "Enable loadable module support"
>
> ...meaning that it currently is not being built as a module by anyone.
> No surprise here, since modular support being a module would be an
> interesting chicken before the egg problem.
>
> Lets remove the use of module_init in this code so that when reading
> the file, there is less doubt that it is builtin-only.
>
> Since module_init translates to device_initcall in the non-modular
> case, the init ordering remains unchanged with this commit. However
> one could argue that fs_initcall makes more sense for proc stuff,
> and we can change the initcall order later and watch for fallout
> if so desired.
This patch is just weird; is this part of something larger you are
trying to do?
I would argue that module_init() should be the default; it implies
no dependencies on the initialization, and it's a common pattern.
Cheers,
Rusty.
> We can't of course delete the module.h include in this case since it
> is used all through the rest of the file.
>
> Cc: Rusty Russell <[email protected]>
> Signed-off-by: Paul Gortmaker <[email protected]>
> ---
>
> [I was undecided as to whether we should do this in one step
> or two, i.e. instead just make the change to fs_initcall here
> and now, and so went with the more cautious/granular approach.]
>
> kernel/module.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 8f051a106676..7750bdcb12fc 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3947,7 +3947,7 @@ static int __init proc_modules_init(void)
> proc_create("modules", 0, NULL, &proc_modules_operations);
> return 0;
> }
> -module_init(proc_modules_init);
> +device_initcall(proc_modules_init);
> #endif
>
> /* Given an address, look for it in the module exception tables. */
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On 2015-08-26 12:06 AM, Rusty Russell wrote:
> Paul Gortmaker <[email protected]> writes:
>> The Kconfig currently controlling compilation of this code is:
>>
>> menuconfig MODULES
>> bool "Enable loadable module support"
>>
>> ...meaning that it currently is not being built as a module by anyone.
>> No surprise here, since modular support being a module would be an
>> interesting chicken before the egg problem.
>>
>> Lets remove the use of module_init in this code so that when reading
>> the file, there is less doubt that it is builtin-only.
>>
>> Since module_init translates to device_initcall in the non-modular
>> case, the init ordering remains unchanged with this commit. However
>> one could argue that fs_initcall makes more sense for proc stuff,
>> and we can change the initcall order later and watch for fallout
>> if so desired.
>
> This patch is just weird; is this part of something larger you are
> trying to do?
Yes, it is part of a larger cleanup; for subsystems with more than
one patch I created a 0/N explanatory note; such as:
https://lkml.kernel.org/r/[email protected]
https://lkml.kernel.org/r/[email protected]
and others. Apologies for the lack of context on this single patch.
> I would argue that module_init() should be the default; it implies
> no dependencies on the initialization, and it's a common pattern.
To summarize briefly, module_init forces everything into one
initcall priority bin, it encourages people to write module_exit
functions that are never used, and it can make the code appear
inconsistent with the Kconfig and/or Makefile settings. So I'd
hope you'd agree that there is value in not using module_init
in code that can never be modular.
Thanks,
Paul.
--
>
> Cheers,
> Rusty.
>
>> We can't of course delete the module.h include in this case since it
>> is used all through the rest of the file.
>>
>> Cc: Rusty Russell <[email protected]>
>> Signed-off-by: Paul Gortmaker <[email protected]>
>> ---
>>
>> [I was undecided as to whether we should do this in one step
>> or two, i.e. instead just make the change to fs_initcall here
>> and now, and so went with the more cautious/granular approach.]
>>
>> kernel/module.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 8f051a106676..7750bdcb12fc 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -3947,7 +3947,7 @@ static int __init proc_modules_init(void)
>> proc_create("modules", 0, NULL, &proc_modules_operations);
>> return 0;
>> }
>> -module_init(proc_modules_init);
>> +device_initcall(proc_modules_init);
>> #endif
>>
>> /* Given an address, look for it in the module exception tables. */
>> --
>> 2.5.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
Paul Gortmaker <[email protected]> writes:
> On 2015-08-26 12:06 AM, Rusty Russell wrote:
>> Paul Gortmaker <[email protected]> writes:
>>> The Kconfig currently controlling compilation of this code is:
>>>
>>> menuconfig MODULES
>>> bool "Enable loadable module support"
>>>
>>> ...meaning that it currently is not being built as a module by anyone.
>>> No surprise here, since modular support being a module would be an
>>> interesting chicken before the egg problem.
>>>
>>> Lets remove the use of module_init in this code so that when reading
>>> the file, there is less doubt that it is builtin-only.
>>>
>>> Since module_init translates to device_initcall in the non-modular
>>> case, the init ordering remains unchanged with this commit. However
>>> one could argue that fs_initcall makes more sense for proc stuff,
>>> and we can change the initcall order later and watch for fallout
>>> if so desired.
>>
>> This patch is just weird; is this part of something larger you are
>> trying to do?
>
> Yes, it is part of a larger cleanup; for subsystems with more than
> one patch I created a 0/N explanatory note; such as:
>
> https://lkml.kernel.org/r/[email protected]
> https://lkml.kernel.org/r/[email protected]
>
> and others. Apologies for the lack of context on this single patch.
>
>> I would argue that module_init() should be the default; it implies
>> no dependencies on the initialization, and it's a common pattern.
>
> To summarize briefly, module_init forces everything into one
> initcall priority bin
That's a feature. Everything should be in one bin unless there's reason
to specify; once you've specified, it's almost impossible to change
because our system is now so complicated.
module_init() says "I don't care".
> , it encourages people to write module_exit
> functions that are never used
Then write a checker. It's not that hard for kbuild to show stuff that
can never be a module, I'm sure.
>, and it can make the code appear
> inconsistent with the Kconfig and/or Makefile settings. So I'd
> hope you'd agree that there is value in not using module_init
> in code that can never be modular.
On the other hand, people cut & paste like crazy, so it makes sense to
have them all use module_init() and not build non-modular code, because
most code is a module.
Since this is total bikeshed, you can apply this yourself:
Acked-by: Rusty Russell <[email protected]> (disagree, but hey...)
Cheers,
Rusty.