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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 20 Mar 2024 17:39:29 +0800, Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> On Wed, 20 Mar 2024 17:22:50 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > On Tue, Mar 19, 2024 at 2:58 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Mar 18, 2024 at 01:59:52PM +0800, Xuan Zhuo wrote:
> > > > On Mon, 18 Mar 2024 12:18:23 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > > > > On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > > > > > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> > > > > > > >
> > > > > > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > > > > > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx>
> > > > > > > > > > Acked-by: Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
> > > > > > > > > > Reviewed-by: Ilpo J=E4rvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > > > > > > > >
> > > > > > > > > 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.
> > >
> > >
> > > Jason what do you think is the way to resolve this?
> >
> > I wonder which driver doesn't use a specific virtqueue in this case.
>
>
> commit 6457f126c888b3481fdae6f702e616cd0c79646e
> Author: Michael S. Tsirkin <mst@xxxxxxxxxx>
> Date:   Wed Sep 5 21:47:45 2012 +0300
>
>     virtio: support reserved vqs
>
>     virtio network device multiqueue support reserves
>     vq 3 for future use (useful both for future extensions and to make it
>     pretty - this way receive vqs have even and transmit - odd numbers).
>     Make it possible to skip initialization for
>     specific vq numbers by specifying NULL for name.
>     Document this usage as well as (existing) NULL callback.
>
>     Drivers using this not coded up yet, so I simply tested
>     with virtio-pci and verified that this patch does
>     not break existing drivers.
>
>     Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
>     Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
>
> This patch introduced this.
>
> Could we remove this? Then we can use the vq.index directly. That will
> be great.

I checked the current kernel.
Just virtio-ballon uses this feture. But we can fix by one line code.

Thanks.


>
> >
> > And it looks to me, introducing a per-vq-config struct might be better
> > then we have
> >
> > virtio_vqs_config {
> >       unsigned int nvqs;
> >       struct virtio_vq_config *configs;
> > }
>
> YES. I prefer this. But we need to refactor every driver.
>
> >
> > So we don't need the cfg_idx stuff.
>
> This still needs cfg_idx.
>
> Thanks
>
>
> >
> > Thanks
> >
> > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > > Thanks.
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > >
> >
>





[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux