[3365] in linux-scsi channel archive
Re: Bug in new SCSI code
daemon@ATHENA.MIT.EDU (Eric Youngdale)
Thu Feb 19 08:53:05 1998
Date: Thu, 19 Feb 1998 09:20:13 -0500 (EST)
From: Eric Youngdale <eric@andante.jic.com>
To: Jakub Jelinek <jj@sunsite.ms.mff.cuni.cz>
cc: torvalds@transmeta.com, linux-scsi@vger.rutgers.edu
In-Reply-To: <199802190739.IAA12528@sunsite.ms.mff.cuni.cz>
On Thu, 19 Feb 1998, Jakub Jelinek wrote:
> > 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.
Assumption is the wrong word. It is a bug :-). WRT cli, see
below.
> 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.
I considered this, but the problem is that some requests might be
serviced from a non-interrupt mode, and if you get interrupted while
holding a spinlock by something which services a scsi interrupt this gets
converted to a deadlock. I guess this implies that you need both a cli
*and* a spinlock. Ugh. Interesting point, however - if you are in
interrupt mode, then a spinlock would be sufficient.
It was to avoid the cli (in what I assumed would be the normal
case) that I had added the performance enhancement.
Then again for your case, perhaps what would be better to split
scsi_done(), so that you could 'finish' multiple commands without
inserting them in the bh queue. Once you are done, you can insert the
whole chain in the bh queue in one operation. If you have 64 requests
finishing, then you would at most have to block interrupts only once (and
the atomic exchange would probably work most of the time).
> 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
Once the requests are all finished, probably not. The bh has two
jobs, really. One is to release any resources related to the request
(and wake up any process waiting for those requests to finish), and
secondly to queue new requests (if there are any). At the moment, I
cannot think of any reason to maintain ordering (as long as we don't lose
them).
> do {
> SCswap = scsi_bh_queue_head;
> SCpnt->bh_next = SCswap;
> } while (cas(&scsi_bh_queue_head, SCswap, SCpnt));
You've lost me. What's 'cas()', and what platforms have it?
>
> and in bh
>
> just
>
> SCswap = xchg(&scsi_bh_queue_head, NULL)
This last part is what we are doing now, actually...
-Eric
