From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
To: Jason Wang <jasowang@redhat.com>
Cc: virtualization@lists.linux.dev,
"Richard Weinberger" <richard@nod.at>,
"Anton Ivanov" <anton.ivanov@cambridgegreys.com>,
"Johannes Berg" <johannes@sipsolutions.net>,
"Hans de Goede" <hdegoede@redhat.com>,
"Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>,
"Vadim Pasternak" <vadimp@nvidia.com>,
"Bjorn Andersson" <andersson@kernel.org>,
"Mathieu Poirier" <mathieu.poirier@linaro.org>,
"Cornelia Huck" <cohuck@redhat.com>,
"Halil Pasic" <pasic@linux.ibm.com>,
"Eric Farman" <farman@linux.ibm.com>,
"Heiko Carstens" <hca@linux.ibm.com>,
"Vasily Gorbik" <gor@linux.ibm.com>,
"Alexander Gordeev" <agordeev@linux.ibm.com>,
"Christian Borntraeger" <borntraeger@linux.ibm.com>,
"Sven Schnelle" <svens@linux.ibm.com>,
linux-um@lists.infradead.org,
platform-driver-x86@vger.kernel.org,
linux-remoteproc@vger.kernel.org, linux-s390@vger.kernel.org,
kvm@vger.kernel.org, "Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [PATCH vhost v3 1/4] virtio: find_vqs: pass struct instead of multi parameters
Date: Thu, 21 Mar 2024 12:11:09 +0800 [thread overview]
Message-ID: <1710994269.7745419-1-xuanzhuo@linux.alibaba.com> (raw)
In-Reply-To: <CACGkMEuEqiLLGj-HY=gx4R1EVh_d=eYrmi=cuLzb1SiqiEHb-A@mail.gmail.com>
On Thu, 21 Mar 2024 12:03:36 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Wed, Mar 20, 2024 at 5:41 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Wed, 20 Mar 2024 17:22:50 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > On Tue, Mar 19, 2024 at 2:58 PM Michael S. Tsirkin <mst@redhat.com> 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@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.
> > > >
> > > >
> > > > 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@redhat.com>
> > 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@redhat.com>
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> >
> > This patch introduced this.
> >
> > Could we remove this? Then we can use the vq.index directly. That will
> > be great.
> >
> > >
> > > 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.
>
> Yes.
>
> >
> > >
> > > So we don't need the cfg_idx stuff.
> >
> > This still needs cfg_idx.
>
> Drive needs to pass config for each virtqueue. We can still check
> config->name so the virtqueue index could be used.
I see. If we pass the config for one queue to virtio ring,
then we do not to pass the idx.
But for now, I will remove the logic of checking name,
then we can use vq->index inside virtio ring.
Thanks.
>
> Thanks
>
> >
> > Thanks
> >
> >
> > >
> > > Thanks
> > >
> > > >
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > > Thanks.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Thanks
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > >
> > >
> >
>
next prev parent reply other threads:[~2024-03-21 4:17 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-12 2:10 [PATCH vhost v3 0/4] refactor the params of find_vqs() Xuan Zhuo
2024-03-12 2:10 ` [PATCH vhost v3 1/4] virtio: find_vqs: pass struct instead of multi parameters Xuan Zhuo
2024-03-14 3:12 ` Jason Wang
2024-03-14 5:58 ` Xuan Zhuo
2024-03-15 3:51 ` Jason Wang
2024-03-15 7:20 ` Xuan Zhuo
2024-03-18 4:18 ` Jason Wang
2024-03-18 5:59 ` Xuan Zhuo
2024-03-19 6:57 ` Michael S. Tsirkin
2024-03-20 9:22 ` Jason Wang
2024-03-20 9:31 ` Jason Wang
2024-03-20 9:39 ` Xuan Zhuo
2024-03-20 9:54 ` Xuan Zhuo
2024-03-21 4:02 ` Jason Wang
2024-03-21 4:03 ` Jason Wang
2024-03-21 4:11 ` Xuan Zhuo [this message]
2024-03-12 2:10 ` [PATCH vhost v3 2/4] virtio: vring_create_virtqueue: " Xuan Zhuo
2024-03-12 2:10 ` [PATCH vhost v3 3/4] virtio: vring_new_virtqueue(): " Xuan Zhuo
2024-03-12 2:10 ` [PATCH vhost v3 4/4] virtio_ring: simplify the parameters of the funcs related to vring_create/new_virtqueue() Xuan Zhuo
2024-03-19 7:01 ` [PATCH vhost v3 0/4] refactor the params of find_vqs() Michael S. Tsirkin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1710994269.7745419-1-xuanzhuo@linux.alibaba.com \
--to=xuanzhuo@linux.alibaba.com \
--cc=agordeev@linux.ibm.com \
--cc=andersson@kernel.org \
--cc=anton.ivanov@cambridgegreys.com \
--cc=borntraeger@linux.ibm.com \
--cc=cohuck@redhat.com \
--cc=farman@linux.ibm.com \
--cc=gor@linux.ibm.com \
--cc=hca@linux.ibm.com \
--cc=hdegoede@redhat.com \
--cc=ilpo.jarvinen@linux.intel.com \
--cc=jasowang@redhat.com \
--cc=johannes@sipsolutions.net \
--cc=kvm@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=linux-um@lists.infradead.org \
--cc=mathieu.poirier@linaro.org \
--cc=mst@redhat.com \
--cc=pasic@linux.ibm.com \
--cc=platform-driver-x86@vger.kernel.org \
--cc=richard@nod.at \
--cc=svens@linux.ibm.com \
--cc=vadimp@nvidia.com \
--cc=virtualization@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).