scsi: Add IRQ_DISABLE_SCSI_QCMD wrapper
PATCH 3/11 scsi: Add IRQ_DISABLE_SCSI_QCMD wrapper
From: Nicholas Bellinger <[email protected]>
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.
--- include/scsi/scsi_host.h | 14 ++++++++++++++ 1 files changed, 14 insertions(+), 0 deletions(-)
include/scsi/scsi_host.h +14 -0
@@ -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
Signature · 1 line
1.7.2.3
Quoted 9 lines · Nicholas A. Bellinger
From: Nicholas Bellinger <[email protected]>
This patch adds a
IRQ_DISABLE_SCSI_QCMD()wrapper macro used by LLDs that can now run inhost_lockless mode, but still need interrupts disabled usinglocal_irq_save()before calling theirlld_queuecommand()dispatcher.jgarzik says this method is in fact slower than doing a
spin_lock_irqsave()on internallib_lld_queuecommand()callers (as is done in libiscsi and libata) but is still needed by the majority oflock_lessLLDs.
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.
Quoted 7 lines · Christoph Hellwig
Quoted 6 lines · Nicholas A. Bellinger
From: Nicholas Bellinger <[email protected]>
This patch adds a
IRQ_DISABLE_SCSI_QCMD()wrapper macro used by LLDs that can now run inhost_lockless mode, but still need interrupts disabled usinglocal_irq_save()before calling theirlld_queuecommand()dispatcher.
Quoted 8 lines
Quoted 3 lines
jgarzik says this method is in fact slower than doing a
spin_lock_irqsave()on internallib_lld_queuecommand()callers (as is done in libiscsi and libata) but is still needed by the majority oflock_lessLLDs.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
Quoted 3 lines · Nicholas A. Bellinger
Hmmm, this is following jgarzik's recommendation for LLDs that we could not immediately identify a internal
spin_lockto 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.
Quoted 9 lines · Christoph Hellwig
Quoted 3 lines · Nicholas A. Bellinger
Hmmm, this is following jgarzik's recommendation for LLDs that we could not immediately identify a internal
spin_lockto 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
Quoted 11 lines · Boaz Harrosh
Quoted 9 lines · Christoph Hellwig
Quoted 3 lines · Nicholas A. Bellinger
Hmmm, this is following jgarzik's recommendation for LLDs that we could not immediately identify a internal
spin_lockto 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.
Quoted 4 lines
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.
Quoted 5 lines
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
Quoted 34 lines · Nicholas A. Bellinger
Quoted 11 lines · Boaz Harrosh
Quoted 9 lines · Christoph Hellwig
Quoted 3 lines · Nicholas A. Bellinger
Hmmm, this is following jgarzik's recommendation for LLDs that we could not immediately identify a internal
spin_lockto 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.
Quoted 4 lines
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, doinglld_work, and reaquring before returning from theirlld_queuecommand().*) Have been tested by folks explictly during the various initial attempts at
lock_lessmode, which still appear to be functionally equivilent minus the precatuion of local_irq_[save,release] usage.Quoted 5 lines
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.
Quoted 4 lines
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
Quoted 35 lines · Jeff Garzik
Quoted 34 lines · Nicholas A. Bellinger
Quoted 11 lines · Boaz Harrosh
Quoted 9 lines · Christoph Hellwig
Quoted 3 lines · Nicholas A. Bellinger
Hmmm, this is following jgarzik's recommendation for LLDs that we could not immediately identify a internal
spin_lockto 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.
Quoted 4 lines
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, doinglld_work, and reaquring before returning from theirlld_queuecommand().*) Have been tested by folks explictly during the various initial attempts at
lock_lessmode, which still appear to be functionally equivilent minus the precatuion of local_irq_[save,release] usage.Quoted 5 lines
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.
Quoted 9 lines
Quoted 4 lines
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.
Quoted 3 lines
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