lkml.org 
[lkml]   [2014]   [Jul]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] drbd: Remove fix me statements
From
On Wed, Jul 23, 2014 at 1:33 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Nick Krause <xerofoify@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2014-07-23 22:01    [W:0.087 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site