virtualization.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
  • * Re: [PATCH 1/6] vhost: get rid of vhost_poll_flush() wrapper
           [not found] <20211115153003.9140-1-arbn@yandex-team.com>
           [not found] ` <20211115153003.9140-6-arbn@yandex-team.com>
    @ 2021-11-16 14:33 ` Stefano Garzarella
      2021-12-03 17:45   ` Mike Christie
           [not found] ` <20211115153003.9140-2-arbn@yandex-team.com>
                       ` (2 subsequent siblings)
      4 siblings, 1 reply; 8+ messages in thread
    From: Stefano Garzarella @ 2021-11-16 14:33 UTC (permalink / raw)
      To: Andrey Ryabinin, Mike Christie
      Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, virtualization,
    	Stefan Hajnoczi
    
    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
    
    ^ permalink raw reply	[flat|nested] 8+ messages in thread
  • [parent not found: <20211115153003.9140-2-arbn@yandex-team.com>]
  • [parent not found: <20211115153003.9140-4-arbn@yandex-team.com>]
  • * Re: [PATCH 1/6] vhost: get rid of vhost_poll_flush() wrapper
           [not found] <20211115153003.9140-1-arbn@yandex-team.com>
                       ` (3 preceding siblings ...)
           [not found] ` <20211115153003.9140-4-arbn@yandex-team.com>
    @ 2021-11-16 14:41 ` Stefano Garzarella
      4 siblings, 0 replies; 8+ messages in thread
    From: Stefano Garzarella @ 2021-11-16 14:41 UTC (permalink / raw)
      To: Andrey Ryabinin
      Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, virtualization,
    	Stefan Hajnoczi
    
    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(-)
    >
    >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);
    >-
    > 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);
    
    Not related to this patch, but while looking at vhost-vsock I'm 
    wondering if we can do the same here in vhost_dev_stop(), I mean move 
    vhost_work_dev_flush() outside the loop and and call it once. (In 
    another patch eventually)
    
    Stefano
    
    _______________________________________________
    Virtualization mailing list
    Virtualization@lists.linux-foundation.org
    https://lists.linuxfoundation.org/mailman/listinfo/virtualization
    
    ^ permalink raw reply	[flat|nested] 8+ messages in thread

  • end of thread, other threads:[~2021-12-03 17:45 UTC | newest]
    
    Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
    -- links below jump to the message on this page --
         [not found] <20211115153003.9140-1-arbn@yandex-team.com>
         [not found] ` <20211115153003.9140-6-arbn@yandex-team.com>
    2021-11-16  5:00   ` [PATCH 6/6] vhost_net: use RCU callbacks instead of synchronize_rcu() Jason Wang
         [not found]     ` <b163233f-090f-baaf-4460-37978cab4d55@yandex-team.com>
    2021-11-22  2:48       ` Jason Wang
    2021-11-22  9:37       ` Michael S. Tsirkin
    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
         [not found] ` <20211115153003.9140-2-arbn@yandex-team.com>
    2021-11-16 14:33   ` [PATCH 2/6] vhost_net: get rid of vhost_net_flush_vq() and extra flush calls Stefano Garzarella
         [not found] ` <20211115153003.9140-4-arbn@yandex-team.com>
    2021-11-16 14:35   ` [PATCH 4/6] vhost_vsock: simplify vhost_vsock_flush() Stefano Garzarella
    2021-11-16 14:41 ` [PATCH 1/6] vhost: get rid of vhost_poll_flush() wrapper Stefano Garzarella
    

    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 NNTP newsgroup(s).