On Mon, Jan 16, 2023 at 4:05 AM Jason Wang <jasow...@redhat.com> wrote: > > > 在 2023/1/13 15:28, Eugenio Perez Martin 写道: > > On Fri, Jan 13, 2023 at 4:53 AM Jason Wang <jasow...@redhat.com> wrote: > >> On Fri, Jan 13, 2023 at 1:24 AM Eugenio Pérez <epere...@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 <epere...@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? 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 > >>> >