Re: [PATCH] add bsg queue resize
On Wed, Jan 24 2007, FUJITA Tomonori wrote: From: Jens Axboe [EMAIL PROTECTED] Subject: Re: [PATCH] add bsg queue resize Date: Tue, 23 Jan 2007 16:23:49 +0100 On Tue, Jan 23 2007, Jens Axboe wrote: On Sat, Jan 20 2007, FUJITA Tomonori wrote: This enables bsg to resize the queue depth via SG_SET_COMMAND_Q. bsg_command structures are allocated via mempool because the previous way to use contiguous memory makes it difficult to resize the queue depth when a bsg_device has outstanding commands. Overall the patch looks fine. I don't think we need a mempool though, and allocations could just use GFP_USER from the user invoked queuing paths. Just make it GFP_USER, we can always extend the bsg_alloc_command() to take a gfp_t argument as well If you get rid of the mempool, then resizing is simply just adjusting bd-max_queue. Like so. Thanks. I thought that pre-allocating bsg_command structures would be nice. But it doesn't matter much for me. Probably not very useful in the end, we need to allocate some other structures for IO anyway. One minor comment is that we could simplify __bsg_alloc_command failpath a bit? Yep, applied. Another thing that needs cleaning up for the allocation is the whole bsg_io_schedule() stuff. -- Jens Axboe - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add bsg queue resize
On Sat, Jan 20 2007, FUJITA Tomonori wrote: This enables bsg to resize the queue depth via SG_SET_COMMAND_Q. bsg_command structures are allocated via mempool because the previous way to use contiguous memory makes it difficult to resize the queue depth when a bsg_device has outstanding commands. Overall the patch looks fine. I don't think we need a mempool though, and allocations could just use GFP_USER from the user invoked queuing paths. Just make it GFP_USER, we can always extend the bsg_alloc_command() to take a gfp_t argument as well If you get rid of the mempool, then resizing is simply just adjusting bd-max_queue. -- Jens Axboe - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add bsg queue resize
From: Jens Axboe [EMAIL PROTECTED]
Subject: Re: [PATCH] add bsg queue resize
Date: Tue, 23 Jan 2007 16:23:49 +0100
On Tue, Jan 23 2007, Jens Axboe wrote:
On Sat, Jan 20 2007, FUJITA Tomonori wrote:
This enables bsg to resize the queue depth via
SG_SET_COMMAND_Q. bsg_command structures are allocated via mempool
because the previous way to use contiguous memory makes it difficult
to resize the queue depth when a bsg_device has outstanding commands.
Overall the patch looks fine. I don't think we need a mempool though,
and allocations could just use GFP_USER from the user invoked queuing
paths. Just make it GFP_USER, we can always extend the
bsg_alloc_command() to take a gfp_t argument as well If you get rid of
the mempool, then resizing is simply just adjusting bd-max_queue.
Like so.
Thanks. I thought that pre-allocating bsg_command structures would be
nice. But it doesn't matter much for me.
One minor comment is that we could simplify __bsg_alloc_command
failpath a bit?
---
From 6cebe0e87adc90ba50067f017c52a233fb9262d6 Mon Sep 17 00:00:00 2001
From: FUJITA Tomonori [EMAIL PROTECTED]
Date: Wed, 24 Jan 2007 11:08:47 +0900
Subject: [PATCH] bsg: simplify __bsg_alloc_command failpath
Signed-off-by: FUJITA Tomonori [EMAIL PROTECTED]
---
block/bsg.c |5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/block/bsg.c b/block/bsg.c
index e97e3ec..c85d961 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -128,7 +128,8 @@ static struct bsg_command *__bsg_alloc_command(struct
bsg_device *bd)
bc = kmem_cache_alloc(bsg_cmd_cachep, GFP_USER);
if (unlikely(!bc)) {
spin_lock_irq(bd-lock);
- goto alloc_fail;
+ bd-queued_cmds--;
+ goto out;
}
memset(bc, 0, sizeof(*bc));
@@ -136,8 +137,6 @@ static struct bsg_command *__bsg_alloc_command(struct
bsg_device *bd)
INIT_LIST_HEAD(bc-list);
dprintk(%s: returning free cmd %p\n, bd-name, bc);
return bc;
-alloc_fail:
- bd-queued_cmds--;
out:
spin_unlock_irq(bd-lock);
return bc;
--
1.4.4.3
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add bsg queue resize
On Sat, Jan 20 2007, FUJITA Tomonori wrote: This enables bsg to resize the queue depth via SG_SET_COMMAND_Q. bsg_command structures are allocated via mempool because the previous way to use contiguous memory makes it difficult to resize the queue depth when a bsg_device has outstanding commands. You work quickly! I'll give this a review during the flight back, if everything is alright I'll merge it up. Thanks! -- Jens Axboe - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] add bsg queue resize
This enables bsg to resize the queue depth via
SG_SET_COMMAND_Q. bsg_command structures are allocated via mempool
because the previous way to use contiguous memory makes it difficult
to resize the queue depth when a bsg_device has outstanding commands.
Signed-off-by: FUJITA Tomonori [EMAIL PROTECTED]
---
block/bsg.c | 112 +++---
1 files changed, 45 insertions(+), 67 deletions(-)
diff --git a/block/bsg.c b/block/bsg.c
index 9d77a0c..af46f54 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -33,8 +33,6 @@ #include scsi/sg.h
static char bsg_version[] = block layer sg (bsg) 0.4;
-struct bsg_command;
-
struct bsg_device {
struct gendisk *disk;
request_queue_t *queue;
@@ -46,8 +44,7 @@ struct bsg_device {
int minor;
int queued_cmds;
int done_cmds;
- unsigned long *cmd_bitmap;
- struct bsg_command *cmd_map;
+ mempool_t *bsg_cmd_q;
wait_queue_head_t wq_done;
wait_queue_head_t wq_free;
char name[BDEVNAME_SIZE];
@@ -55,19 +52,26 @@ struct bsg_device {
unsigned long flags;
};
+/*
+ * our internal command type
+ */
+struct bsg_command {
+ struct bsg_device *bd;
+ struct list_head list;
+ struct request *rq;
+ struct bio *bio;
+ int err;
+ struct sg_io_v4 hdr;
+ struct sg_io_v4 __user *uhdr;
+ char sense[SCSI_SENSE_BUFFERSIZE];
+};
+
enum {
BSG_F_BLOCK = 1,
BSG_F_WRITE_PERM= 2,
};
-/*
- * command allocation bitmap defines
- */
-#define BSG_CMDS_PAGE_ORDER(1)
-#define BSG_CMDS_PER_LONG (sizeof(unsigned long) * 8)
-#define BSG_CMDS_MASK (BSG_CMDS_PER_LONG - 1)
-#define BSG_CMDS_BYTES (PAGE_SIZE * (1 BSG_CMDS_PAGE_ORDER))
-#define BSG_CMDS (BSG_CMDS_BYTES / sizeof(struct bsg_command))
+#define BSG_DEFAULT_CMDS (64)
#undef BSG_DEBUG
@@ -94,31 +98,18 @@ static struct hlist_head bsg_device_list
static struct class *bsg_class;
static LIST_HEAD(bsg_class_list);
-/*
- * our internal command type
- */
-struct bsg_command {
- struct bsg_device *bd;
- struct list_head list;
- struct request *rq;
- struct bio *bio;
- int err;
- struct sg_io_v4 hdr;
- struct sg_io_v4 __user *uhdr;
- char sense[SCSI_SENSE_BUFFERSIZE];
-};
+static struct kmem_cache *bsg_cmd_cachep;
static void bsg_free_command(struct bsg_command *bc)
{
struct bsg_device *bd = bc-bd;
- unsigned long bitnr = bc - bd-cmd_map;
unsigned long flags;
- dprintk(%s: command bit offset %lu\n, bd-name, bitnr);
+ dprintk(%s: command free %p\n, bd-name, bc);
+ mempool_free(bc, bd-bsg_cmd_q);
spin_lock_irqsave(bd-lock, flags);
bd-queued_cmds--;
- __clear_bit(bitnr, bd-cmd_bitmap);
spin_unlock_irqrestore(bd-lock, flags);
wake_up(bd-wq_free);
@@ -127,29 +118,18 @@ static void bsg_free_command(struct bsg_
static struct bsg_command *__bsg_alloc_command(struct bsg_device *bd)
{
struct bsg_command *bc = NULL;
- unsigned long *map;
- int free_nr;
spin_lock_irq(bd-lock);
-
if (bd-queued_cmds = bd-max_queue)
goto out;
-
- for (free_nr = 0, map = bd-cmd_bitmap; *map == ~0UL; map++)
- free_nr += BSG_CMDS_PER_LONG;
-
- BUG_ON(*map == ~0UL);
-
bd-queued_cmds++;
- free_nr += ffz(*map);
- __set_bit(free_nr, bd-cmd_bitmap);
spin_unlock_irq(bd-lock);
- bc = bd-cmd_map + free_nr;
+ bc = mempool_alloc(bd-bsg_cmd_q, GFP_NOWAIT);
memset(bc, 0, sizeof(*bc));
bc-bd = bd;
INIT_LIST_HEAD(bc-list);
- dprintk(%s: returning free cmd %p (bit %d)\n, bd-name, bc, free_nr);
+ dprintk(%s: returning free cmd %p\n, bd-name, bc);
return bc;
out:
dprintk(%s: failed (depth %d)\n, bd-name, bd-queued_cmds);
@@ -356,8 +336,8 @@ static void bsg_rq_end_io(struct request
struct bsg_device *bd = bc-bd;
unsigned long flags;
- dprintk(%s: finished rq %p bc %p, bio %p offset %Zd stat %d\n,
- bd-name, rq, bc, bc-bio, bc - bd-cmd_map, uptodate);
+ dprintk(%s: finished rq %p bc %p, bio %p stat %d\n,
+ bd-name, rq, bc, bc-bio, uptodate);
bc-hdr.duration = jiffies_to_msecs(jiffies - bc-hdr.duration);
@@ -705,19 +685,13 @@ bsg_write(struct file *file, const char
static void bsg_free_device(struct bsg_device *bd)
{
- if (bd-cmd_map)
- free_pages((unsigned long) bd-cmd_map, BSG_CMDS_PAGE_ORDER);
-
- kfree(bd-cmd_bitmap);
+ mempool_destroy(bd-bsg_cmd_q);
kfree(bd);
}
static struct bsg_device *bsg_alloc_device(void)
{
- struct bsg_command *cmd_map;
- unsigned long *cmd_bitmap;
struct bsg_device *bd;
- int bits;
bd = kzalloc(sizeof(struct bsg_device), GFP_KERNEL);
if
