On Tue, 9 Aug 2016, Steven Rostedt wrote: > On Tue, 9 Aug 2016 10:16:00 +0200 (CEST) > Miroslav Benes <mbe...@suse.cz> wrote: > > > > I agree it is kind of shooting oneself in the foot bug, because explicit > > call to a sleeping function may not be the brightest thing to do. However > > I see two (closely related) issues with this. > > > > 1. It is a change in behaviour. Ftrace silently relies on an atomicity of > > ops->func(). I don't see it documented anywhere, but it did not matter > > because the atomicity was always guaranteed as described above. Now there > > is a possibility to achieve a situation which breaks the assumption. It > > makes me worried. > > Why? It's something that a kernel developer should be aware of. I mean, > that ops->func can easily be called from *any* context, like irq, > softirq, or even an NMI. One who hooks into any function of the kernel > should understand that it has special requirements, just like we don't > document that you can't sleep in an NMI. > > And if you only hook to functions that can sleep, then great! You are > allowed to do that too. Just like calling a module function that can > sleep. You need to make sure nothing is calling your function when you > unload the module. I don't see anything that is deceptive here.
At least the comment in ftrace_shutdown() is deceptive. But well, I understood your opinion from the first reply. I just didn't agree with it and that's why I expressed it. > > > > 2. Previously if someone called a function which could sleep he was > > immediately warned not to do so via "sleeping in atomic context" BUG. Now > > he wouldn't know. That's because in_atomic() and might_sleep() > > infrastructure does not work in ops->func(). in_atomic() gives 0 even if > > it is an atomic context in fact. But well, the comment for in_atomic() in > > linux/preempt.h warns about exactly this situation I guess. > > It will warn if you hook to a function that can sleep. And if you never > do, then there's nothing wrong. If the only functions you hook to can > sleep, then it is fine for you to sleep in your code too. But if you > do, you must synchronize that logic. You must make sure all functions > are out of the sleep when you unresgister. Just like you must make sure > all functions are out of a sleeping function in a module. This is > kernel programming 101. > > I never saw a need to have sleeping functions being called by > ops->func() and I don't know of a case that would. If there is a > legitimate case (not hypothetical) and then I could add a way to > postpone freeing of an ops if need be. > > Because note, that TASK_RCU will only be called when CONFIG_PREEMPT is > enabled. It would be overkill to do it for !CONFIG_PREEMPT, thus it > will not solve what you want here. Fair enough. I can live with that. Miroslav
