Re: [rfc][patch 1/3] block: non-atomic queue_flags prep
On Tue, Dec 18 2007, Nick Piggin wrote: > On Tue, Dec 18, 2007 at 08:44:40AM +0100, Jens Axboe wrote: > > On Sat, Dec 15 2007, Nick Piggin wrote: > > > Hi, > > > > > > This is just an idea I had, which might make request processing a little > > > bit cheaper depending on queue behaviour. For example if it is getting > > > plugged > > > unplugged frequently (as I think is the case for some database workloads), > > > then we might save one or two atomic operations per request. > > > > > > Anyway, I'm not completely sure if I have ensured all queue_flags users > > > are > > > safe (I think md may need a bit of help). But overall it seems quite > > > doable. > > > > Looks ok to me, I'll throw it into the testing mix. Thanks Nick! > > OK... actually if you are expecting it to be widely tested, can you change > the BUG_ONs in queue_flag_set / queue_flag_clear into WARN_ON? > > That way it's less likely to take down people's systems... Agree, will do so. -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [rfc][patch 1/3] block: non-atomic queue_flags prep
On Tue, Dec 18 2007, Nick Piggin wrote: On Tue, Dec 18, 2007 at 08:44:40AM +0100, Jens Axboe wrote: On Sat, Dec 15 2007, Nick Piggin wrote: Hi, This is just an idea I had, which might make request processing a little bit cheaper depending on queue behaviour. For example if it is getting plugged unplugged frequently (as I think is the case for some database workloads), then we might save one or two atomic operations per request. Anyway, I'm not completely sure if I have ensured all queue_flags users are safe (I think md may need a bit of help). But overall it seems quite doable. Looks ok to me, I'll throw it into the testing mix. Thanks Nick! OK... actually if you are expecting it to be widely tested, can you change the BUG_ONs in queue_flag_set / queue_flag_clear into WARN_ON? That way it's less likely to take down people's systems... Agree, will do so. -- Jens Axboe -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [rfc][patch 1/3] block: non-atomic queue_flags prep
On Tue, Dec 18, 2007 at 08:44:40AM +0100, Jens Axboe wrote: > On Sat, Dec 15 2007, Nick Piggin wrote: > > Hi, > > > > This is just an idea I had, which might make request processing a little > > bit cheaper depending on queue behaviour. For example if it is getting > > plugged > > unplugged frequently (as I think is the case for some database workloads), > > then we might save one or two atomic operations per request. > > > > Anyway, I'm not completely sure if I have ensured all queue_flags users are > > safe (I think md may need a bit of help). But overall it seems quite doable. > > Looks ok to me, I'll throw it into the testing mix. Thanks Nick! OK... actually if you are expecting it to be widely tested, can you change the BUG_ONs in queue_flag_set / queue_flag_clear into WARN_ON? That way it's less likely to take down people's systems... Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [rfc][patch 1/3] block: non-atomic queue_flags prep
On Tue, Dec 18, 2007 at 08:44:40AM +0100, Jens Axboe wrote: On Sat, Dec 15 2007, Nick Piggin wrote: Hi, This is just an idea I had, which might make request processing a little bit cheaper depending on queue behaviour. For example if it is getting plugged unplugged frequently (as I think is the case for some database workloads), then we might save one or two atomic operations per request. Anyway, I'm not completely sure if I have ensured all queue_flags users are safe (I think md may need a bit of help). But overall it seems quite doable. Looks ok to me, I'll throw it into the testing mix. Thanks Nick! OK... actually if you are expecting it to be widely tested, can you change the BUG_ONs in queue_flag_set / queue_flag_clear into WARN_ON? That way it's less likely to take down people's systems... Thanks! -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [rfc][patch 1/3] block: non-atomic queue_flags prep
On Sat, Dec 15 2007, Nick Piggin wrote: > Hi, > > This is just an idea I had, which might make request processing a little > bit cheaper depending on queue behaviour. For example if it is getting plugged > unplugged frequently (as I think is the case for some database workloads), > then we might save one or two atomic operations per request. > > Anyway, I'm not completely sure if I have ensured all queue_flags users are > safe (I think md may need a bit of help). But overall it seems quite doable. Looks ok to me, I'll throw it into the testing mix. Thanks Nick! -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [rfc][patch 1/3] block: non-atomic queue_flags prep
On Sat, Dec 15 2007, Nick Piggin wrote: Hi, This is just an idea I had, which might make request processing a little bit cheaper depending on queue behaviour. For example if it is getting plugged unplugged frequently (as I think is the case for some database workloads), then we might save one or two atomic operations per request. Anyway, I'm not completely sure if I have ensured all queue_flags users are safe (I think md may need a bit of help). But overall it seems quite doable. Looks ok to me, I'll throw it into the testing mix. Thanks Nick! -- Jens Axboe -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[rfc][patch 1/3] block: non-atomic queue_flags prep
Hi,
This is just an idea I had, which might make request processing a little
bit cheaper depending on queue behaviour. For example if it is getting plugged
unplugged frequently (as I think is the case for some database workloads),
then we might save one or two atomic operations per request.
Anyway, I'm not completely sure if I have ensured all queue_flags users are
safe (I think md may need a bit of help). But overall it seems quite doable.
Comments?
---
Prep queue_flags to be non-atomic and taking protection from queue_lock.
Do this by ensuring the queue_lock is held everywhere that queue_flags
are changed. Locking additions are confined to slowpaths.
Index: linux-2.6/block/elevator.c
===
--- linux-2.6.orig/block/elevator.c
+++ linux-2.6/block/elevator.c
@@ -1066,7 +1066,10 @@ static int elevator_switch(struct reques
* finally exit old elevator and turn off BYPASS.
*/
elevator_exit(old_elevator);
+ spin_lock_irq(q->queue_lock);
clear_bit(QUEUE_FLAG_ELVSWITCH, >queue_flags);
+ spin_unlock_irq(q->queue_lock);
+
return 1;
fail_register:
@@ -1077,7 +1080,11 @@ fail_register:
elevator_exit(e);
q->elevator = old_elevator;
elv_register_queue(q);
+
+ spin_lock_irq(q->queue_lock);
clear_bit(QUEUE_FLAG_ELVSWITCH, >queue_flags);
+ spin_unlock_irq(q->queue_lock);
+
return 0;
}
Index: linux-2.6/block/ll_rw_blk.c
===
--- linux-2.6.orig/block/ll_rw_blk.c
+++ linux-2.6/block/ll_rw_blk.c
@@ -1732,14 +1732,11 @@ void blk_sync_queue(struct request_queue
EXPORT_SYMBOL(blk_sync_queue);
/**
- * blk_run_queue - run a single device queue
+ * __blk_run_queue - run a single device queue. queue_lock must be held.
* @q: The queue to run
*/
-void blk_run_queue(struct request_queue *q)
+void __blk_run_queue(struct request_queue *q)
{
- unsigned long flags;
-
- spin_lock_irqsave(q->queue_lock, flags);
blk_remove_plug(q);
/*
@@ -1755,7 +1752,19 @@ void blk_run_queue(struct request_queue
kblockd_schedule_work(>unplug_work);
}
}
+}
+EXPORT_SYMBOL(__blk_run_queue);
+/**
+ * blk_run_queue - run a single device queue
+ * @q: The queue to run
+ */
+void blk_run_queue(struct request_queue *q)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(q->queue_lock, flags);
+ __blk_run_queue(q);
spin_unlock_irqrestore(q->queue_lock, flags);
}
EXPORT_SYMBOL(blk_run_queue);
@@ -1804,7 +1813,9 @@ EXPORT_SYMBOL(blk_put_queue);
void blk_cleanup_queue(struct request_queue * q)
{
mutex_lock(>sysfs_lock);
+ spin_lock_irq(q->queue_lock);
set_bit(QUEUE_FLAG_DEAD, >queue_flags);
+ spin_unlock_irq(q->queue_lock);
mutex_unlock(>sysfs_lock);
if (q->elevator)
Index: linux-2.6/drivers/scsi/scsi_lib.c
===
--- linux-2.6.orig/drivers/scsi/scsi_lib.c
+++ linux-2.6/drivers/scsi/scsi_lib.c
@@ -532,6 +532,9 @@ static void scsi_run_queue(struct reques
!shost->host_blocked && !shost->host_self_blocked &&
!((shost->can_queue > 0) &&
(shost->host_busy >= shost->can_queue))) {
+
+ int flagset;
+
/*
* As long as shost is accepting commands and we have
* starved queues, call blk_run_queue. scsi_request_fn
@@ -547,17 +550,17 @@ static void scsi_run_queue(struct reques
list_del_init(>starved_entry);
spin_unlock_irqrestore(shost->host_lock, flags);
-
- if (test_bit(QUEUE_FLAG_REENTER, >queue_flags) &&
- !test_and_set_bit(QUEUE_FLAG_REENTER,
- >request_queue->queue_flags)) {
- blk_run_queue(sdev->request_queue);
+ spin_lock(sdev->request_queue->queue_lock);
+ flagset = test_bit(QUEUE_FLAG_REENTER, >queue_flags) &&
+ !test_and_set_bit(QUEUE_FLAG_REENTER,
+ >request_queue->queue_flags);
+ __blk_run_queue(sdev->request_queue);
+ if (flagset)
clear_bit(QUEUE_FLAG_REENTER,
- >request_queue->queue_flags);
- } else
- blk_run_queue(sdev->request_queue);
+ >request_queue->queue_flags);
+ spin_unlock(sdev->request_queue->queue_lock);
- spin_lock_irqsave(shost->host_lock, flags);
+ spin_lock(shost->host_lock);
if (unlikely(!list_empty(>starved_entry)))
/*
* sdev lost a race, and was put back on the
Index: linux-2.6/drivers/block/loop.c
[rfc][patch 1/3] block: non-atomic queue_flags prep
Hi,
This is just an idea I had, which might make request processing a little
bit cheaper depending on queue behaviour. For example if it is getting plugged
unplugged frequently (as I think is the case for some database workloads),
then we might save one or two atomic operations per request.
Anyway, I'm not completely sure if I have ensured all queue_flags users are
safe (I think md may need a bit of help). But overall it seems quite doable.
Comments?
---
Prep queue_flags to be non-atomic and taking protection from queue_lock.
Do this by ensuring the queue_lock is held everywhere that queue_flags
are changed. Locking additions are confined to slowpaths.
Index: linux-2.6/block/elevator.c
===
--- linux-2.6.orig/block/elevator.c
+++ linux-2.6/block/elevator.c
@@ -1066,7 +1066,10 @@ static int elevator_switch(struct reques
* finally exit old elevator and turn off BYPASS.
*/
elevator_exit(old_elevator);
+ spin_lock_irq(q-queue_lock);
clear_bit(QUEUE_FLAG_ELVSWITCH, q-queue_flags);
+ spin_unlock_irq(q-queue_lock);
+
return 1;
fail_register:
@@ -1077,7 +1080,11 @@ fail_register:
elevator_exit(e);
q-elevator = old_elevator;
elv_register_queue(q);
+
+ spin_lock_irq(q-queue_lock);
clear_bit(QUEUE_FLAG_ELVSWITCH, q-queue_flags);
+ spin_unlock_irq(q-queue_lock);
+
return 0;
}
Index: linux-2.6/block/ll_rw_blk.c
===
--- linux-2.6.orig/block/ll_rw_blk.c
+++ linux-2.6/block/ll_rw_blk.c
@@ -1732,14 +1732,11 @@ void blk_sync_queue(struct request_queue
EXPORT_SYMBOL(blk_sync_queue);
/**
- * blk_run_queue - run a single device queue
+ * __blk_run_queue - run a single device queue. queue_lock must be held.
* @q: The queue to run
*/
-void blk_run_queue(struct request_queue *q)
+void __blk_run_queue(struct request_queue *q)
{
- unsigned long flags;
-
- spin_lock_irqsave(q-queue_lock, flags);
blk_remove_plug(q);
/*
@@ -1755,7 +1752,19 @@ void blk_run_queue(struct request_queue
kblockd_schedule_work(q-unplug_work);
}
}
+}
+EXPORT_SYMBOL(__blk_run_queue);
+/**
+ * blk_run_queue - run a single device queue
+ * @q: The queue to run
+ */
+void blk_run_queue(struct request_queue *q)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(q-queue_lock, flags);
+ __blk_run_queue(q);
spin_unlock_irqrestore(q-queue_lock, flags);
}
EXPORT_SYMBOL(blk_run_queue);
@@ -1804,7 +1813,9 @@ EXPORT_SYMBOL(blk_put_queue);
void blk_cleanup_queue(struct request_queue * q)
{
mutex_lock(q-sysfs_lock);
+ spin_lock_irq(q-queue_lock);
set_bit(QUEUE_FLAG_DEAD, q-queue_flags);
+ spin_unlock_irq(q-queue_lock);
mutex_unlock(q-sysfs_lock);
if (q-elevator)
Index: linux-2.6/drivers/scsi/scsi_lib.c
===
--- linux-2.6.orig/drivers/scsi/scsi_lib.c
+++ linux-2.6/drivers/scsi/scsi_lib.c
@@ -532,6 +532,9 @@ static void scsi_run_queue(struct reques
!shost-host_blocked !shost-host_self_blocked
!((shost-can_queue 0)
(shost-host_busy = shost-can_queue))) {
+
+ int flagset;
+
/*
* As long as shost is accepting commands and we have
* starved queues, call blk_run_queue. scsi_request_fn
@@ -547,17 +550,17 @@ static void scsi_run_queue(struct reques
list_del_init(sdev-starved_entry);
spin_unlock_irqrestore(shost-host_lock, flags);
-
- if (test_bit(QUEUE_FLAG_REENTER, q-queue_flags)
- !test_and_set_bit(QUEUE_FLAG_REENTER,
- sdev-request_queue-queue_flags)) {
- blk_run_queue(sdev-request_queue);
+ spin_lock(sdev-request_queue-queue_lock);
+ flagset = test_bit(QUEUE_FLAG_REENTER, q-queue_flags)
+ !test_and_set_bit(QUEUE_FLAG_REENTER,
+ sdev-request_queue-queue_flags);
+ __blk_run_queue(sdev-request_queue);
+ if (flagset)
clear_bit(QUEUE_FLAG_REENTER,
- sdev-request_queue-queue_flags);
- } else
- blk_run_queue(sdev-request_queue);
+ sdev-request_queue-queue_flags);
+ spin_unlock(sdev-request_queue-queue_lock);
- spin_lock_irqsave(shost-host_lock, flags);
+ spin_lock(shost-host_lock);
if (unlikely(!list_empty(sdev-starved_entry)))
/*
* sdev lost a race, and was put back on the
Index: linux-2.6/drivers/block/loop.c
