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

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

[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.

The interrupt handler for each port should really be using ap->lock, right?
But accesses to the host-level (shared among ports) interrupt registers
probably requires ap->host->lock.  Right again?

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

???

Thanks

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

[Jeff Garzik]

Mark Lord wrote:
> 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...

	Jeff

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

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>> 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.

Something to ponder, I guess.

Cheers

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

[Mark Lord]

Mark Lord wrote:
> Jeff Garzik wrote:
>> Mark Lord wrote:
>>> 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.
..

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.

???

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

[Jeff Garzik]

Mark Lord wrote:
> Mark Lord wrote:
>> Jeff Garzik wrote:
>>> Mark Lord wrote:
>>>> 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.
> ..
> 
> 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.

It depends on what needs to be locked, obviously...

(and no, that is not a sarcastic answer)

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().

All the msleep() calls in the reset routines should tell you that 
sleeping is OK, which means locking during EH is mostly accordingly to 
the needs of your driver.

EH makes sure various command submission machinery is shut down, thus 
creating the _possibility_ for lockless work, but your driver may need 
additional guarantees (and thus additional locking).

	Jeff

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

[Jeff Garzik]

Jeff Garzik wrote:
> 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.

	Jeff

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

[Tejun Heo]

Jeff Garzik wrote:
> Jeff Garzik wrote:
>> 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.

I think it's generally safe not to grab any locks during resets as all
the submission machinery && the interrupt handler are shut down, so it
should be safe to go lockless.  There may be exceptions tho.

-- 
tejun

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

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
>> Mark Lord wrote:
>>> 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.

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...

	Jeff

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

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>> Jeff Garzik wrote:
>>> Mark Lord wrote:
>>>> 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.
> 
> 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).

Cheers

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

[Jeff Garzik]

Mark Lord wrote:
> 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...


> 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.

And its not worth the fuss for older controllers, because they're, well, 
old and SMP locking performance is not a pressing issue there.  :)


Off the top of my head, here are two issues much more pressing than 
host-versus-port libata locking:


* batch submission.  The block and scsi layers pass commands to us 
one-at-a-time, which is awful for any modern hardware.  For each command 
in the request queue:  lock+queuecommand+unlock.  It is better for both 
locking and hardware (DMA ring) submission to add a batch of commands to 
the queue, and then kick the hardware.


* figuring out an interrupt mitigation scheme that is useful in the real 
world.  Right now, even a 16-port SATA card will not give you a lot of 
interrupts.  At some point, it does become useful to turn on interrupt 
mitigation.

That point is dynamic, and must be measured as such:  it depends on 
overall system load (not just load on a single SATA chip).  Networking's 
NAPI takes this into account, though over time we've seen the best 
results by combining hardware interrupt mitigation with software 
interrupt mitigation.


Regards,

	Jeff

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

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>> 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...
> 
> 
>> 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.

At LSF'08 there was much discussion about the coming 100000 ops/second SSDs,
which exist in the marketplace already.  At some point, this stuff will
matter as much for SATA as it did/does for networking.

Cheers

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

[Jeff Garzik]

Mark Lord wrote:
> 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.

	Jeff

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

[Andi Kleen]

Jeff Garzik <jgarzik@pobox.com> writes:

> Mark Lord wrote:
> > 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.

Multi port nics typically have one spinlock per port though, don't they?

-Andi