Re: [PATCH vhost 07/17] virtio: find_vqs: pass struct instead of multi parameters

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

 



On Wed, 31 Jan 2024 17:12:34 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> On Tue, Jan 30, 2024 at 7:42 PM 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:
> >
> > But every time,
> >  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.
> >
> > And squish the parameters from transport to a structure.
> 
> The patch did more than what is described here, it also switch to use
> a structure for vring_create_virtqueue() etc.
> 
> Is it better to split?

Sure.

> 
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> > ---
> >  arch/um/drivers/virtio_uml.c             | 29 +++++++-----
> >  drivers/platform/mellanox/mlxbf-tmfifo.c | 14 +++---
> >  drivers/remoteproc/remoteproc_virtio.c   | 28 ++++++-----
> >  drivers/s390/virtio/virtio_ccw.c         | 33 ++++++-------
> >  drivers/virtio/virtio_mmio.c             | 30 ++++++------
> >  drivers/virtio/virtio_pci_common.c       | 59 +++++++++++-------------
> >  drivers/virtio/virtio_pci_common.h       |  9 +---
> >  drivers/virtio/virtio_pci_legacy.c       | 16 ++++---
> >  drivers/virtio/virtio_pci_modern.c       | 24 +++++-----
> >  drivers/virtio/virtio_ring.c             | 59 ++++++++++--------------
> >  drivers/virtio/virtio_vdpa.c             | 33 +++++++------
> >  include/linux/virtio_config.h            | 51 ++++++++++++++++----
> >  include/linux/virtio_ring.h              | 40 ++++++----------
> >  13 files changed, 217 insertions(+), 208 deletions(-)
> >
> > diff --git a/arch/um/drivers/virtio_uml.c b/arch/um/drivers/virtio_uml.c
> > index 8adca2000e51..161bac67e454 100644
> > --- a/arch/um/drivers/virtio_uml.c
> > +++ b/arch/um/drivers/virtio_uml.c
> > @@ -937,11 +937,12 @@ static int vu_setup_vq_call_fd(struct virtio_uml_device *vu_dev,
> >  }
> >
> >  static struct virtqueue *vu_setup_vq(struct virtio_device *vdev,
> > -                                    unsigned index, vq_callback_t *callback,
> > -                                    const char *name, bool ctx)
> > +                                    unsigned index,
> > +                                    struct virtio_vq_config *cfg)
> >  {
> >         struct virtio_uml_device *vu_dev = to_virtio_uml_device(vdev);
> >         struct platform_device *pdev = vu_dev->pdev;
> > +       struct transport_vq_config tp_cfg = {};
> 
> Nit: what did "tp" short for?

tp: transport

Any better?


> 
> >         struct virtio_uml_vq_info *info;
> >         struct virtqueue *vq;
> >         int num = MAX_SUPPORTED_QUEUE_SIZE;
> > @@ -953,10 +954,15 @@ static struct virtqueue *vu_setup_vq(struct virtio_device *vdev,
> >                 goto error_kzalloc;
> >         }
> >         snprintf(info->name, sizeof(info->name), "%s.%d-%s", pdev->name,
> > -                pdev->id, name);
> > +                pdev->id, cfg->names[cfg->cfg_idx]);
> >
> > -               dev_err(dev, "vring_new_virtqueue %s failed\n", name);

[...]


> > -       if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
> > -               return vring_create_virtqueue_packed(index, num, vring_align,
> > -                               vdev, weak_barriers, may_reduce_num,
> > -                               context, notify, callback, name, vdev->dev.parent);
> > +       dma_dev = tp_cfg->dma_dev;
> > +       if (!dma_dev)
> > +               dma_dev  = vdev->dev.parent;
> 
> Nit: This seems suboptimal than using "?:" ?

YES


> 
> >
> > -       return vring_create_virtqueue_split(index, num, vring_align,
> > -                       vdev, weak_barriers, may_reduce_num,
> > -                       context, notify, callback, name, vdev->dev.parent);
> > -}

[...]

> >                         err = PTR_ERR(vqs[i]);
> >                         goto err_setup_vq;
> > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> > index 2b3438de2c4d..e2c72e125dae 100644
> > --- a/include/linux/virtio_config.h
> > +++ b/include/linux/virtio_config.h
> > @@ -94,6 +94,20 @@ typedef void vq_callback_t(struct virtqueue *);
> >   *     If disable_vq_and_reset is set, then enable_vq_after_reset must also be
> >   *     set.
> >   */
> > +
> > +struct virtio_vq_config {
> > +       unsigned int nvqs;
> > +
> > +       /* the vq index may not eq to the cfg index of the other array items */
> 
> What does this mean?


When we read from the names/ctx/callbacks array, we can use the vq index,
because some names maybe null, the vq index may not equal to the array index.
We must save a cfg idx for the names/ctx/callbacks array.



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

		cfg->cfg_idx = i;
		vqs[i] = vp_setup_vq(vdev, queue_idx++, cfg, VIRTIO_MSI_NO_VECTOR);
		if (IS_ERR(vqs[i])) {
			err = PTR_ERR(vqs[i]);
			goto out_del_vqs;
		}
	}

notice "i" and "queue_idx"

Thanks.


> 
> > +       unsigned int cfg_idx;
> > +
> > +       struct virtqueue **vqs;
> > +       vq_callback_t **callbacks;
> > +       const char *const *names;
> > +       const bool *ctx;
> > +       struct irq_affinity *desc;
> > +};
> > +
> >  struct virtio_config_ops {
> >         void (*get)(struct virtio_device *vdev, unsigned offset,
> >                     void *buf, unsigned len);

[...]

> > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> > index 9b33df741b63..0de46ed17cc0 100644
> > --- a/include/linux/virtio_ring.h
> > +++ b/include/linux/virtio_ring.h
> > @@ -5,6 +5,7 @@
> >  #include <asm/barrier.h>
> >  #include <linux/irqreturn.h>
> >  #include <uapi/linux/virtio_ring.h>
> > +#include <linux/virtio_config.h>
> >
> >  /*
> >   * Barriers in virtio are tricky.  Non-SMP virtio guests can't assume
> > @@ -60,38 +61,25 @@ struct virtio_device;
> >  struct virtqueue;
> >  struct device;
> >
> > +struct transport_vq_config {
> 
> To reduce the confusion, let's rename this as "vq_transport_config"

OK

Thanks.


> 
> Thanks
> 





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux