* [PATCH 03/11] scsi: Add IRQ_DISABLE_SCSI_QCMD wrapper
@ 2010-11-17 22:18 Nicholas A. Bellinger
2010-11-17 22:27 ` Christoph Hellwig
0 siblings, 1 reply; 9+ messages in thread
From: Nicholas A. Bellinger @ 2010-11-17 22:18 UTC (permalink / raw)
To: linux-scsi, Jeff Garzik, James Bottomley, Christoph Hellwig
Cc: Mike Christie, Ravi Anand, Andrew Vasquez, Joe Eykholt,
James Smart, Vasu Dev, Tim Chen, Andi Kleen, Tejun Heo,
Mike Anderson, MPTFusionLinux, Nicholas Bellinger
From: Nicholas Bellinger <nab@linux-iscsi.org>
This patch adds a IRQ_DISABLE_SCSI_QCMD() wrapper macro used by LLDs
that can now run in host_lock less mode, but still need interrupts disabled
using local_irq_save() before calling their lld_queuecommand() dispatcher.
jgarzik says this method is in fact slower than doing a spin_lock_irqsave() on
internal lib_lld_queuecommand() callers (as is done in libiscsi and libata)
but is still needed by the majority of lock_less LLDs.
Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
include/scsi/scsi_host.h | 14 ++++++++++++++
1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index e7e3858..321b114 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -521,6 +521,20 @@ struct scsi_host_template {
return rc; \
}
+/*
+ * Used for LLDs running in lock-less mode, but that still require having
+ * interrupts disable during their lld_queuecommand() dispatch.
+ */
+#define IRQ_DISABLE_SCSI_QCMD(func_name) \
+ int func_name(struct Scsi_Host *shost, struct scsi_cmnd *cmd) \
+ { \
+ unsigned long irq_flags; \
+ int rc; \
+ local_irq_save(irq_flags); \
+ rc = func_name##_irq_disable(cmd, cmd->scsi_done); \
+ local_irq_restore(irq_flags); \
+ return rc; \
+ }
/*
* shost state: If you alter this, you also need to alter scsi_sysfs.c
--
1.7.2.3
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 03/11] scsi: Add IRQ_DISABLE_SCSI_QCMD wrapper
2010-11-17 22:18 [PATCH 03/11] scsi: Add IRQ_DISABLE_SCSI_QCMD wrapper Nicholas A. Bellinger
@ 2010-11-17 22:27 ` Christoph Hellwig
2010-11-17 22:29 ` Nicholas A. Bellinger
0 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2010-11-17 22:27 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: linux-scsi, Jeff Garzik, James Bottomley, Christoph Hellwig,
Mike Christie, Ravi Anand, Andrew Vasquez, Joe Eykholt,
James Smart, Vasu Dev, Tim Chen, Andi Kleen, Tejun Heo,
Mike Anderson, MPTFusionLinux
On Wed, Nov 17, 2010 at 02:18:42PM -0800, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch adds a IRQ_DISABLE_SCSI_QCMD() wrapper macro used by LLDs
> that can now run in host_lock less mode, but still need interrupts disabled
> using local_irq_save() before calling their lld_queuecommand() dispatcher.
>
> jgarzik says this method is in fact slower than doing a spin_lock_irqsave() on
> internal lib_lld_queuecommand() callers (as is done in libiscsi and libata)
> but is still needed by the majority of lock_less LLDs.
It's not an overly helpful macro. There's no reason a driver should
ever disable irqs on it's own without actually taking a lock. Please
invest the additional couple of minutes and do a proper conversion.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 03/11] scsi: Add IRQ_DISABLE_SCSI_QCMD wrapper
2010-11-17 22:27 ` Christoph Hellwig
@ 2010-11-17 22:29 ` Nicholas A. Bellinger
2010-11-17 22:37 ` Christoph Hellwig
0 siblings, 1 reply; 9+ messages in thread
From: Nicholas A. Bellinger @ 2010-11-17 22:29 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-scsi, Jeff Garzik, James Bottomley, Christoph Hellwig,
Mike Christie, Ravi Anand, Andrew Vasquez, Joe Eykholt,
James Smart, Vasu Dev, Tim Chen, Andi Kleen, Tejun Heo,
Mike Anderson, MPTFusionLinux
On Wed, 2010-11-17 at 17:27 -0500, Christoph Hellwig wrote:
> On Wed, Nov 17, 2010 at 02:18:42PM -0800, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > This patch adds a IRQ_DISABLE_SCSI_QCMD() wrapper macro used by LLDs
> > that can now run in host_lock less mode, but still need interrupts disabled
> > using local_irq_save() before calling their lld_queuecommand() dispatcher.
> >
> > jgarzik says this method is in fact slower than doing a spin_lock_irqsave() on
> > internal lib_lld_queuecommand() callers (as is done in libiscsi and libata)
> > but is still needed by the majority of lock_less LLDs.
>
> It's not an overly helpful macro. There's no reason a driver should
> ever disable irqs on it's own without actually taking a lock. Please
> invest the additional couple of minutes and do a proper conversion.
>
Hmmm, this is following jgarzik's recommendation for LLDs that we could
not immediately identify a internal spin_lock to disable interrupts
upon. (eg: not libiscsi and libata).
--nab
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 03/11] scsi: Add IRQ_DISABLE_SCSI_QCMD wrapper
2010-11-17 22:29 ` Nicholas A. Bellinger
@ 2010-11-17 22:37 ` Christoph Hellwig
2010-11-18 10:25 ` Boaz Harrosh
0 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2010-11-17 22:37 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Christoph Hellwig, linux-scsi, Jeff Garzik, James Bottomley,
Christoph Hellwig, Mike Christie, Ravi Anand, Andrew Vasquez,
Joe Eykholt, James Smart, Vasu Dev, Tim Chen, Andi Kleen,
Tejun Heo, Mike Anderson, MPTFusionLinux
On Wed, Nov 17, 2010 at 02:29:30PM -0800, Nicholas A. Bellinger wrote:
> Hmmm, this is following jgarzik's recommendation for LLDs that we could
> not immediately identify a internal spin_lock to disable interrupts
> upon. (eg: not libiscsi and libata).
In that case wait for the driver author to identify it. If there's
no maintainer in reach chance is the driver doesn't care about the push
down. No need to rush any of this, do it one driver at a time and get
it right.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 03/11] scsi: Add IRQ_DISABLE_SCSI_QCMD wrapper
2010-11-17 22:37 ` Christoph Hellwig
@ 2010-11-18 10:25 ` Boaz Harrosh
2010-11-18 23:05 ` Nicholas A. Bellinger
0 siblings, 1 reply; 9+ messages in thread
From: Boaz Harrosh @ 2010-11-18 10:25 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Nicholas A. Bellinger, linux-scsi, Jeff Garzik, James Bottomley,
Christoph Hellwig, Mike Christie, Ravi Anand, Andrew Vasquez,
Joe Eykholt, James Smart, Vasu Dev, Tim Chen, Andi Kleen,
Tejun Heo, Mike Anderson, MPTFusionLinux
On 11/18/2010 12:37 AM, Christoph Hellwig wrote:
> On Wed, Nov 17, 2010 at 02:29:30PM -0800, Nicholas A. Bellinger wrote:
>> Hmmm, this is following jgarzik's recommendation for LLDs that we could
>> not immediately identify a internal spin_lock to disable interrupts
>> upon. (eg: not libiscsi and libata).
>
> In that case wait for the driver author to identify it. If there's
> no maintainer in reach chance is the driver doesn't care about the push
> down. No need to rush any of this, do it one driver at a time and get
> it right.
I totally agree with Christoph. Patches 5, 6, 8, 11 all change behaviour
I would like to see an "I audit the driver and ..." Please see my comment
to [patch 6]
Do it one by one and open-code the local_irq_save/restore inside the
main function. It's not like you can get away from a total audit and
testing.
Boaz
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 03/11] scsi: Add IRQ_DISABLE_SCSI_QCMD wrapper
2010-11-18 10:25 ` Boaz Harrosh
@ 2010-11-18 23:05 ` Nicholas A. Bellinger
2010-11-18 23:15 ` Jeff Garzik
0 siblings, 1 reply; 9+ messages in thread
From: Nicholas A. Bellinger @ 2010-11-18 23:05 UTC (permalink / raw)
To: Boaz Harrosh
Cc: Christoph Hellwig, linux-scsi, Jeff Garzik, James Bottomley,
Christoph Hellwig, Mike Christie, Ravi Anand, Andrew Vasquez,
Joe Eykholt, James Smart, Vasu Dev, Tim Chen, Andi Kleen,
Tejun Heo, Mike Anderson, MPTFusionLinux
On Thu, 2010-11-18 at 12:25 +0200, Boaz Harrosh wrote:
> On 11/18/2010 12:37 AM, Christoph Hellwig wrote:
> > On Wed, Nov 17, 2010 at 02:29:30PM -0800, Nicholas A. Bellinger wrote:
> >> Hmmm, this is following jgarzik's recommendation for LLDs that we could
> >> not immediately identify a internal spin_lock to disable interrupts
> >> upon. (eg: not libiscsi and libata).
> >
> > In that case wait for the driver author to identify it. If there's
> > no maintainer in reach chance is the driver doesn't care about the push
> > down. No need to rush any of this, do it one driver at a time and get
> > it right.
>
> I totally agree with Christoph. Patches 5, 6, 8, 11 all change behaviour
>
> I would like to see an "I audit the driver and ..." Please see my comment
> to [patch 6]
Just to reiterate the point here.. The only LLDs that have been made
'lock-less' in this first round are LLDs that:
*) Where already using the legacy optimization of releasing host_lock,
doing lld_work, and reaquring before returning from their
lld_queuecommand().
*) Have been tested by folks explictly during the various initial
attempts at lock_less mode, which still appear to be functionally
equivilent minus the precatuion of local_irq_[save,release] usage.
>
> Do it one by one and open-code the local_irq_save/restore inside the
> main function. It's not like you can get away from a total audit and
> testing.
>
Hmmm, I am not sure I agree with open coding these atm. I would much
rather get LLD maintainer feedback first for these initial cases so we
can figure out which of these patches can be further optimized along the
lines of libiscsi and libata.
--nab
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 03/11] scsi: Add IRQ_DISABLE_SCSI_QCMD wrapper
2010-11-18 23:05 ` Nicholas A. Bellinger
@ 2010-11-18 23:15 ` Jeff Garzik
2010-11-19 0:04 ` Nicholas A. Bellinger
0 siblings, 1 reply; 9+ messages in thread
From: Jeff Garzik @ 2010-11-18 23:15 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Boaz Harrosh, Christoph Hellwig, linux-scsi, James Bottomley,
Christoph Hellwig, Mike Christie, Ravi Anand, Andrew Vasquez,
Joe Eykholt, James Smart, Vasu Dev, Tim Chen, Andi Kleen,
Tejun Heo, Mike Anderson, MPTFusionLinux
On 11/18/2010 06:05 PM, Nicholas A. Bellinger wrote:
> On Thu, 2010-11-18 at 12:25 +0200, Boaz Harrosh wrote:
>> On 11/18/2010 12:37 AM, Christoph Hellwig wrote:
>>> On Wed, Nov 17, 2010 at 02:29:30PM -0800, Nicholas A. Bellinger wrote:
>>>> Hmmm, this is following jgarzik's recommendation for LLDs that we could
>>>> not immediately identify a internal spin_lock to disable interrupts
>>>> upon. (eg: not libiscsi and libata).
>>>
>>> In that case wait for the driver author to identify it. If there's
>>> no maintainer in reach chance is the driver doesn't care about the push
>>> down. No need to rush any of this, do it one driver at a time and get
>>> it right.
>>
>> I totally agree with Christoph. Patches 5, 6, 8, 11 all change behaviour
>>
>> I would like to see an "I audit the driver and ..." Please see my comment
>> to [patch 6]
>
> Just to reiterate the point here.. The only LLDs that have been made
> 'lock-less' in this first round are LLDs that:
>
> *) Where already using the legacy optimization of releasing host_lock,
> doing lld_work, and reaquring before returning from their
> lld_queuecommand().
>
> *) Have been tested by folks explictly during the various initial
> attempts at lock_less mode, which still appear to be functionally
> equivilent minus the precatuion of local_irq_[save,release] usage.
>
>>
>> Do it one by one and open-code the local_irq_save/restore inside the
>> main function. It's not like you can get away from a total audit and
>> testing.
>>
>
> Hmmm, I am not sure I agree with open coding these atm. I would much
> rather get LLD maintainer feedback first for these initial cases so we
> can figure out which of these patches can be further optimized along the
> lines of libiscsi and libata.
I don't think we'll be able to get around looking at each case
individually, therefore I do not see the utility of adding
IRQ_DISABLE_SCSI_QCMD().
If you're looking at each case, then you likely have an idea of exactly
how the code should be updated.
Jeff
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 03/11] scsi: Add IRQ_DISABLE_SCSI_QCMD wrapper
2010-11-18 23:15 ` Jeff Garzik
@ 2010-11-19 0:04 ` Nicholas A. Bellinger
0 siblings, 0 replies; 9+ messages in thread
From: Nicholas A. Bellinger @ 2010-11-19 0:04 UTC (permalink / raw)
To: Jeff Garzik
Cc: Boaz Harrosh, Christoph Hellwig, linux-scsi, James Bottomley,
Christoph Hellwig, Mike Christie, Ravi Anand, Andrew Vasquez,
Joe Eykholt, James Smart, Vasu Dev, Tim Chen, Andi Kleen,
Tejun Heo, Mike Anderson, MPTFusionLinux
On Thu, 2010-11-18 at 18:15 -0500, Jeff Garzik wrote:
> On 11/18/2010 06:05 PM, Nicholas A. Bellinger wrote:
> > On Thu, 2010-11-18 at 12:25 +0200, Boaz Harrosh wrote:
> >> On 11/18/2010 12:37 AM, Christoph Hellwig wrote:
> >>> On Wed, Nov 17, 2010 at 02:29:30PM -0800, Nicholas A. Bellinger wrote:
> >>>> Hmmm, this is following jgarzik's recommendation for LLDs that we could
> >>>> not immediately identify a internal spin_lock to disable interrupts
> >>>> upon. (eg: not libiscsi and libata).
> >>>
> >>> In that case wait for the driver author to identify it. If there's
> >>> no maintainer in reach chance is the driver doesn't care about the push
> >>> down. No need to rush any of this, do it one driver at a time and get
> >>> it right.
> >>
> >> I totally agree with Christoph. Patches 5, 6, 8, 11 all change behaviour
> >>
> >> I would like to see an "I audit the driver and ..." Please see my comment
> >> to [patch 6]
> >
> > Just to reiterate the point here.. The only LLDs that have been made
> > 'lock-less' in this first round are LLDs that:
> >
> > *) Where already using the legacy optimization of releasing host_lock,
> > doing lld_work, and reaquring before returning from their
> > lld_queuecommand().
> >
> > *) Have been tested by folks explictly during the various initial
> > attempts at lock_less mode, which still appear to be functionally
> > equivilent minus the precatuion of local_irq_[save,release] usage.
> >
> >>
> >> Do it one by one and open-code the local_irq_save/restore inside the
> >> main function. It's not like you can get away from a total audit and
> >> testing.
> >>
> >
> > Hmmm, I am not sure I agree with open coding these atm. I would much
> > rather get LLD maintainer feedback first for these initial cases so we
> > can figure out which of these patches can be further optimized along the
> > lines of libiscsi and libata.
>
> I don't think we'll be able to get around looking at each case
> individually, therefore I do not see the utility of adding
> IRQ_DISABLE_SCSI_QCMD().
>
I am definately not trying to imply that this should be merged as
lock_less without the necessary individual review and maintainer
signoffs.
> If you're looking at each case, then you likely have an idea of exactly
> how the code should be updated.
>
Sure, the main purpose of this series is to get the ball rolling on LLDs
that we (you can read this as selfish we ;) collectively care about for
high performance modern lock_less usage.. I am more than happy to get
NACKs on a per LLD basis for hardware that I am not currently able to
test with in order to get the maintainers involved to make the call if
proper semi optimized -> fully optimized lock_less operation is in their
near future.
Best,
--nab
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 03/11] scsi: Add IRQ_DISABLE_SCSI_QCMD wrapper
@ 2010-11-12 0:13 Nicholas A. Bellinger
0 siblings, 0 replies; 9+ messages in thread
From: Nicholas A. Bellinger @ 2010-11-12 0:13 UTC (permalink / raw)
To: linux-scsi, Jeff Garzik, James Bottomley, Christoph Hellwig
Cc: Mike Christie, Ravi Anand, Andrew Vasquez, Joe Eykholt,
James Smart, Vasu Dev, Tim Chen, Andi Kleen, Tejun Heo,
Mike Anderson, MPTFusionLinux, Nicholas Bellinger
From: Nicholas Bellinger <nab@linux-iscsi.org>
This patch adds a IRQ_DISABLE_SCSI_QCMD() wrapper macro used by LLDs
that can now run in host_lock less mode, but still need interrupts disabled
using local_irq_save() before calling their lld_queuecommand() dispatcher.
jgarzik says this method is in fact slower than doing a spin_lock_irqsave() on
internal lib_lld_queuecommand() callers (as is done in libiscsi and libata)
but is still needed by the majority of lock_less LLDs.
Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
include/scsi/scsi_host.h | 15 +++++++++++++++
1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 6909038..ddb0622 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -523,6 +523,21 @@ struct scsi_host_template {
return rc; \
}
+/*
+ * Used for LLDs running in lock-less mode, but that still require having
+ * interrupts disable during their lld_queuecommand() dispatch.
+ */
+#define IRQ_DISABLE_SCSI_QCMD(func_name) \
+ int func_name(struct scsi_cmnd *cmd, \
+ void (*done)(struct scsi_cmnd *)) \
+ { \
+ unsigned long irq_flags; \
+ int rc; \
+ local_irq_save(irq_flags); \
+ rc = func_name##_irq_disable (cmd, done); \
+ local_irq_restore(irq_flags); \
+ return rc; \
+ }
/*
* shost state: If you alter this, you also need to alter scsi_sysfs.c
--
1.7.2.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-11-19 0:09 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-17 22:18 [PATCH 03/11] scsi: Add IRQ_DISABLE_SCSI_QCMD wrapper Nicholas A. Bellinger
2010-11-17 22:27 ` Christoph Hellwig
2010-11-17 22:29 ` Nicholas A. Bellinger
2010-11-17 22:37 ` Christoph Hellwig
2010-11-18 10:25 ` Boaz Harrosh
2010-11-18 23:05 ` Nicholas A. Bellinger
2010-11-18 23:15 ` Jeff Garzik
2010-11-19 0:04 ` Nicholas A. Bellinger
-- strict thread matches above, loose matches on Subject: below --
2010-11-12 0:13 Nicholas A. Bellinger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.