Re: [PATCH vhost v3 1/4] virtio: find_vqs: pass struct instead of multi parameters

[Xuan Zhuo <xuanzhuo@linux.alibaba.com>]

On Mon, 18 Mar 2024 12:18:23 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > >
> > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > > >
> > > > > > Now, we pass multi parameters to find_vqs. These parameters
> > > > > > may work for transport or work for vring.
> > > > > >
> > > > > > And find_vqs has multi implements in many places:
> > > > > >
> > > > > >  arch/um/drivers/virtio_uml.c
> > > > > >  drivers/platform/mellanox/mlxbf-tmfifo.c
> > > > > >  drivers/remoteproc/remoteproc_virtio.c
> > > > > >  drivers/s390/virtio/virtio_ccw.c
> > > > > >  drivers/virtio/virtio_mmio.c
> > > > > >  drivers/virtio/virtio_pci_legacy.c
> > > > > >  drivers/virtio/virtio_pci_modern.c
> > > > > >  drivers/virtio/virtio_vdpa.c
> > > > > >
> > > > > > Every time, we try to add a new parameter, that is difficult.
> > > > > > We must change every find_vqs implement.
> > > > > >
> > > > > > One the other side, if we want to pass a parameter to vring,
> > > > > > we must change the call path from transport to vring.
> > > > > > Too many functions need to be changed.
> > > > > >
> > > > > > So it is time to refactor the find_vqs. We pass a structure
> > > > > > cfg to find_vqs(), that will be passed to vring by transport.
> > > > > >
> > > > > > Because the vp_modern_create_avq() use the "const char *names[]",
> > > > > > and the virtio_uml.c changes the name in the subsequent commit, so
> > > > > > change the "names" inside the virtio_vq_config from "const char *const
> > > > > > *names" to "const char **names".
> > > > > >
> > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > > Acked-by: Johannes Berg <johannes@sipsolutions.net>
> > > > > > Reviewed-by: Ilpo J=E4rvinen <ilpo.jarvinen@linux.intel.com>
> > > > >
> > > > > The name seems broken here.
> > > >
> > > > Email APP bug.
> > > >
> > > > I will fix.
> > > >
> > > >
> > > > >
> > > > > [...]
> > > > >
> > > > > >
> > > > > >  typedef void vq_callback_t(struct virtqueue *);
> > > > > >
> > > > > > +/**
> > > > > > + * struct virtio_vq_config - configure for find_vqs()
> > > > > > + * @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > > > > > + *     During the initialization of each vq(vring setup), we need to know which
> > > > > > + *     item in the array should be used at that time. But since the item in
> > > > > > + *     names can be null, which causes some item of array to be skipped, we
> > > > > > + *     cannot use vq.index as the current id. So add a cfg_idx to let vring
> > > > > > + *     know how to get the current configuration from the array when
> > > > > > + *     initializing vq.
> > > > >
> > > > > So this design is not good. If it is not something that the driver
> > > > > needs to care about, the core needs to hide it from the API.
> > > >
> > > > The driver just ignore it. That will be beneficial to the virtio core.
> > > > Otherwise, we must pass one more parameter everywhere.
> > >
> > > I don't get here, it's an internal logic and we've already done that.
> >
> >
> > ## Then these must add one param "cfg_idx";
> >
> >  struct virtqueue *vring_create_virtqueue(struct virtio_device *vdev,
> >                                          unsigned int index,
> >                                          struct vq_transport_config *tp_cfg,
> >                                          struct virtio_vq_config *cfg,
> > -->                                      uint cfg_idx);
> >
> >  struct virtqueue *vring_new_virtqueue(struct virtio_device *vdev,
> >                                       unsigned int index,
> >                                       void *pages,
> >                                       struct vq_transport_config *tp_cfg,
> >                                       struct virtio_vq_config *cfg,
> > -->                                      uint cfg_idx);
> >
> >
> > ## The functions inside virtio_ring also need to add a new param, such as:
> >
> >  static struct virtqueue *vring_create_virtqueue_split(struct virtio_device *vdev,
> >                                                       unsigned int index,
> >                                                       struct vq_transport_config *tp_cfg,
> >                                                       struct virtio_vq_config,
> > -->                                                   uint cfg_idx);
> >
> >
> >
>
> I guess what I'm missing is when could the index differ from cfg_idx?


 @cfg_idx: Used by virtio core. The drivers should set this to 0.
     During the initialization of each vq(vring setup), we need to know which
     item in the array should be used at that time. But since the item in
     names can be null, which causes some item of array to be skipped, we
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     cannot use vq.index as the current id. So add a cfg_idx to let vring
     know how to get the current configuration from the array when
     initializing vq.


static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,

	................

	for (i = 0; i < nvqs; ++i) {
		if (!names[i]) {
			vqs[i] = NULL;
			continue;
		}

		if (!callbacks[i])
			msix_vec = VIRTIO_MSI_NO_VECTOR;
		else if (vp_dev->per_vq_vectors)
			msix_vec = allocated_vectors++;
		else
			msix_vec = VP_MSIX_VQ_VECTOR;
		vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
				     ctx ? ctx[i] : false,
				     msix_vec);


Thanks.

>
> Thanks
>
> > Thanks.
> >
> >
> >
> >
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks.
> > > >
> > > > >
> > > > > Thanks
> > > > >
> > > >
> > >
> >
>