All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Cc: peterz@infradead.org, maz@kernel.org, keirf@google.com,
	linux-kernel@vger.kernel.org,
	virtualization@lists.linux-foundation.org, tglx@linutronix.de
Subject: Re: [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore()
Date: Thu, 24 Mar 2022 06:48:05 -0400	[thread overview]
Message-ID: <20220324064205-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20220324084004.14349-2-jasowang@redhat.com>

On Thu, Mar 24, 2022 at 04:40:02PM +0800, Jason Wang wrote:
> From: Stefano Garzarella <sgarzare@redhat.com>
> 
> This avoids setting DRIVER_OK twice for those drivers that call
> virtio_device_ready() in the .restore

Is this trying to say it's faster?
If yes this one looks like a red herring. Yes we skip a write but we
replace it with a read which is not better performance-wise.
If we want to optimize this, it is better to just do that inside
virtio_add_status:



diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 75c8d560bbd3..cd943c31bdbb 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -161,8 +161,14 @@ static void virtio_config_enable(struct virtio_device *dev)
 
 void virtio_add_status(struct virtio_device *dev, unsigned int status)
 {
+	unsigned int device_status;
+
 	might_sleep();
-	dev->config->set_status(dev, dev->config->get_status(dev) | status);
+
+	device_status = dev->config->get_status(dev);
+
+	if (status & ~device_status)
+		dev->config->set_status(dev, device_status | status);
 }
 EXPORT_SYMBOL_GPL(virtio_add_status);
 

> and it will allows us to do
> extension on virtio_device_ready() without duplicating codes.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/virtio/virtio.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 22f15f444f75..75c8d560bbd3 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -526,8 +526,9 @@ int virtio_device_restore(struct virtio_device *dev)
>  			goto err;
>  	}
>  
> -	/* Finally, tell the device we're all set */
> -	virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> +	/* If restore didn't do it, mark device DRIVER_OK ourselves. */

I preferred the original comment, it said why we are doing this,
new one repeats what code is doing.

> +	if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
> +		virtio_device_ready(dev);
>  
>  	virtio_config_enable(dev);
>  
> -- 
> 2.25.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Cc: virtualization@lists.linux-foundation.org,
	linux-kernel@vger.kernel.org, maz@kernel.org, tglx@linutronix.de,
	peterz@infradead.org, sgarzare@redhat.com, keirf@google.com
Subject: Re: [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore()
Date: Thu, 24 Mar 2022 06:48:05 -0400	[thread overview]
Message-ID: <20220324064205-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20220324084004.14349-2-jasowang@redhat.com>

On Thu, Mar 24, 2022 at 04:40:02PM +0800, Jason Wang wrote:
> From: Stefano Garzarella <sgarzare@redhat.com>
> 
> This avoids setting DRIVER_OK twice for those drivers that call
> virtio_device_ready() in the .restore

Is this trying to say it's faster?
If yes this one looks like a red herring. Yes we skip a write but we
replace it with a read which is not better performance-wise.
If we want to optimize this, it is better to just do that inside
virtio_add_status:



diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 75c8d560bbd3..cd943c31bdbb 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -161,8 +161,14 @@ static void virtio_config_enable(struct virtio_device *dev)
 
 void virtio_add_status(struct virtio_device *dev, unsigned int status)
 {
+	unsigned int device_status;
+
 	might_sleep();
-	dev->config->set_status(dev, dev->config->get_status(dev) | status);
+
+	device_status = dev->config->get_status(dev);
+
+	if (status & ~device_status)
+		dev->config->set_status(dev, device_status | status);
 }
 EXPORT_SYMBOL_GPL(virtio_add_status);
 

> and it will allows us to do
> extension on virtio_device_ready() without duplicating codes.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/virtio/virtio.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 22f15f444f75..75c8d560bbd3 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -526,8 +526,9 @@ int virtio_device_restore(struct virtio_device *dev)
>  			goto err;
>  	}
>  
> -	/* Finally, tell the device we're all set */
> -	virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> +	/* If restore didn't do it, mark device DRIVER_OK ourselves. */

I preferred the original comment, it said why we are doing this,
new one repeats what code is doing.

> +	if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
> +		virtio_device_ready(dev);
>  
>  	virtio_config_enable(dev);
>  
> -- 
> 2.25.1


  reply	other threads:[~2022-03-24 10:48 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-24  8:40 [PATCH 0/3] rework on the IRQ hardening of virtio Jason Wang
2022-03-24  8:40 ` Jason Wang
2022-03-24  8:40 ` [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore() Jason Wang
2022-03-24  8:40   ` Jason Wang
2022-03-24 10:48   ` Michael S. Tsirkin [this message]
2022-03-24 10:48     ` Michael S. Tsirkin
2022-03-24 11:03     ` Stefano Garzarella
2022-03-24 11:03       ` Stefano Garzarella
2022-03-24 11:07       ` Michael S. Tsirkin
2022-03-24 11:07         ` Michael S. Tsirkin
2022-03-24 11:31         ` Stefano Garzarella
2022-03-24 11:31           ` Stefano Garzarella
2022-03-25  3:05           ` Jason Wang
2022-03-25  3:05             ` Jason Wang
2022-03-30  5:22             ` Michael S. Tsirkin
2022-03-30  5:22               ` Michael S. Tsirkin
2022-03-24  8:40 ` [PATCH 2/3] virtio: use virtio_reset_device() when possible Jason Wang
2022-03-24  8:40   ` Jason Wang
2022-03-24 11:04   ` Stefano Garzarella
2022-03-24 11:04     ` Stefano Garzarella
2022-03-24  8:40 ` [PATCH 3/3] virtio: harden vring IRQ Jason Wang
2022-03-24  8:40   ` Jason Wang
2022-03-24 11:03   ` Michael S. Tsirkin
2022-03-24 11:03     ` Michael S. Tsirkin
2022-03-25  3:04     ` Jason Wang
2022-03-25  3:04       ` Jason Wang

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=20220324064205-mutt-send-email-mst@kernel.org \
    --to=mst@redhat.com \
    --cc=jasowang@redhat.com \
    --cc=keirf@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maz@kernel.org \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    --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: link
Be 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.