diff mbox

[03/15] scsi: move request lists to QTAILQ.

Message ID 1258453071-3496-4-git-send-email-kraxel@redhat.com
State New
Headers show

Commit Message

Gerd Hoffmann Nov. 17, 2009, 10:17 a.m. UTC 
Changes:
 * Move from open-coded lists to QTAILQ macros.
 * Move the struct elements to the common data structures
   (SCSIDevice + SCSIRequest).
 * Fix request cleanup in the destroy callback.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/scsi-bus.c     |    1 +
 hw/scsi-disk.c    |   58 ++++++++++++++++++----------------------------
 hw/scsi-generic.c |   66 ++++++++++++++--------------------------------------
 hw/scsi.h         |    2 +
 4 files changed, 44 insertions(+), 83 deletions(-)

Comments

Christoph Hellwig Nov. 17, 2009, 11:15 a.m. UTC | #1 
On Tue, Nov 17, 2009 at 11:17:39AM +0100, Gerd Hoffmann wrote:
> Changes:
>  * Move from open-coded lists to QTAILQ macros.
>  * Move the struct elements to the common data structures
>    (SCSIDevice + SCSIRequest).
>  * Fix request cleanup in the destroy callback.

Perfect, the old, duplicated list handling was quite horrible.


Reviewed-by: Christoph Hellwig <hch@lst.de>

> +    QTAILQ_REMOVE(&r->req.dev->requests, &r->req, next);
> +    QTAILQ_INSERT_HEAD(&free_requests, &r->req, next);

Would be nice if we had a move from one head to another abstraction.  Or
just a copy of the linux list handling header :)
Paul Brook Nov. 17, 2009, 11:59 a.m. UTC | #2 
On Tuesday 17 November 2009, Gerd Hoffmann wrote:
> Changes:
>  * Move from open-coded lists to QTAILQ macros.
>  * Move the struct elements to the common data structures
>    (SCSIDevice + SCSIRequest).
>  * Fix request cleanup in the destroy callback.

This feels like the abstraction boundaries wrong. If the request chain fields 
are in common structures then I'd also expect the allocation and linking code 
to be common.

Paul
Gerd Hoffmann Nov. 17, 2009, 12:39 p.m. UTC | #3 
On 11/17/09 12:59, Paul Brook wrote:
> On Tuesday 17 November 2009, Gerd Hoffmann wrote:
>> Changes:
>>   * Move from open-coded lists to QTAILQ macros.
>>   * Move the struct elements to the common data structures
>>     (SCSIDevice + SCSIRequest).
>>   * Fix request cleanup in the destroy callback.
>
> This feels like the abstraction boundaries wrong. If the request chain fields
> are in common structures then I'd also expect the allocation and linking code
> to be common.

Isn't that easy because scsi-disk and scsi-generic keep a pool of unused 
request structures.  I didn't change that for now, although I've 
considered dropping it.  Not sure how important it is these days, malloc 
implementations don't do slow+stupid list walks any more ...

When the pools are gone we can easily move scsi_remove_request() to 
common code and have the common scsi_req_init() function handle 
allocation and linking too (and probably name it 'alloc' or 'get').

cheers,
   Gerd
Gerd Hoffmann Nov. 18, 2009, 9:44 a.m. UTC | #4 
On 11/17/09 13:39, Gerd Hoffmann wrote:
> Isn't that easy because scsi-disk and scsi-generic keep a pool of unused
> request structures. I didn't change that for now, although I've
> considered dropping it. Not sure how important it is these days, malloc
> implementations don't do slow+stupid list walks any more ...

No strong objections -> I'll go zap the pools and move more bits into 
common code.

cheers,
   Gerd
diff mbox

Patch

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 641db81..801922b 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -50,6 +50,7 @@  static int scsi_qdev_init(DeviceState *qdev, DeviceInfo *base)
     bus->devs[dev->id] = dev;
 
     dev->info = info;
+    QTAILQ_INIT(&dev->requests);
     rc = dev->info->init(dev);
     if (rc != 0) {
         bus->devs[dev->id] = NULL;
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 142d81d..2b13635 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -55,7 +55,6 @@  typedef struct SCSIDiskReq {
     uint32_t sector_count;
     struct iovec iov;
     QEMUIOVector qiov;
-    struct SCSIDiskReq *next;
     uint32_t status;
 } SCSIDiskReq;
 
@@ -63,7 +62,6 @@  struct SCSIDiskState
 {
     SCSIDevice qdev;
     DriveInfo *dinfo;
-    SCSIDiskReq *requests;
     /* The qemu block layer uses a fixed 512 byte sector size.
        This is the number of 512 byte blocks in a single scsi sector.  */
     int cluster_size;
@@ -74,16 +72,15 @@  struct SCSIDiskState
 };
 
 /* Global pool of SCSIRequest structures.  */
-static SCSIDiskReq *free_requests = NULL;
+static QTAILQ_HEAD(, SCSIRequest) free_requests = QTAILQ_HEAD_INITIALIZER(free_requests);
 
 static SCSIDiskReq *scsi_new_request(SCSIDevice *d, uint32_t tag)
 {
-    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
     SCSIDiskReq *r;
 
-    if (free_requests) {
-        r = free_requests;
-        free_requests = r->next;
+    if (!QTAILQ_EMPTY(&free_requests)) {
+        r = DO_UPCAST(SCSIDiskReq, req, QTAILQ_FIRST(&free_requests));
+        QTAILQ_REMOVE(&free_requests, &r->req, next);
     } else {
         r = qemu_malloc(sizeof(SCSIDiskReq));
         r->iov.iov_base = qemu_memalign(512, SCSI_DMA_BUF_SIZE);
@@ -96,41 +93,26 @@  static SCSIDiskReq *scsi_new_request(SCSIDevice *d, uint32_t tag)
     r->req.aiocb = NULL;
     r->status = 0;
 
-    r->next = s->requests;
-    s->requests = r;
+    QTAILQ_INSERT_TAIL(&d->requests, &r->req, next);
     return r;
 }
 
 static void scsi_remove_request(SCSIDiskReq *r)
 {
-    SCSIDiskReq *last;
-    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
-
-    if (s->requests == r) {
-        s->requests = r->next;
-    } else {
-        last = s->requests;
-        while (last && last->next != r)
-            last = last->next;
-        if (last) {
-            last->next = r->next;
-        } else {
-            BADF("Orphaned request\n");
-        }
-    }
-    r->next = free_requests;
-    free_requests = r;
+    QTAILQ_REMOVE(&r->req.dev->requests, &r->req, next);
+    QTAILQ_INSERT_HEAD(&free_requests, &r->req, next);
 }
 
 static SCSIDiskReq *scsi_find_request(SCSIDiskState *s, uint32_t tag)
 {
-    SCSIDiskReq *r;
+    SCSIRequest *req;
 
-    r = s->requests;
-    while (r && r->req.tag != tag)
-        r = r->next;
-
-    return r;
+    QTAILQ_FOREACH(req, &s->qdev.requests, next) {
+        if (req->tag == tag) {
+            return DO_UPCAST(SCSIDiskReq, req, req);
+        }
+    }
+    return NULL;
 }
 
 /* Helper function for command completion.  */
@@ -310,17 +292,18 @@  static int scsi_write_data(SCSIDevice *d, uint32_t tag)
 static void scsi_dma_restart_bh(void *opaque)
 {
     SCSIDiskState *s = opaque;
-    SCSIDiskReq *r = s->requests;
+    SCSIRequest *req;
+    SCSIDiskReq *r;
 
     qemu_bh_delete(s->bh);
     s->bh = NULL;
 
-    while (r) {
+    QTAILQ_FOREACH(req, &s->qdev.requests, next) {
+        r = DO_UPCAST(SCSIDiskReq, req, req);
         if (r->status & SCSI_REQ_STATUS_RETRY) {
             r->status &= ~SCSI_REQ_STATUS_RETRY;
             scsi_write_request(r); 
         }
-        r = r->next;
     }
 }
 
@@ -959,7 +942,12 @@  static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
 static void scsi_destroy(SCSIDevice *dev)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
+    SCSIDiskReq *r;
 
+    while (!QTAILQ_EMPTY(&s->qdev.requests)) {
+        r = DO_UPCAST(SCSIDiskReq, req, QTAILQ_FIRST(&s->qdev.requests));
+        scsi_remove_request(r);
+    }
     drive_uninit(s->dinfo);
 }
 
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 89f3080..5ca6840 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -56,7 +56,6 @@  typedef struct SCSIGenericState SCSIGenericState;
 
 typedef struct SCSIGenericReq {
     SCSIRequest req;
-    struct SCSIGenericReq *next;
     uint8_t cmd[SCSI_CMD_BUF_SIZE];
     int cmdlen;
     uint8_t *buf;
@@ -68,7 +67,6 @@  typedef struct SCSIGenericReq {
 struct SCSIGenericState
 {
     SCSIDevice qdev;
-    SCSIGenericReq *requests;
     DriveInfo *dinfo;
     int type;
     int blocksize;
@@ -79,16 +77,15 @@  struct SCSIGenericState
 };
 
 /* Global pool of SCSIGenericReq structures.  */
-static SCSIGenericReq *free_requests = NULL;
+static QTAILQ_HEAD(, SCSIRequest) free_requests = QTAILQ_HEAD_INITIALIZER(free_requests);
 
 static SCSIGenericReq *scsi_new_request(SCSIDevice *d, uint32_t tag)
 {
-    SCSIGenericState *s = DO_UPCAST(SCSIGenericState, qdev, d);
     SCSIGenericReq *r;
 
-    if (free_requests) {
-        r = free_requests;
-        free_requests = r->next;
+    if (!QTAILQ_EMPTY(&free_requests)) {
+        r = DO_UPCAST(SCSIGenericReq, req, QTAILQ_FIRST(&free_requests));
+        QTAILQ_REMOVE(&free_requests, &r->req, next);
     } else {
         r = qemu_malloc(sizeof(SCSIGenericReq));
         r->buf = NULL;
@@ -103,43 +100,26 @@  static SCSIGenericReq *scsi_new_request(SCSIDevice *d, uint32_t tag)
     r->len = 0;
     r->req.aiocb = NULL;
 
-    /* link */
-
-    r->next = s->requests;
-    s->requests = r;
+    QTAILQ_INSERT_TAIL(&d->requests, &r->req, next);
     return r;
 }
 
 static void scsi_remove_request(SCSIGenericReq *r)
 {
-    SCSIGenericReq *last;
-    SCSIGenericState *s = DO_UPCAST(SCSIGenericState, qdev, r->req.dev);
-
-    if (s->requests == r) {
-        s->requests = r->next;
-    } else {
-        last = s->requests;
-        while (last && last->next != r)
-            last = last->next;
-        if (last) {
-            last->next = r->next;
-        } else {
-            BADF("Orphaned request\n");
-        }
-    }
-    r->next = free_requests;
-    free_requests = r;
+    QTAILQ_REMOVE(&r->req.dev->requests, &r->req, next);
+    QTAILQ_INSERT_HEAD(&free_requests, &r->req, next);
 }
 
 static SCSIGenericReq *scsi_find_request(SCSIGenericState *s, uint32_t tag)
 {
-    SCSIGenericReq *r;
-
-    r = s->requests;
-    while (r && r->req.tag != tag)
-        r = r->next;
+    SCSIRequest *req;
 
-    return r;
+    QTAILQ_FOREACH(req, &s->qdev.requests, next) {
+        if (req->tag == tag) {
+            return DO_UPCAST(SCSIGenericReq, req, req);
+        }
+    }
+    return NULL;
 }
 
 /* Helper function for command completion.  */
@@ -653,22 +633,12 @@  static int get_stream_blocksize(BlockDriverState *bdrv)
 static void scsi_destroy(SCSIDevice *d)
 {
     SCSIGenericState *s = DO_UPCAST(SCSIGenericState, qdev, d);
-    SCSIGenericReq *r, *n;
-
-    r = s->requests;
-    while (r) {
-        n = r->next;
-        qemu_free(r);
-        r = n;
-    }
+    SCSIGenericReq *r;
 
-    r = free_requests;
-    while (r) {
-        n = r->next;
-        qemu_free(r);
-        r = n;
+    while (!QTAILQ_EMPTY(&s->qdev.requests)) {
+        r = DO_UPCAST(SCSIGenericReq, req, QTAILQ_FIRST(&s->qdev.requests));
+        scsi_remove_request(r);
     }
-
     drive_uninit(s->dinfo);
 }
 
diff --git a/hw/scsi.h b/hw/scsi.h
index 7906877..a9b846c 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -21,6 +21,7 @@  typedef struct SCSIRequest {
     SCSIDevice        *dev;
     uint32_t          tag;
     BlockDriverAIOCB  *aiocb;
+    QTAILQ_ENTRY(SCSIRequest) next;
 } SCSIRequest;
 
 struct SCSIDevice
@@ -28,6 +29,7 @@  struct SCSIDevice
     DeviceState qdev;
     uint32_t id;
     SCSIDeviceInfo *info;
+    QTAILQ_HEAD(, SCSIRequest) requests;
 };
 
 /* cdrom.c */