Note: "permalinks" may not be as permanent as we would like,
direct links of old sources may well be a few messages off.
On Wed, Jul 23, 2014 at 1:33 PM, Bjørn Mork <bjorn at mork.no> wrote: > Nick Krause <xerofoify at gmail.com> writes: > >> Bjorn, >> Can we remove the double locking as you are stating or do we still need it >> to protect against the list being accessed as the list seems to be moving >> to a spinlock protected list. > > I wouldn't know. > > The only thing I know is that the original author of those lines, who we > must assume has thorough knowlegde of this code, did not know how to fix > that in a simple and straight forward way. > > From this we can deduce that there is more to it than just changing a > couple of lines. If you don't alrady know this code in and out, then you > would have to start by analyzing the current locking model and > understanding that. Then, assuming the current double locking is in > fact necessary, you would need to redesign it so that you can make one > of the locks go away. Then you need to implement your new design. Then > test it _thoroughly_ to eliminate all the small bugs. Everyone adds bugs > when writing non-trivial code. (You seem to think that you can delegate > all the bug squashing to others simply because you don't own the > hardware. That is not so. If you don't have access to hardware for > testing, then you should not add any bugs. Yes, this implies that you > cannot write non-trivial code for hardware you don't have). Then you must > verify that the result is at least as efficient as the old code was. Or > there would be no point, would there? > > When all this is done, and the testing shows it is a success, *then* you > can remove the FIXME comment with a nice commit message explaining the > new locking model and why it now is safe to drop one of the locks. > > There is a fat chance that this just isn't worth all the work. Which is > most likely why the FIXME was stuck there in the first place. > > You should understand that noone will add a FIXME for anything trivial. > And if an author who knows the code well finds something non-trivial, > then you should definitely not touch it without investing enough time to > have a similar understanding of the code. > > Note again that I am writing all this as purely generic comments. I > don't know anything at all about the code in question, and I wouldn't > dare touching it without spending a lot of time understanding it first. > > As Steven said: find an area to focus on. Spend some time understanding > a small part of the kernel instead of jumping all around. > > And: Being able to test code yourself is absolutely necessary in the > beginning. But you don't necessarily have to run out and buy some odd > new hardware for that. I'm pretty sure many drivers and other parts of > the kernel is in use on the hardware you already have at hand :-) > Choose among those parts for your learning experience. > > Your USB hcd patch is a nice example of code that you most likely can > test yourself. And the pacth was fine too, except for the lack of a > proper commit message explaining why it was OK. But most of us will > just look at the "Acked-by: Alan Stern" line and figure that the change > definitely must be fine :-) > > > Bjørn (who also has sumitted his share of buggy patches, creating > unnecessary work for innoncent maintainers in the past. Sorry about > that Greg, Oliver, Alan, David, Mauro and all the others... I'm afraid I > cannot even guarantee that it won't happen again, but I do try my best) Bjorn , Thanks for the reply and the advice seems this is more work then I am time for now. Cheers Nick