Skip to content

Commit 9d89606

Browse files
mikechristiemstsirkin
authored andcommitted
vhost-scsi: Reduce response iov mem use
We have to save N iov entries to copy the virtio_scsi_cmd_resp struct back to the guest's buffer. The difficulty is that we can't assume the virtio_scsi_cmd_resp will be in 1 iov because older virtio specs allowed you to break it up. The worst case is that the guest was doing something like breaking up the virtio_scsi_cmd_resp struct into 108 (the struct is 108 bytes) byte sized vecs like: iov[0].iov_base = ((unsigned char *)virtio_scsi_cmd_resp)[0] iov[0].iov_len = 1 iov[1].iov_base = ((unsigned char *)virtio_scsi_cmd_resp)[1] iov[1].iov_len = 1 .... iov[107].iov_base = ((unsigned char *)virtio_scsi_cmd_resp)[107] iov[1].iov_len = 1 Right now we allocate UIO_MAXIOV vecs which is 1024 and so for a small device with just 1 queue and 128 commands per queue, we are wasting 1.8 MB = (1024 current entries - 108) * 16 bytes per entry * 128 cmds The most common case is going to be where the initiator puts the entire virtio_scsi_cmd_resp in the first iov and does not split it. We've always done it this way for Linux and the windows driver looks like it's always done the same. It's highly unlikely anyone has ever split the response and if they did it might just be where they have the sense in a second iov but that doesn't seem likely as well. So to optimize for the common implementation, this has us only pre-allocate the single iovec. If we do hit the split up response case this has us allocate the needed iovec when needed. Signed-off-by: Mike Christie <michael.christie@oracle.com> Message-Id: <20241203191705.19431-9-michael.christie@oracle.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
1 parent fd47976 commit 9d89606

File tree

1 file changed

+60
-22
lines changed

1 file changed

+60
-22
lines changed

drivers/vhost/scsi.c

Lines changed: 60 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,11 @@
4545
#define VHOST_SCSI_PREALLOC_SGLS 2048
4646
#define VHOST_SCSI_PREALLOC_UPAGES 2048
4747
#define VHOST_SCSI_PREALLOC_PROT_SGLS 2048
48+
/*
49+
* For the legacy descriptor case we allocate an iov per byte in the
50+
* virtio_scsi_cmd_resp struct.
51+
*/
52+
#define VHOST_SCSI_MAX_RESP_IOVS sizeof(struct virtio_scsi_cmd_resp)
4853

4954
static unsigned int vhost_scsi_inline_sg_cnt = VHOST_SCSI_PREALLOC_SGLS;
5055

@@ -106,8 +111,6 @@ struct vhost_scsi_inflight {
106111
struct vhost_scsi_cmd {
107112
/* Descriptor from vhost_get_vq_desc() for virt_queue segment */
108113
int tvc_vq_desc;
109-
/* virtio-scsi response incoming iovecs */
110-
int tvc_in_iovs;
111114
/* The number of scatterlists associated with this cmd */
112115
u32 tvc_sgl_count;
113116
u32 tvc_prot_sgl_count;
@@ -118,8 +121,12 @@ struct vhost_scsi_cmd {
118121
struct sg_table table;
119122
struct scatterlist *prot_sgl;
120123
struct sg_table prot_table;
121-
/* Pointer to response header iovec */
122-
struct iovec *tvc_resp_iov;
124+
/* Fast path response header iovec used when only one vec is needed */
125+
struct iovec tvc_resp_iov;
126+
/* Number of iovs for response */
127+
unsigned int tvc_resp_iovs_cnt;
128+
/* Pointer to response header iovecs if more than one is needed */
129+
struct iovec *tvc_resp_iovs;
123130
/* Pointer to vhost_virtqueue for the cmd */
124131
struct vhost_virtqueue *tvc_vq;
125132
/* The TCM I/O descriptor that is accessed via container_of() */
@@ -391,6 +398,8 @@ static void vhost_scsi_release_cmd_res(struct se_cmd *se_cmd)
391398
sg_free_table_chained(&tv_cmd->prot_table, vs->inline_sg_cnt);
392399
}
393400

401+
if (tv_cmd->tvc_resp_iovs != &tv_cmd->tvc_resp_iov)
402+
kfree(tv_cmd->tvc_resp_iovs);
394403
sbitmap_clear_bit(&svq->scsi_tags, se_cmd->map_tag);
395404
vhost_scsi_put_inflight(inflight);
396405
}
@@ -638,8 +647,8 @@ static void vhost_scsi_complete_cmd_work(struct vhost_work *work)
638647
se_cmd->scsi_sense_length);
639648
}
640649

641-
iov_iter_init(&iov_iter, ITER_DEST, cmd->tvc_resp_iov,
642-
cmd->tvc_in_iovs, sizeof(v_rsp));
650+
iov_iter_init(&iov_iter, ITER_DEST, cmd->tvc_resp_iovs,
651+
cmd->tvc_resp_iovs_cnt, sizeof(v_rsp));
643652
ret = copy_to_iter(&v_rsp, sizeof(v_rsp), &iov_iter);
644653
if (likely(ret == sizeof(v_rsp))) {
645654
signal = true;
@@ -662,7 +671,6 @@ vhost_scsi_get_cmd(struct vhost_virtqueue *vq, u64 scsi_tag)
662671
struct vhost_scsi_virtqueue, vq);
663672
struct vhost_scsi_cmd *cmd;
664673
struct scatterlist *sgl, *prot_sgl;
665-
struct iovec *tvc_resp_iov;
666674
int tag;
667675

668676
tag = sbitmap_get(&svq->scsi_tags);
@@ -674,13 +682,11 @@ vhost_scsi_get_cmd(struct vhost_virtqueue *vq, u64 scsi_tag)
674682
cmd = &svq->scsi_cmds[tag];
675683
sgl = cmd->sgl;
676684
prot_sgl = cmd->prot_sgl;
677-
tvc_resp_iov = cmd->tvc_resp_iov;
678685
memset(cmd, 0, sizeof(*cmd));
679686
cmd->sgl = sgl;
680687
cmd->prot_sgl = prot_sgl;
681688
cmd->tvc_se_cmd.map_tag = tag;
682689
cmd->inflight = vhost_scsi_get_inflight(vq);
683-
cmd->tvc_resp_iov = tvc_resp_iov;
684690

685691
return cmd;
686692
}
@@ -1124,6 +1130,43 @@ vhost_scsi_get_req(struct vhost_virtqueue *vq, struct vhost_scsi_ctx *vc,
11241130
return ret;
11251131
}
11261132

1133+
static int
1134+
vhost_scsi_setup_resp_iovs(struct vhost_scsi_cmd *cmd, struct iovec *in_iovs,
1135+
unsigned int in_iovs_cnt)
1136+
{
1137+
int i, cnt;
1138+
1139+
if (!in_iovs_cnt)
1140+
return 0;
1141+
/*
1142+
* Initiator's normally just put the virtio_scsi_cmd_resp in the first
1143+
* iov, but just in case they wedged in some data with it we check for
1144+
* greater than or equal to the response struct.
1145+
*/
1146+
if (in_iovs[0].iov_len >= sizeof(struct virtio_scsi_cmd_resp)) {
1147+
cmd->tvc_resp_iovs = &cmd->tvc_resp_iov;
1148+
cmd->tvc_resp_iovs_cnt = 1;
1149+
} else {
1150+
/*
1151+
* Legacy descriptor layouts didn't specify that we must put
1152+
* the entire response in one iov. Worst case we have a
1153+
* iov per byte.
1154+
*/
1155+
cnt = min(VHOST_SCSI_MAX_RESP_IOVS, in_iovs_cnt);
1156+
cmd->tvc_resp_iovs = kcalloc(cnt, sizeof(struct iovec),
1157+
GFP_KERNEL);
1158+
if (!cmd->tvc_resp_iovs)
1159+
return -ENOMEM;
1160+
1161+
cmd->tvc_resp_iovs_cnt = cnt;
1162+
}
1163+
1164+
for (i = 0; i < cmd->tvc_resp_iovs_cnt; i++)
1165+
cmd->tvc_resp_iovs[i] = in_iovs[i];
1166+
1167+
return 0;
1168+
}
1169+
11271170
static u16 vhost_buf_to_lun(u8 *lun_buf)
11281171
{
11291172
return ((lun_buf[2] << 8) | lun_buf[3]) & 0x3FFF;
@@ -1141,7 +1184,7 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
11411184
struct iov_iter in_iter, prot_iter, data_iter;
11421185
u64 tag;
11431186
u32 exp_data_len, data_direction;
1144-
int ret, prot_bytes, i, c = 0;
1187+
int ret, prot_bytes, c = 0;
11451188
u16 lun;
11461189
u8 task_attr;
11471190
bool t10_pi = vhost_has_feature(vq, VIRTIO_SCSI_F_T10_PI);
@@ -1303,9 +1346,13 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
13031346
goto err;
13041347
}
13051348
cmd->tvc_vq = vq;
1306-
for (i = 0; i < vc.in ; i++)
1307-
cmd->tvc_resp_iov[i] = vq->iov[vc.out + i];
1308-
cmd->tvc_in_iovs = vc.in;
1349+
1350+
ret = vhost_scsi_setup_resp_iovs(cmd, &vq->iov[vc.out], vc.in);
1351+
if (ret) {
1352+
vq_err(vq, "Failed to alloc recv iovs\n");
1353+
vhost_scsi_release_cmd_res(&cmd->tvc_se_cmd);
1354+
goto err;
1355+
}
13091356

13101357
pr_debug("vhost_scsi got command opcode: %#02x, lun: %d\n",
13111358
cdb[0], lun);
@@ -1686,7 +1733,6 @@ static void vhost_scsi_destroy_vq_cmds(struct vhost_virtqueue *vq)
16861733

16871734
kfree(tv_cmd->sgl);
16881735
kfree(tv_cmd->prot_sgl);
1689-
kfree(tv_cmd->tvc_resp_iov);
16901736
}
16911737

16921738
sbitmap_free(&svq->scsi_tags);
@@ -1735,14 +1781,6 @@ static int vhost_scsi_setup_vq_cmds(struct vhost_virtqueue *vq, int max_cmds)
17351781
}
17361782
}
17371783

1738-
tv_cmd->tvc_resp_iov = kcalloc(UIO_MAXIOV,
1739-
sizeof(struct iovec),
1740-
GFP_KERNEL);
1741-
if (!tv_cmd->tvc_resp_iov) {
1742-
pr_err("Unable to allocate tv_cmd->tvc_resp_iov\n");
1743-
goto out;
1744-
}
1745-
17461784
if (vhost_has_feature(vq, VIRTIO_SCSI_F_T10_PI) &&
17471785
vs->inline_sg_cnt) {
17481786
tv_cmd->prot_sgl = kcalloc(vs->inline_sg_cnt,

0 commit comments

Comments
 (0)