Re: [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore()
On Fri, Mar 25, 2022 at 11:05:22AM +0800, Jason Wang wrote: > > 在 2022/3/24 下午7:31, Stefano Garzarella 写道: > > On Thu, Mar 24, 2022 at 07:07:09AM -0400, Michael S. Tsirkin wrote: > > > On Thu, Mar 24, 2022 at 12:03:07PM +0100, Stefano Garzarella wrote: > > > > On Thu, Mar 24, 2022 at 06:48:05AM -0400, Michael S. Tsirkin wrote: > > > > > On Thu, Mar 24, 2022 at 04:40:02PM +0800, Jason Wang wrote: > > > > > > From: Stefano Garzarella > > > > > > > > > > > > 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? > > > > > > > > Nope, I mean, when I wrote the original version, I meant to do the same > > > > things that we do in virtio_dev_probe() where we called > > > > virtio_device_ready() which not only set the state, but also called > > > > .enable_cbs callback. > > > > > > > > Was this a side effect and maybe more compliant with the spec? > > > > > > > > > Sorry I don't understand the question. it says "avoids setting > > > DRIVER_OK twice" - > > > why is that advantageous and worth calling out in the commit log? > > > > I just wanted to say that it seems strange to set DRIVER_OK twice if we > > read the spec. I don't think it's wrong, but weird. > > > > Yes, maybe we should rewrite the commit message saying that we want to > > use virtio_device_ready() everywhere to complete the setup before > > setting DRIVER_OK so we can do all the necessary operations inside (like > > in patch 3 or call enable_cbs). > > > > Jason rewrote the commit log, so I don't know if he agrees. > > > > Thanks, > > Stefano > > > I agree, I will tweak the log in V2. > > Thanks Still waiting for that v2. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore()
在 2022/3/24 下午7:31, Stefano Garzarella 写道: On Thu, Mar 24, 2022 at 07:07:09AM -0400, Michael S. Tsirkin wrote: On Thu, Mar 24, 2022 at 12:03:07PM +0100, Stefano Garzarella wrote: On Thu, Mar 24, 2022 at 06:48:05AM -0400, Michael S. Tsirkin wrote: > On Thu, Mar 24, 2022 at 04:40:02PM +0800, Jason Wang wrote: > > From: Stefano Garzarella > > > > 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? Nope, I mean, when I wrote the original version, I meant to do the same things that we do in virtio_dev_probe() where we called virtio_device_ready() which not only set the state, but also called .enable_cbs callback. Was this a side effect and maybe more compliant with the spec? Sorry I don't understand the question. it says "avoids setting DRIVER_OK twice" - why is that advantageous and worth calling out in the commit log? I just wanted to say that it seems strange to set DRIVER_OK twice if we read the spec. I don't think it's wrong, but weird. Yes, maybe we should rewrite the commit message saying that we want to use virtio_device_ready() everywhere to complete the setup before setting DRIVER_OK so we can do all the necessary operations inside (like in patch 3 or call enable_cbs). Jason rewrote the commit log, so I don't know if he agrees. Thanks, Stefano I agree, I will tweak the log in V2. Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore()
On Thu, Mar 24, 2022 at 07:07:09AM -0400, Michael S. Tsirkin wrote: On Thu, Mar 24, 2022 at 12:03:07PM +0100, Stefano Garzarella wrote: On Thu, Mar 24, 2022 at 06:48:05AM -0400, Michael S. Tsirkin wrote: > On Thu, Mar 24, 2022 at 04:40:02PM +0800, Jason Wang wrote: > > From: Stefano Garzarella > > > > 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? Nope, I mean, when I wrote the original version, I meant to do the same things that we do in virtio_dev_probe() where we called virtio_device_ready() which not only set the state, but also called .enable_cbs callback. Was this a side effect and maybe more compliant with the spec? Sorry I don't understand the question. it says "avoids setting DRIVER_OK twice" - why is that advantageous and worth calling out in the commit log? I just wanted to say that it seems strange to set DRIVER_OK twice if we read the spec. I don't think it's wrong, but weird. Yes, maybe we should rewrite the commit message saying that we want to use virtio_device_ready() everywhere to complete the setup before setting DRIVER_OK so we can do all the necessary operations inside (like in patch 3 or call enable_cbs). Jason rewrote the commit log, so I don't know if he agrees. Thanks, Stefano ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore()
On Thu, Mar 24, 2022 at 12:03:07PM +0100, Stefano Garzarella wrote: > On Thu, Mar 24, 2022 at 06:48:05AM -0400, Michael S. Tsirkin wrote: > > On Thu, Mar 24, 2022 at 04:40:02PM +0800, Jason Wang wrote: > > > From: Stefano Garzarella > > > > > > 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? > > Nope, I mean, when I wrote the original version, I meant to do the same > things that we do in virtio_dev_probe() where we called > virtio_device_ready() which not only set the state, but also called > .enable_cbs callback. > > Was this a side effect and maybe more compliant with the spec? Sorry I don't understand the question. it says "avoids setting DRIVER_OK twice" - why is that advantageous and worth calling out in the commit log? > > 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); > > Could there be a case where we want to set the status again even though the > device tells us it's already set? > > I think not, so I guess it's okay. > > > > > > > > and it will allows us to do > > > extension on virtio_device_ready() without duplicating codes. > > > > > > Signed-off-by: Stefano Garzarella > > > Signed-off-by: Jason Wang > > > --- > > > 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. > > I agree, copy & paste from virtio_dev_probe(). > > Jason can you fix this patch or should I resend? > > Thanks, > Stefano ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore()
On Thu, Mar 24, 2022 at 06:48:05AM -0400, Michael S. Tsirkin wrote: On Thu, Mar 24, 2022 at 04:40:02PM +0800, Jason Wang wrote: From: Stefano Garzarella 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? Nope, I mean, when I wrote the original version, I meant to do the same things that we do in virtio_dev_probe() where we called virtio_device_ready() which not only set the state, but also called .enable_cbs callback. Was this a side effect and maybe more compliant with the spec? 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); Could there be a case where we want to set the status again even though the device tells us it's already set? I think not, so I guess it's okay. and it will allows us to do extension on virtio_device_ready() without duplicating codes. Signed-off-by: Stefano Garzarella Signed-off-by: Jason Wang --- 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. I agree, copy & paste from virtio_dev_probe(). Jason can you fix this patch or should I resend? Thanks, Stefano ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore()
On Thu, Mar 24, 2022 at 04:40:02PM +0800, Jason Wang wrote: > From: Stefano Garzarella > > 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 > Signed-off-by: Jason Wang > --- > 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
[PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore()
From: Stefano Garzarella This avoids setting DRIVER_OK twice for those drivers that call virtio_device_ready() in the .restore and it will allows us to do extension on virtio_device_ready() without duplicating codes. Signed-off-by: Stefano Garzarella Signed-off-by: Jason Wang --- 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. */ + 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