From: Jes S. <je...@sg...> - 2006-05-18 09:33:23
|
Matt Helsley wrote: > I think the name is technically appropriate. I don't think the name is > a product of your ingenuity or specific to your implementation. > Furthermore it seems common practice to change kernel code without > changing function/concept names -- especially when discussing > alternatives by posting patches. > > I believe I've acted appropriately and I don't see why you object. I objected because task notification IMHO means notifying the task, but either way I think we can agree that at least the name wouldn't be 100% clear in this case. In addition if we are going to end up with both levels of notifiers, then the task notifiers (eg. notifying the task) would be based on the code we posted earlier. By keeping all the same names, you basically require me to change every single line of code. > That said, I have no desire to drag things out with name ownership > arguments. Given your sensitivity to my use of names you've introduced I > think I'll rename it to task watchers. Don't get me wrong, I am not particularly attached to the name, but I do not think the naming in your patch doesn't match what it is doing. 'Task watchers' would be better. >> Second, patch 8/11, changing it to a blocking chain is not good. It >> does exactly what we've been discussing for a while, adding an expensive >> semaphore to the wrong place. This is where Alan Stern's per CPU locks >> would be ideal. > > This series gives us the opporunity to test these paths with the > semaphore with or without Alan's per CPU "locks". This allows us to > confirm that the added complexity is necessary. The notifier chain > functions insulate us from that change. Since we know Alan's code will do better, why not go for it instead of starting to rely on something which we know is bad? >> Last I really don't see how the proof of concept 11/11 patch proves how >> to do things. It means you lose the ability to pass in any data to the >> notifier chain since the task struct is passed in instead. This may not > > Isn't that what your patches did -- pass in the task struct? Hmmmm I know I needed to pass in other bits in previous patches, I can't remember the exact details, in one place bprm was needed. > I can see how the task struct might not be sufficient. I noticed the > taskstats functions which require more arguments are generally nearby. > I've got an idea how to circumvent this limitation, but it's rather > ugly: That could work, but I agree it's not good for the eyes :) Cheers, Jes |