Re: [RFC v2 02/13] vdpa net: move iova tree creation from init to start

[Jason Wang <jasowang@redhat.com>]

在 2023/1/16 17:14, Eugenio Perez Martin 写道:
> On Mon, Jan 16, 2023 at 4:05 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> 在 2023/1/13 15:28, Eugenio Perez Martin 写道:
>>> On Fri, Jan 13, 2023 at 4:53 AM Jason Wang <jasowang@redhat.com> wrote:
>>>> On Fri, Jan 13, 2023 at 1:24 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>>>>> Only create iova_tree if and when it is needed.
>>>>>
>>>>> The cleanup keeps being responsability of last VQ but this change allows
>>>>> to merge both cleanup functions.
>>>>>
>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>> ---
>>>>>    net/vhost-vdpa.c | 101 +++++++++++++++++++++++++++++++++--------------
>>>>>    1 file changed, 71 insertions(+), 30 deletions(-)
>>>>>
>>>>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>>>>> index de5ed8ff22..75cca497c8 100644
>>>>> --- a/net/vhost-vdpa.c
>>>>> +++ b/net/vhost-vdpa.c
>>>>> @@ -178,13 +178,9 @@ err_init:
>>>>>    static void vhost_vdpa_cleanup(NetClientState *nc)
>>>>>    {
>>>>>        VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
>>>>> -    struct vhost_dev *dev = &s->vhost_net->dev;
>>>>>
>>>>>        qemu_vfree(s->cvq_cmd_out_buffer);
>>>>>        qemu_vfree(s->status);
>>>>> -    if (dev->vq_index + dev->nvqs == dev->vq_index_end) {
>>>>> -        g_clear_pointer(&s->vhost_vdpa.iova_tree, vhost_iova_tree_delete);
>>>>> -    }
>>>>>        if (s->vhost_net) {
>>>>>            vhost_net_cleanup(s->vhost_net);
>>>>>            g_free(s->vhost_net);
>>>>> @@ -234,10 +230,64 @@ static ssize_t vhost_vdpa_receive(NetClientState *nc, const uint8_t *buf,
>>>>>        return size;
>>>>>    }
>>>>>
>>>>> +/** From any vdpa net client, get the netclient of first queue pair */
>>>>> +static VhostVDPAState *vhost_vdpa_net_first_nc_vdpa(VhostVDPAState *s)
>>>>> +{
>>>>> +    NICState *nic = qemu_get_nic(s->nc.peer);
>>>>> +    NetClientState *nc0 = qemu_get_peer(nic->ncs, 0);
>>>>> +
>>>>> +    return DO_UPCAST(VhostVDPAState, nc, nc0);
>>>>> +}
>>>>> +
>>>>> +static void vhost_vdpa_net_data_start_first(VhostVDPAState *s)
>>>>> +{
>>>>> +    struct vhost_vdpa *v = &s->vhost_vdpa;
>>>>> +
>>>>> +    if (v->shadow_vqs_enabled) {
>>>>> +        v->iova_tree = vhost_iova_tree_new(v->iova_range.first,
>>>>> +                                           v->iova_range.last);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +static int vhost_vdpa_net_data_start(NetClientState *nc)
>>>>> +{
>>>>> +    VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
>>>>> +    struct vhost_vdpa *v = &s->vhost_vdpa;
>>>>> +
>>>>> +    assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>>>>> +
>>>>> +    if (v->index == 0) {
>>>>> +        vhost_vdpa_net_data_start_first(s);
>>>>> +        return 0;
>>>>> +    }
>>>>> +
>>>>> +    if (v->shadow_vqs_enabled) {
>>>>> +        VhostVDPAState *s0 = vhost_vdpa_net_first_nc_vdpa(s);
>>>>> +        v->iova_tree = s0->vhost_vdpa.iova_tree;
>>>>> +    }
>>>> It looks to me the logic here is somehow the same as
>>>> vhost_vdpa_net_cvq_start(), can we unify the them?
>>>>
>>> It depends on what you mean by unify :). But we can explore it for sure.
>>>
>>> We can call vhost_vdpa_net_data_start, but the steps to do if
>>> s0->vhost_vdpa.iova_tree == NULL are different. Data queues must do
>>> nothing, but CVQ must create a new iova tree.
>>>
>>> So one possibility is to convert this part of vhost_vdpa_net_cvq_start:
>>>       s0 = vhost_vdpa_net_first_nc_vdpa(s);
>>>       if (s0->vhost_vdpa.iova_tree) {
>>>           /* SVQ is already configured for all virtqueues */
>>>           v->iova_tree = s0->vhost_vdpa.iova_tree;
>>>       } else {
>>>           v->iova_tree = vhost_iova_tree_new(v->iova_range.first,
>>>                                              v->iova_range.last);
>>>       }
>>>
>>> into:
>>>       vhost_vdpa_net_data_start(nc);
>>>       if (!v->iova_tree) {
>>>           v->iova_tree = vhost_iova_tree_new(v->iova_range.first,
>>>                                              v->iova_range.last);
>>>       }
>>>
>>> I'm ok with the change but it's less clear in my opinion: it's not
>>> obvious that net_data_start is in charge of setting v->iova_tree to
>>> me.
>>
>> Ok.
>>
>>
>>> Another possibility is to abstract something like
>>> first_nc_iova_tree(), but we need to check more fields of s0 later
>>> (shadow_data) so I'm not sure about the benefit.
>>>
>>> Is that what you have in mind?
>>
>> Kind of, but I think we can leave the code as is.
>>
>> In the future, as discussed, we need to introduce something like a
>> parent or opaque structure for NetClientState structure, it can simply a
>> lot of things: we can have one same common parent for all queues, then
>> there's no need for the trick like first_nc_iova_tree() and other
>> similar tricks.
>>
> So we can ack this patch or you prefer to explore the change for the
> next series?


Let's let it for future.

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks


>
> Thanks!
>
>> Thanks
>>
>>> Thanks!
>>>
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static void vhost_vdpa_net_client_stop(NetClientState *nc)
>>>>> +{
>>>>> +    VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
>>>>> +    struct vhost_dev *dev;
>>>>> +
>>>>> +    assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>>>>> +
>>>>> +    dev = s->vhost_vdpa.dev;
>>>>> +    if (dev->vq_index + dev->nvqs == dev->vq_index_end) {
>>>>> +        g_clear_pointer(&s->vhost_vdpa.iova_tree, vhost_iova_tree_delete);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>    static NetClientInfo net_vhost_vdpa_info = {
>>>>>            .type = NET_CLIENT_DRIVER_VHOST_VDPA,
>>>>>            .size = sizeof(VhostVDPAState),
>>>>>            .receive = vhost_vdpa_receive,
>>>>> +        .start = vhost_vdpa_net_data_start,
>>>>> +        .stop = vhost_vdpa_net_client_stop,
>>>>>            .cleanup = vhost_vdpa_cleanup,
>>>>>            .has_vnet_hdr = vhost_vdpa_has_vnet_hdr,
>>>>>            .has_ufo = vhost_vdpa_has_ufo,
>>>>> @@ -351,7 +401,7 @@ dma_map_err:
>>>>>
>>>>>    static int vhost_vdpa_net_cvq_start(NetClientState *nc)
>>>>>    {
>>>>> -    VhostVDPAState *s;
>>>>> +    VhostVDPAState *s, *s0;
>>>>>        struct vhost_vdpa *v;
>>>>>        uint64_t backend_features;
>>>>>        int64_t cvq_group;
>>>>> @@ -415,8 +465,6 @@ static int vhost_vdpa_net_cvq_start(NetClientState *nc)
>>>>>            return r;
>>>>>        }
>>>>>
>>>>> -    v->iova_tree = vhost_iova_tree_new(v->iova_range.first,
>>>>> -                                       v->iova_range.last);
>>>>>        v->shadow_vqs_enabled = true;
>>>>>        s->vhost_vdpa.address_space_id = VHOST_VDPA_NET_CVQ_ASID;
>>>>>
>>>>> @@ -425,6 +473,15 @@ out:
>>>>>            return 0;
>>>>>        }
>>>>>
>>>>> +    s0 = vhost_vdpa_net_first_nc_vdpa(s);
>>>>> +    if (s0->vhost_vdpa.iova_tree) {
>>>>> +        /* SVQ is already configured for all virtqueues */
>>>>> +        v->iova_tree = s0->vhost_vdpa.iova_tree;
>>>>> +    } else {
>>>>> +        v->iova_tree = vhost_iova_tree_new(v->iova_range.first,
>>>>> +                                           v->iova_range.last);
>>>>> +    }
>>>>> +
>>>>>        r = vhost_vdpa_cvq_map_buf(&s->vhost_vdpa, s->cvq_cmd_out_buffer,
>>>>>                                   vhost_vdpa_net_cvq_cmd_page_len(), false);
>>>>>        if (unlikely(r < 0)) {
>>>>> @@ -449,15 +506,9 @@ static void vhost_vdpa_net_cvq_stop(NetClientState *nc)
>>>>>        if (s->vhost_vdpa.shadow_vqs_enabled) {
>>>>>            vhost_vdpa_cvq_unmap_buf(&s->vhost_vdpa, s->cvq_cmd_out_buffer);
>>>>>            vhost_vdpa_cvq_unmap_buf(&s->vhost_vdpa, s->status);
>>>>> -        if (!s->always_svq) {
>>>>> -            /*
>>>>> -             * If only the CVQ is shadowed we can delete this safely.
>>>>> -             * If all the VQs are shadows this will be needed by the time the
>>>>> -             * device is started again to register SVQ vrings and similar.
>>>>> -             */
>>>>> -            g_clear_pointer(&s->vhost_vdpa.iova_tree, vhost_iova_tree_delete);
>>>>> -        }
>>>>>        }
>>>>> +
>>>>> +    vhost_vdpa_net_client_stop(nc);
>>>>>    }
>>>>>
>>>>>    static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState *s, size_t out_len,
>>>>> @@ -667,8 +718,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
>>>>>                                           int nvqs,
>>>>>                                           bool is_datapath,
>>>>>                                           bool svq,
>>>>> -                                       struct vhost_vdpa_iova_range iova_range,
>>>>> -                                       VhostIOVATree *iova_tree)
>>>>> +                                       struct vhost_vdpa_iova_range iova_range)
>>>>>    {
>>>>>        NetClientState *nc = NULL;
>>>>>        VhostVDPAState *s;
>>>>> @@ -690,7 +740,6 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
>>>>>        s->vhost_vdpa.shadow_vqs_enabled = svq;
>>>>>        s->vhost_vdpa.iova_range = iova_range;
>>>>>        s->vhost_vdpa.shadow_data = svq;
>>>>> -    s->vhost_vdpa.iova_tree = iova_tree;
>>>>>        if (!is_datapath) {
>>>>>            s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
>>>>>                                                vhost_vdpa_net_cvq_cmd_page_len());
>>>>> @@ -760,7 +809,6 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>>>>>        uint64_t features;
>>>>>        int vdpa_device_fd;
>>>>>        g_autofree NetClientState **ncs = NULL;
>>>>> -    g_autoptr(VhostIOVATree) iova_tree = NULL;
>>>>>        struct vhost_vdpa_iova_range iova_range;
>>>>>        NetClientState *nc;
>>>>>        int queue_pairs, r, i = 0, has_cvq = 0;
>>>>> @@ -812,12 +860,8 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>>>>>            goto err;
>>>>>        }
>>>>>
>>>>> -    if (opts->x_svq) {
>>>>> -        if (!vhost_vdpa_net_valid_svq_features(features, errp)) {
>>>>> -            goto err_svq;
>>>>> -        }
>>>>> -
>>>>> -        iova_tree = vhost_iova_tree_new(iova_range.first, iova_range.last);
>>>>> +    if (opts->x_svq && !vhost_vdpa_net_valid_svq_features(features, errp)) {
>>>>> +        goto err;
>>>>>        }
>>>>>
>>>>>        ncs = g_malloc0(sizeof(*ncs) * queue_pairs);
>>>>> @@ -825,7 +869,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>>>>>        for (i = 0; i < queue_pairs; i++) {
>>>>>            ncs[i] = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
>>>>>                                         vdpa_device_fd, i, 2, true, opts->x_svq,
>>>>> -                                     iova_range, iova_tree);
>>>>> +                                     iova_range);
>>>>>            if (!ncs[i])
>>>>>                goto err;
>>>>>        }
>>>>> @@ -833,13 +877,11 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>>>>>        if (has_cvq) {
>>>>>            nc = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
>>>>>                                     vdpa_device_fd, i, 1, false,
>>>>> -                                 opts->x_svq, iova_range, iova_tree);
>>>>> +                                 opts->x_svq, iova_range);
>>>>>            if (!nc)
>>>>>                goto err;
>>>>>        }
>>>>>
>>>>> -    /* iova_tree ownership belongs to last NetClientState */
>>>>> -    g_steal_pointer(&iova_tree);
>>>>>        return 0;
>>>>>
>>>>>    err:
>>>>> @@ -849,7 +891,6 @@ err:
>>>>>            }
>>>>>        }
>>>>>
>>>>> -err_svq:
>>>>>        qemu_close(vdpa_device_fd);
>>>>>
>>>>>        return -1;
>>>>> --
>>>>> 2.31.1
>>>>>