From: Stefano Garzarella <sgarzare@redhat.com> To: Andrey Ryabinin <arbn@yandex-team.com>, Mike Christie <michael.christie@oracle.com> Cc: "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, kvm@vger.kernel.org, virtualization@lists.linux-foundation.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/6] vhost: get rid of vhost_poll_flush() wrapper Date: Tue, 16 Nov 2021 15:33:23 +0100 [thread overview] Message-ID: <20211116143323.g7c27u2ho4vpp4cp@steredhat> (raw) In-Reply-To: <20211115153003.9140-1-arbn@yandex-team.com> On Mon, Nov 15, 2021 at 06:29:58PM +0300, Andrey Ryabinin wrote: >vhost_poll_flush() is a simple wrapper around vhost_work_dev_flush(). >It gives wrong impression that we are doing some work over vhost_poll, >while in fact it flushes vhost_poll->dev. >It only complicate understanding of the code and leads to mistakes >like flushing the same vhost_dev several times in a row. > >Just remove vhost_poll_flush() and call vhost_work_dev_flush() directly. > >Signed-off-by: Andrey Ryabinin <arbn@yandex-team.com> >--- > drivers/vhost/net.c | 4 ++-- > drivers/vhost/test.c | 2 +- > drivers/vhost/vhost.c | 12 ++---------- > drivers/vhost/vsock.c | 2 +- > 4 files changed, 6 insertions(+), 14 deletions(-) Adding Mike since these changes could be relevant for "[PATCH V4 00/12] vhost: multiple worker support" [1] series. > >diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c >index 28ef323882fb..11221f6d11b8 100644 >--- a/drivers/vhost/net.c >+++ b/drivers/vhost/net.c >@@ -1375,8 +1375,8 @@ static void vhost_net_stop(struct vhost_net *n, struct socket **tx_sock, > > static void vhost_net_flush_vq(struct vhost_net *n, int index) > { >- vhost_poll_flush(n->poll + index); >- vhost_poll_flush(&n->vqs[index].vq.poll); >+ vhost_work_dev_flush(n->poll[index].dev); >+ vhost_work_dev_flush(n->vqs[index].vq.poll.dev); > } > > static void vhost_net_flush(struct vhost_net *n) >diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c >index a09dedc79f68..1a8ab1d8cb1c 100644 >--- a/drivers/vhost/test.c >+++ b/drivers/vhost/test.c >@@ -146,7 +146,7 @@ static void vhost_test_stop(struct vhost_test *n, void **privatep) > > static void vhost_test_flush_vq(struct vhost_test *n, int index) > { >- vhost_poll_flush(&n->vqs[index].poll); >+ vhost_work_dev_flush(n->vqs[index].poll.dev); > } > > static void vhost_test_flush(struct vhost_test *n) >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >index 59edb5a1ffe2..ca088481da0e 100644 >--- a/drivers/vhost/vhost.c >+++ b/drivers/vhost/vhost.c >@@ -245,14 +245,6 @@ void vhost_work_dev_flush(struct vhost_dev *dev) > } > EXPORT_SYMBOL_GPL(vhost_work_dev_flush); > >-/* Flush any work that has been scheduled. When calling this, don't hold any >- * locks that are also used by the callback. */ >-void vhost_poll_flush(struct vhost_poll *poll) >-{ >- vhost_work_dev_flush(poll->dev); >-} >-EXPORT_SYMBOL_GPL(vhost_poll_flush); >- We should remove also the declaration in vhost.h: --- a/drivers/vhost/vhost.h +++ b/drivers/vhost/vhost.h @@ -45,7 +44,6 @@ void vhost_poll_init(struct vhost_poll *poll, vhost_work_fn_t fn, __poll_t mask, struct vhost_dev *dev); int vhost_poll_start(struct vhost_poll *poll, struct file *file); void vhost_poll_stop(struct vhost_poll *poll); -void vhost_poll_flush(struct vhost_poll *poll); void vhost_poll_queue(struct vhost_poll *poll); void vhost_work_dev_flush(struct vhost_dev *dev); > void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work) > { > if (!dev->worker) >@@ -663,7 +655,7 @@ void vhost_dev_stop(struct vhost_dev *dev) > for (i = 0; i < dev->nvqs; ++i) { > if (dev->vqs[i]->kick && dev->vqs[i]->handle_kick) { > vhost_poll_stop(&dev->vqs[i]->poll); >- vhost_poll_flush(&dev->vqs[i]->poll); >+ vhost_work_dev_flush(dev->vqs[i]->poll.dev); > } > } > } >@@ -1712,7 +1704,7 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *arg > mutex_unlock(&vq->mutex); > > if (pollstop && vq->handle_kick) >- vhost_poll_flush(&vq->poll); >+ vhost_work_dev_flush(vq->poll.dev); > return r; > } > EXPORT_SYMBOL_GPL(vhost_vring_ioctl); >diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c >index 938aefbc75ec..b0361ebbd695 100644 >--- a/drivers/vhost/vsock.c >+++ b/drivers/vhost/vsock.c >@@ -711,7 +711,7 @@ static void vhost_vsock_flush(struct vhost_vsock *vsock) > > for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) > if (vsock->vqs[i].handle_kick) >- vhost_poll_flush(&vsock->vqs[i].poll); >+ vhost_work_dev_flush(vsock->vqs[i].poll.dev); > vhost_work_dev_flush(&vsock->dev); > } > >-- >2.32.0 > The rest LGTM. Thanks, Stefano [1] https://lore.kernel.org/virtualization/20211104190502.7053-1-michael.christie@oracle.com/
WARNING: multiple messages have this Message-ID (diff)
From: Stefano Garzarella <sgarzare@redhat.com> To: Andrey Ryabinin <arbn@yandex-team.com>, Mike Christie <michael.christie@oracle.com> Cc: kvm@vger.kernel.org, "Michael S. Tsirkin" <mst@redhat.com>, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Stefan Hajnoczi <stefanha@redhat.com> Subject: Re: [PATCH 1/6] vhost: get rid of vhost_poll_flush() wrapper Date: Tue, 16 Nov 2021 15:33:23 +0100 [thread overview] Message-ID: <20211116143323.g7c27u2ho4vpp4cp@steredhat> (raw) In-Reply-To: <20211115153003.9140-1-arbn@yandex-team.com> On Mon, Nov 15, 2021 at 06:29:58PM +0300, Andrey Ryabinin wrote: >vhost_poll_flush() is a simple wrapper around vhost_work_dev_flush(). >It gives wrong impression that we are doing some work over vhost_poll, >while in fact it flushes vhost_poll->dev. >It only complicate understanding of the code and leads to mistakes >like flushing the same vhost_dev several times in a row. > >Just remove vhost_poll_flush() and call vhost_work_dev_flush() directly. > >Signed-off-by: Andrey Ryabinin <arbn@yandex-team.com> >--- > drivers/vhost/net.c | 4 ++-- > drivers/vhost/test.c | 2 +- > drivers/vhost/vhost.c | 12 ++---------- > drivers/vhost/vsock.c | 2 +- > 4 files changed, 6 insertions(+), 14 deletions(-) Adding Mike since these changes could be relevant for "[PATCH V4 00/12] vhost: multiple worker support" [1] series. > >diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c >index 28ef323882fb..11221f6d11b8 100644 >--- a/drivers/vhost/net.c >+++ b/drivers/vhost/net.c >@@ -1375,8 +1375,8 @@ static void vhost_net_stop(struct vhost_net *n, struct socket **tx_sock, > > static void vhost_net_flush_vq(struct vhost_net *n, int index) > { >- vhost_poll_flush(n->poll + index); >- vhost_poll_flush(&n->vqs[index].vq.poll); >+ vhost_work_dev_flush(n->poll[index].dev); >+ vhost_work_dev_flush(n->vqs[index].vq.poll.dev); > } > > static void vhost_net_flush(struct vhost_net *n) >diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c >index a09dedc79f68..1a8ab1d8cb1c 100644 >--- a/drivers/vhost/test.c >+++ b/drivers/vhost/test.c >@@ -146,7 +146,7 @@ static void vhost_test_stop(struct vhost_test *n, void **privatep) > > static void vhost_test_flush_vq(struct vhost_test *n, int index) > { >- vhost_poll_flush(&n->vqs[index].poll); >+ vhost_work_dev_flush(n->vqs[index].poll.dev); > } > > static void vhost_test_flush(struct vhost_test *n) >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >index 59edb5a1ffe2..ca088481da0e 100644 >--- a/drivers/vhost/vhost.c >+++ b/drivers/vhost/vhost.c >@@ -245,14 +245,6 @@ void vhost_work_dev_flush(struct vhost_dev *dev) > } > EXPORT_SYMBOL_GPL(vhost_work_dev_flush); > >-/* Flush any work that has been scheduled. When calling this, don't hold any >- * locks that are also used by the callback. */ >-void vhost_poll_flush(struct vhost_poll *poll) >-{ >- vhost_work_dev_flush(poll->dev); >-} >-EXPORT_SYMBOL_GPL(vhost_poll_flush); >- We should remove also the declaration in vhost.h: --- a/drivers/vhost/vhost.h +++ b/drivers/vhost/vhost.h @@ -45,7 +44,6 @@ void vhost_poll_init(struct vhost_poll *poll, vhost_work_fn_t fn, __poll_t mask, struct vhost_dev *dev); int vhost_poll_start(struct vhost_poll *poll, struct file *file); void vhost_poll_stop(struct vhost_poll *poll); -void vhost_poll_flush(struct vhost_poll *poll); void vhost_poll_queue(struct vhost_poll *poll); void vhost_work_dev_flush(struct vhost_dev *dev); > void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work) > { > if (!dev->worker) >@@ -663,7 +655,7 @@ void vhost_dev_stop(struct vhost_dev *dev) > for (i = 0; i < dev->nvqs; ++i) { > if (dev->vqs[i]->kick && dev->vqs[i]->handle_kick) { > vhost_poll_stop(&dev->vqs[i]->poll); >- vhost_poll_flush(&dev->vqs[i]->poll); >+ vhost_work_dev_flush(dev->vqs[i]->poll.dev); > } > } > } >@@ -1712,7 +1704,7 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *arg > mutex_unlock(&vq->mutex); > > if (pollstop && vq->handle_kick) >- vhost_poll_flush(&vq->poll); >+ vhost_work_dev_flush(vq->poll.dev); > return r; > } > EXPORT_SYMBOL_GPL(vhost_vring_ioctl); >diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c >index 938aefbc75ec..b0361ebbd695 100644 >--- a/drivers/vhost/vsock.c >+++ b/drivers/vhost/vsock.c >@@ -711,7 +711,7 @@ static void vhost_vsock_flush(struct vhost_vsock *vsock) > > for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) > if (vsock->vqs[i].handle_kick) >- vhost_poll_flush(&vsock->vqs[i].poll); >+ vhost_work_dev_flush(vsock->vqs[i].poll.dev); > vhost_work_dev_flush(&vsock->dev); > } > >-- >2.32.0 > The rest LGTM. Thanks, Stefano [1] https://lore.kernel.org/virtualization/20211104190502.7053-1-michael.christie@oracle.com/ _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-11-16 14:34 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-11-15 15:29 [PATCH 1/6] vhost: get rid of vhost_poll_flush() wrapper Andrey Ryabinin 2021-11-15 15:29 ` [PATCH 2/6] vhost_net: get rid of vhost_net_flush_vq() and extra flush calls Andrey Ryabinin 2021-11-16 14:33 ` Stefano Garzarella 2021-11-16 14:33 ` Stefano Garzarella 2021-11-19 10:31 ` Andrey Ryabinin 2021-11-15 15:30 ` [PATCH 3/6] vhost_test: remove vhost_test_flush_vq() Andrey Ryabinin 2021-11-15 15:30 ` [PATCH 4/6] vhost_vsock: simplify vhost_vsock_flush() Andrey Ryabinin 2021-11-16 14:35 ` Stefano Garzarella 2021-11-16 14:35 ` Stefano Garzarella 2021-11-15 15:30 ` [PATCH 5/6] vhost_net: remove NOP vhost_net_flush() in vhost_net_release() Andrey Ryabinin 2021-11-15 15:30 ` [PATCH 6/6] vhost_net: use RCU callbacks instead of synchronize_rcu() Andrey Ryabinin 2021-11-16 5:00 ` Jason Wang 2021-11-16 5:00 ` Jason Wang 2021-11-19 11:32 ` Andrey Ryabinin 2021-11-22 2:48 ` Jason Wang 2021-11-22 2:48 ` Jason Wang 2021-11-22 9:37 ` Michael S. Tsirkin 2021-11-22 9:37 ` Michael S. Tsirkin 2021-11-22 15:12 ` Andrey Ryabinin 2021-11-16 14:33 ` Stefano Garzarella [this message] 2021-11-16 14:33 ` [PATCH 1/6] vhost: get rid of vhost_poll_flush() wrapper Stefano Garzarella 2021-12-03 17:45 ` Mike Christie 2021-12-03 17:45 ` Mike Christie 2021-11-16 14:39 ` Denis Kirjanov 2021-11-16 14:41 ` Stefano Garzarella 2021-11-16 14:41 ` Stefano Garzarella 2021-11-19 10:30 ` Andrey Ryabinin
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=20211116143323.g7c27u2ho4vpp4cp@steredhat \ --to=sgarzare@redhat.com \ --cc=arbn@yandex-team.com \ --cc=jasowang@redhat.com \ --cc=kvm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=michael.christie@oracle.com \ --cc=mst@redhat.com \ --cc=netdev@vger.kernel.org \ --cc=stefanha@redhat.com \ --cc=virtualization@lists.linux-foundation.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.