Correct use of ap->lock versus ap->host->lock ?

From a libata core point of view, what does ap->host->lock protect?

Jeff / Tejun / Alan,

I'm trying to sort out the spinlocks in sata_mv.

In some places, the existing code uses ap->lock.
But in others, notably the interrupt handling, it uses ap->host->lock.

This looks buggy to me, and I'm wondering how to make it bulletproof.

Quoted 8 lines · Mark Lord

Jeff / Tejun / Alan,

I'm trying to sort out the spinlocks in sata_mv.

In some places, the existing code uses ap->lock.
But in others, notably the interrupt handling, it uses ap->host->lock.

This looks buggy to me, and I'm wondering how to make it bulletproof.

Look closely, there is only one lock. ata_port does not have a spinlock, just a pointer...

Quoted 8 lines · Jeff Garzik

Quoted 7 lines · Mark Lord

Jeff / Tejun / Alan,

I'm trying to sort out the spinlocks in sata_mv.

In some places, the existing code uses ap->lock.
But in others, notably the interrupt handling, it uses ap->host->lock.

Quoted 3 lines

Quoted 1 line

This looks buggy to me, and I'm wondering how to make it bulletproof.

Look closely, there is only one lock. ata_port does not have a spinlock, just a pointer...

..

Ahh.. in ata_port_alloc(). Thanks.

Quoted 14 lines · Mark Lord

Quoted 11 lines · Jeff Garzik

Quoted 8 lines · Mark Lord

Jeff / Tejun / Alan,

I'm trying to sort out the spinlocks in sata_mv.

In some places, the existing code uses ap->lock.
But in others, notably the interrupt handling, it uses ap->host->lock.

This looks buggy to me, and I'm wondering how to make it bulletproof.

Look closely, there is only one lock. ata_port does not have a spinlock, just a pointer...

..

Quoted 1 line

Ahh.. in ata_port_alloc(). Thanks.

..

Okay. Does the LLD even need to bother with this lock in the various pre/soft/hard reset routines ?

I don't think so, but sata_mv currently tries to lock there.

You need to perform your own LLD-specific locking analysis to see if it is safe to do, e.g. __mv_stop_dma() rather than mv_stop_dma().

Quoted 2 lines · Jeff Garzik

You need to perform your own LLD-specific locking analysis to see if it is safe to do, e.g. __mv_stop_dma() rather than mv_stop_dma().

And speaking as the probable author of some of the code that takes a
lock during EH in sata_mv.... reinforcing that the above quoted
statement is true.

Generally I would put in a lock during EH around register or data manipulations that were locked elsewhere in the driver, thus guaranteeing such code is safe.

However, it may also be the case that such a lock during EH is unnecessary because command submission machinery is disabled. I chose a path that potentially added more locking, but was much easier to verify correct.

Quoted 11 lines · Jeff Garzik

Quoted 8 lines · Mark Lord

Jeff / Tejun / Alan,

I'm trying to sort out the spinlocks in sata_mv.

In some places, the existing code uses ap->lock.
But in others, notably the interrupt handling, it uses ap->host->lock.

This looks buggy to me, and I'm wondering how to make it bulletproof.

Look closely, there is only one lock. ata_port does not have a spinlock, just a pointer...

..

Ahh.. in ata_port_alloc(). Thanks.

Mmmm... so this reduces potential parallelism in libata, meaning we could probably achieve better SMP performance if the ap->locks were unique for each port.

But at the expense of very tricky and difficult coding around shared host resources.

Not worth it today for spinning media, but this could be a big limitation for solid-state media in the near future.

Quoted 23 lines · Mark Lord

Quoted 11 lines · Jeff Garzik

Quoted 8 lines · Mark Lord

Jeff / Tejun / Alan,

I'm trying to sort out the spinlocks in sata_mv.

In some places, the existing code uses ap->lock.
But in others, notably the interrupt handling, it uses ap->host->lock.

This looks buggy to me, and I'm wondering how to make it bulletproof.

Look closely, there is only one lock. ata_port does not have a spinlock, just a pointer...

..

Ahh.. in ata_port_alloc(). Thanks.

Mmmm... so this reduces potential parallelism in libata, meaning we could probably achieve better SMP performance if the ap->locks were unique for each port.

But at the expense of very tricky and difficult coding around shared host resources.

Quoted 2 lines

Not worth it today for spinning media, but this could be a big limitation for solid-state media in the near future.

Its questionable whether it is worth it even for RAM-based ATA devices like gigabyte i-Ram.

The only thing being locked is software state involved in submission and completion (either host-wide or port-wide) and a couple register writes, which is a very tiny piece of the whole puzzle.

You have a long, long list of bottlenecks before you ever get there...

There are definitely other fish to fry elsewhere,
but don't discount the effect of "a couple register writes", which are frequently done with readbacks to flush them, at a cost equivalent to several thousand CPU cycles per readback.

This prevents new command issue from overlapping interrupt handling for any ports of the same host. Again, not a biggie today, but tomorrow perhaps..

And still probably not worth the fuss on any hardware that has registers shared across multiple ports (eg. Marvell controllers).

Quoted 4 lines · Mark Lord

There are definitely other fish to fry elsewhere,
but don't discount the effect of "a couple register writes", which are frequently done with readbacks to flush them, at a cost equivalent to several thousand CPU cycles per readback.

Those numbers are for slower PIO, not MMIO as found on new SATA controllers...

Quoted 6 lines

This prevents new command issue from overlapping interrupt handling for any ports of the same host. Again, not a biggie today, but tomorrow perhaps..

And still probably not worth the fuss on any hardware that has registers shared across multiple ports (eg. Marvell controllers).

Remember, I come from the land of networking, where we already see over 500k packets per second. None of this is new stuff.

In networking you lock both TX submission (analogy: scsi queuecommand) and TX completion (analogy: completion via irq handler), and we don't see any such problems on multi-port controllers.

It is not worth the fuss on new SATA controllers, which look just like NIC hardware has looked for a decade -- DMA rings, with a single MMIO write (or write+read) to indicate software has new packets for hardware.
Even at exponentially higher FIS rates, locking doesn't become an issue.

The big difference here, is that a single SATA controller in a system can have up to eight quasi-independent ports. So when we lock, we block activity on all 8 interfaces, rather than just the one we care about.

Quoted 3 lines · Mark Lord

The big difference here, is that a single SATA controller in a system can have up to eight quasi-independent ports. So when we lock, we block activity on all 8 interfaces, rather than just the one we care about.

...hence the mention of multi-port NICs.