[3363] in linux-scsi channel archive
Re: Bug in new SCSI code
daemon@ATHENA.MIT.EDU (Jakub Jelinek)
Thu Feb 19 02:37:45 1998
From: Jakub Jelinek <jj@sunsite.ms.mff.cuni.cz>
To: eric@andante.jic.com
Date: Thu, 19 Feb 1998 08:39:53 +0100 (MET)
Cc: jj@sunsite.ms.mff.cuni.cz, torvalds@transmeta.com,
linux-scsi@vger.rutgers.edu
In-Reply-To: <Pine.LNX.3.95.980218231903.138B-100000@andante.jic.com> from "Eric Youngdale" at Feb 18, 98 11:29:08 pm
> Yes, it is my code, but there is no such assumption in the code.
> There is however a performance enhancement that you are seeing. The
> general idea is that if the list is empty, then we can insert something
> into the head of the linked list in an atomic operation without incurring
> the overhead of turning off interrupts. If there was already a request
> in the list, then we grab the lock and insert it.
There is an assumption at most two (see below), and grabbing a global cli is
an overkill, I think. Other CPUs should be able to have IRQs going.
>
> This isn't to say that there aren't bugs - there could very well
> be, and if you have seen evidence of it, then we can look at it.
...
>
> Have you any evidence that requests are being lost in the bottom
> half queue, or are you losing requests somewhere and are speculating that
> this is what is happening?
No, I had it in logs, and applying the patch cured it.
Consider what happens if scsi_done is called three times and the bh is not
serviced in between.
1) SCpnt1 is put into head
2) SCpnt2 is put into head while SCswap is SCpnt1; cli;
SCpnt1->bh_next=SCpnt2; head=SCpnt1; restore_flags
3) SCpnt3 is put into head while SCswap is SCpnt1; cli;
SCpnt1->bh_next=SCpnt3; head=SCpnt1; restore_flags
and you lost SCpnt2 in the chain (exactly this I saw in the logs -
once bh was run, it serviced SCpnt1 first and then SCpnt3, nobody mentioned
SCpnt2 and the dd I was using was waiting on exactly the SCpnt2's buffer).
And even if that worked, cli does not protect you against bh, so if things
are just right, a bh can e.g. steal you head=SCpnt1 and move to
SCpnt1->bh_next fast enough, so SCpnt1 will be serviced twice, etc.
I think a spinlock is a safe solution.
On the other side, do you think it does matter if we reorder the SCpnt queue
for bh? If we could do that, then on machines with cas we could do
do {
SCswap = scsi_bh_queue_head;
SCpnt->bh_next = SCswap;
} while (cas(&scsi_bh_queue_head, SCswap, SCpnt));
and in bh
just
SCswap = xchg(&scsi_bh_queue_head, NULL);
Cheers,
Jakub
___________________________________________________________________
Jakub Jelinek | jj@sunsite.mff.cuni.cz | http://sunsite.mff.cuni.cz
Administrator of SunSITE Czech Republic, MFF, Charles University
___________________________________________________________________
Ultralinux - first 64bit OS to take full power of the UltraSparc
Linux version 2.1.80 on a sparc64 machine (498.80 BogoMips).
___________________________________________________________________
