| Message ID | 20240325090419.33677-2-xuanzhuo@linux.alibaba.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | refactor the params of find_vqs() | expand |
On 25.03.24 10:04, Xuan Zhuo wrote: > Currently, the init_vqs function within the virtio_balloon driver relies > on the condition that certain names array entries are null in order to > skip the initialization of some virtual queues (vqs). This behavior is > unique to this part of the codebase. In an upcoming commit, we plan to > eliminate this dependency by removing the function entirely. Therefore, > with this change, we are ensuring that the virtio_balloon no longer > depends on the aforementioned function. > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > --- > drivers/virtio/virtio_balloon.c | 46 +++++++++++++-------------------- > 1 file changed, 18 insertions(+), 28 deletions(-) > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index 1f5b3dd31fcf..8409642e54d7 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -531,49 +531,39 @@ static int init_vqs(struct virtio_balloon *vb) > struct virtqueue *vqs[VIRTIO_BALLOON_VQ_MAX]; > vq_callback_t *callbacks[VIRTIO_BALLOON_VQ_MAX]; > const char *names[VIRTIO_BALLOON_VQ_MAX]; > - int err; > + int err, nvqs = 0, idx = 0; Re-reading, you could just use a single variable for both purposes. Assuming I didn't miss a functional change Acked-by: David Hildenbrand <david@redhat.com>
On Mon, Mar 25, 2024 at 5:04 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > Currently, the init_vqs function within the virtio_balloon driver relies > on the condition that certain names array entries are null in order to > skip the initialization of some virtual queues (vqs). If there's a respin I would add something like: 1) the virtqueue index is contiguous for all the existing devices. 2) the current behaviour of virtio-balloon device is different from what is described in the spec 1.0-1.2 3) there's no functional changes and explain why > This behavior is > unique to this part of the codebase. In an upcoming commit, we plan to > eliminate this dependency by removing the function entirely. Therefore, > with this change, we are ensuring that the virtio_balloon no longer > depends on the aforementioned function. > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> With the above tweak. Acked-by: Jason Wang <jasowang@redhat.com> Thanks
On Tue, 26 Mar 2024 12:14:17 +0800, Jason Wang <jasowang@redhat.com> wrote: > On Mon, Mar 25, 2024 at 5:04 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > Currently, the init_vqs function within the virtio_balloon driver relies > > on the condition that certain names array entries are null in order to > > skip the initialization of some virtual queues (vqs). > > If there's a respin I would add something like: > > 1) the virtqueue index is contiguous for all the existing devices. > 2) the current behaviour of virtio-balloon device is different from > what is described in the spec 1.0-1.2 > 3) there's no functional changes and explain why > > > This behavior is > > unique to this part of the codebase. In an upcoming commit, we plan to > > eliminate this dependency by removing the function entirely. Therefore, > > with this change, we are ensuring that the virtio_balloon no longer > > depends on the aforementioned function. > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > With the above tweak. OK. I will add these in next version. Thanks. > > Acked-by: Jason Wang <jasowang@redhat.com> > > Thanks >
On Mon, 25 Mar 2024 10:12:51 +0100, David Hildenbrand <david@redhat.com> wrote: > On 25.03.24 10:04, Xuan Zhuo wrote: > > Currently, the init_vqs function within the virtio_balloon driver relies > > on the condition that certain names array entries are null in order to > > skip the initialization of some virtual queues (vqs). This behavior is > > unique to this part of the codebase. In an upcoming commit, we plan to > > eliminate this dependency by removing the function entirely. Therefore, > > with this change, we are ensuring that the virtio_balloon no longer > > depends on the aforementioned function. > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > --- > > drivers/virtio/virtio_balloon.c | 46 +++++++++++++-------------------- > > 1 file changed, 18 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > > index 1f5b3dd31fcf..8409642e54d7 100644 > > --- a/drivers/virtio/virtio_balloon.c > > +++ b/drivers/virtio/virtio_balloon.c > > @@ -531,49 +531,39 @@ static int init_vqs(struct virtio_balloon *vb) > > struct virtqueue *vqs[VIRTIO_BALLOON_VQ_MAX]; > > vq_callback_t *callbacks[VIRTIO_BALLOON_VQ_MAX]; > > const char *names[VIRTIO_BALLOON_VQ_MAX]; > > - int err; > > + int err, nvqs = 0, idx = 0; > > Re-reading, you could just use a single variable for both purposes. OK. Will update in next version. Thanks. > > Assuming I didn't miss a functional change > > Acked-by: David Hildenbrand <david@redhat.com> > > -- > Cheers, > > David / dhildenb >
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 1f5b3dd31fcf..8409642e54d7 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -531,49 +531,39 @@ static int init_vqs(struct virtio_balloon *vb) struct virtqueue *vqs[VIRTIO_BALLOON_VQ_MAX]; vq_callback_t *callbacks[VIRTIO_BALLOON_VQ_MAX]; const char *names[VIRTIO_BALLOON_VQ_MAX]; - int err; + int err, nvqs = 0, idx = 0; - /* - * Inflateq and deflateq are used unconditionally. The names[] - * will be NULL if the related feature is not enabled, which will - * cause no allocation for the corresponding virtqueue in find_vqs. - */ - callbacks[VIRTIO_BALLOON_VQ_INFLATE] = balloon_ack; - names[VIRTIO_BALLOON_VQ_INFLATE] = "inflate"; - callbacks[VIRTIO_BALLOON_VQ_DEFLATE] = balloon_ack; - names[VIRTIO_BALLOON_VQ_DEFLATE] = "deflate"; - callbacks[VIRTIO_BALLOON_VQ_STATS] = NULL; - names[VIRTIO_BALLOON_VQ_STATS] = NULL; - callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL; - names[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL; - names[VIRTIO_BALLOON_VQ_REPORTING] = NULL; + callbacks[nvqs] = balloon_ack; + names[nvqs++] = "inflate"; + callbacks[nvqs] = balloon_ack; + names[nvqs++] = "deflate"; if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) { - names[VIRTIO_BALLOON_VQ_STATS] = "stats"; - callbacks[VIRTIO_BALLOON_VQ_STATS] = stats_request; + names[nvqs] = "stats"; + callbacks[nvqs++] = stats_request; } if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) { - names[VIRTIO_BALLOON_VQ_FREE_PAGE] = "free_page_vq"; - callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL; + names[nvqs] = "free_page_vq"; + callbacks[nvqs++] = NULL; } if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING)) { - names[VIRTIO_BALLOON_VQ_REPORTING] = "reporting_vq"; - callbacks[VIRTIO_BALLOON_VQ_REPORTING] = balloon_ack; + names[nvqs] = "reporting_vq"; + callbacks[nvqs++] = balloon_ack; } - err = virtio_find_vqs(vb->vdev, VIRTIO_BALLOON_VQ_MAX, vqs, - callbacks, names, NULL); + err = virtio_find_vqs(vb->vdev, nvqs, vqs, callbacks, names, NULL); if (err) return err; - vb->inflate_vq = vqs[VIRTIO_BALLOON_VQ_INFLATE]; - vb->deflate_vq = vqs[VIRTIO_BALLOON_VQ_DEFLATE]; + vb->inflate_vq = vqs[idx++]; + vb->deflate_vq = vqs[idx++]; + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) { struct scatterlist sg; unsigned int num_stats; - vb->stats_vq = vqs[VIRTIO_BALLOON_VQ_STATS]; + vb->stats_vq = vqs[idx++]; /* * Prime this virtqueue with one buffer so the hypervisor can @@ -593,10 +583,10 @@ static int init_vqs(struct virtio_balloon *vb) } if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) - vb->free_page_vq = vqs[VIRTIO_BALLOON_VQ_FREE_PAGE]; + vb->free_page_vq = vqs[idx++]; if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING)) - vb->reporting_vq = vqs[VIRTIO_BALLOON_VQ_REPORTING]; + vb->reporting_vq = vqs[idx++]; return 0; }
Currently, the init_vqs function within the virtio_balloon driver relies on the condition that certain names array entries are null in order to skip the initialization of some virtual queues (vqs). This behavior is unique to this part of the codebase. In an upcoming commit, we plan to eliminate this dependency by removing the function entirely. Therefore, with this change, we are ensuring that the virtio_balloon no longer depends on the aforementioned function. Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> --- drivers/virtio/virtio_balloon.c | 46 +++++++++++++-------------------- 1 file changed, 18 insertions(+), 28 deletions(-)