On 2015/12/16 1:37, Steven Rostedt wrote: > On Tue, 15 Dec 2015 11:26:41 +0800 > "Zhang, Yanmin" <yanmin_zh...@linux.intel.com> wrote: > >>> This seems very hackish, although I can't think of a better way at the >>> moment. But I would like not to add more code into module.c if >>> possible, and just use a notifier unless there's a real reason we can't >>> (as there was when we added the hook in the first place). >>> We would double-check it again. It's not a good idea to change common >>> module codes. >> I double-checked it. If using notifier, we have to add 2 new module >> notification >> events in enum module_state. >> For example, MODULE_STATE_INITING, MODULE_STATE_INITING_FAILED. >> At MODULE_STATE_INITING, call ftrace_module_init(mod); >> At MODULE_STATE_INITING_FAILED, call ftrace_release_mod (or similar >> function); >> At MODULE_STATE_COMING, call a new function which links the new start_pg >> to ftrace_pages. >> Such design can also fix another bug in current kernel that ftrace start_pg >> of the module is not cleaned up if complete_formation fails. >> However, we change module common codes. The new events only serve ftrace. >> >> If not holding ftrace_mutex across start_pg setup and complete_formation, >> some other variables are not protected well, such like ftrace_pages, >> ftrace_start_up, and so on. > OK, that was basically the reason the hook was added in the first place. > > The reason the modules notifier doesn't work here is that there's too > many exits from the module code that may leave the mutex held.
Yes. > > I thought about this some more. So the issue is that we add the module > functions to the ftrace records. Then another task enables function > tracing before the module is fully set up. As ftrace is enabling > function tracing, the module sets its text to read-only, outside the > scope of ftrace setting all text to read-write. When ftrace gets to the > module code, it fails to do the modifications and you get the > ftrace_bug() splat. > > Sounds right? Right, indeed. My case is worse than that as my android uses kernel 3.14.50, which has another bug around remove_breakpoint as it calls probe_kernel_write directly. The latest upstream has no such issue. So we just need focus on the race you explain well. > > Now, I really dislike the taking of the ftrace mutex and returning back > to module handling. That is very subtle, fragile and prone to bugs. Yes, just like that the current codes are not perfect. :) > I > took a step back to find another solution and I think I found one. > > Take the below patch and add to it (I'll keep this patch as mine, and > you can submit another patch to do the following). You don't need to > resend this patch, just send me a patch that does this: Ok. > > > Add the module notifier that calls ftrace_module_enable(mod), removing > it from ftrace_init_module(). Feel free to monkey with that function to > be able to be called directly if it can't already. > > What my patch does is to create a new ftrace record flag DISABLED, > which is set to all functions in a module as it is added. Then later on > (in the module notifier that you will add), the flag is cleared. In > between, the module functions will be ignored. It's a good idea. > > If the module errors out and the notifier is not called, we don't care. > The records will simply be removed, and the flag will be meaningless. One question: if complete_formation fails, load_module wouldn't send any notification and ftrace_release_mod is not called. How can ftrace core remove all pg->records of the module? If complete_formation succeeds, further calls cause module errors out, load_module would send MODULE_STATE_GOING, ftrace_release_mod is called. Thanks for your quick response. Yanmin -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
