| Message ID | 1381871068-27660-5-git-send-email-dave.long@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/15, David Long wrote: > > Add a weak function for any architecture-specific initialization. ARM > will use this to register the handlers for the undefined instructions it > uses to implement uprobes. Could you explain why ARM can't simply do the necessary initialization in arch/arm/kernel/uprobes-arm.c ? > +int __weak __init arch_uprobes_init(void) > +{ > + return 0; > +} > + > static int __init init_uprobes(void) > { > + int ret; > int i; > > for (i = 0; i < UPROBES_HASH_SZ; i++) > @@ -1870,6 +1876,10 @@ static int __init init_uprobes(void) > if (percpu_init_rwsem(&dup_mmap_sem)) > return -ENOMEM; > > + ret = arch_uprobes_init(); > + if (ret) > + return ret; > + > return register_die_notifier(&uprobe_exception_nb); > } > module_init(init_uprobes); IOW, why do we need to call arch_uprobes_init() from init_uprobes(). Oleg.
On 10/19/13 12:42, Oleg Nesterov wrote: > On 10/15, David Long wrote: >> >> Add a weak function for any architecture-specific initialization. ARM >> will use this to register the handlers for the undefined instructions it >> uses to implement uprobes. > > Could you explain why ARM can't simply do the necessary initialization in > arch/arm/kernel/uprobes-arm.c ? > > >> +int __weak __init arch_uprobes_init(void) >> +{ >> + return 0; >> +} >> + >> static int __init init_uprobes(void) >> { >> + int ret; >> int i; >> >> for (i = 0; i < UPROBES_HASH_SZ; i++) >> @@ -1870,6 +1876,10 @@ static int __init init_uprobes(void) >> if (percpu_init_rwsem(&dup_mmap_sem)) >> return -ENOMEM; >> >> + ret = arch_uprobes_init(); >> + if (ret) >> + return ret; >> + >> return register_die_notifier(&uprobe_exception_nb); >> } >> module_init(init_uprobes); > > IOW, why do we need to call arch_uprobes_init() from init_uprobes(). > > Oleg > I don't know how you would do the initialization without invoking it through the module_init function, which I think you can only have one of. Could you explain in more detail what you had in mind? -dl
On 10/22, David Long wrote: > > On 10/19/13 12:42, Oleg Nesterov wrote: >> On 10/15, David Long wrote: >>> >>> Add a weak function for any architecture-specific initialization. ARM >>> will use this to register the handlers for the undefined instructions it >>> uses to implement uprobes. >> >> Could you explain why ARM can't simply do the necessary initialization in >> arch/arm/kernel/uprobes-arm.c ? >> >> >>> +int __weak __init arch_uprobes_init(void) >>> +{ >>> + return 0; >>> +} >>> + >>> static int __init init_uprobes(void) >>> { >>> + int ret; >>> int i; >>> >>> for (i = 0; i < UPROBES_HASH_SZ; i++) >>> @@ -1870,6 +1876,10 @@ static int __init init_uprobes(void) >>> if (percpu_init_rwsem(&dup_mmap_sem)) >>> return -ENOMEM; >>> >>> + ret = arch_uprobes_init(); >>> + if (ret) >>> + return ret; >>> + >>> return register_die_notifier(&uprobe_exception_nb); >>> } >>> module_init(init_uprobes); >> >> IOW, why do we need to call arch_uprobes_init() from init_uprobes(). >> >> Oleg >> > > I don't know how you would do the initialization without invoking it > through the module_init function, which I think you can only have one > of. Could you explain in more detail what you had in mind? I simply do not understand why uprobes.c uses module_init/module_exit, it can't be compiled as a module. I think that module_exit/exit_uprobes should be killed, and module_init() should be turned into __initcall(). uprobes-arm.c can have another one. Oleg.
On 10/28/13 14:58, Oleg Nesterov wrote: > On 10/22, David Long wrote: > I simply do not understand why uprobes.c uses module_init/module_exit, > it can't be compiled as a module. I guess that makes sense, assuming it can never be made a module. I saw you recent commit for this. > I think that module_exit/exit_uprobes should be killed, and module_init() > should be turned into __initcall(). uprobes-arm.c can have another one. > I will see if I can make this work. Right now the arch-specific initialization call is done in the middle of the generic initialization code, but I don't know that it *has* to be that way. I have some concern too about getting the order right, since these are built from different makefiles. -dl
On 10/31, David Long wrote: > On 10/28/13 14:58, Oleg Nesterov wrote: >> On 10/22, David Long wrote: >> I simply do not understand why uprobes.c uses module_init/module_exit, >> it can't be compiled as a module. > > I guess that makes sense, assuming it can never be made a module. I saw > you recent commit for this. > >> I think that module_exit/exit_uprobes should be killed, and module_init() >> should be turned into __initcall(). uprobes-arm.c can have another one. >> > > I will see if I can make this work. If this can't work, then we need the new hook (this patch). But in this case please update the changelog to explain the reason. > Right now the arch-specific > initialization call is done in the middle of the generic initialization > code, but I don't know that it *has* to be that way. I have some > concern too about getting the order right, since these are built from > different makefiles. Not sure I understand... But grep shows a lot of core_initcall()'s in arch/arm/ which do register_undef_hook(). And I guess you can use any initcall level. Oleg.
On 11/01/13 09:52, Oleg Nesterov wrote: > On 10/31, David Long wrote: >> On 10/28/13 14:58, Oleg Nesterov wrote: >>> On 10/22, David Long wrote: >>> I simply do not understand why uprobes.c uses module_init/module_exit, >>> it can't be compiled as a module. >> >> I guess that makes sense, assuming it can never be made a module. I saw >> you recent commit for this. >> >>> I think that module_exit/exit_uprobes should be killed, and module_init() >>> should be turned into __initcall(). uprobes-arm.c can have another one. >>> >> >> I will see if I can make this work. > > If this can't work, then we need the new hook (this patch). But in this > case please update the changelog to explain the reason. > >> Right now the arch-specific >> initialization call is done in the middle of the generic initialization >> code, but I don't know that it *has* to be that way. I have some >> concern too about getting the order right, since these are built from >> different makefiles. > > Not sure I understand... But grep shows a lot of core_initcall()'s in > arch/arm/ which do register_undef_hook(). And I guess you can use any > initcall level. Just to close on this, I implemented your suggested __initcall change and it tested out fine. -dl
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h index 2556ab6..a28dcbe 100644 --- a/include/linux/uprobes.h +++ b/include/linux/uprobes.h @@ -135,6 +135,7 @@ extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs) extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs *regs); extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs); extern void __weak arch_uprobe_xol_copy(struct arch_uprobe *auprobe, void *vaddr); +extern int __weak arch_uprobes_init(void); #else /* !CONFIG_UPROBES */ struct uprobes_state { }; diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index 22d0121..9734a5f 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -1860,8 +1860,14 @@ static struct notifier_block uprobe_exception_nb = { .priority = INT_MAX-1, /* notified after kprobes, kgdb */ }; +int __weak __init arch_uprobes_init(void) +{ + return 0; +} + static int __init init_uprobes(void) { + int ret; int i; for (i = 0; i < UPROBES_HASH_SZ; i++) @@ -1870,6 +1876,10 @@ static int __init init_uprobes(void) if (percpu_init_rwsem(&dup_mmap_sem)) return -ENOMEM; + ret = arch_uprobes_init(); + if (ret) + return ret; + return register_die_notifier(&uprobe_exception_nb); } module_init(init_uprobes);