This removes no longer needed fix me statements as the spinlocks
are needed to protect against other users of the list accessing
it when items on it are being moved off it.
Signed-off-by: Nicholas Krause <[email protected]>
---
drivers/block/drbd/drbd_worker.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/block/drbd/drbd_worker.c b/drivers/block/drbd/drbd_worker.c
index d8f57b6..8793a32 100644
--- a/drivers/block/drbd/drbd_worker.c
+++ b/drivers/block/drbd/drbd_worker.c
@@ -1847,12 +1847,12 @@ static void wait_for_work(struct drbd_connection *connection, struct list_head *
int send_barrier;
prepare_to_wait(&connection->sender_work.q_wait, &wait, TASK_INTERRUPTIBLE);
spin_lock_irq(&connection->resource->req_lock);
- spin_lock(&connection->sender_work.q_lock); /* FIXME get rid of this one? */
+ spin_lock(&connection->sender_work.q_lock);
/* dequeue single item only,
* we still use drbd_queue_work_front() in some places */
if (!list_empty(&connection->sender_work.q))
list_move(connection->sender_work.q.next, work_list);
- spin_unlock(&connection->sender_work.q_lock); /* FIXME get rid of this one? */
+ spin_unlock(&connection->sender_work.q_lock);
if (!list_empty(work_list) || signal_pending(current)) {
spin_unlock_irq(&connection->resource->req_lock);
break;
--
1.9.1
Nicholas Krause <[email protected]> writes:
> This removes no longer needed fix me statements
If you claim that they no longer are needed then I expect you to explain
what's changed since they were added.
> as the spinlocks
> are needed to protect against other users of the list accessing
> it when items on it are being moved off it.
Yes, that's most likely why the locking is there.
The FIXMEs on the other hand, are probably there to tell you that it
would be nice to get rid of the double locking. I don't see that
changing.
Bjørn
On Wed, Jul 23, 2014 at 8:27 AM, Bjørn Mork <[email protected]> wrote:
> Nicholas Krause <[email protected]> writes:
>
>> This removes no longer needed fix me statements
>
> If you claim that they no longer are needed then I expect you to explain
> what's changed since they were added.
>
>> as the spinlocks
>> are needed to protect against other users of the list accessing
>> it when items on it are being moved off it.
>
> Yes, that's most likely why the locking is there.
>
> The FIXMEs on the other hand, are probably there to tell you that it
> would be nice to get rid of the double locking. I don't see that
> changing.
>
>
> Bjørn
>
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.
Cheers Nick
Nick Krause <[email protected]> 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)
On Wed, Jul 23, 2014 at 1:33 PM, Bjørn Mork <[email protected]> wrote:
> Nick Krause <[email protected]> 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