diff mbox series

[4/7] hw/scsi/virtio-scsi: Use VIRTIO_SCSI_COMMON() macro

Message ID 20231017140150.44995-5-philmd@linaro.org
State Superseded
Headers show
Series hw: Few more QOM/QDev cleanups | expand

Commit Message

Philippe Mathieu-Daudé Oct. 17, 2023, 2:01 p.m. UTC
Access QOM parent with the proper QOM VIRTIO_SCSI_COMMON() macro.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/scsi/virtio-scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Manos Pitsidianakis Oct. 17, 2023, 5:19 p.m. UTC | #1
On Tue, 17 Oct 2023 17:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Access QOM parent with the proper QOM VIRTIO_SCSI_COMMON() macro.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> hw/scsi/virtio-scsi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
>index 45b95ea070..fa53f0902c 100644
>--- a/hw/scsi/virtio-scsi.c
>+++ b/hw/scsi/virtio-scsi.c
>@@ -761,7 +761,7 @@ static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
> 
> static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
> {
>-    VirtIOSCSICommon *vs = &s->parent_obj;
>+    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
>     SCSIDevice *d;
>     int rc;
> 
>-- 
>2.41.0
>
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Mark Cave-Ayland Oct. 17, 2023, 8:13 p.m. UTC | #2
On 17/10/2023 15:01, Philippe Mathieu-Daudé wrote:

> Access QOM parent with the proper QOM VIRTIO_SCSI_COMMON() macro.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/scsi/virtio-scsi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 45b95ea070..fa53f0902c 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -761,7 +761,7 @@ static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
>   
>   static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
>   {
> -    VirtIOSCSICommon *vs = &s->parent_obj;
> +    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
>       SCSIDevice *d;
>       int rc;

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.
Kevin Wolf Oct. 31, 2023, 1:17 p.m. UTC | #3
Am 17.10.2023 um 16:01 hat Philippe Mathieu-Daudé geschrieben:
> Access QOM parent with the proper QOM VIRTIO_SCSI_COMMON() macro.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/scsi/virtio-scsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 45b95ea070..fa53f0902c 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -761,7 +761,7 @@ static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
>  
>  static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
>  {
> -    VirtIOSCSICommon *vs = &s->parent_obj;
> +    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
>      SCSIDevice *d;
>      int rc;

Why is a dynamic cast more "proper" than a static type-safe access, even
more so in a hot I/O path?

Rich Jones posted a flamegraph the other day that surprised me because
object_class_dynamic_class_assert() and object_dynamic_cast_assert()
were shown to be a big part of scsi_req_new(). In the overall
performance, it's probably dwarved by other issues, but unnecessary
little things can add up, too.

Kevin
Peter Maydell Oct. 31, 2023, 1:21 p.m. UTC | #4
On Tue, 31 Oct 2023 at 13:18, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 17.10.2023 um 16:01 hat Philippe Mathieu-Daudé geschrieben:
> > Access QOM parent with the proper QOM VIRTIO_SCSI_COMMON() macro.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >  hw/scsi/virtio-scsi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> > index 45b95ea070..fa53f0902c 100644
> > --- a/hw/scsi/virtio-scsi.c
> > +++ b/hw/scsi/virtio-scsi.c
> > @@ -761,7 +761,7 @@ static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
> >
> >  static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
> >  {
> > -    VirtIOSCSICommon *vs = &s->parent_obj;
> > +    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
> >      SCSIDevice *d;
> >      int rc;
>
> Why is a dynamic cast more "proper" than a static type-safe access, even
> more so in a hot I/O path?

I dunno, but it is absolutely the standard QOM style to use
the cast macro to do the downcast, rather than fishing about in
the struct for the parent_obj field. If QOM casts are getting
too expensive we should ideally fix them rather than adding
more and more code that takes some side route around them...

thanks
-- PMM
Richard W.M. Jones Oct. 31, 2023, 1:48 p.m. UTC | #5
On Tue, Oct 31, 2023 at 02:17:56PM +0100, Kevin Wolf wrote:
> Am 17.10.2023 um 16:01 hat Philippe Mathieu-Daudé geschrieben:
> > Access QOM parent with the proper QOM VIRTIO_SCSI_COMMON() macro.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >  hw/scsi/virtio-scsi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> > index 45b95ea070..fa53f0902c 100644
> > --- a/hw/scsi/virtio-scsi.c
> > +++ b/hw/scsi/virtio-scsi.c
> > @@ -761,7 +761,7 @@ static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
> >  
> >  static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
> >  {
> > -    VirtIOSCSICommon *vs = &s->parent_obj;
> > +    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
> >      SCSIDevice *d;
> >      int rc;
> 
> Why is a dynamic cast more "proper" than a static type-safe access, even
> more so in a hot I/O path?
> 
> Rich Jones posted a flamegraph the other day that surprised me because
> object_class_dynamic_class_assert() and object_dynamic_cast_assert()
> were shown to be a big part of scsi_req_new(). In the overall
> performance, it's probably dwarved by other issues, but unnecessary
> little things can add up, too.

I think Kevin is referring to one of these flamegraphs:

  http://oirase.annexia.org/tmp/2023-kvm-build-on-device.svg
  http://oirase.annexia.org/tmp/2023-kvm-build.svg

Here's a zoom showing scsi_req_new (hopefully this URL is stable ...):

  http://oirase.annexia.org/tmp/2023-kvm-build-on-device.svg?s=scsi_req_new&x=512.9&y=501

Note that qemu has been compiled with QOM cast debug.  This is the
default for Fedora (not RHEL) because we'd like to get early detection
of bugs from Fedora users.

There was another patch recently where a simple change saved about 5%
of total runtime in RISC-V TCG guests (admittedly a much more hot path
than this one).

  https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg02388.html

Rich.
Kevin Wolf Oct. 31, 2023, 4:35 p.m. UTC | #6
Am 31.10.2023 um 14:48 hat Richard W.M. Jones geschrieben:
> On Tue, Oct 31, 2023 at 02:17:56PM +0100, Kevin Wolf wrote:
> > Am 17.10.2023 um 16:01 hat Philippe Mathieu-Daudé geschrieben:
> > > Access QOM parent with the proper QOM VIRTIO_SCSI_COMMON() macro.
> > > 
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > >  hw/scsi/virtio-scsi.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> > > index 45b95ea070..fa53f0902c 100644
> > > --- a/hw/scsi/virtio-scsi.c
> > > +++ b/hw/scsi/virtio-scsi.c
> > > @@ -761,7 +761,7 @@ static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
> > >  
> > >  static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
> > >  {
> > > -    VirtIOSCSICommon *vs = &s->parent_obj;
> > > +    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
> > >      SCSIDevice *d;
> > >      int rc;
> > 
> > Why is a dynamic cast more "proper" than a static type-safe access, even
> > more so in a hot I/O path?
> > 
> > Rich Jones posted a flamegraph the other day that surprised me because
> > object_class_dynamic_class_assert() and object_dynamic_cast_assert()
> > were shown to be a big part of scsi_req_new(). In the overall
> > performance, it's probably dwarved by other issues, but unnecessary
> > little things can add up, too.
> 
> I think Kevin is referring to one of these flamegraphs:
> 
>   http://oirase.annexia.org/tmp/2023-kvm-build-on-device.svg
>   http://oirase.annexia.org/tmp/2023-kvm-build.svg
> 
> Here's a zoom showing scsi_req_new (hopefully this URL is stable ...):
> 
>   http://oirase.annexia.org/tmp/2023-kvm-build-on-device.svg?s=scsi_req_new&x=512.9&y=501
> 
> Note that qemu has been compiled with QOM cast debug.  This is the
> default for Fedora (not RHEL) because we'd like to get early detection
> of bugs from Fedora users.

Indeed, object_dynamic_cast_assert() probably wouldn't show up any more
if you disabled QOM cast debugging. But we enable it by default, so
that's an explicit setting users have to make, and as you say, at least
Fedora doesn't.

The other one, object_class_dynamic_cast_assert() isn't even affected
by the debug setting because it does more than just asserting a plain
cast. Working around it isn't as trivial either, but I did think about
caching it in the object to avoid a string based class lookup for every
single request.

Looking at it again, strangely CONFIG_QOM_CAST_DEBUG enables a cache in
object_class_dynamic_cast_assert() that isn't used otherwise. Why don't
we want this optimisation without the debug flag?

Kevin
Kevin Wolf Oct. 31, 2023, 4:42 p.m. UTC | #7
Am 31.10.2023 um 14:48 hat Richard W.M. Jones geschrieben:
> On Tue, Oct 31, 2023 at 02:17:56PM +0100, Kevin Wolf wrote:
> > Am 17.10.2023 um 16:01 hat Philippe Mathieu-Daudé geschrieben:
> > > Access QOM parent with the proper QOM VIRTIO_SCSI_COMMON() macro.
> > > 
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > >  hw/scsi/virtio-scsi.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> > > index 45b95ea070..fa53f0902c 100644
> > > --- a/hw/scsi/virtio-scsi.c
> > > +++ b/hw/scsi/virtio-scsi.c
> > > @@ -761,7 +761,7 @@ static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
> > >  
> > >  static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
> > >  {
> > > -    VirtIOSCSICommon *vs = &s->parent_obj;
> > > +    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
> > >      SCSIDevice *d;
> > >      int rc;
> > 
> > Why is a dynamic cast more "proper" than a static type-safe access, even
> > more so in a hot I/O path?
> > 
> > Rich Jones posted a flamegraph the other day that surprised me because
> > object_class_dynamic_class_assert() and object_dynamic_cast_assert()
> > were shown to be a big part of scsi_req_new(). In the overall
> > performance, it's probably dwarved by other issues, but unnecessary
> > little things can add up, too.
> 
> I think Kevin is referring to one of these flamegraphs:
> 
>   http://oirase.annexia.org/tmp/2023-kvm-build-on-device.svg
>   http://oirase.annexia.org/tmp/2023-kvm-build.svg
> 
> Here's a zoom showing scsi_req_new (hopefully this URL is stable ...):
> 
>   http://oirase.annexia.org/tmp/2023-kvm-build-on-device.svg?s=scsi_req_new&x=512.9&y=501

Oh, this one (kvm-build-on-device) doesn't even show the object cast.
I was looking at kvm-build, it seems, where both the class and the
object cast are visible:

http://oirase.annexia.org/tmp/2023-kvm-build.svg?s=scsi_req_new&x=455.4&y=533

Kevin
Richard W.M. Jones Oct. 31, 2023, 4:48 p.m. UTC | #8
On Tue, Oct 31, 2023 at 05:42:37PM +0100, Kevin Wolf wrote:
> Am 31.10.2023 um 14:48 hat Richard W.M. Jones geschrieben:
> > On Tue, Oct 31, 2023 at 02:17:56PM +0100, Kevin Wolf wrote:
> > > Am 17.10.2023 um 16:01 hat Philippe Mathieu-Daudé geschrieben:
> > > > Access QOM parent with the proper QOM VIRTIO_SCSI_COMMON() macro.
> > > > 
> > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > ---
> > > >  hw/scsi/virtio-scsi.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> > > > index 45b95ea070..fa53f0902c 100644
> > > > --- a/hw/scsi/virtio-scsi.c
> > > > +++ b/hw/scsi/virtio-scsi.c
> > > > @@ -761,7 +761,7 @@ static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
> > > >  
> > > >  static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
> > > >  {
> > > > -    VirtIOSCSICommon *vs = &s->parent_obj;
> > > > +    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
> > > >      SCSIDevice *d;
> > > >      int rc;
> > > 
> > > Why is a dynamic cast more "proper" than a static type-safe access, even
> > > more so in a hot I/O path?
> > > 
> > > Rich Jones posted a flamegraph the other day that surprised me because
> > > object_class_dynamic_class_assert() and object_dynamic_cast_assert()
> > > were shown to be a big part of scsi_req_new(). In the overall
> > > performance, it's probably dwarved by other issues, but unnecessary
> > > little things can add up, too.
> > 
> > I think Kevin is referring to one of these flamegraphs:
> > 
> >   http://oirase.annexia.org/tmp/2023-kvm-build-on-device.svg
> >   http://oirase.annexia.org/tmp/2023-kvm-build.svg
> > 
> > Here's a zoom showing scsi_req_new (hopefully this URL is stable ...):
> > 
> >   http://oirase.annexia.org/tmp/2023-kvm-build-on-device.svg?s=scsi_req_new&x=512.9&y=501
> 
> Oh, this one (kvm-build-on-device) doesn't even show the object cast.
> I was looking at kvm-build, it seems, where both the class and the
> object cast are visible:
> 
> http://oirase.annexia.org/tmp/2023-kvm-build.svg?s=scsi_req_new&x=455.4&y=533

Not sure if this is the reason why, but the difference between these
two runs is that kvm-build is backed by a qcow2 file and
kvm-build-on-device is backed by a host block device.  I believe they
both were otherwise identically configured qemu.

More background in this Fedora thread:

https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/MSJAL7OE2TBO6U4ZWXKTKQLDSGRFK6YR/

Rich.
diff mbox series

Patch

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 45b95ea070..fa53f0902c 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -761,7 +761,7 @@  static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
 
 static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
 {
-    VirtIOSCSICommon *vs = &s->parent_obj;
+    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
     SCSIDevice *d;
     int rc;