2.5.15 IDE 61
Hi... Please correct me if I'm wrong but:
I think this patch is broken wrt serialization of channels on buggy chipsets.
The hwgroup was serialized so that in certain cases, it can contain BOTH channels, and thus only one channel is active at a time (e.g. cmd640). With this patch, you are now serializing only channels, not hwgroups (which makes hwgroup totally redundant, yes?), and I can't see which bit of your patch protects the chipsets that need both channels to be serialized.
I think I see where you're going with the cleanup (and this isn't unrelated to the conversation about IDE-62) but as it stands, this patch will IMHO totally fsck any machines with dodgy chipsets.
Neil
Uz.ytkownik Neil Conway napisa?:
Quoted 11 lines
Hi... Please correct me if I'm wrong but:
I think this patch is broken wrt serialization of channels on buggy chipsets.
The hwgroup was serialized so that in certain cases, it can contain BOTH
channels, and thus only one channel is active at a time (e.g. cmd640). With this patch, you are now serializing only channels, not hwgroups (which makes hwgroup totally redundant, yes?), and I can't see which bit
of your patch protects the chipsets that need both channels to be
serialized.
Quoted 5 lines
I think I see where you're going with the cleanup (and this isn't unrelated to the conversation about IDE-62) but as it stands, this patch will IMHO totally fsck any machines with dodgy chipsets.
Neil
No it will not, since we act serialized on ide_lock anyway. However I have right now per channel (or serialization group) lock running right now / modulo locking order problems.
Quoted 9 lines · Martin Dalecki
Uz.ytkownik Neil Conway napisa?:
Quoted 7 lines
The hwgroup was serialized so that in certain cases, it can contain BOTH channels, and thus only one channel is active at a time (e.g. cmd640). With this patch, you are now serializing only channels, not hwgroups (which makes hwgroup totally redundant, yes?), and I can't see which bit of your patch protects the chipsets that need both channels to be serialized.
Quoted 7 lines
Quoted 3 lines
I think I see where you're going with the cleanup (and this isn't unrelated to the conversation about IDE-62) but as it stands, this patch will IMHO totally fsck any machines with dodgy chipsets.
No it will not, since we act serialized on
ide_lockanyway. However I have right now per channel (or serialization group) lock running right now / modulo locking order problems.
One of us is missing the point (and I'm the newbie so blame me ;-)), so here goes:
Only the calls from the block layer to the request_fn are serialized by ide_lock. Not the actual data transfers. Here's the scenario:
Firstly, an I/O request is queued by ide_do_request(), and then it returns. Let's assume that DMA is now in progress. Once ide_do_request() returns, the lock is released by the block layer. Now the corruption scenario: another request can come in for the other channel while our first I/O is in flight, and since the ide_lock isn't held, and the second channel isn't BUSY, ide_do_request() will be happy to try and start an I/O on that channel too. BOOM.
Or is there a dumb mistake in my logic?
Neil
PS: I appreciate that your code is in a transition phase but I think it's desirable to avoid badly broken 2.5's all the same.
Uz.ytkownik Neil Conway napisa?:
Quoted 34 lines
Quoted 9 lines · Martin Dalecki
Uz.ytkownik Neil Conway napisa?:
Quoted 7 lines
The hwgroup was serialized so that in certain cases, it can contain BOTH channels, and thus only one channel is active at a time (e.g. cmd640). With this patch, you are now serializing only channels, not hwgroups (which makes hwgroup totally redundant, yes?), and I can't see which bit of your patch protects the chipsets that need both channels to be serialized.
Quoted 7 lines
Quoted 3 lines
I think I see where you're going with the cleanup (and this isn't unrelated to the conversation about IDE-62) but as it stands, this patch will IMHO totally fsck any machines with dodgy chipsets.
No it will not, since we act serialized on
ide_lockanyway. However I have right now per channel (or serialization group) lock running right now / modulo locking order problems.One of us is missing the point (and I'm the newbie so blame me ;-)), so here goes:
Only the calls from the block layer to the
request_fnare serialized byide_lock. Not the actual data transfers. Here's the scenario:Firstly, an I/O request is queued by
ide_do_request(), and then it returns. Let's assume that DMA is now in progress. Onceide_do_request()returns, the lock is released by the block layer. Now the corruption scenario: another request can come in for the other channel while our first I/O is in flight, and since theide_lockisn't held, and the second channel isn't BUSY,ide_do_request()will be happy to try and start an I/O on that channel too. BOOM.
Quoted 1 line
Or is there a dumb mistake in my logic?
There is no problem to go in paralell on different channels for requests. The serialization has only to be done
for the drive setup.
Quoted 5 lines
Neil
PS: I appreciate that your code is in a transition phase but I think it's desirable to avoid badly broken 2.5's all the same.
Quoted 3 lines · Martin Dalecki
There is no problem to go in paralell on different channels for requests. The serialization has only to be done
for the drive setup.
I agree for general chipsets, but my whole point was with regard to buggy chipsets which need to be serialized on both channels.
If you're saying that even these broken chipsets are OK with having transfers on one channel while setting up transfers on another channel, then perhaps you are right but that's not what I believed to be the case (can't find info to tell me either way right now).
But if that really were the case, then how could the (e.g.) cmd640 problem ever have been manifested? A spinlock is ALWAYS held while ide_do_request is executing. Even if it weren't, only an SMP machine could be trying to program both channels simultaneously because interrupts are disabled too.
Neil
Uz.ytkownik Neil Conway napisa?:
Quoted 15 lines
Quoted 3 lines · Martin Dalecki
There is no problem to go in paralell on different channels for requests. The serialization has only to be done
for the drive setup.I agree for general chipsets, but my whole point was with regard to buggy chipsets which need to be serialized on both channels.
If you're saying that even these broken chipsets are OK with having transfers on one channel while setting up transfers on another channel, then perhaps you are right but that's not what I believed to be the case (can't find info to tell me either way right now).
Quoted 5 lines
But if that really were the case, then how could the (e.g.) cmd640 problem ever have been manifested? A spinlock is ALWAYS held while
ide_do_requestis executing. Even if it weren't, only an SMP machine could be trying to program both channels simultaneously because interrupts are disabled too.
Well in the next patch round the hwgroup will be replaced with a spin lock, which is supposed to be shared between channels which need forced access serialization between them. Please look
at patches 62a and 63 :-).
Quoted 4 lines · Martin Dalecki
Well in the next patch round the hwgroup will be replaced with a spin lock, which is supposed to be shared between channels which need forced access serialization between them. Please look
at patches 62a and 63 :-).
Something here smells fishy here - you shouldn't hold a spinlock for a long time (a long time === spinlocking, setting up the drive, possibly scheduling, transferring data, getting status, then unlocking). Also, remember, spinlocks are no-ops on uniprocessor systems.
Signature · 2 lines
Russell King ([email protected]) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html
Quoted 2 lines
Something here smells fishy here - you shouldn't hold a spinlock for a long time (a long time === spinlocking, setting up the drive, possibly scheduling,
You can't hold it while scheduling or you may deadlock
Quoted 2 lines
transferring data, getting status, then unlocking). Also, remember, spinlocks are no-ops on uniprocessor systems.
Its possible it can be done with a semaphore but the whole business is pretty tricky. IDE command processing occurs a fair bit at interrupt level and you definitely don't want to block interrupts for long periods.
If the queue abstraction is right then the block layer should do all the synchronization work that is required. It may cost a few cycles on the odd case you can do overlapped command setup but that versus a nasty locking mess its got to be better to lose those few cycles.
I don't even Martin here, the ide locking is currently utterly vile
Uz.ytkownik Russell King napisa?:
Quoted 8 lines · Martin Dalecki
Quoted 5 lines
Well in the next patch round the hwgroup will be replaced with a spin lock, which is supposed to be shared between channels which need forced access serialization between them. Please look
at patches 62a and 63 :-).
Quoted 4 lines
Something here smells fishy here - you shouldn't hold a spinlock for a long time (a long time === spinlocking, setting up the drive, possibly scheduling, transferring data, getting status, then unlocking). Also, remember, spinlocks are no-ops on uniprocessor systems.
Well, let's just have a look at how to share request queues between channels which need this treatment in addition then?
Quoted 1 line · Alan Cox
I don't even Martin here, the ide locking is currently utterly vile
Agreed.
I'm adding BUG_ON()s to the IDE drivers I maintain where we must ensure only one channel is DMAing to catch possible data corruption before it happens.
Signature · 2 lines
Russell King ([email protected]) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html
Uz.ytkownik Russell King napisa?:
Quoted 10 lines · Alan Cox
Quoted 2 lines
I don't even Martin here, the ide locking is currently utterly vile
Agreed.
I'm adding BUG_ON()s to the IDE drivers I maintain where we must ensure only one channel is DMAing to catch possible data corruption before it happens.
That is indeed a good idea!
Uz.ytkownik Alan Cox napisa?:
Quoted 7 lines
Quoted 2 lines
Something here smells fishy here - you shouldn't hold a spinlock for a long time (a long time === spinlocking, setting up the drive, possibly scheduling,
You can't hold it while scheduling or you may deadlock
Quoted 7 lines
Quoted 2 lines
transferring data, getting status, then unlocking). Also, remember, spinlocks are no-ops on uniprocessor systems.
Its possible it can be done with a semaphore but the whole business is pretty tricky. IDE command processing occurs a fair bit at interrupt level and you definitely don't want to block interrupts for long periods.
... Becouse the chances are fscking high - that you will miss command completion interrupts for the "other drive" on the same channel. The dready heritage of "dangling ISA bus" with *idiotic* edge triggered interrupts bite us here. Someone just please shoot this enginer who saved the few pullup resistors in the head or send him alternatively for "hunting white bears" in Siberia... about 15 years would be fine in my opinnion.
Quoted 6 lines
If the queue abstraction is right then the block layer should do all the synchronization work that is required. It may cost a few cycles on the odd case you can do overlapped command setup but that versus a nasty locking mess its got to be better to lose those few cycles.
I don't even Martin here, the ide locking is currently utterly vile
Don't worry - I got your point.
Quoted 6 lines
Quoted 3 lines
Its possible it can be done with a semaphore but the whole business is pretty tricky. IDE command processing occurs a fair bit at interrupt level and you definitely don't want to block interrupts for long periods.
... Becouse the chances are fscking high - that you will miss command completion interrupts for the "other drive" on the same channel.
The shared IRQ capable IDE ards I am aware of all do have proper tristates
but you still have to handle the edge trigger very carefully.
If you can miss a command completion interrupt you have a bug. Since you know each drive on the bus you can poll each afflicted device for interrupts until you reach a point where you completed an entire poll loop and nobody had an IRQ pending.
At that point you know an edge transition has occurred and that a real IRQ will be posted when the next event occurs because that too will cause an edge.
A good place for examples of this in the DOS world is things like serial drivers, many of which could handle broken shared IRQ ISA setups correctly using this technique.
In the case without tristates the stronger driver tends to win the argument about the line in either direction and nothing works at all.
Quoted 3 lines · Martin Dalecki
There is no problem to go in paralell on different channels for requests. The serialization has only to be done
for the drive setup.
This assumption is *not* valid in all cases!
For example, get dtips10.pdf from the CMD website and learn how the CMD646 may fail on concurrent requests on both channels.
There are other chips which need runtime serialization.
Ciao,
Dani
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Daniela Engert, systems engineer at MEDAV GmbH
Gräfenberger Str. 34, 91080 Uttenreuth, Germany
Phone ++49-9131-583-348, Fax ++49-9131-583-11
Quoted 2 lines · Alan Cox
If the queue abstraction is right then the block layer should do all the synchronization work that is required.
I think you're wrong Alan. Take a good IDE chipset as an example: both channels can be active at the same time, but you still can't talk to one drive while the other drive on the same channel is DMAing.
I'm not a block layer expert, but it appears to me that the block layer only synchronises requests by use of the spinlock. If I'm right, then the block layer has no way of knowing that hda is DMAing when a request is initiated for hdb. This was the whole reason (as I see it) that hwgroup->busy existed: to prevent attempts to use the same IDE cable for two things at the same time.
It doesn't matter how you perform the queue abstraction in this case:
the fact that the device+channel+cable is busy in an asynchronous manner makes it impossible for the block layer to deal with this. [[Or am I way off base?!]]
The right way is the way it is being done at present surely: if the busy flag on the hwgroup is set, then ide_do_request() just returns. (NB:
When I say "right way", I don't mean to imply that the code is elegant, desirable, or even bug-free, just that it correctly handles this busy state.)
Quoted 3 lines
It may cost a few cycles on the odd
case you can do overlapped command setup but that versus a nasty locking mess its got to be better to lose those few cycles.
Well, Jens and others are busy implementing TCQ where things are just so much easier to fsck up :-))
Quoted 1 line
I don't even Martin here, the ide locking is currently utterly vile
Agreed, but surely with some concerted effort we can truly fix the IDE code. Can't be beyond us all can it?
Neil
Arbitrate your incoming interrupts. In the classic dual interrupt you are scott free for the most part, erm interrupt sharing will eat you alive. HBA's w/ N channels will eat the BLOCK Layer of lunch and purge it for dinner. If you notice SCSI originally had queues based on the HBA for a reason, one can read all day long form any devices on the chain, but issue a write and everything grinds to a halt.
The original taskfile driver was to permit set and go calls of a multi-level queues. It was also to permit fake local tags with additional load balancing on the channel w/ mixed devices and/or w/ broken hardware force a simplex behavior.
hwif[n].drives[m].queue \ --- hwif[n].queue hwif[n].drives[m+1].queue / hwgroup.queue hwif[n+1].drives[m].queue \ --- hwif[n+1].queue
hwif[n+1].drives[m+1].queue /
In the future:
hwif[0].drives[0].queue <> hwif[0].queue hwif[1].drives[0].queue <> hwif[1].queue hwif[2].drives[0].queue <> hwif[2].queue
...
hwif[n-1].drives[0].queue <> hwif[n-1].queue
hwif[n].drives[0].queue <> hwif[n].queue
Where "n" ranges from 2->20 all on the same hwgroup.queue
Now how is your spinlock going to process all of those in parallel? I can tell you first hand, it can't.
Cheers,
Quoted 38 lines · Neil Conway
Quoted 2 lines · Alan Cox
If the queue abstraction is right then the block layer should do all the synchronization work that is required.
I think you're wrong Alan. Take a good IDE chipset as an example: both channels can be active at the same time, but you still can't talk to one drive while the other drive on the same channel is DMAing.
I'm not a block layer expert, but it appears to me that the block layer only synchronises requests by use of the spinlock. If I'm right, then the block layer has no way of knowing that hda is DMAing when a request is initiated for hdb. This was the whole reason (as I see it) that
hwgroup->busyexisted: to prevent attempts to use the same IDE cable for two things at the same time.It doesn't matter how you perform the queue abstraction in this case:
the fact that the device+channel+cable is busy in an asynchronous manner makes it impossible for the block layer to deal with this. [[Or am I way off base?!]]The right way is the way it is being done at present surely: if the busy flag on the hwgroup is set, then
ide_do_request()just returns. (NB:
When I say "right way", I don't mean to imply that the code is elegant, desirable, or even bug-free, just that it correctly handles this busy state.)Quoted 3 lines
It may cost a few cycles on the odd
case you can do overlapped command setup but that versus a nasty locking mess its got to be better to lose those few cycles.Well, Jens and others are busy implementing TCQ where things are just so much easier to fsck up :-))
Quoted 1 line
I don't even Martin here, the ide locking is currently utterly vile
Agreed, but surely with some concerted effort we can truly fix the IDE code. Can't be beyond us all can it?
Quoted 7 lines
Neil
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Andre Hedrick
LAD Storage Consulting Group
Uz.ytkownik Alan Cox napisa?:
Quoted 20 lines
Quoted 6 lines
Quoted 3 lines
Its possible it can be done with a semaphore but the whole business is pretty tricky. IDE command processing occurs a fair bit at interrupt level and you definitely don't want to block interrupts for long periods.
... Becouse the chances are fscking high - that you will miss command completion interrupts for the "other drive" on the same channel.
The shared IRQ capable IDE ards I am aware of all do have proper tristates
but you still have to handle the edge trigger very carefully.If you can miss a command completion interrupt you have a bug. Since you know each drive on the bus you can poll each afflicted device for interrupts until you reach a point where you completed an entire poll loop and nobody had an IRQ pending.
At that point you know an edge transition has occurred and that a real IRQ will be posted when the next event occurs because that too will cause an edge.
Quoted 6 lines
A good place for examples of this in the DOS world is things like serial drivers, many of which could handle broken shared IRQ ISA setups correctly using this technique.
In the case without tristates the stronger driver tends to win the argument about the line in either direction and nothing works at all.
Well anyway. What we have right now, looking for the channel perspective, is indeed some nIEN tricks done here and there. However the problem is that we postpone the disabling of interface interrupts now until the time a next request gets queued. In addition the driver is doing quite a lot of polling for the next expected interrutp as well.
Right now the consequences are indeed very simple for me. The time I started to sanitize the data structures I first had to paint down diagram of pointers between them. Now it's simple time to write down a state diagram for the IRQ / request handling code paths :-).
Quoted 3 lines
I think you're wrong Alan. Take a good IDE chipset as an example: both channels can be active at the same time, but you still can't talk to one drive while the other drive on the same channel is DMAing.
Sure.
Quoted 6 lines
I'm not a block layer expert, but it appears to me that the block layer only synchronises requests by use of the spinlock. If I'm right, then the block layer has no way of knowing that hda is DMAing when a request is initiated for hdb. This was the whole reason (as I see it) that
hwgroup->busyexisted: to prevent attempts to use the same IDE cable for two things at the same time.
The newer block code has queues. Its up to the block layer to deal with the queue locking.
Quoted 4 lines
It doesn't matter how you perform the queue abstraction in this case:
the fact that the device+channel+cable is busy in an asynchronous manner makes it impossible for the block layer to deal with this. [[Or am I way off base?!]]
I think you are way off base. If you have a single queue for both hda and hdb then requests will get dumped into that in a way that processing that queue implicitly does the ordering you require.
Quoted 1 line
From an abstract hardware point of view each ide controller is a queue not
each device. Not following that is I think the cause of much of the existing pain and suffering.
Alan
Uz.ytkownik Alan Cox napisa?:
Quoted 26 lines
Quoted 3 lines
I think you're wrong Alan. Take a good IDE chipset as an example: both channels can be active at the same time, but you still can't talk to one drive while the other drive on the same channel is DMAing.
Sure.
Quoted 6 lines
I'm not a block layer expert, but it appears to me that the block layer only synchronises requests by use of the spinlock. If I'm right, then the block layer has no way of knowing that hda is DMAing when a request is initiated for hdb. This was the whole reason (as I see it) that
hwgroup->busyexisted: to prevent attempts to use the same IDE cable for two things at the same time.The newer block code has queues. Its up to the block layer to deal with the queue locking.
Quoted 4 lines
It doesn't matter how you perform the queue abstraction in this case:
the fact that the device+channel+cable is busy in an asynchronous manner makes it impossible for the block layer to deal with this. [[Or am I way off base?!]]
Quoted 7 lines
I think you are way off base. If you have a single queue for both hda and hdb then requests will get dumped into that in a way that processing that queue implicitly does the ordering you require.
Quoted 1 line
From an abstract hardware point of view each ide controller is a queue not
each device. Not following that is I think the cause of much of the existing pain and suffering.
Yes thinking about it longer and longer I tend to the same conclusion, that we just shouldn't have per device queue but per channel queues instead. The only problem here is the fact that some device properties
are attached to the queue right now. Like for example sector size and friends.
I didn't have a too deep look in to the generic blk layer. But I would rather expect that since the lower layers are allowed to pass an spin lock up to the queue intialization, sharing a spin lock between two request queues should just serialize them with respect to each other. And this is precisely what 63 does.
BTW> FreeBSD does simple use per channel queues.
Quoted 6 lines · Alan Cox
Quoted 4 lines
I think you're wrong Alan. Take a good IDE chipset as an example: both channels can be active at the same time, but you still can't talk to one drive while the other drive on the same channel is DMAing.
Sure.
Phew, at least we agree on something. In fact I _think_ we were maybe talking at cross-purposes and not disagreeing about anything that actually mattered :-)
Quoted 8 lines
Quoted 4 lines
It doesn't matter how you perform the queue abstraction in this case:
the fact that the device+channel+cable is busy in an asynchronous manner makes it impossible for the block layer to deal with this. [[Or am I way off base?!]]I think you are way off base. If you have a single queue for both hda and hdb then requests will get dumped into that in a way that processing that queue implicitly does the ordering you require.
Hmm, OK. So the queue will potentially fill up with requests for both hda and hdb. The request_fn for this queue will get called, sometimes by the block layer, sometimes not, and can handle all/some/none of the requests at each call. If it decides to handle (say) one of the requests by initiating DMA and then returns while bytes are transferring, then let's think what happens next time it gets called... If it's called as a result of the DMA-complete interrupt, then no worries because the DMAing is finished. If it's called instead by the block-layer because another request has been added, it needs to know that it shouldn't do anything until the DMAing finishes. It could find that out by looking at a channel->busy flag. If it doesn't use a busy flag, then what provides the locking?
I'm slowly growing more sure that I've missed the point. But please point to the mistake in the above logic to help me learn...
Quoted 3 lines
From an abstract hardware point of view each ide controller is a queue not each device. Not following that is I think the cause of much of the existing pain and suffering.
Agreed. And in the case of a cmd640 (etc.), the queue should handle both channels.
cheers
Neil
Quoted 10 lines · Martin Dalecki
Uz.ytkownik Alan Cox napisa?:
Quoted 3 lines
Quoted 1 line
From an abstract hardware point of view each ide controller is a queue not
each device. Not following that is I think the cause of much of the existing pain and suffering.
Yes thinking about it longer and longer I tend to the same conclusion, that we just shouldn't have per device queue but per channel queues instead. The only problem here is the fact that some device properties
are attached to the queue right now. Like for example sector size and friends.
OK..
Quoted 5 lines
I didn't have a too deep look in to the generic blk layer. But I would rather expect that since the lower layers are allowed to pass an spin lock up to the queue intialization, sharing a spin lock between two request queues should just serialize them with respect to each other. And this is precisely what 63 does.
(You're planning to have two queues per channel but sharing the same lock?)
On the serialisation issue: what does serialisation of the queues with respect to each other mean to you? I understand it to mean that we won't ever call the request_fn of both queues at the same time - because that's all the actual spinlock buys you. It does not IIUC mean that you can't get a call to request_fn of one queue while the other queue has lots of requests in it (which are potentially being serviced by DMA). Or does it? Does the block layer track which requests are "active" somehow?
My main question could be posed thus: am I right in thinking that we MUST track the busy-ness of the channel at all times? (and for broken chipsets, track the logical OR of both channels' busy-ness)
Neil
PS: I'm hoping someone will either say "here's why you're wrong" or "you're right" RSN...
Quoted 4 lines
block-layer because another request has been added, it needs to know that it shouldn't do anything until the DMAing finishes. It could find that out by looking at a
channel->busyflag. If it doesn't use a busy flag, then what provides the locking?
You can either use device status information or the queue lock. It might well be using channel->busy or queue->channel->busy type flags. However you've now got a single queue and a single channel lock flowing through a single function - which seems cleaner to me than splitting stuff into multiple queues, locking them against one another and the like
Quoted 6 lines
Quoted 3 lines
From an abstract hardware point of view each ide controller is a queue not each device. Not following that is I think the cause of much of the existing pain and suffering.
Agreed. And in the case of a cmd640 (etc.), the queue should handle both channels.
Yep
Quoted 2 lines · Martin Dalecki
Yes thinking about it longer and longer I tend to the same conclusion, that we just shouldn't have per device queue but per channel queues instead.
Right, I see that as the only right way to get the right synchronization too.
Quoted 3 lines
The only problem here is the fact that some device properties are attached to the queue right now. Like for example sector size and friends.
Hmm yes, hardsect_size comes to mind. It will just have to be 'lowest common denominator' for a while I suppose.
Quoted 5 lines
I didn't have a too deep look in to the generic blk layer. But I would rather expect that since the lower layers are allowed to pass an spin lock up to the queue intialization, sharing a spin lock between two request queues should just serialize them with respect to each other. And this is precisely what 63 does.
I think you are mixing up two very different serialization issues. A shared queue lock will indeed protect however much you want, but only at the queue level. It will _not_ provide synchronization for hardware access in any sane way, like a shared queue between two devices will.
You could alternatively move requests to an internal queue of your own width, that would synchronize drive operations at any level you want (you set the rules). The block layer can still sanely handle the locking for you, the scope of that lock will just be a bit wider.
Signature · 1 line
Jens Axboe
Quoted 6 lines · Neil Conway
On the serialisation issue: what does serialisation of the queues with respect to each other mean to you? I understand it to mean that we won't ever call the
request_fnof both queues at the same time - because that's all the actual spinlock buys you. It does not IIUC mean that you can't get a call torequest_fnof one queue while the other queue has lots of requests in it (which are potentially being serviced by DMA).
Bingo, this is exactly right and makes the point a hell of a lot better than I did in my previous mail. Shared locks will only buy you that noone fiddles with one list while the other is busy (ie nothing for us). To really serialize operations the queue _must_ be shared with whoever requires serialiation.
If not, the problem will have to be solved at the IDE level, not the block level. And that has not looked pretty in the past.
Signature · 1 line
Jens Axboe
Quoted 8 lines · Jens Axboe
Quoted 3 lines · Neil Conway
that's all the actual spinlock buys you. It does not IIUC mean that you can't get a call to
request_fnof one queue while the other queue has lots of requests in it (which are potentially being serviced by DMA).Bingo, this is exactly right and makes the point a hell of a lot better than I did in my previous mail. Shared locks will only buy you that noone fiddles with one list while the other is busy (ie nothing for us).
Cool, thanks ;-) Now watch me blow all my cred with this post:
Quoted 2 lines
To really serialize operations the queue _must_ be shared with whoever requires serialiation.
Why will this help? The hardware can still be doing DMA on hda while the queue's request_fn is called quite legitimately for a hdb request - and the IDE code MUST impose the serialization here to avoid hitting the cable with commands destined for hdb. (For example, by waiting for !channel->busy.)
Quoted 2 lines
If not, the problem will have to be solved at the IDE level, not the block level. And that has not looked pretty in the past.
I just can't see a way for the block level to remove the need for the busy flag. I _think_ Alan just agreed with me. I'm not sure but I get the impression that you are saying the IDE code doesn't need to do this serialization...
I'm certainly learning, thanks guys.
Neil
Quoted 8 lines · Neil Conway
Quoted 2 lines
To really serialize operations the queue _must_ be shared with whoever requires serialiation.
Why will this help? The hardware can still be doing DMA on hda while the queue's
request_fnis called quite legitimately for a hdb request - and the IDE code MUST impose the serialization here to avoid hitting the cable with commands destined for hdb. (For example, by waiting for !channel->busy.)
Current IDE code leaves a request on the list until it has completed (this is ignoring TCQ of course), so there's no way that you could start serving a second request before the first one completes.
Quoted 7 lines
Quoted 2 lines
If not, the problem will have to be solved at the IDE level, not the block level. And that has not looked pretty in the past.
I just can't see a way for the block level to remove the need for the busy flag. I _think_ Alan just agreed with me. I'm not sure but I get the impression that you are saying the IDE code doesn't need to do this serialization...
To be honest, I haven't given it too much thought right now. The nice thing about the queue level serialization is that it all happens automagically for IDE, without having it maintain any busy state on that itself.
However, I may just talking out of my ass and implementation details will mean it's still better to at least manage some of the serialization at the ide level.
Signature · 1 line
Jens Axboe
Quoted 13 lines · Martin Dalecki
Uz.ytkownik Alan Cox napisa?:
Quoted 6 lines
I think you are way off base. If you have a single queue for both hda and hdb then requests will get dumped into that in a way that processing that queue implicitly does the ordering you require.
From an abstract hardware point of view each ide controller is a queue not each device. Not following that is I think the cause of much of the existing pain and suffering.Yes thinking about it longer and longer I tend to the same conclusion, that we just shouldn't have per device queue but per channel queues instead. The only problem here is the fact that some device properties
are attached to the queue right now. Like for example sector size and friends.
Quoted 5 lines
I didn't have a too deep look in to the generic blk layer. But I would rather expect that since the lower layers are allowed to pass an spin lock up to the queue intialization, sharing a spin lock between two request queues should just serialize them with respect to each other. And this is precisely what 63 does.
Hi Martin,
instead of having per channel queue, you could have per device queue, but use the same lock for both, i.e. don't make the lock part of "struct queue" (or whatever it is called) but instead make the address of the lock be attached to "struct queue".
That allows you on "good" controllers to use individual locks for individual drives and also allows you to share the same lock (multiple "struct queue" point to same lock) among _any_ number of devices on same channel.
Further if a controller is truly broken and you need to synchronize multiple channels you could share the lock among those.
I know that means allocating a lock, etc, but heck you can make a slab cache for spinlocks or semaphores or whatever locking primite you use if you consider that important.
I don't know much about the IDE or block layers but it strikes me as the simplest approach to control the level of "lock sharing".
Best regards,
Anton
Signature · 5 lines
"I've not lost my mind. It's backed up on tape somewhere." - Unknown -- Anton Altaparmakov <aia21 at cantab.net> (replace at with @) Linux NTFS Maintainer / IRC: #ntfs on irc.openprojects.net WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/
Quoted 9 lines · Neil Conway
Quoted 2 lines · Jens Axboe
To really serialize operations the queue _must_ be shared with whoever requires serialiation.
Why will this help? The hardware can still be doing DMA on hda while the queue's
request_fnis called quite legitimately for a hdb request - and the IDE code MUST impose the serialization here to avoid hitting the cable with commands destined for hdb. (For example, by waiting for !channel->busy.)
First of all, why do you think that when a new request is queued it hits the hardware immediately? I mean, it is a queue after all...
I'd immagine that you would have one function that is triggered by DMA completion interrupts that reads the queue and starts the hardware for a request.
While that function is operating the hardware, it should't care about the queue, so you can add requests to the queue for both (think hda and hdb) devices on the channel.
channel = ide cable
There is a hardware limitation for IDE in that it doesn't allow more than one device to be active on one channel at a time. So, there is no point in having seperate queues for each device on the channel since the two queues would just be waiting on each other, and would probably end up a pretty big mess.
If a host controller (think ide chipset with multiple channels 2,4,8 etc) needs serialization between multiple channels, then you can simply use one queue for each group of channels that need searialization between each other.
Mike
(OT, now that IDE is getting TCQ, will the main difference between SCSI and IDE be just the number of devices per cable? I'm not talking electrically or about the protocol, but mainly performance from say the block layer's perspective)
Quoted 23 lines · Anton Altaparmakov
Quoted 16 lines · Martin Dalecki
Uz.ytkownik Alan Cox napisa?:
Quoted 7 lines
I think you are way off base. If you have a single queue for both hda and hdb then requests will get dumped into that in a way that processing that queue implicitly does the ordering you require.
From an abstract hardware point of view each ide controller is a queue not each device. Not following that is I think the cause of much of the existing
pain and suffering.Yes thinking about it longer and longer I tend to the same conclusion, that we just shouldn't have per device queue but per channel queues instead.
The only problem here is the fact that some device properties are attached to the queue right now. Like for example sector size and friends.Quoted 5 lines
I didn't have a too deep look in to the generic blk layer. But I would rather expect that since the lower layers are allowed to pass an spin lock up to the queue intialization, sharing a spin lock between two request queues should just serialize them with respect to each other. And this is precisely what 63 does.
Quoted 6 lines
Hi Martin,
instead of having per channel queue, you could have per device queue, but use the same lock for both, i.e. don't make the lock part of "struct queue" (or whatever it is called) but instead make the address of the lock be attached to "struct queue".
See request_queue_t, the lock can already be shared. And in fact the ide layer used a global ide_lock shared between all queues until just recently.
Quoted 2 lines
Further if a controller is truly broken and you need to synchronize multiple channels you could share the lock among those.
Again, this is not enough! The lock will only, at best, serialize direct queue actions. So I can share a lock between queue A and B and only one of them will start a request at any given time, but as soon as request X is started for queue A, then we can happily start request Y for queue B.
This is what the hwgroup busy flag protects right now, only one queue is allowed to mark the hwgroup busy naturally. So only when request X for queue A completes will the hwgroup busy flag be cleared, and queue B can start request Y.
Signature · 1 line
Jens Axboe
Quoted 8 lines · Jens Axboe
Quoted 2 lines · Anton Altaparmakov
instead of having per channel queue, you could have per device queue, but use the same lock for both, i.e. don't make the lock part of "struct
queue"
Quoted 2 lines
(or whatever it is called) but instead make the address of the lock be attached to "struct queue".
See
request_queue_t, the lock can already be shared.
/me looks.
So it can. And I thought I had come up with a clever idea... (-;
Quoted 11 lines
And in fact the ide layer used a global
ide_lockshared between all queues until just recently.Quoted 2 lines
Further if a controller is truly broken and you need to synchronize multiple channels you could share the lock among those.
Again, this is not enough! The lock will only, at best, serialize direct queue actions. So I can share a lock between queue A and B and only one of them will start a request at any given time, but as soon as request X is started for queue A, then we can happily start request Y for queue B.
Quoted 4 lines
This is what the hwgroup busy flag protects right now, only one queue is allowed to mark the hwgroup busy naturally. So only when request X for queue A completes will the hwgroup busy flag be cleared, and queue B can start request Y.
Yes, I understand that, could you not extend the shared lock idea to a shared flags idea? The two could even be in the same memory allocated block as both the lock and the flags would always be shared by the same users. That would just now contain only QUEUE_SHARED_FLAG_BUSY but could be extended later if the need arises.
From what I gather from the posts on this topic, this would be entirely sufficient to fully lock both request queue handling and request submission to the hardware while maintaining per-device queues.
I may be way off base but I would think that per-device queues are desirable to improve the request merging abilities of the elevator. (Again, code I haven't looked at so it may well be intelligent enough to resort/merge requests with different destinations on the same queue but I am sure you will tell me if this is the case. (-;)
Best regards,
Anton
Signature · 5 lines
"I've not lost my mind. It's backed up on tape somewhere." - Unknown -- Anton Altaparmakov <aia21 at cantab.net> (replace at with @) Linux NTFS Maintainer / IRC: #ntfs on irc.openprojects.net WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/
Uz.ytkownik Anton Altaparmakov napisa?:
Quoted 24 lines
Quoted 15 lines · Martin Dalecki
Uz.ytkownik Alan Cox napisa?:
Quoted 6 lines
I think you are way off base. If you have a single queue for both hda and hdb then requests will get dumped into that in a way that processing that queue implicitly does the ordering you require.
From an abstract hardware point of view each ide controller is a queue not each device. Not following that is I think the cause of much of the existing pain and suffering.Yes thinking about it longer and longer I tend to the same conclusion, that we just shouldn't have per device queue but per channel queues instead. The only problem here is the fact that some device properties
are attached to the queue right now. Like for example sector size and friends.Quoted 5 lines
I didn't have a too deep look in to the generic blk layer. But I would rather expect that since the lower layers are allowed to pass an spin lock up to the queue intialization, sharing a spin lock between two request queues should just serialize them with respect to each other. And this is precisely what 63 does.
Quoted 3 lines
Hi Martin,
instead of having per channel queue, you could have per device queue, but use the same lock for both, i.e. don't make the lock part of "struct queue" (or whatever it is called) but instead make the address of the lock be attached to "struct queue".
In 63 we have:
blk_init_queue(q, do_ide_request, drive->channel->lock);
struct ata_channel {
struct device dev; /* device handle */
int unit; /* channel number */
/* This lock is used to serialize requests on the same device queue or
* between differen queues sharing the same irq line.
*/
spinlock_t *lock;+ The whole glory of lock sharing for IRQ sharing and serialization.
Pretty much what you describe.
Quoted 5 lines
That allows you on "good" controllers to use individual locks for individual drives and also allows you to share the same lock (multiple "struct queue" point to same lock) among _any_ number of devices on same channel.
Further if a controller is truly broken and you need to synchronize multiple channels you could share the lock among those.
I know that means allocating a lock, etc, but heck you can make a slab cache for spinlocks or semaphores or whatever locking primite you use if you consider that important.
Not worth the trouble for the 2 or 4 allocations at initialization.
Quoted 1 line
I don't know much about the IDE or block layers but it strikes me as the simplest approach to control the level of "lock sharing".
The only problem is that having a shared lock between two queues apparently doesn't imply that the queues are behaving atomic on the request level among each others. The lock is just only making sure that access to the lists and stragety routines is atomic between them.
However 63 doesn't really change the previous behaviour of the driver, since we are simply using the lock pointer instead of the custom hwgroup structure as our "serialization token" between channels and the busy flag is preserved, since it's only used to protect reentrancy bfore request completion (the long while(test_bit loop). It doesn't have to be shared between channels, since in every case we hve to look up the channel anyway.
Apparenty the "sublimation" of the hwgroup and overall cleanup of data structures, just made many people awake and be aware of problems which where there already for a very very long time...
Uz.ytkownik Anton Altaparmakov napisa?:
Quoted 34 lines
Quoted 9 lines · Jens Axboe
Quoted 1 line · Anton Altaparmakov
instead of having per channel queue, you could have per device
queue, but
Quoted 1 line
use the same lock for both, i.e. don't make the lock part of "struct
queue"
Quoted 2 lines
(or whatever it is called) but instead make the address of the lock be attached to "struct queue".
See
request_queue_t, the lock can already be shared./me looks.
So it can. And I thought I had come up with a clever idea... (-;
Quoted 11 lines
And in fact the ide layer used a global
ide_lockshared between all queues until just recently.Quoted 2 lines
Further if a controller is truly broken and you need to synchronize multiple channels you could share the lock among those.
Again, this is not enough! The lock will only, at best, serialize direct queue actions. So I can share a lock between queue A and B and only one of them will start a request at any given time, but as soon as request X is started for queue A, then we can happily start request Y for queue B.
Quoted 4 lines
This is what the hwgroup busy flag protects right now, only one queue is allowed to mark the hwgroup busy naturally. So only when request X for queue A completes will the hwgroup busy flag be cleared, and queue B can start request Y.
Quoted 4 lines
Yes, I understand that, could you not extend the shared lock idea to a shared flags idea? The two could even be in the same memory allocated block as both the lock and the flags would always be shared by the same users. That would just now contain only
QUEUE_SHARED_FLAG_BUSYbut could be extended later if the need arises.From what I gather from the posts on this topic, this would be entirely
sufficient to fully lock both request queue handling and request submission to the hardware while maintaining per-device queues.
The clean solution whould be to make it possible to be able to use multiple hardsect and friend on a single queue. Becouse then one could just use the same queue for all operations.
But... wait a minute. The only really problematic case where the queue properties differ is the case where we have a disk and ATAPI device mixed on the same channel. Hmm what about using two distinguished queues on a channel - one for ATAPI and the second for ATA requests then? In esp. since it's quite easy to identify ATAPI request as beeing in flight. Hmm... the longer I think about it the more apeal this solution has to me.
Quoted 7 lines
I may be way off base but I would think that per-device queues are desirable to improve the request merging abilities of the elevator. (Again, code I haven't looked at so it may well be intelligent enough to resort/merge requests with different destinations on the same queue but I am sure you will tell me if this is the case. (-;)
Best regards,
Anton
Signature · 3 lines
- phone: +49 214 8656 283 - job: eVision-Ventures AG, LEV .de (MY OPINIONS ARE MY OWN!) - langs: de_DE.ISO8859-1, en_US, pl_PL.ISO8859-2, last ressort: ru_RU.KOI8-R
Quoted 9 lines · Jens Axboe
Quoted 7 lines · Neil Conway
Quoted 2 lines
To really serialize operations the queue _must_ be shared with whoever requires serialiation.
Why will this help? The hardware can still be doing DMA on hda while the queue's
request_fnis called quite legitimately for a hdb request - and the IDE code MUST impose the serialization here to avoid hitting the cable with commands destined for hdb. (For example, by waiting for !channel->busy.)
Quoted 3 lines
Current IDE code leaves a request on the list until it has completed (this is ignoring TCQ of course), so there's no way that you could start serving a second request before the first one completes.
I didn't understand why this was the case, so I've been reading source code (and sleeping for a few minutes too ;-)). Wow, is it hard for a newbie to follow - if I didn't know better I'd swear the kernel was obfuscated.
I _think_ I now understand what's going on here: you guys already know this stuff but perhaps others can learn from my slog through the code (?) (and/or spot the dumb mistakes). (I did notice what could be a potential problem too, skip to the end if you want to find it.)
On a "totally" idle system, if a process decides to read from a file, the sequence appears to be (with minor simplifications for clarity):
sys_read(), file->fops->read(), usually then into generic_file_read(), page_cache_readahead(), and then the important one:do_page_cache_readahead(). This does a couple of seriously important things. (The name implies it's only used for readahead but the comments show this isn't the case (if one reads them!)).
Firstly, do_page_cache_readahead() invokes the call chain:a_ops->readpage(), block_read_full_page(), submit_bh(), submit_bio(), generic_make_request() (this one finds the right queue for the I/O) and down into __make_request_fcn(q,bio). This routine is also key: it adds requests to the device queue, and a little more: if the queue is empty it "plugs" the device (before adding the request), ("plugging" the device refers to preventing I/O while a request queue is populated), and also queues a task to the tq_disk task-queue which will unplug the device at some later time. The "unplug" routine is central: see below.
Control returns to do_page_cache_readahead() after the call to readpage(). It then calls run_task_queue(&tq_disk), thus starting the call chain: generic_unplug_device(),..., q->request_fn(). This is the end of the line for the block-layer: request_fn() is part of the device code - for IDE it's do_ide_request().
So this call to request_fn()/do_ide_request() is the first time the IDE code is really involved in the loop.
The key bit to notice here is that request_fn() is only called if the device was plugged. This in turn only happens (well almost, see below) if the queue was empty. Thus one concludes that if do_ide_request() is busy servicing requests and thus the queue is non-empty, the block layer will never again call do_ide_request(); it will be allowed to get on with things in its own time.
This I believe, is what you meant Jens when you said "so there's no way that you could start serving a second request before the first one completes". Am I right so far?
Now a possible fly in the ointment: __make_request_fcn() will actually plug a non-empty queue if BIO_RW_BARRIER is set. This appears right now to be impossible because I can't find any code in the entire kernel that uses bio_barrier() or any similar construct. But if it's a work in progress and this bit is set one day, then it seems to me that the block layer could plug a non-empty queue, and then subsequently any call to run_task_queue(&tq_disk) would call the block-device's request_fn(). This would violate the assumption that request_fn() is never called twice without the queue emptying in between. The current IDE code would survive this because it checks for hwgroup->busy.
Anyway, IFF the BARRIER stuff is not an issue, then I guess the block-layer really can do all the serialization we need if we set things up right. I now probably have to retract my assertions that we vitally need hwgroup->busy (or equiv) because there really does appear to be no route into request_fn() from the block layer other than if the queue was empty... In the real world though, as you suggested, it's probably worth having.
all the best,
Neil
PS: Errors and Omissions Expected.
Quoted 3 lines · Martin Dalecki
Quoted 3 lines
Quoted 3 lines
Yes thinking about it longer and longer I tend to the same conclusion, that we just shouldn't have per device queue but per channel queues instead.
IMHO logically request queue is a separate entity for each disk. IDE can have 2 devices on one cable (or more: think about buggy cmd640 like chipset as a weird IDE with four disks on a channel since we can talk to one disk only at a time).
This fact is a hardware limitation which can be hidden, IDE code should arbitrare access to IDE channel. But why mix up queues?
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
The only problem here is the fact that some device properties are attached to the queue right now. Like for example sector size and friends.
This is naturally happening with one queue per disk.
Quoted 17 lines
Quoted 5 lines
Hi Martin,
instead of having per channel queue, you could have per device queue, but use the same lock for both, i.e. don't make the lock part of "struct queue" (or whatever it is called) but instead make the address of the lock be attached to "struct queue".In 63 we have:
blk_init_queue(q, do_ide_request, drive->channel->lock); struct ata_channel { struct device dev; /* device handle */ int unit; /* channel number */ /* This lock is used to serialize requests on the same device queue or * between differen queues sharing the same irq line. */ spinlock_t *lock;
Quoted 1 line
+ The whole glory of lock sharing for IRQ sharing and serialization.
It is a _spin_ lock.
Does this mean you will spin on it while IDE request for other disk is processed?
Somebody enlighten me: can IDE mix reqests and completion like this:
host ----read reqest---> master
host ----read reqest---> slave (is this possible?)
host <---interrupt------ master
host ----okay,send it--> master
host <---data----------- master
host <---data----------- master (do slave has to wait until end here?) host <---data----------- master
host <---that's all----- master
host <---interrupt------ slave
(can host issue another read for master here?)
host ----okay,send it--> slave
host <---data----------- slave
Please comment what is allowed and what is not.
I suspect we are in IDE chipset bugs land here.
Does current IDE code (as Martin sees it) utilize this fully but does not push IDE over limit?
Hm, seems like IDE code needs to remember set of ATA capabilities (i.e. what should be serialized and what can be safely intermixed in time) for each controller in order to run full speed on good chips yet don't croak on buggy ones.
--
vda
Quoted 3 lines · Martin Dalecki
The only problem is that having a shared lock between two queues apparently doesn't imply that the queues are behaving atomic on the request level among each others.
Correct - both queues can be active with I/O in flight at the same time. But think about it: if this weren't the case, then the older kernels (using global io_request_lock) would have had to serialize ALL I/O, one request-queue active at a time, for every single block-device...
Quoted 3 lines
Apparenty the "sublimation" of the hwgroup and overall cleanup of data structures, just made many people awake and be aware of problems which where there already for a very very long time...
I'm not quite sure which problems you mean. The busy flag prevents any clash. (But sure, if you change to per-device queues AND you ditch the busy flag you're screwed.) Where is the problem?
cheers
Neil
Quoted 10 lines · Denis Vlasenko
Quoted 3 lines · Martin Dalecki
Quoted 3 lines
Quoted 3 lines
Yes thinking about it longer and longer I tend to the same conclusion, that we just shouldn't have per device queue but per channel queues instead.
IMHO logically request queue is a separate entity for each disk. IDE can have 2 devices on one cable (or more: think about buggy cmd640 like chipset as a weird IDE with four disks on a channel since we can talk to one disk only at a time).
Actually, since Martin pointed out the hardsect stuff, I'm inclined to think per-device queues but with a per-channel busy flag are the best idea. Or perhaps I should call it a per-channel "Don't touch the cable" flag. See below.
Quoted 3 lines
It is a _spin_ lock.
Does this mean you will spin on it while IDE request for other disk is processed?
No. An SMP machine will spin on it only while the queue-handling code is active. During the pause while the disk fetches the sectors from the media, no problem.
Quoted 1 line
Somebody enlighten me: can IDE mix reqests and completion like this:
I'll try (!) Treat my words with caution ;-)
Quoted 2 lines
host ----read reqest---> master
host ----read reqest---> slave (is this possible?)
No, in general. Not unless you're using tagged command queueing. This is because the master won't have performed a bus-release on any normal R/W command. Once you are using TCQ it's OK though (tricky but OK).
You can (and must) safely "touch the cable" in between TCQ commands in the right circumstances. You are therefore touching the cable while the hwgroup is busy, hence my suggestion that the flag we use to prevent touching the cable during DMA should be named something other than busy.
Neil
Uz.ytkownik Neil Conway napisa?:
Quoted 14 lines
Quoted 3 lines · Martin Dalecki
The only problem is that having a shared lock between two queues apparently doesn't imply that the queues are behaving atomic on the request level among each others.
Correct - both queues can be active with I/O in flight at the same time. But think about it: if this weren't the case, then the older kernels (using global
io_request_lock) would have had to serialize ALL I/O, one request-queue active at a time, for every single block-device...
Quoted 8 lines
Quoted 3 lines
Apparenty the "sublimation" of the hwgroup and overall cleanup of data structures, just made many people awake and be aware of problems which where there already for a very very long time...
I'm not quite sure which problems you mean. The busy flag prevents any clash. (But sure, if you change to per-device queues AND you ditch the busy flag you're screwed.) Where is the problem?
The problem is that with the busy flag on we are wasting quite a significant amount of CPU time spinning around it for no good...
Right now I'm quite confident about the idea of just having two queues ata_queue and atapi_queue on the channel possible shared among two channels. This will make the CPU cycle waste occur only in the case of channels where ATA and ATAPI devices are mixed as master and slave. This will work becouse the only really crucial queue property which has to be device type specific is the hardsect size.
Quite the same design as in, well, FreeBSD.
As a second step we could do just the following - block one of the queues if the second one is active right now... This should be simpler than doing it on the per device level by the busy flag and wasting CPU time around it.
Quoted 2 lines
The problem is that with the busy flag on we are wasting quite a significant amount of CPU time spinning around it for no good...
Why spin on the busy flag. Instead you just let the person who clears the flag check the pending work and do it.
Quoted 5 lines
Quoted 2 lines
The problem is that with the busy flag on we are wasting quite a significant amount of CPU time spinning around it for no good...
Why spin on the busy flag. Instead you just let the person who clears the flag check the pending work and do it.
Which is what happened in most cases, pending work could be resumed by calling ide_do_request() (in the previous codebase).
I used/needed this feature when implementing machine sleep support on PowerMac laptops. I basically got the lock, set busy flag on all interfaces then release the lock (well, I did the proper wait for interface not to be busy etc... but you get the point).
That way, I am sure that newly incoming requests would be queued and not sent to HW while the controller is going to sleep (and then the entire machine).
On wakeup, I do the opposite. After reviving the controller and the disk, I clear the busy flag and call ide_do_request() to get things back.
I need to be able to do something similar with the new codebase, though I beleive that should be part of the generic code for power management when the controller gets a suspend request. In theory, it should issue a device-specific suspend command (that is SLEEP for IDE disks, etc...) and make sure the busy flag doesn't get cleared upon termination on this command (thus blocking queues) until the machine gets woken up.
Ben.
Quoted 6 lines · Alan Cox
Quoted 3 lines
The problem is that with the busy flag on we are wasting quite a significant amount of CPU time spinning around it for no good...
Why spin on the busy flag. Instead you just let the person who clears the flag check the pending work and do it.
Isn't that what happens? Since when are we spinning on busy? Certainly not in vanilla 2.5.14, unless I'm much mistaken.
Martin - I haven't read your last couple of patches yet but did you really change it this drastically?
Neil
Uz.ytkownik Neil Conway napisa?:
Quoted 9 lines
Quoted 5 lines · Alan Cox
Quoted 2 lines
The problem is that with the busy flag on we are wasting quite a significant amount of CPU time spinning around it for no good...
Why spin on the busy flag. Instead you just let the person who clears the flag check the pending work and do it.
Quoted 5 lines
Isn't that what happens? Since when are we spinning on busy? Certainly not in vanilla 2.5.14, unless I'm much mistaken.
Martin - I haven't read your last couple of patches yet but did you really change it this drastically?
Yeep. Not quite that drastically but basically it's the upper layer going down the driver and up again.
Anyway. This all will be gone soon, since I have just already made the distinction between ata_queue and atapi_queue... and well things turn out nicely at least in the lieu of code. The synchronization between them can be done by deploying precisely the same blk_queue_plug and blk_queue_unplug games already played in tcq.c. And it's very likely that we will end up unwinding the REQ_DRIVE_CMD and REQ_DRIVE_ACB cases by just providing two different request handlers... And thus finally unwinding ATA ver. ATAPI handling entierly, which should indeed simplyfy things quite a lot...
Hi!
Quoted 4 lines
interrupts bite us here. Someone just please shoot this enginer who saved the few pullup resistors in the head or send him alternatively for "hunting white bears" in Siberia... about 15 years would be fine in my opinnion.
Actually, at least unknown card wanting attetion does not kill whole system like in level-triggered case. You can use timer to recover lost interrupts, or you just should not loose them in the first place.
Pavel
Signature · 2 lines
Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt, details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.
Quoted 5 lines · Neil Conway
You can (and must) safely "touch the cable" in between TCQ commands in the right circumstances. You are therefore touching the cable while the hwgroup is busy, hence my suggestion that the flag we use to prevent touching the cable during DMA should be named something other than busy.
Ahh, but with TCQ the concept of busy changes. The wire (simplified) is only busy when the tags are being transfered, otherwise the cable is unused unless the cable has been "locked" by one of the devices.
Quoted 8 lines · Mike Fedyk
Quoted 4 lines · Neil Conway
You can (and must) safely "touch the cable" in between TCQ commands in the right circumstances. You are therefore touching the cable while the hwgroup is busy, hence my suggestion that the flag we use to prevent touching the cable during DMA should be named something other than busy.
Ahh, but with TCQ the concept of busy changes. The wire (simplified) is only busy when the tags are being transfered, otherwise the cable is unused unless the cable has been "locked" by one of the devices.
Hmm: "locked by one of the devices": do you mean a DMA transfer for example? These are initiated by the host, but proceed asynchronously, so I'm not sure I'd describe it as being locked "by the device" as such. At any rate, the IDE code has to remember that the cable is asynchronously active until DMA ends... (Or I suppose it could just check the hwif BMDMA bits for the active state.)
I don't think you're actually disagreeing with me here btw; if you are then I've obviously missed your point ;-))
Neil
Uz.ytkownik Neil Conway napisa?:
Quoted 14 lines
Quoted 10 lines · Mike Fedyk
Quoted 4 lines · Neil Conway
You can (and must) safely "touch the cable" in between TCQ commands in the right circumstances. You are therefore touching the cable while the hwgroup is busy, hence my suggestion that the flag we use to prevent touching the cable during DMA should be named something other than busy.
Ahh, but with TCQ the concept of busy changes. The wire (simplified) is only busy when the tags are being transfered, otherwise the cable is unused unless the cable has been "locked" by one of the devices.
Quoted 6 lines
Hmm: "locked by one of the devices": do you mean a DMA transfer for example? These are initiated by the host, but proceed asynchronously, so I'm not sure I'd describe it as being locked "by the device" as such. At any rate, the IDE code has to remember that the cable is asynchronously active until DMA ends... (Or I suppose it could just check the hwif BMDMA bits for the active state.)
Grep for IDE_DMA busy bits to see it - it does precisely this.