[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 7/8] vdpa: Introduce cursors to vhost_vdpa_net_loadx()
|
From: |
Hawkins Jiawei |
|
Subject: |
Re: [PATCH v4 7/8] vdpa: Introduce cursors to vhost_vdpa_net_loadx() |
|
Date: |
Sun, 8 Oct 2023 10:03:37 +0800 |
在 2023/10/4 15:21, Eugenio Perez Martin 写道:
> On Tue, Aug 29, 2023 at 7:55 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
>>
>> This patch introduces two new arugments, `out_cursor`
>> and `in_cursor`, to vhost_vdpa_net_loadx(). Addtionally,
>> it includes a helper function
>> vhost_vdpa_net_load_cursor_reset() for resetting these
>> cursors.
>>
>> Furthermore, this patch refactors vhost_vdpa_net_load_cmd()
>> so that vhost_vdpa_net_load_cmd() prepares buffers
>> for the device using the cursors arguments, instead
>> of directly accesses `s->cvq_cmd_out_buffer` and
>> `s->status` fields.
>>
>> By making these change, next patches in this series
>> can refactor vhost_vdpa_net_load_cmd() directly to
>> iterate through the control commands shadow buffers,
>> allowing QEMU to send CVQ state load commands in parallel
>> at device startup.
>>
>> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
>> ---
>> v4:
>> - use `struct iovec` instead of `void **` as cursor
>> suggested by Eugenio
>> - add vhost_vdpa_net_load_cursor_reset() helper function
>> to reset the cursors
>> - refactor vhost_vdpa_net_load_cmd() to prepare buffers
>> by iov_copy() instead of accessing `in` and `out` directly
>> suggested by Eugenio
>>
>> v3:
>> https://lore.kernel.org/all/bf390934673f2b613359ea9d7ac6c89199c31384.1689748694.git.yin31149@gmail.com/
>>
>> net/vhost-vdpa.c | 114 ++++++++++++++++++++++++++++++++---------------
>> 1 file changed, 77 insertions(+), 37 deletions(-)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index d9b8b3cf6c..a71e8c9090 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -633,7 +633,22 @@ static uint16_t
>> vhost_vdpa_net_svq_available_slots(VhostVDPAState *s)
>> return vhost_svq_available_slots(svq);
>> }
>>
>> -static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class,
>> +static void vhost_vdpa_net_load_cursor_reset(VhostVDPAState *s,
>> + struct iovec *out_cursor,
>> + struct iovec *in_cursor)
>> +{
>> + /* reset the cursor of the output buffer for the device */
>> + out_cursor->iov_base = s->cvq_cmd_out_buffer;
>> + out_cursor->iov_len = vhost_vdpa_net_cvq_cmd_page_len();
>> +
>> + /* reset the cursor of the in buffer for the device */
>> + in_cursor->iov_base = s->status;
>> + in_cursor->iov_len = vhost_vdpa_net_cvq_cmd_page_len();
>> +}
>> +
>> +static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s,
>> + struct iovec *out_cursor,
>> + struct iovec *in_cursor, uint8_t
>> class,
>> uint8_t cmd, const struct iovec
>> *data_sg,
>> size_t data_num)
>> {
>> @@ -641,28 +656,25 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState
>> *s, uint8_t class,
>> .class = class,
>> .cmd = cmd,
>> };
>> - size_t data_size = iov_size(data_sg, data_num);
>> - /* Buffers for the device */
>> - const struct iovec out = {
>> - .iov_base = s->cvq_cmd_out_buffer,
>> - .iov_len = sizeof(ctrl) + data_size,
>> - };
>> - const struct iovec in = {
>> - .iov_base = s->status,
>> - .iov_len = sizeof(*s->status),
>> - };
>> + size_t data_size = iov_size(data_sg, data_num),
>> + cmd_size = sizeof(ctrl) + data_size;
>> + struct iovec out, in;
>> ssize_t r;
>>
>> assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl));
>> /* Each CVQ command has one out descriptor and one in descriptor */
>> assert(vhost_vdpa_net_svq_available_slots(s) >= 2);
>>
>> - /* pack the CVQ command header */
>> - memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl));
>> + /* Prepare the buffer for out descriptor for the device */
>> + iov_copy(&out, 1, out_cursor, 1, 0, cmd_size);
>
> I may be missing something here, but cmd_size should be the bytes
> available in "out", so we don't overrun it.
>
>> + /* Prepare the buffer for in descriptor for the device. */
>> + iov_copy(&in, 1, in_cursor, 1, 0, sizeof(*s->status));
>>
>
> Same here, although it is impossible for the moment to overrun it as
> all cmds only return one byte.
>
Here we just use iov_copy() to initialize the `out` and `in` variables,
something like:
/* extract the required buffer from the cursor for output */
out.iov_len = cmd_size;
out.iov_base = out_cursor->iov_base;
/* extract the required buffer from the cursor for input */
in.iov_len = sizeof(*s->status);
in.iov_base = in_cursor->iov_base;
I think iov_copy() can improve readability, what do you think?
>> + /* pack the CVQ command header */
>> + iov_from_buf(&out, 1, 0, &ctrl, sizeof(ctrl));
>> /* pack the CVQ command command-specific-data */
>> iov_to_buf(data_sg, data_num, 0,
>> - s->cvq_cmd_out_buffer + sizeof(ctrl), data_size);
>> + out.iov_base + sizeof(ctrl), data_size);
>
> Is it possible to replace this by:
> iov_to_buf(data_sg, data_num, sizeof(ctrl), out.iov_base, data_size)
>
> In other words, let iov_to_but handle the offset in the buffer instead
> of adding it manually.
It seems that iov_to_buf(const struct iovec *iov, const unsigned int
iov_cnt, size_t offset, void *buf, size_t bytes) is copying data
from `iov` starting at offset `offset` and with a length of `bytes`,
to buf starting at offset `0`.
Yet here we want to copy data from `data_sg` starting at offset `0`
and with a length of `data_size`, to `out.iov_base` starting at offset
`sizeof(ctrl)`, so I think we cannot replace this(Please correct me if I
am wrong).
Thanks!
>
> The rest looks good to me.
>
>>
>> r = vhost_vdpa_net_cvq_add(s, &out, 1, &in, 1);
>> if (unlikely(r < 0)) {
>> @@ -676,14 +688,17 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState
>> *s, uint8_t class,
>> return vhost_vdpa_net_svq_poll(s, 1);
>> }
>>
>> -static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
>> +static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n,
>> + struct iovec *out_cursor,
>> + struct iovec *in_cursor)
>> {
>> if (virtio_vdev_has_feature(&n->parent_obj,
>> VIRTIO_NET_F_CTRL_MAC_ADDR)) {
>> const struct iovec data = {
>> .iov_base = (void *)n->mac,
>> .iov_len = sizeof(n->mac),
>> };
>> - ssize_t dev_written = vhost_vdpa_net_load_cmd(s,
>> VIRTIO_NET_CTRL_MAC,
>> + ssize_t dev_written = vhost_vdpa_net_load_cmd(s, out_cursor,
>> in_cursor,
>> + VIRTIO_NET_CTRL_MAC,
>>
>> VIRTIO_NET_CTRL_MAC_ADDR_SET,
>> &data, 1);
>> if (unlikely(dev_written < 0)) {
>> @@ -735,7 +750,7 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
>> const VirtIONet *n)
>> .iov_len = mul_macs_size,
>> },
>> };
>> - ssize_t dev_written = vhost_vdpa_net_load_cmd(s,
>> + ssize_t dev_written = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>> VIRTIO_NET_CTRL_MAC,
>> VIRTIO_NET_CTRL_MAC_TABLE_SET,
>> data, ARRAY_SIZE(data));
>> @@ -750,7 +765,9 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
>> const VirtIONet *n)
>> }
>>
>> static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
>> - const VirtIONet *n)
>> + const VirtIONet *n,
>> + struct iovec *out_cursor,
>> + struct iovec *in_cursor)
>> {
>> struct virtio_net_ctrl_mq mq;
>> ssize_t dev_written;
>> @@ -764,7 +781,8 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
>> .iov_base = &mq,
>> .iov_len = sizeof(mq),
>> };
>> - dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_MQ,
>> + dev_written = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>> + VIRTIO_NET_CTRL_MQ,
>> VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
>> &data, 1);
>> if (unlikely(dev_written < 0)) {
>> @@ -778,7 +796,9 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
>> }
>>
>> static int vhost_vdpa_net_load_offloads(VhostVDPAState *s,
>> - const VirtIONet *n)
>> + const VirtIONet *n,
>> + struct iovec *out_cursor,
>> + struct iovec *in_cursor)
>> {
>> uint64_t offloads;
>> ssize_t dev_written;
>> @@ -809,7 +829,8 @@ static int vhost_vdpa_net_load_offloads(VhostVDPAState
>> *s,
>> .iov_base = &offloads,
>> .iov_len = sizeof(offloads),
>> };
>> - dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_GUEST_OFFLOADS,
>> + dev_written = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>> + VIRTIO_NET_CTRL_GUEST_OFFLOADS,
>>
>> VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET,
>> &data, 1);
>> if (unlikely(dev_written < 0)) {
>> @@ -823,6 +844,8 @@ static int vhost_vdpa_net_load_offloads(VhostVDPAState
>> *s,
>> }
>>
>> static int vhost_vdpa_net_load_rx_mode(VhostVDPAState *s,
>> + struct iovec *out_cursor,
>> + struct iovec *in_cursor,
>> uint8_t cmd,
>> uint8_t on)
>> {
>> @@ -832,7 +855,8 @@ static int vhost_vdpa_net_load_rx_mode(VhostVDPAState *s,
>> };
>> ssize_t dev_written;
>>
>> - dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_RX,
>> + dev_written = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>> + VIRTIO_NET_CTRL_RX,
>> cmd, &data, 1);
>> if (unlikely(dev_written < 0)) {
>> return dev_written;
>> @@ -845,7 +869,9 @@ static int vhost_vdpa_net_load_rx_mode(VhostVDPAState *s,
>> }
>>
>> static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>> - const VirtIONet *n)
>> + const VirtIONet *n,
>> + struct iovec *out_cursor,
>> + struct iovec *in_cursor)
>> {
>> ssize_t r;
>>
>> @@ -872,7 +898,8 @@ static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>> * configuration only at live migration.
>> */
>> if (!n->mac_table.uni_overflow && !n->promisc) {
>> - r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC, 0);
>> + r = vhost_vdpa_net_load_rx_mode(s, out_cursor, in_cursor,
>> + VIRTIO_NET_CTRL_RX_PROMISC, 0);
>> if (unlikely(r < 0)) {
>> return r;
>> }
>> @@ -896,7 +923,8 @@ static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>> * configuration only at live migration.
>> */
>> if (n->mac_table.multi_overflow || n->allmulti) {
>> - r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_ALLMULTI, 1);
>> + r = vhost_vdpa_net_load_rx_mode(s, out_cursor, in_cursor,
>> + VIRTIO_NET_CTRL_RX_ALLMULTI, 1);
>> if (unlikely(r < 0)) {
>> return r;
>> }
>> @@ -917,7 +945,8 @@ static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>> * configuration only at live migration.
>> */
>> if (n->alluni) {
>> - r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_ALLUNI, 1);
>> + r = vhost_vdpa_net_load_rx_mode(s, out_cursor, in_cursor,
>> + VIRTIO_NET_CTRL_RX_ALLUNI, 1);
>> if (r < 0) {
>> return r;
>> }
>> @@ -934,7 +963,8 @@ static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>> * configuration only at live migration.
>> */
>> if (n->nomulti) {
>> - r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_NOMULTI, 1);
>> + r = vhost_vdpa_net_load_rx_mode(s, out_cursor, in_cursor,
>> + VIRTIO_NET_CTRL_RX_NOMULTI, 1);
>> if (r < 0) {
>> return r;
>> }
>> @@ -951,7 +981,8 @@ static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>> * configuration only at live migration.
>> */
>> if (n->nouni) {
>> - r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_NOUNI, 1);
>> + r = vhost_vdpa_net_load_rx_mode(s, out_cursor, in_cursor,
>> + VIRTIO_NET_CTRL_RX_NOUNI, 1);
>> if (r < 0) {
>> return r;
>> }
>> @@ -968,7 +999,8 @@ static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>> * configuration only at live migration.
>> */
>> if (n->nobcast) {
>> - r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_NOBCAST, 1);
>> + r = vhost_vdpa_net_load_rx_mode(s, out_cursor, in_cursor,
>> + VIRTIO_NET_CTRL_RX_NOBCAST, 1);
>> if (r < 0) {
>> return r;
>> }
>> @@ -979,13 +1011,16 @@ static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>>
>> static int vhost_vdpa_net_load_single_vlan(VhostVDPAState *s,
>> const VirtIONet *n,
>> + struct iovec *out_cursor,
>> + struct iovec *in_cursor,
>> uint16_t vid)
>> {
>> const struct iovec data = {
>> .iov_base = &vid,
>> .iov_len = sizeof(vid),
>> };
>> - ssize_t dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_VLAN,
>> + ssize_t dev_written = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>> + VIRTIO_NET_CTRL_VLAN,
>> VIRTIO_NET_CTRL_VLAN_ADD,
>> &data, 1);
>> if (unlikely(dev_written < 0)) {
>> @@ -999,7 +1034,9 @@ static int
>> vhost_vdpa_net_load_single_vlan(VhostVDPAState *s,
>> }
>>
>> static int vhost_vdpa_net_load_vlan(VhostVDPAState *s,
>> - const VirtIONet *n)
>> + const VirtIONet *n,
>> + struct iovec *out_cursor,
>> + struct iovec *in_cursor)
>> {
>> int r;
>>
>> @@ -1010,7 +1047,8 @@ static int vhost_vdpa_net_load_vlan(VhostVDPAState *s,
>> for (int i = 0; i < MAX_VLAN >> 5; i++) {
>> for (int j = 0; n->vlans[i] && j <= 0x1f; j++) {
>> if (n->vlans[i] & (1U << j)) {
>> - r = vhost_vdpa_net_load_single_vlan(s, n, (i << 5) + j);
>> + r = vhost_vdpa_net_load_single_vlan(s, n, out_cursor,
>> + in_cursor, (i << 5) +
>> j);
>> if (unlikely(r != 0)) {
>> return r;
>> }
>> @@ -1027,6 +1065,7 @@ static int vhost_vdpa_net_load(NetClientState *nc)
>> struct vhost_vdpa *v = &s->vhost_vdpa;
>> const VirtIONet *n;
>> int r;
>> + struct iovec out_cursor, in_cursor;
>>
>> assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>>
>> @@ -1035,23 +1074,24 @@ static int vhost_vdpa_net_load(NetClientState *nc)
>> }
>>
>> n = VIRTIO_NET(v->dev->vdev);
>> - r = vhost_vdpa_net_load_mac(s, n);
>> + vhost_vdpa_net_load_cursor_reset(s, &out_cursor, &in_cursor);
>> + r = vhost_vdpa_net_load_mac(s, n, &out_cursor, &in_cursor);
>> if (unlikely(r < 0)) {
>> return r;
>> }
>> - r = vhost_vdpa_net_load_mq(s, n);
>> + r = vhost_vdpa_net_load_mq(s, n, &out_cursor, &in_cursor);
>> if (unlikely(r)) {
>> return r;
>> }
>> - r = vhost_vdpa_net_load_offloads(s, n);
>> + r = vhost_vdpa_net_load_offloads(s, n, &out_cursor, &in_cursor);
>> if (unlikely(r)) {
>> return r;
>> }
>> - r = vhost_vdpa_net_load_rx(s, n);
>> + r = vhost_vdpa_net_load_rx(s, n, &out_cursor, &in_cursor);
>> if (unlikely(r)) {
>> return r;
>> }
>> - r = vhost_vdpa_net_load_vlan(s, n);
>> + r = vhost_vdpa_net_load_vlan(s, n, &out_cursor, &in_cursor);
>> if (unlikely(r)) {
>> return r;
>> }
>> --
>> 2.25.1
>>
>