Skip to content

Commit

Permalink
virtio-blk: embed VirtQueueElement in VirtIOBlockReq
Browse files Browse the repository at this point in the history 
The memory allocation between hw/block/virtio-blk.c,
hw/block/dataplane/virtio-blk.c, and hw/virtio/dataplane/vring.c is
messy.  Structs are allocated in different files than they are freed in.
This is risky and makes memory leaks easier.

Embed VirtQueueElement in VirtIOBlockReq to reduce the amount of memory
allocation we need to juggle.  This also makes vring.c and virtio.c
slightly more similar.

Signed-off-by: Stefan Hajnoczi <[email protected]>
Signed-off-by: Kevin Wolf <[email protected]>
  • Loading branch information
@stefanhaRH @kevmw
stefanhaRH authored and kevmw committed Jul 14, 2014
1 parent 869d66a commit f897bf7
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 52 deletions.
29 changes: 14 additions & 15 deletions hw/block/dataplane/virtio-blk.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ static void complete_request_vring(VirtIOBlockReq *req, unsigned char status)
{
stb_p(&req->in->status, status);

vring_push(&req->dev->dataplane->vring, req->elem,
vring_push(&req->dev->dataplane->vring, &req->elem,
req->qiov.size + sizeof(*req->in));
notify_guest(req->dev->dataplane);
}
Expand All @@ -74,33 +74,32 @@ static void handle_notify(EventNotifier *e)
{
VirtIOBlockDataPlane *s = container_of(e, VirtIOBlockDataPlane,
host_notifier);

VirtQueueElement *elem;
VirtIOBlockReq *req;
int ret;
MultiReqBuffer mrb = {
.num_writes = 0,
};
VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);

event_notifier_test_and_clear(&s->host_notifier);
bdrv_io_plug(s->blk->conf.bs);
for (;;) {
MultiReqBuffer mrb = {
.num_writes = 0,
};
int ret;

/* Disable guest->host notifies to avoid unnecessary vmexits */
vring_disable_notification(s->vdev, &s->vring);

for (;;) {
ret = vring_pop(s->vdev, &s->vring, &elem);
VirtIOBlockReq *req = virtio_blk_alloc_request(vblk);

ret = vring_pop(s->vdev, &s->vring, &req->elem);
if (ret < 0) {
assert(elem == NULL);
virtio_blk_free_request(req);
break; /* no more requests */
}

trace_virtio_blk_data_plane_process_request(s, elem->out_num,
elem->in_num, elem->index);
trace_virtio_blk_data_plane_process_request(s, req->elem.out_num,
req->elem.in_num,
req->elem.index);

req = g_slice_new(VirtIOBlockReq);
req->dev = VIRTIO_BLK(s->vdev);
req->elem = elem;
virtio_blk_handle_request(req, &mrb);
}

Expand Down
46 changes: 22 additions & 24 deletions hw/block/virtio-blk.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,20 +29,18 @@
#include "hw/virtio/virtio-bus.h"
#include "hw/virtio/virtio-access.h"

static VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s)
VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s)
{
VirtIOBlockReq *req = g_slice_new(VirtIOBlockReq);
req->dev = s;
req->qiov.size = 0;
req->next = NULL;
req->elem = g_slice_new(VirtQueueElement);
return req;
}

static void virtio_blk_free_request(VirtIOBlockReq *req)
void virtio_blk_free_request(VirtIOBlockReq *req)
{
if (req) {
g_slice_free(VirtQueueElement, req->elem);
g_slice_free(VirtIOBlockReq, req);
}
}
Expand All @@ -56,7 +54,7 @@ static void virtio_blk_complete_request(VirtIOBlockReq *req,
trace_virtio_blk_req_complete(req, status);

stb_p(&req->in->status, status);
virtqueue_push(s->vq, req->elem, req->qiov.size + sizeof(*req->in));
virtqueue_push(s->vq, &req->elem, req->qiov.size + sizeof(*req->in));
virtio_notify(vdev, s->vq);
}

Expand Down Expand Up @@ -121,7 +119,7 @@ static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s)
{
VirtIOBlockReq *req = virtio_blk_alloc_request(s);

if (!virtqueue_pop(s->vq, req->elem)) {
if (!virtqueue_pop(s->vq, &req->elem)) {
virtio_blk_free_request(req);
return NULL;
}
Expand Down Expand Up @@ -254,7 +252,7 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
{
int status;

status = virtio_blk_handle_scsi_req(req->dev, req->elem);
status = virtio_blk_handle_scsi_req(req->dev, &req->elem);
virtio_blk_req_complete(req, status);
virtio_blk_free_request(req);
}
Expand Down Expand Up @@ -351,12 +349,12 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req)
void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
{
uint32_t type;
struct iovec *in_iov = req->elem->in_sg;
struct iovec *iov = req->elem->out_sg;
unsigned in_num = req->elem->in_num;
unsigned out_num = req->elem->out_num;
struct iovec *in_iov = req->elem.in_sg;
struct iovec *iov = req->elem.out_sg;
unsigned in_num = req->elem.in_num;
unsigned out_num = req->elem.out_num;

if (req->elem->out_num < 1 || req->elem->in_num < 1) {
if (req->elem.out_num < 1 || req->elem.in_num < 1) {
error_report("virtio-blk missing headers");
exit(1);
}
Expand Down Expand Up @@ -393,19 +391,19 @@ void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
* NB: per existing s/n string convention the string is
* terminated by '\0' only when shorter than buffer.
*/
strncpy(req->elem->in_sg[0].iov_base,
strncpy(req->elem.in_sg[0].iov_base,
s->blk.serial ? s->blk.serial : "",
MIN(req->elem->in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES));
MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES));
virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
virtio_blk_free_request(req);
} else if (type & VIRTIO_BLK_T_OUT) {
qemu_iovec_init_external(&req->qiov, &req->elem->out_sg[1],
req->elem->out_num - 1);
qemu_iovec_init_external(&req->qiov, &req->elem.out_sg[1],
req->elem.out_num - 1);
virtio_blk_handle_write(req, mrb);
} else if (type == VIRTIO_BLK_T_IN || type == VIRTIO_BLK_T_BARRIER) {
/* VIRTIO_BLK_T_IN is 0, so we can't just & it. */
qemu_iovec_init_external(&req->qiov, &req->elem->in_sg[0],
req->elem->in_num - 1);
qemu_iovec_init_external(&req->qiov, &req->elem.in_sg[0],
req->elem.in_num - 1);
virtio_blk_handle_read(req);
} else {
virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
Expand Down Expand Up @@ -629,7 +627,7 @@ static void virtio_blk_save_device(VirtIODevice *vdev, QEMUFile *f)

while (req) {
qemu_put_sbyte(f, 1);
qemu_put_buffer(f, (unsigned char *)req->elem,
qemu_put_buffer(f, (unsigned char *)&req->elem,
sizeof(VirtQueueElement));
req = req->next;
}
Expand All @@ -654,15 +652,15 @@ static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,

while (qemu_get_sbyte(f)) {
VirtIOBlockReq *req = virtio_blk_alloc_request(s);
qemu_get_buffer(f, (unsigned char *)req->elem,
qemu_get_buffer(f, (unsigned char *)&req->elem,
sizeof(VirtQueueElement));
req->next = s->rq;
s->rq = req;

virtqueue_map_sg(req->elem->in_sg, req->elem->in_addr,
req->elem->in_num, 1);
virtqueue_map_sg(req->elem->out_sg, req->elem->out_addr,
req->elem->out_num, 0);
virtqueue_map_sg(req->elem.in_sg, req->elem.in_addr,
req->elem.in_num, 1);
virtqueue_map_sg(req->elem.out_sg, req->elem.out_addr,
req->elem.out_num, 0);
}

return 0;
Expand Down
17 changes: 6 additions & 11 deletions hw/virtio/dataplane/vring.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,14 +301,16 @@ static void vring_unmap_element(VirtQueueElement *elem)
* Stolen from linux/drivers/vhost/vhost.c.
*/
int vring_pop(VirtIODevice *vdev, Vring *vring,
VirtQueueElement **p_elem)
VirtQueueElement *elem)
{
struct vring_desc desc;
unsigned int i, head, found = 0, num = vring->vr.num;
uint16_t avail_idx, last_avail_idx;
VirtQueueElement *elem = NULL;
int ret;

/* Initialize elem so it can be safely unmapped */
elem->in_num = elem->out_num = 0;

/* If there was a fatal error then refuse operation */
if (vring->broken) {
ret = -EFAULT;
Expand Down Expand Up @@ -340,10 +342,8 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
* the index we've seen. */
head = vring->vr.avail->ring[last_avail_idx % num];

elem = g_slice_new(VirtQueueElement);
elem->index = head;
elem->in_num = elem->out_num = 0;


/* If their number is silly, that's an error. */
if (unlikely(head >= num)) {
error_report("Guest says index %u > %u is available", head, num);
Expand Down Expand Up @@ -391,19 +391,14 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,

/* On success, increment avail index. */
vring->last_avail_idx++;
*p_elem = elem;
return head;

out:
assert(ret < 0);
if (ret == -EFAULT) {
vring->broken = true;
}
if (elem) {
vring_unmap_element(elem);
g_slice_free(VirtQueueElement, elem);
}
*p_elem = NULL;
vring_unmap_element(elem);
return ret;
}

Expand Down
2 changes: 1 addition & 1 deletion include/hw/virtio/dataplane/vring.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ void vring_teardown(Vring *vring, VirtIODevice *vdev, int n);
void vring_disable_notification(VirtIODevice *vdev, Vring *vring);
bool vring_enable_notification(VirtIODevice *vdev, Vring *vring);
bool vring_should_notify(VirtIODevice *vdev, Vring *vring);
int vring_pop(VirtIODevice *vdev, Vring *vring, VirtQueueElement **elem);
int vring_pop(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem);
void vring_push(Vring *vring, VirtQueueElement *elem, int len);

#endif /* VRING_H */
6 changes: 5 additions & 1 deletion include/hw/virtio/virtio-blk.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,14 +144,18 @@ typedef struct MultiReqBuffer {

typedef struct VirtIOBlockReq {
VirtIOBlock *dev;
VirtQueueElement *elem;
VirtQueueElement elem;
struct virtio_blk_inhdr *in;
struct virtio_blk_outhdr out;
QEMUIOVector qiov;
struct VirtIOBlockReq *next;
BlockAcctCookie acct;
} VirtIOBlockReq;

VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s);

void virtio_blk_free_request(VirtIOBlockReq *req);

int virtio_blk_handle_scsi_req(VirtIOBlock *blk,
VirtQueueElement *elem);

Expand Down

0 comments on commit f897bf7

Please sign in to comment.