diff mbox

scsi: Implement alloc_req_iov callback

Message ID 20101122101536.2B09BF90AF@ochil.suse.de
State New
Headers show

Commit Message

Hannes Reinecke Nov. 22, 2010, 10:15 a.m. UTC 
Add callback to create a request with a predefined iovec.
This is required for drivers which can use the iovec
of a command directly.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 hw/scsi-disk.c    |   25 +++++++++++++++++++++----
 hw/scsi-generic.c |   44 ++++++++++++++++++++++++++++++++++----------
 hw/scsi.h         |    2 ++
 3 files changed, 57 insertions(+), 14 deletions(-)

Comments

Stefan Hajnoczi Nov. 22, 2010, 9:48 p.m. UTC | #1 
On Mon, Nov 22, 2010 at 10:15 AM, Hannes Reinecke <hare@suse.de> wrote:
Looks good.  If you send out another version of the patchset you might
like to fix this nitpick:

> +    if (!r->io_header.iovec_count) {
> +        if (r->buflen != r->req.cmd.xfer) {
> +            if (r->buf != NULL)
> +                qemu_free(r->buf);

qemu_free(NULL) is a nop so it's safe to drop the if (r->buf != NULL)
check and just use qemu_free(r->buf) unconditionally.  That's nice
since it also fixes the if statement without curly braces.

Stefan
Hannes Reinecke Nov. 23, 2010, 8:12 a.m. UTC | #2 
On 11/22/2010 10:48 PM, Stefan Hajnoczi wrote:
> On Mon, Nov 22, 2010 at 10:15 AM, Hannes Reinecke <hare@suse.de> wrote:
> Looks good.  If you send out another version of the patchset you might
> like to fix this nitpick:
> 
>> +    if (!r->io_header.iovec_count) {
>> +        if (r->buflen != r->req.cmd.xfer) {
>> +            if (r->buf != NULL)
>> +                qemu_free(r->buf);
> 
> qemu_free(NULL) is a nop so it's safe to drop the if (r->buf != NULL)
> check and just use qemu_free(r->buf) unconditionally.  That's nice
> since it also fixes the if statement without curly braces.
> 
Really?

qemu-malloc.c has:

void qemu_free(void *ptr)
{
    trace_qemu_free(ptr);
    free(ptr);
}


and 'free' doesn't normally do an error checking on the argument.
Am I missing something?

Cheers,

Hannes
Paolo Bonzini Nov. 23, 2010, 8:31 a.m. UTC | #3 
On 11/23/2010 09:12 AM, Hannes Reinecke wrote:
> qemu-malloc.c has:
>
> void qemu_free(void *ptr)
> {
>      trace_qemu_free(ptr);
>      free(ptr);
> }
>
>
> and 'free' doesn't normally do an error checking on the argument.
> Am I missing something?

It's not error checking: from free(3),

free() frees the memory space pointed to by ptr, which must have been 
returned by a previous call to malloc(), calloc() or realloc(). 
Otherwise, or if free(ptr) has already been called before, undefined 
behavior occurs. If ptr is NULL, no operation is performed.

Which means, that unless ptr is so often NULL that there is a measurable 
overhead from the call (unlikely in any case, not just this one) the 
"if" is actually going to be done by "free", and thus causing actually 
worse performance.

Not that man pages are always right, but in this case they agree with 
POSIX. :)

Paolo
diff mbox

Patch

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index d1b7f74..88a2f74 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -96,14 +96,30 @@  static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag,
     return req;
 }
 
+static SCSIRequest *scsi_new_request_iovec(SCSIDevice *d, uint32_t tag,
+        uint32_t lun, struct iovec *iov, int iov_num)
+{
+    SCSIRequest *req;
+    SCSIDiskReq *r;
+
+    req = scsi_req_alloc(sizeof(SCSIDiskReq), d, tag, lun);
+    r = DO_UPCAST(SCSIDiskReq, req, req);
+    r->iov = iov;
+    r->iov_num = iov_num;
+    r->iov_buf = NULL;
+    return req;
+}
+
 static void scsi_remove_request(SCSIRequest *req)
 {
     SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
 
-    qemu_vfree(r->iov);
-    r->iov = NULL;
-    qemu_vfree(r->iov_buf);
-    r->iov_buf = NULL;
+    if (r->iov_buf) {
+        qemu_vfree(r->iov);
+        r->iov = NULL;
+        qemu_vfree(r->iov_buf);
+        r->iov_buf = NULL;
+    }
     scsi_req_free(&r->req);
 }
 
@@ -1207,6 +1223,7 @@  static SCSIDeviceInfo scsi_disk_info = {
     .init         = scsi_disk_initfn,
     .destroy      = scsi_destroy,
     .alloc_req    = scsi_new_request,
+    .alloc_req_iov = scsi_new_request_iovec,
     .free_req     = scsi_remove_request,
     .send_command = scsi_send_command,
     .read_data    = scsi_read_data,
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 5c0f6ab..7d30115 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -107,6 +107,25 @@  static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun)
     return req;
 }
 
+static SCSIRequest *scsi_new_request_iovec(SCSIDevice *d, uint32_t tag,
+                                           uint32_t lun, struct iovec *iov, int iov_num)
+{
+    SCSIRequest *req;
+    SCSIGenericReq *r;
+    int i;
+
+    req = scsi_req_alloc(sizeof(SCSIGenericReq), d, tag, lun);
+    r = DO_UPCAST(SCSIGenericReq, req, req);
+    r->io_header.dxferp = iov;
+    r->io_header.iovec_count = iov_num;
+    r->io_header.dxfer_len = 0;
+    for (i = 0; i < iov_num; i++)
+        r->io_header.dxfer_len += iov[i].iov_len;
+    r->buf = NULL;
+    r->buflen = 0;
+    return req;
+}
+
 static void scsi_remove_request(SCSIRequest *req)
 {
     SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
@@ -179,8 +198,10 @@  static int execute_command(BlockDriverState *bdrv,
 
     r->io_header.interface_id = 'S';
     r->io_header.dxfer_direction = direction;
-    r->io_header.dxferp = r->buf;
-    r->io_header.dxfer_len = r->buflen;
+    if (r->buf) {
+        r->io_header.dxferp = r->buf;
+        r->io_header.dxfer_len = r->buflen;
+    }
     r->io_header.cmdp = r->req.cmd.buf;
     r->io_header.cmd_len = r->req.cmd.len;
     r->io_header.mx_sb_len = sizeof(s->sensebuf);
@@ -286,7 +307,7 @@  static int scsi_write_data(SCSIRequest *req)
 
     DPRINTF("scsi_write_data 0x%x\n", req->tag);
 
-    if (r->len == 0) {
+    if (r->len == 0 && r->io_header.dxfer_len == 0) {
         r->len = r->buflen;
         r->req.bus->complete(&r->req, SCSI_REASON_DATA, r->len);
         return 0;
@@ -380,14 +401,16 @@  static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
         return 0;
     }
 
-    if (r->buflen != r->req.cmd.xfer) {
-        if (r->buf != NULL)
-            qemu_free(r->buf);
-        r->buf = qemu_malloc(r->req.cmd.xfer);
-        r->buflen = r->req.cmd.xfer;
-    }
+    if (!r->io_header.iovec_count) {
+        if (r->buflen != r->req.cmd.xfer) {
+            if (r->buf != NULL)
+                qemu_free(r->buf);
+            r->buf = qemu_malloc(r->req.cmd.xfer);
+            r->buflen = r->req.cmd.xfer;
+        }
 
-    memset(r->buf, 0, r->buflen);
+        memset(r->buf, 0, r->buflen);
+    }
     r->len = r->req.cmd.xfer;
     if (r->req.cmd.mode == SCSI_XFER_TO_DEV) {
         r->len = 0;
@@ -560,6 +583,7 @@  static SCSIDeviceInfo scsi_generic_info = {
     .init         = scsi_generic_initfn,
     .destroy      = scsi_destroy,
     .alloc_req    = scsi_new_request,
+    .alloc_req_iov  = scsi_new_request_iovec,
     .free_req     = scsi_remove_request,
     .send_command = scsi_send_command,
     .read_data    = scsi_read_data,
diff --git a/hw/scsi.h b/hw/scsi.h
index 0c467d1..538ae54 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -72,6 +72,8 @@  struct SCSIDeviceInfo {
     scsi_qdev_initfn init;
     void (*destroy)(SCSIDevice *s);
     SCSIRequest *(*alloc_req)(SCSIDevice *s, uint32_t tag, uint32_t lun);
+    SCSIRequest *(*alloc_req_iov)(SCSIDevice *s, uint32_t tag, uint32_t lun,
+                                  struct iovec *iov, int iov_num);
     void (*free_req)(SCSIRequest *req);
     int32_t (*send_command)(SCSIRequest *req, uint8_t *buf);
     void (*read_data)(SCSIRequest *req);