Re: ATA Patch #6
I'm glad you're back :)
On Sat, Nov 27, 2004 at 12:45:40PM -0800, Matthew Dillon wrote:
> :2) adding DELAY() in ata_command() unconditionally was critical on performance.
>
> I can believe that... but by how much? I don't think that delay is
> optional or, at least, the interrupt code may have to add the delay
> itself. For the moment I would prefer to be conservative.
>
> There are two unconditional delays. One at the beginning of
> ata_command() (in the 'The 10uS delay here could probably be reduced
> to 1 uS' section), and one at the end (in the 'Start the command
> rolling...' section).
>
> The performance difference shouldn't be terrible, though, what are
> you seeing?
I'm only talking about the report from dd command when I pressed ctrl+T,
but I believe reducing the rate from 5Mbytes/sec to 3.5Mbytes/sec has some
impact on the real-world performance.
On my laptop(DynaBook SS3500), If I removed or reduced only the first
DELAY(10), the read rate recovers while the write rate stays reduced. If
I reduced both, the write rate recovers too.
> :3) you removed the conditional from the following code in ata_command()
> : and made it always control interrupt from the device:
> : /* disable interrupt from device */
> : if (atadev->channel->flags & ATA_QUEUED)
> : ATA_OUTB(atadev->channel->r_altio, ATA_ALTSTAT, ATA_A_IDS | ATA_A_4BIT);
> :
> : but it led to timeout or lock-up on two of my DragonFly machines.
> : if I put the conditional back in, the timeout won't happen.
>
> This is kinda central to the changes. I added the interrupt interlock
> but in order to prevent an interrupt from freezing the system due to
> livelock (because the interrupt just returns if the interlock is set and
> doesn't try to clear anything) that means the interrupt bit must be
> unconditionally disabled until the command has been completely set up.
>
> If there is an issue here with the interrupt disablement I think
> it may be the key to solving the lockup issue. The ATA spec is
> fairly clear on how ATA_A_IDS is supposed to work so I thought it was
> safe to manipulate.
I hope so(I'm talking without knowing the ATA spec, do you have a reference
to a documentation I can read?), but unfortunately, writing to that port
seems to have some side-effect so you can't turn the bit on and off at will,
at least on my DragonFly boxes. I even tried dropping ATA_A_4BIT, but it
still timed out or locked up(even with DELAY()'s you added unchanged).
To avoid lock-ups, I needed to backout all `ATA_A_IDS | ATA_A_4BIT' lines,
and remove ATA_OUTB() in ata_clear_interlock() (it may not work correctly,
but it's too scary to bring it out of single-user mode).
BTW I noticed that you changed
DELAY(50000);
ATA_OUTB(ch->r_altio, ATA_ALTSTAT, ATA_A_4BIT);
to
DELAY(50000);
ATA_OUTB(ch->r_altio, ATA_ALTSTAT, ATA_A_IDS | ATA_A_4BIT);
where is ATA_A_IDS supposed to be cleared?
> :4) in ata_command(), calls to crit_enter() and crit_exit() don't correspond.
> :
>
> Oops. Yes, I see them. Here's a new patch (we'll call it #7) with
> the critical section handling fixed. Though I think the mismatch
> only occurs if an error/warning is also reported to the console.
I also noticed while trying #7, a call to ata_clear_interlock() is missing
after a few calls to ata_command() with ATA_WAIT_READY(which is conflicting
to the comment in ata_command() which says caller should clear the interlock).
討論串 (同標題文章)
完整討論串 (本文為第 14 之 16 篇)：