[PATCH 08/14] scsi: convert device_busy to atomic_t
Avoid taking the queue_lock to check the per-device queue limit. Instead
we do an atomic_inc_return early on to grab our slot in the queue,
and if necessary decrement it after finishing all checks.
Unlike the host and target busy counters this doesn't allow us to avoid the
queue_lock in the request_fn due to the way the interface works, but it'll
allow us to prepare for using the blk-mq code, which doesn't use the
queue_lock at all, and it at least avoids a queue_lock round trip in
scsi_device_unbusy, which is still important given how busy the queue_lock
is.
Signed-off-by: Christoph Hellwig
Reviewed-by: Hannes Reinecke
Reviewed-by: Webb Scales
Acked-by: Jens Axboe
Tested-by: Bart Van Assche
Tested-by: Robert Elliott
---
drivers/message/fusion/mptsas.c | 2 +-
drivers/scsi/scsi_lib.c | 50 +++--
drivers/scsi/scsi_sysfs.c | 10 -
drivers/scsi/sg.c | 2 +-
include/scsi/scsi_device.h | 4 +---
5 files changed, 40 insertions(+), 28 deletions(-)
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 711fcb5..d636dbe 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -3763,7 +3763,7 @@ mptsas_send_link_status_event(struct fw_event_work
*fw_event)
printk(MYIOC_s_DEBUG_FMT
"SDEV OUTSTANDING CMDS"
"%d\n", ioc->name,
- sdev->device_busy));
+
atomic_read(&sdev->device_busy)));
}
}
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index d0bd7e0..1ddf0fb 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -302,9 +302,7 @@ void scsi_device_unbusy(struct scsi_device *sdev)
spin_unlock_irqrestore(shost->host_lock, flags);
}
- spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
- sdev->device_busy--;
- spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
+ atomic_dec(&sdev->device_busy);
}
/*
@@ -355,9 +353,9 @@ static void scsi_single_lun_run(struct scsi_device
*current_sdev)
static inline int scsi_device_is_busy(struct scsi_device *sdev)
{
- if (sdev->device_busy >= sdev->queue_depth || sdev->device_blocked)
+ if (atomic_read(&sdev->device_busy) >= sdev->queue_depth ||
+ sdev->device_blocked)
return 1;
-
return 0;
}
@@ -1204,7 +1202,7 @@ scsi_prep_return(struct request_queue *q, struct request
*req, int ret)
* queue must be restarted, so we schedule a callback to happen
* shortly.
*/
- if (sdev->device_busy == 0)
+ if (atomic_read(&sdev->device_busy) == 0)
blk_delay_queue(q, SCSI_QUEUE_DELAY);
break;
default:
@@ -1255,26 +1253,33 @@ static void scsi_unprep_fn(struct request_queue *q,
struct request *req)
static inline int scsi_dev_queue_ready(struct request_queue *q,
struct scsi_device *sdev)
{
- if (sdev->device_busy == 0 && sdev->device_blocked) {
+ unsigned int busy;
+
+ busy = atomic_inc_return(&sdev->device_busy) - 1;
+ if (sdev->device_blocked) {
+ if (busy)
+ goto out_dec;
+
/*
* unblock after device_blocked iterates to zero
*/
- if (--sdev->device_blocked == 0) {
- SCSI_LOG_MLQUEUE(3,
- sdev_printk(KERN_INFO, sdev,
- "unblocking device at zero depth\n"));
- } else {
+ if (--sdev->device_blocked != 0) {
blk_delay_queue(q, SCSI_QUEUE_DELAY);
- return 0;
+ goto out_dec;
}
+ SCSI_LOG_MLQUEUE(3, sdev_printk(KERN_INFO, sdev,
+ "unblocking device at zero depth\n"));
}
- if (scsi_device_is_busy(sdev))
- return 0;
+
+ if (busy >= sdev->queue_depth)
+ goto out_dec;
return 1;
+out_dec:
+ atomic_dec(&sdev->device_busy);
+ return 0;
}
-
/*
* scsi_target_queue_ready: checks if there we can send commands to target
* @sdev: scsi device on starget to check.
@@ -1448,7 +1453,7 @@ static void scsi_kill_request(struct request *req, struct
request_queue *q)
* bump busy counts. To bump the counters, we need to dance
* with the locks as normal issue path does.
*/
- sdev->device_busy++;
+ atomic_inc(&sdev->device_busy);
atomic_inc(&shost->host_busy);
atomic_inc(&starget->target_bus
Re: [PATCH 08/14] scsi: convert device_busy to atomic_t
On Wed, Jul 09, 2014 at 09:49:56AM -0700, James Bottomley wrote: > As far as I can tell from the block MQ, we get one CPU thread per LUN. No, that's entirely incorrect. IFF a device supports multiple hardware queues we only submit I/O from CPUs (there might be more than one) this queue is bound to. With the single hardware queue supported by most hardware submissions can and will happen from any CPU. Note that this patchset doesn't even support multiple hardware queues yet, although it should be fairly simple to add once the low level driver support is ready. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/14] scsi: convert device_busy to atomic_t
On Wed, 2014-06-25 at 18:51 +0200, Christoph Hellwig wrote: > Avoid taking the queue_lock to check the per-device queue limit. Instead > we do an atomic_inc_return early on to grab our slot in the queue, > and if nessecary decrement it after finishing all checks. > > Unlike the host and target busy counters this doesn't allow us to avoid the > queue_lock in the request_fn due to the way the interface works, but it'll > allow us to prepare for using the blk-mq code, which doesn't use the > queue_lock at all, and it at least avoids a queue_lock rountrip in > scsi_device_unbusy, which is still important given how busy the queue_lock > is. Most of these patches look fine to me, but this one worries me largely because of the expense of atomics. As far as I can tell from the block MQ, we get one CPU thread per LUN. Doesn't this mean that we only need true atomics for variables that cross threads? That does mean target and host, but shouldn't mean device, since device == LUN. As long as we protect from local interrupts, we should be able to exclusively update all LUN local variables without having to change them to being atomic. This view depends on correct CPU steering of returning interrupts, since the LUN thread model only works if the same CPU handles issue and completion, but it looks like that works in MQ, even if it doesn't work in vanilla. James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/14] scsi: convert device_busy to atomic_t
On 06/25/2014 06:51 PM, Christoph Hellwig wrote:
Avoid taking the queue_lock to check the per-device queue limit. Instead
we do an atomic_inc_return early on to grab our slot in the queue,
and if nessecary decrement it after finishing all checks.
Unlike the host and target busy counters this doesn't allow us to avoid the
queue_lock in the request_fn due to the way the interface works, but it'll
allow us to prepare for using the blk-mq code, which doesn't use the
queue_lock at all, and it at least avoids a queue_lock rountrip in
scsi_device_unbusy, which is still important given how busy the queue_lock
is.
Signed-off-by: Christoph Hellwig
---
drivers/message/fusion/mptsas.c |2 +-
drivers/scsi/scsi_lib.c | 50 ++-
drivers/scsi/scsi_sysfs.c | 10 +++-
drivers/scsi/sg.c |2 +-
include/scsi/scsi_device.h |4 +---
5 files changed, 40 insertions(+), 28 deletions(-)
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 711fcb5..d636dbe 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -3763,7 +3763,7 @@ mptsas_send_link_status_event(struct fw_event_work
*fw_event)
printk(MYIOC_s_DEBUG_FMT
"SDEV OUTSTANDING CMDS"
"%d\n", ioc->name,
- sdev->device_busy));
+
atomic_read(&sdev->device_busy)));
}
}
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 5d37d79..e23fef5 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -302,9 +302,7 @@ void scsi_device_unbusy(struct scsi_device *sdev)
spin_unlock_irqrestore(shost->host_lock, flags);
}
- spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
- sdev->device_busy--;
- spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
+ atomic_dec(&sdev->device_busy);
}
/*
@@ -355,9 +353,10 @@ static void scsi_single_lun_run(struct scsi_device
*current_sdev)
static inline int scsi_device_is_busy(struct scsi_device *sdev)
{
- if (sdev->device_busy >= sdev->queue_depth || sdev->device_blocked)
+ if (atomic_read(&sdev->device_busy) >= sdev->queue_depth)
+ return 1;
+ if (sdev->device_blocked)
return 1;
-
return 0;
}
@@ -1224,7 +1223,7 @@ scsi_prep_return(struct request_queue *q, struct request
*req, int ret)
* queue must be restarted, so we schedule a callback to happen
* shortly.
*/
- if (sdev->device_busy == 0)
+ if (atomic_read(&sdev->device_busy) == 0)
blk_delay_queue(q, SCSI_QUEUE_DELAY);
break;
default:
@@ -1281,26 +1280,32 @@ static void scsi_unprep_fn(struct request_queue *q,
struct request *req)
static inline int scsi_dev_queue_ready(struct request_queue *q,
struct scsi_device *sdev)
{
- if (sdev->device_busy == 0 && sdev->device_blocked) {
+ unsigned int busy;
+
+ busy = atomic_inc_return(&sdev->device_busy) - 1;
+ if (busy == 0 && sdev->device_blocked) {
/*
* unblock after device_blocked iterates to zero
*/
- if (--sdev->device_blocked == 0) {
- SCSI_LOG_MLQUEUE(3,
- sdev_printk(KERN_INFO, sdev,
- "unblocking device at zero depth\n"));
- } else {
+ if (--sdev->device_blocked != 0) {
blk_delay_queue(q, SCSI_QUEUE_DELAY);
- return 0;
+ goto out_dec;
}
+ SCSI_LOG_MLQUEUE(3, sdev_printk(KERN_INFO, sdev,
+ "unblocking device at zero depth\n"));
}
- if (scsi_device_is_busy(sdev))
- return 0;
+
+ if (busy >= sdev->queue_depth)
+ goto out_dec;
+ if (sdev->device_blocked)
+ goto out_dec;
return 1;
+out_dec:
+ atomic_dec(&sdev->device_busy);
+ return 0;
}
-
/*
* scsi_target_queue_ready: checks if there we can send commands to target
* @sdev: scsi device on starget to check.
@@ -1470,7 +1475,7 @@ static void scsi_kill_request(struct request *req, struct
request_queue *q)
* bump busy counts. To bump the counters, we need to dance
* with the locks as normal issue path does.
*/
- sdev->device_busy++;
+ atomic_inc(&sdev->device_busy);
atomic_inc(&shost->host_busy);
atomic_inc(&starget->target_busy);
@@ -1566,7 +1571,7 @@ stat
[PATCH 08/14] scsi: convert device_busy to atomic_t
Avoid taking the queue_lock to check the per-device queue limit. Instead
we do an atomic_inc_return early on to grab our slot in the queue,
and if nessecary decrement it after finishing all checks.
Unlike the host and target busy counters this doesn't allow us to avoid the
queue_lock in the request_fn due to the way the interface works, but it'll
allow us to prepare for using the blk-mq code, which doesn't use the
queue_lock at all, and it at least avoids a queue_lock rountrip in
scsi_device_unbusy, which is still important given how busy the queue_lock
is.
Signed-off-by: Christoph Hellwig
---
drivers/message/fusion/mptsas.c |2 +-
drivers/scsi/scsi_lib.c | 50 ++-
drivers/scsi/scsi_sysfs.c | 10 +++-
drivers/scsi/sg.c |2 +-
include/scsi/scsi_device.h |4 +---
5 files changed, 40 insertions(+), 28 deletions(-)
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 711fcb5..d636dbe 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -3763,7 +3763,7 @@ mptsas_send_link_status_event(struct fw_event_work
*fw_event)
printk(MYIOC_s_DEBUG_FMT
"SDEV OUTSTANDING CMDS"
"%d\n", ioc->name,
- sdev->device_busy));
+
atomic_read(&sdev->device_busy)));
}
}
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 5d37d79..e23fef5 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -302,9 +302,7 @@ void scsi_device_unbusy(struct scsi_device *sdev)
spin_unlock_irqrestore(shost->host_lock, flags);
}
- spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
- sdev->device_busy--;
- spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
+ atomic_dec(&sdev->device_busy);
}
/*
@@ -355,9 +353,10 @@ static void scsi_single_lun_run(struct scsi_device
*current_sdev)
static inline int scsi_device_is_busy(struct scsi_device *sdev)
{
- if (sdev->device_busy >= sdev->queue_depth || sdev->device_blocked)
+ if (atomic_read(&sdev->device_busy) >= sdev->queue_depth)
+ return 1;
+ if (sdev->device_blocked)
return 1;
-
return 0;
}
@@ -1224,7 +1223,7 @@ scsi_prep_return(struct request_queue *q, struct request
*req, int ret)
* queue must be restarted, so we schedule a callback to happen
* shortly.
*/
- if (sdev->device_busy == 0)
+ if (atomic_read(&sdev->device_busy) == 0)
blk_delay_queue(q, SCSI_QUEUE_DELAY);
break;
default:
@@ -1281,26 +1280,32 @@ static void scsi_unprep_fn(struct request_queue *q,
struct request *req)
static inline int scsi_dev_queue_ready(struct request_queue *q,
struct scsi_device *sdev)
{
- if (sdev->device_busy == 0 && sdev->device_blocked) {
+ unsigned int busy;
+
+ busy = atomic_inc_return(&sdev->device_busy) - 1;
+ if (busy == 0 && sdev->device_blocked) {
/*
* unblock after device_blocked iterates to zero
*/
- if (--sdev->device_blocked == 0) {
- SCSI_LOG_MLQUEUE(3,
- sdev_printk(KERN_INFO, sdev,
- "unblocking device at zero depth\n"));
- } else {
+ if (--sdev->device_blocked != 0) {
blk_delay_queue(q, SCSI_QUEUE_DELAY);
- return 0;
+ goto out_dec;
}
+ SCSI_LOG_MLQUEUE(3, sdev_printk(KERN_INFO, sdev,
+ "unblocking device at zero depth\n"));
}
- if (scsi_device_is_busy(sdev))
- return 0;
+
+ if (busy >= sdev->queue_depth)
+ goto out_dec;
+ if (sdev->device_blocked)
+ goto out_dec;
return 1;
+out_dec:
+ atomic_dec(&sdev->device_busy);
+ return 0;
}
-
/*
* scsi_target_queue_ready: checks if there we can send commands to target
* @sdev: scsi device on starget to check.
@@ -1470,7 +1475,7 @@ static void scsi_kill_request(struct request *req, struct
request_queue *q)
* bump busy counts. To bump the counters, we need to dance
* with the locks as normal issue path does.
*/
- sdev->device_busy++;
+ atomic_inc(&sdev->device_busy);
atomic_inc(&shost->host_busy);
atomic_inc(&starget->target_busy);
@@ -1566,7 +1571,7 @@ static void scsi_request_fn(struct request_queue *q)
[PATCH 08/14] scsi: convert device_busy to atomic_t
Avoid taking the queue_lock to check the per-device queue limit. Instead
we do an atomic_inc_return early on to grab our slot in the queue,
and if nessecary decrement it after finishing all checks.
Unlike the host and target busy counters this doesn't allow us to avoid the
queue_lock in the request_fn due to the way the interface works, but it'll
allow us to prepare for using the blk-mq code, which doesn't use the
queue_lock at all, and it at least avoids a queue_lock rountrip in
scsi_device_unbusy, which is still important given how busy the queue_lock
is.
Signed-off-by: Christoph Hellwig
---
drivers/message/fusion/mptsas.c |2 +-
drivers/scsi/scsi_lib.c | 50 ++-
drivers/scsi/scsi_sysfs.c | 10 +++-
drivers/scsi/sg.c |2 +-
include/scsi/scsi_device.h |4 +---
5 files changed, 40 insertions(+), 28 deletions(-)
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 711fcb5..d636dbe 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -3763,7 +3763,7 @@ mptsas_send_link_status_event(struct fw_event_work
*fw_event)
printk(MYIOC_s_DEBUG_FMT
"SDEV OUTSTANDING CMDS"
"%d\n", ioc->name,
- sdev->device_busy));
+
atomic_read(&sdev->device_busy)));
}
}
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 3f51bb8..c36c313 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -302,9 +302,7 @@ void scsi_device_unbusy(struct scsi_device *sdev)
spin_unlock_irqrestore(shost->host_lock, flags);
}
- spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
- sdev->device_busy--;
- spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
+ atomic_dec(&sdev->device_busy);
}
/*
@@ -355,9 +353,10 @@ static void scsi_single_lun_run(struct scsi_device
*current_sdev)
static inline int scsi_device_is_busy(struct scsi_device *sdev)
{
- if (sdev->device_busy >= sdev->queue_depth || sdev->device_blocked)
+ if (atomic_read(&sdev->device_busy) >= sdev->queue_depth)
+ return 1;
+ if (sdev->device_blocked)
return 1;
-
return 0;
}
@@ -1224,7 +1223,7 @@ scsi_prep_return(struct request_queue *q, struct request
*req, int ret)
* queue must be restarted, so we schedule a callback to happen
* shortly.
*/
- if (sdev->device_busy == 0)
+ if (atomic_read(&sdev->device_busy) == 0)
blk_delay_queue(q, SCSI_QUEUE_DELAY);
break;
default:
@@ -1281,26 +1280,32 @@ static void scsi_unprep_fn(struct request_queue *q,
struct request *req)
static inline int scsi_dev_queue_ready(struct request_queue *q,
struct scsi_device *sdev)
{
- if (sdev->device_busy == 0 && sdev->device_blocked) {
+ unsigned int busy;
+
+ busy = atomic_inc_return(&sdev->device_busy) - 1;
+ if (busy == 0 && sdev->device_blocked) {
/*
* unblock after device_blocked iterates to zero
*/
- if (--sdev->device_blocked == 0) {
- SCSI_LOG_MLQUEUE(3,
- sdev_printk(KERN_INFO, sdev,
- "unblocking device at zero depth\n"));
- } else {
+ if (--sdev->device_blocked != 0) {
blk_delay_queue(q, SCSI_QUEUE_DELAY);
- return 0;
+ goto out_dec;
}
+ SCSI_LOG_MLQUEUE(3, sdev_printk(KERN_INFO, sdev,
+ "unblocking device at zero depth\n"));
}
- if (scsi_device_is_busy(sdev))
- return 0;
+
+ if (busy >= sdev->queue_depth)
+ goto out_dec;
+ if (sdev->device_blocked)
+ goto out_dec;
return 1;
+out_dec:
+ atomic_dec(&sdev->device_busy);
+ return 0;
}
-
/*
* scsi_target_queue_ready: checks if there we can send commands to target
* @sdev: scsi device on starget to check.
@@ -1470,7 +1475,7 @@ static void scsi_kill_request(struct request *req, struct
request_queue *q)
* bump busy counts. To bump the counters, we need to dance
* with the locks as normal issue path does.
*/
- sdev->device_busy++;
+ atomic_inc(&sdev->device_busy);
atomic_inc(&shost->host_busy);
atomic_inc(&starget->target_busy);
@@ -1566,7 +1571,7 @@ static void scsi_request_fn(struct request_queue *q)
