[PATCH] via-ide: Avoid expensive operations in irq handler

[PATCH] via-ide: Avoid expensive operations in irq handler

[BALATON Zoltan]

Cache the pointer to PCI function 0 (ISA bridge, that this IDE device
has to use for IRQs) in the PCIIDEState and pass that as the opaque
data for the interrupt handler to eliminate both the need to look up
function 0 at every interrupt and also a QOM type cast of the opaque
pointer as that's also expensive (mainly due to qom-debug being
enabled by default).

Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/via.c         | 11 ++++++-----
 include/hw/ide/pci.h |  1 +
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 82def819c4..30566bc409 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -104,15 +104,15 @@ static void bmdma_setup_bar(PCIIDEState *d)
 
 static void via_ide_set_irq(void *opaque, int n, int level)
 {
-    PCIDevice *d = PCI_DEVICE(opaque);
+    PCIIDEState *d = opaque;
 
     if (level) {
-        d->config[0x70 + n * 8] |= 0x80;
+        d->parent_obj.config[0x70 + n * 8] |= 0x80;
     } else {
-        d->config[0x70 + n * 8] &= ~0x80;
+        d->parent_obj.config[0x70 + n * 8] &= ~0x80;
     }
 
-    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
+    via_isa_set_irq(d->func0, 14 + n, level);
 }
 
 static void via_ide_reset(DeviceState *dev)
@@ -188,7 +188,8 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
     bmdma_setup_bar(d);
     pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
 
-    qdev_init_gpio_in(ds, via_ide_set_irq, 2);
+    d->func0 = pci_get_function_0(dev);
+    qdev_init_gpio_in_named_with_opaque(ds, via_ide_set_irq, d, NULL, 2);
     for (i = 0; i < 2; i++) {
         ide_bus_init(&d->bus[i], sizeof(d->bus[i]), ds, i, 2);
         ide_init2(&d->bus[i], qdev_get_gpio_in(ds, i));
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index d8384e1c42..89d14abf95 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -50,6 +50,7 @@ struct PCIIDEState {
     IDEBus bus[2];
     BMDMAState bmdma[2];
     uint32_t secondary; /* used only for cmd646 */
+    PCIDevice *func0; /* used only by IDE functions of superio chips */
     MemoryRegion bmdma_bar;
     MemoryRegion cmd_bar[2];
     MemoryRegion data_bar[2];
-- 
2.21.4

Re: [PATCH] via-ide: Avoid expensive operations in irq handler

[Philippe Mathieu-Daudé]

On 10/18/21 03:36, BALATON Zoltan wrote:
> Cache the pointer to PCI function 0 (ISA bridge, that this IDE device
> has to use for IRQs) in the PCIIDEState and pass that as the opaque
> data for the interrupt handler to eliminate both the need to look up
> function 0 at every interrupt and also a QOM type cast of the opaque
> pointer as that's also expensive (mainly due to qom-debug being
> enabled by default).
> 
> Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/ide/via.c         | 11 ++++++-----
>  include/hw/ide/pci.h |  1 +
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index 82def819c4..30566bc409 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -104,15 +104,15 @@ static void bmdma_setup_bar(PCIIDEState *d)
>  
>  static void via_ide_set_irq(void *opaque, int n, int level)
>  {
> -    PCIDevice *d = PCI_DEVICE(opaque);
> +    PCIIDEState *d = opaque;
>  
>      if (level) {
> -        d->config[0x70 + n * 8] |= 0x80;
> +        d->parent_obj.config[0x70 + n * 8] |= 0x80;
>      } else {
> -        d->config[0x70 + n * 8] &= ~0x80;
> +        d->parent_obj.config[0x70 + n * 8] &= ~0x80;
>      }

Better not to access parent_obj, so I'd rather keep the previous
code as it. The rest is OK, thanks. If you don't want to respin
I can fix and take via mips-next.

>  
> -    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
> +    via_isa_set_irq(d->func0, 14 + n, level);
>  }
>  
>  static void via_ide_reset(DeviceState *dev)
> @@ -188,7 +188,8 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
>      bmdma_setup_bar(d);
>      pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
>  
> -    qdev_init_gpio_in(ds, via_ide_set_irq, 2);
> +    d->func0 = pci_get_function_0(dev);
> +    qdev_init_gpio_in_named_with_opaque(ds, via_ide_set_irq, d, NULL, 2);
>      for (i = 0; i < 2; i++) {
>          ide_bus_init(&d->bus[i], sizeof(d->bus[i]), ds, i, 2);
>          ide_init2(&d->bus[i], qdev_get_gpio_in(ds, i));
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index d8384e1c42..89d14abf95 100644
> --- a/include/hw/ide/pci.h
> +++ b/include/hw/ide/pci.h
> @@ -50,6 +50,7 @@ struct PCIIDEState {
>      IDEBus bus[2];
>      BMDMAState bmdma[2];
>      uint32_t secondary; /* used only for cmd646 */
> +    PCIDevice *func0; /* used only by IDE functions of superio chips */
>      MemoryRegion bmdma_bar;
>      MemoryRegion cmd_bar[2];
>      MemoryRegion data_bar[2];
> 

Re: [PATCH] via-ide: Avoid expensive operations in irq handler

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3116 bytes --]

On Mon, 18 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/18/21 03:36, BALATON Zoltan wrote:
>> Cache the pointer to PCI function 0 (ISA bridge, that this IDE device
>> has to use for IRQs) in the PCIIDEState and pass that as the opaque
>> data for the interrupt handler to eliminate both the need to look up
>> function 0 at every interrupt and also a QOM type cast of the opaque
>> pointer as that's also expensive (mainly due to qom-debug being
>> enabled by default).
>>
>> Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/ide/via.c         | 11 ++++++-----
>>  include/hw/ide/pci.h |  1 +
>>  2 files changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index 82def819c4..30566bc409 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -104,15 +104,15 @@ static void bmdma_setup_bar(PCIIDEState *d)
>>
>>  static void via_ide_set_irq(void *opaque, int n, int level)
>>  {
>> -    PCIDevice *d = PCI_DEVICE(opaque);
>> +    PCIIDEState *d = opaque;
>>
>>      if (level) {
>> -        d->config[0x70 + n * 8] |= 0x80;
>> +        d->parent_obj.config[0x70 + n * 8] |= 0x80;
>>      } else {
>> -        d->config[0x70 + n * 8] &= ~0x80;
>> +        d->parent_obj.config[0x70 + n * 8] &= ~0x80;
>>      }
>
> Better not to access parent_obj, so I'd rather keep the previous
> code as it. The rest is OK, thanks. If you don't want to respin
> I can fix and take via mips-next.

Why not? If it's OK to access other fields why not parent_obj? That avoids 
the QOM cast PCI_DEVICE(opaque) or PCI_DEVICE(d) after this patch. We know 
it's a PCIIDEState and has PCIDevice parent_obj field because we set the 
opaque pointer when adding this callback so I think this works and is the 
less expensive way. But feel free to change it any way you like and use it 
that way. I'd keep it as it is.

Reagards,
BALATON Zoltan

>> -    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
>> +    via_isa_set_irq(d->func0, 14 + n, level);
>>  }
>>
>>  static void via_ide_reset(DeviceState *dev)
>> @@ -188,7 +188,8 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
>>      bmdma_setup_bar(d);
>>      pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
>>
>> -    qdev_init_gpio_in(ds, via_ide_set_irq, 2);
>> +    d->func0 = pci_get_function_0(dev);
>> +    qdev_init_gpio_in_named_with_opaque(ds, via_ide_set_irq, d, NULL, 2);
>>      for (i = 0; i < 2; i++) {
>>          ide_bus_init(&d->bus[i], sizeof(d->bus[i]), ds, i, 2);
>>          ide_init2(&d->bus[i], qdev_get_gpio_in(ds, i));
>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>> index d8384e1c42..89d14abf95 100644
>> --- a/include/hw/ide/pci.h
>> +++ b/include/hw/ide/pci.h
>> @@ -50,6 +50,7 @@ struct PCIIDEState {
>>      IDEBus bus[2];
>>      BMDMAState bmdma[2];
>>      uint32_t secondary; /* used only for cmd646 */
>> +    PCIDevice *func0; /* used only by IDE functions of superio chips */
>>      MemoryRegion bmdma_bar;
>>      MemoryRegion cmd_bar[2];
>>      MemoryRegion data_bar[2];
>>
>
>

Re: [PATCH] via-ide: Avoid expensive operations in irq handler

[Philippe Mathieu-Daudé]

On 10/18/21 11:51, BALATON Zoltan wrote:
> On Mon, 18 Oct 2021, Philippe Mathieu-Daudé wrote:
>> On 10/18/21 03:36, BALATON Zoltan wrote:
>>> Cache the pointer to PCI function 0 (ISA bridge, that this IDE device
>>> has to use for IRQs) in the PCIIDEState and pass that as the opaque
>>> data for the interrupt handler to eliminate both the need to look up
>>> function 0 at every interrupt and also a QOM type cast of the opaque
>>> pointer as that's also expensive (mainly due to qom-debug being
>>> enabled by default).
>>>
>>> Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/ide/via.c         | 11 ++++++-----
>>>  include/hw/ide/pci.h |  1 +
>>>  2 files changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index 82def819c4..30566bc409 100644
>>> --- a/hw/ide/via.c
>>> +++ b/hw/ide/via.c
>>> @@ -104,15 +104,15 @@ static void bmdma_setup_bar(PCIIDEState *d)
>>>
>>>  static void via_ide_set_irq(void *opaque, int n, int level)
>>>  {
>>> -    PCIDevice *d = PCI_DEVICE(opaque);
>>> +    PCIIDEState *d = opaque;
>>>
>>>      if (level) {
>>> -        d->config[0x70 + n * 8] |= 0x80;
>>> +        d->parent_obj.config[0x70 + n * 8] |= 0x80;
>>>      } else {
>>> -        d->config[0x70 + n * 8] &= ~0x80;
>>> +        d->parent_obj.config[0x70 + n * 8] &= ~0x80;
>>>      }
>>
>> Better not to access parent_obj, so I'd rather keep the previous
>> code as it. The rest is OK, thanks. If you don't want to respin
>> I can fix and take via mips-next.
> 
> Why not? If it's OK to access other fields why not parent_obj? That
> avoids the QOM cast PCI_DEVICE(opaque) or PCI_DEVICE(d) after this
> patch. We know it's a PCIIDEState and has PCIDevice parent_obj field
> because we set the opaque pointer when adding this callback so I think
> this works and is the less expensive way. But feel free to change it any
> way you like and use it that way. I'd keep it as it is.

My understanding of QOM is we shouldn't access internal states that
way, because 1/ this makes object refactors harder and 2/ this is
not the style/example we want in the codebase, but it doesn't seem
documented, so Cc'ing Markus/Eduardo for confirmation.

> 
> Reagards,
> BALATON Zoltan
> 
>>> -    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
>>> +    via_isa_set_irq(d->func0, 14 + n, level);
>>>  }
>>>
>>>  static void via_ide_reset(DeviceState *dev)
>>> @@ -188,7 +188,8 @@ static void via_ide_realize(PCIDevice *dev, Error
>>> **errp)
>>>      bmdma_setup_bar(d);
>>>      pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
>>>
>>> -    qdev_init_gpio_in(ds, via_ide_set_irq, 2);
>>> +    d->func0 = pci_get_function_0(dev);
>>> +    qdev_init_gpio_in_named_with_opaque(ds, via_ide_set_irq, d,
>>> NULL, 2);
>>>      for (i = 0; i < 2; i++) {
>>>          ide_bus_init(&d->bus[i], sizeof(d->bus[i]), ds, i, 2);
>>>          ide_init2(&d->bus[i], qdev_get_gpio_in(ds, i));
>>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>>> index d8384e1c42..89d14abf95 100644
>>> --- a/include/hw/ide/pci.h
>>> +++ b/include/hw/ide/pci.h
>>> @@ -50,6 +50,7 @@ struct PCIIDEState {
>>>      IDEBus bus[2];
>>>      BMDMAState bmdma[2];
>>>      uint32_t secondary; /* used only for cmd646 */
>>> +    PCIDevice *func0; /* used only by IDE functions of superio chips */
>>>      MemoryRegion bmdma_bar;
>>>      MemoryRegion cmd_bar[2];
>>>      MemoryRegion data_bar[2];
>>>
>>
>>

Re: [PATCH] via-ide: Avoid expensive operations in irq handler

[Eduardo Habkost]

On Mon, Oct 18, 2021 at 12:10:04PM +0200, Philippe Mathieu-Daudé wrote:
> On 10/18/21 11:51, BALATON Zoltan wrote:
> > On Mon, 18 Oct 2021, Philippe Mathieu-Daudé wrote:
> >> On 10/18/21 03:36, BALATON Zoltan wrote:
> >>> Cache the pointer to PCI function 0 (ISA bridge, that this IDE device
> >>> has to use for IRQs) in the PCIIDEState and pass that as the opaque
> >>> data for the interrupt handler to eliminate both the need to look up
> >>> function 0 at every interrupt and also a QOM type cast of the opaque
> >>> pointer as that's also expensive (mainly due to qom-debug being
> >>> enabled by default).
> >>>
> >>> Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >>> ---
> >>>  hw/ide/via.c         | 11 ++++++-----
> >>>  include/hw/ide/pci.h |  1 +
> >>>  2 files changed, 7 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/hw/ide/via.c b/hw/ide/via.c
> >>> index 82def819c4..30566bc409 100644
> >>> --- a/hw/ide/via.c
> >>> +++ b/hw/ide/via.c
> >>> @@ -104,15 +104,15 @@ static void bmdma_setup_bar(PCIIDEState *d)
> >>>
> >>>  static void via_ide_set_irq(void *opaque, int n, int level)
> >>>  {
> >>> -    PCIDevice *d = PCI_DEVICE(opaque);
> >>> +    PCIIDEState *d = opaque;
> >>>
> >>>      if (level) {
> >>> -        d->config[0x70 + n * 8] |= 0x80;
> >>> +        d->parent_obj.config[0x70 + n * 8] |= 0x80;
> >>>      } else {
> >>> -        d->config[0x70 + n * 8] &= ~0x80;
> >>> +        d->parent_obj.config[0x70 + n * 8] &= ~0x80;
> >>>      }
> >>
> >> Better not to access parent_obj, so I'd rather keep the previous
> >> code as it. The rest is OK, thanks. If you don't want to respin
> >> I can fix and take via mips-next.
> > 
> > Why not? If it's OK to access other fields why not parent_obj? That
> > avoids the QOM cast PCI_DEVICE(opaque) or PCI_DEVICE(d) after this
> > patch. We know it's a PCIIDEState and has PCIDevice parent_obj field
> > because we set the opaque pointer when adding this callback so I think
> > this works and is the less expensive way. But feel free to change it any
> > way you like and use it that way. I'd keep it as it is.
> 
> My understanding of QOM is we shouldn't access internal states that
> way, because 1/ this makes object refactors harder and 2/ this is
> not the style/example we want in the codebase, but it doesn't seem
> documented, so Cc'ing Markus/Eduardo for confirmation.

`PCI_DEVICE(d)` is preferred instead `of d.parent_obj` (parent_obj is
just a QOM implementation detail).  If there are real performance
reasons to avoid it, we need numbers to support that.

Also, note that `OBJECT_CHECK(obj)` is just `return obj` if
CONFIG_QOM_CAST_DEBUG is disabled.

-- 
Eduardo

Re: [PATCH] via-ide: Avoid expensive operations in irq handler

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3248 bytes --]

On Wed, 20 Oct 2021, Eduardo Habkost wrote:
> On Mon, Oct 18, 2021 at 12:10:04PM +0200, Philippe Mathieu-Daudé wrote:
>> On 10/18/21 11:51, BALATON Zoltan wrote:
>>> On Mon, 18 Oct 2021, Philippe Mathieu-Daudé wrote:
>>>> On 10/18/21 03:36, BALATON Zoltan wrote:
>>>>> Cache the pointer to PCI function 0 (ISA bridge, that this IDE device
>>>>> has to use for IRQs) in the PCIIDEState and pass that as the opaque
>>>>> data for the interrupt handler to eliminate both the need to look up
>>>>> function 0 at every interrupt and also a QOM type cast of the opaque
>>>>> pointer as that's also expensive (mainly due to qom-debug being
>>>>> enabled by default).
>>>>>
>>>>> Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>> ---
>>>>>  hw/ide/via.c         | 11 ++++++-----
>>>>>  include/hw/ide/pci.h |  1 +
>>>>>  2 files changed, 7 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>>>> index 82def819c4..30566bc409 100644
>>>>> --- a/hw/ide/via.c
>>>>> +++ b/hw/ide/via.c
>>>>> @@ -104,15 +104,15 @@ static void bmdma_setup_bar(PCIIDEState *d)
>>>>>
>>>>>  static void via_ide_set_irq(void *opaque, int n, int level)
>>>>>  {
>>>>> -    PCIDevice *d = PCI_DEVICE(opaque);
>>>>> +    PCIIDEState *d = opaque;
>>>>>
>>>>>      if (level) {
>>>>> -        d->config[0x70 + n * 8] |= 0x80;
>>>>> +        d->parent_obj.config[0x70 + n * 8] |= 0x80;
>>>>>      } else {
>>>>> -        d->config[0x70 + n * 8] &= ~0x80;
>>>>> +        d->parent_obj.config[0x70 + n * 8] &= ~0x80;
>>>>>      }
>>>>
>>>> Better not to access parent_obj, so I'd rather keep the previous
>>>> code as it. The rest is OK, thanks. If you don't want to respin
>>>> I can fix and take via mips-next.
>>>
>>> Why not? If it's OK to access other fields why not parent_obj? That
>>> avoids the QOM cast PCI_DEVICE(opaque) or PCI_DEVICE(d) after this
>>> patch. We know it's a PCIIDEState and has PCIDevice parent_obj field
>>> because we set the opaque pointer when adding this callback so I think
>>> this works and is the less expensive way. But feel free to change it any
>>> way you like and use it that way. I'd keep it as it is.
>>
>> My understanding of QOM is we shouldn't access internal states that
>> way, because 1/ this makes object refactors harder and 2/ this is
>> not the style/example we want in the codebase, but it doesn't seem
>> documented, so Cc'ing Markus/Eduardo for confirmation.
>
> `PCI_DEVICE(d)` is preferred instead `of d.parent_obj` (parent_obj is
> just a QOM implementation detail).  If there are real performance
> reasons to avoid it, we need numbers to support that.

OK I can try to do some measurement or go back to PCI_DEVICE() then.

> Also, note that `OBJECT_CHECK(obj)` is just `return obj` if
> CONFIG_QOM_CAST_DEBUG is disabled.

But configure enables it by default even without --enable-debug so I think 
most people don't even know and it's enabled practically everywhere 
(probably even in distros). Why can't we have it disabled by default if 
it's a developer debug option and enable it only for developers where 
it's useful to catch bugs?

Regards,
BALATON Zoltan

Re: [PATCH] via-ide: Avoid expensive operations in irq handler

[Eduardo Habkost]

On Wed, Oct 20, 2021 at 04:58:58PM +0200, BALATON Zoltan wrote:
> On Wed, 20 Oct 2021, Eduardo Habkost wrote:
> > On Mon, Oct 18, 2021 at 12:10:04PM +0200, Philippe Mathieu-Daudé wrote:
> > > On 10/18/21 11:51, BALATON Zoltan wrote:
> > > > On Mon, 18 Oct 2021, Philippe Mathieu-Daudé wrote:
> > > > > On 10/18/21 03:36, BALATON Zoltan wrote:
> > > > > > Cache the pointer to PCI function 0 (ISA bridge, that this IDE device
> > > > > > has to use for IRQs) in the PCIIDEState and pass that as the opaque
> > > > > > data for the interrupt handler to eliminate both the need to look up
> > > > > > function 0 at every interrupt and also a QOM type cast of the opaque
> > > > > > pointer as that's also expensive (mainly due to qom-debug being
> > > > > > enabled by default).
> > > > > > 
> > > > > > Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > > > > ---
> > > > > >  hw/ide/via.c         | 11 ++++++-----
> > > > > >  include/hw/ide/pci.h |  1 +
> > > > > >  2 files changed, 7 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/ide/via.c b/hw/ide/via.c
> > > > > > index 82def819c4..30566bc409 100644
> > > > > > --- a/hw/ide/via.c
> > > > > > +++ b/hw/ide/via.c
> > > > > > @@ -104,15 +104,15 @@ static void bmdma_setup_bar(PCIIDEState *d)
> > > > > > 
> > > > > >  static void via_ide_set_irq(void *opaque, int n, int level)
> > > > > >  {
> > > > > > -    PCIDevice *d = PCI_DEVICE(opaque);
> > > > > > +    PCIIDEState *d = opaque;
> > > > > > 
> > > > > >      if (level) {
> > > > > > -        d->config[0x70 + n * 8] |= 0x80;
> > > > > > +        d->parent_obj.config[0x70 + n * 8] |= 0x80;
> > > > > >      } else {
> > > > > > -        d->config[0x70 + n * 8] &= ~0x80;
> > > > > > +        d->parent_obj.config[0x70 + n * 8] &= ~0x80;
> > > > > >      }
> > > > > 
> > > > > Better not to access parent_obj, so I'd rather keep the previous
> > > > > code as it. The rest is OK, thanks. If you don't want to respin
> > > > > I can fix and take via mips-next.
> > > > 
> > > > Why not? If it's OK to access other fields why not parent_obj? That
> > > > avoids the QOM cast PCI_DEVICE(opaque) or PCI_DEVICE(d) after this
> > > > patch. We know it's a PCIIDEState and has PCIDevice parent_obj field
> > > > because we set the opaque pointer when adding this callback so I think
> > > > this works and is the less expensive way. But feel free to change it any
> > > > way you like and use it that way. I'd keep it as it is.
> > > 
> > > My understanding of QOM is we shouldn't access internal states that
> > > way, because 1/ this makes object refactors harder and 2/ this is
> > > not the style/example we want in the codebase, but it doesn't seem
> > > documented, so Cc'ing Markus/Eduardo for confirmation.
> > 
> > `PCI_DEVICE(d)` is preferred instead `of d.parent_obj` (parent_obj is
> > just a QOM implementation detail).  If there are real performance
> > reasons to avoid it, we need numbers to support that.
> 
> OK I can try to do some measurement or go back to PCI_DEVICE() then.
> 
> > Also, note that `OBJECT_CHECK(obj)` is just `return obj` if
> > CONFIG_QOM_CAST_DEBUG is disabled.
> 
> But configure enables it by default even without --enable-debug so I think
> most people don't even know and it's enabled practically everywhere
> (probably even in distros). Why can't we have it disabled by default if it's
> a developer debug option and enable it only for developers where it's useful
> to catch bugs?

It's a trade-off, I don't think there's a right answer for
everybody.  Personally, I'd say the benefits outweigh the risks
of disabling it for most users (especially the ones building QEMU
directly from source).

I don't mean to say it's OK for CONFIG_QOM_CAST_DEBUG=y builds to
have visibly poor performance.  If the typecast above causes a
measurable performance impact, it might be reasonable to have a
workaround like:

  static void via_ide_set_irq(void *opaque, int n, int level)
  {
      PCIIDEState *ide = opaque;
      PCIDevice *pci = opaque;
      ...
  }

-- 
Eduardo

Re: [PATCH] via-ide: Avoid expensive operations in irq handler

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 4650 bytes --]

On Wed, Oct 20, 2021 at 11:18 AM Eduardo Habkost <ehabkost@redhat.com>
wrote:

> On Wed, Oct 20, 2021 at 04:58:58PM +0200, BALATON Zoltan wrote:
> > On Wed, 20 Oct 2021, Eduardo Habkost wrote:
> > > On Mon, Oct 18, 2021 at 12:10:04PM +0200, Philippe Mathieu-Daudé wrote:
> > > > On 10/18/21 11:51, BALATON Zoltan wrote:
> > > > > On Mon, 18 Oct 2021, Philippe Mathieu-Daudé wrote:
> > > > > > On 10/18/21 03:36, BALATON Zoltan wrote:
> > > > > > > Cache the pointer to PCI function 0 (ISA bridge, that this IDE
> device
> > > > > > > has to use for IRQs) in the PCIIDEState and pass that as the
> opaque
> > > > > > > data for the interrupt handler to eliminate both the need to
> look up
> > > > > > > function 0 at every interrupt and also a QOM type cast of the
> opaque
> > > > > > > pointer as that's also expensive (mainly due to qom-debug being
> > > > > > > enabled by default).
> > > > > > >
> > > > > > > Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > > > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > > > > > ---
> > > > > > >  hw/ide/via.c         | 11 ++++++-----
> > > > > > >  include/hw/ide/pci.h |  1 +
> > > > > > >  2 files changed, 7 insertions(+), 5 deletions(-)
> > > > > > >
> > > > > > > diff --git a/hw/ide/via.c b/hw/ide/via.c
> > > > > > > index 82def819c4..30566bc409 100644
> > > > > > > --- a/hw/ide/via.c
> > > > > > > +++ b/hw/ide/via.c
> > > > > > > @@ -104,15 +104,15 @@ static void bmdma_setup_bar(PCIIDEState
> *d)
> > > > > > >
> > > > > > >  static void via_ide_set_irq(void *opaque, int n, int level)
> > > > > > >  {
> > > > > > > -    PCIDevice *d = PCI_DEVICE(opaque);
> > > > > > > +    PCIIDEState *d = opaque;
> > > > > > >
> > > > > > >      if (level) {
> > > > > > > -        d->config[0x70 + n * 8] |= 0x80;
> > > > > > > +        d->parent_obj.config[0x70 + n * 8] |= 0x80;
> > > > > > >      } else {
> > > > > > > -        d->config[0x70 + n * 8] &= ~0x80;
> > > > > > > +        d->parent_obj.config[0x70 + n * 8] &= ~0x80;
> > > > > > >      }
> > > > > >
> > > > > > Better not to access parent_obj, so I'd rather keep the previous
> > > > > > code as it. The rest is OK, thanks. If you don't want to respin
> > > > > > I can fix and take via mips-next.
> > > > >
> > > > > Why not? If it's OK to access other fields why not parent_obj? That
> > > > > avoids the QOM cast PCI_DEVICE(opaque) or PCI_DEVICE(d) after this
> > > > > patch. We know it's a PCIIDEState and has PCIDevice parent_obj
> field
> > > > > because we set the opaque pointer when adding this callback so I
> think
> > > > > this works and is the less expensive way. But feel free to change
> it any
> > > > > way you like and use it that way. I'd keep it as it is.
> > > >
> > > > My understanding of QOM is we shouldn't access internal states that
> > > > way, because 1/ this makes object refactors harder and 2/ this is
> > > > not the style/example we want in the codebase, but it doesn't seem
> > > > documented, so Cc'ing Markus/Eduardo for confirmation.
> > >
> > > `PCI_DEVICE(d)` is preferred instead `of d.parent_obj` (parent_obj is
> > > just a QOM implementation detail).  If there are real performance
> > > reasons to avoid it, we need numbers to support that.
> >
> > OK I can try to do some measurement or go back to PCI_DEVICE() then.
> >
> > > Also, note that `OBJECT_CHECK(obj)` is just `return obj` if
> > > CONFIG_QOM_CAST_DEBUG is disabled.
> >
> > But configure enables it by default even without --enable-debug so I
> think
> > most people don't even know and it's enabled practically everywhere
> > (probably even in distros). Why can't we have it disabled by default if
> it's
> > a developer debug option and enable it only for developers where it's
> useful
> > to catch bugs?
>
> It's a trade-off, I don't think there's a right answer for
> everybody.  Personally, I'd say the benefits outweigh the risks
> of disabling it for most users (especially the ones building QEMU
> directly from source).
>
> I don't mean to say it's OK for CONFIG_QOM_CAST_DEBUG=y builds to
> have visibly poor performance.  If the typecast above causes a
> measurable performance impact, it might be reasonable to have a
> workaround like:
>
>   static void via_ide_set_irq(void *opaque, int n, int level)
>   {
>       PCIIDEState *ide = opaque;
>       PCIDevice *pci = opaque;
>       ...
>   }
>
> --
> Eduardo
>
>
For the record here, I'm going to defer to consensus judgment between
Eduardo, Philippe, and BALATON. Let me know when you're all happy with the
patch.

--js

[-- Attachment #2: Type: text/html, Size: 6337 bytes --]

Re: [PATCH] via-ide: Avoid expensive operations in irq handler

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3630 bytes --]

On Wed, 20 Oct 2021, Eduardo Habkost wrote:
> On Mon, Oct 18, 2021 at 12:10:04PM +0200, Philippe Mathieu-Daudé wrote:
>> On 10/18/21 11:51, BALATON Zoltan wrote:
>>> On Mon, 18 Oct 2021, Philippe Mathieu-Daudé wrote:
>>>> On 10/18/21 03:36, BALATON Zoltan wrote:
>>>>> Cache the pointer to PCI function 0 (ISA bridge, that this IDE device
>>>>> has to use for IRQs) in the PCIIDEState and pass that as the opaque
>>>>> data for the interrupt handler to eliminate both the need to look up
>>>>> function 0 at every interrupt and also a QOM type cast of the opaque
>>>>> pointer as that's also expensive (mainly due to qom-debug being
>>>>> enabled by default).
>>>>>
>>>>> Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>> ---
>>>>>  hw/ide/via.c         | 11 ++++++-----
>>>>>  include/hw/ide/pci.h |  1 +
>>>>>  2 files changed, 7 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>>>> index 82def819c4..30566bc409 100644
>>>>> --- a/hw/ide/via.c
>>>>> +++ b/hw/ide/via.c
>>>>> @@ -104,15 +104,15 @@ static void bmdma_setup_bar(PCIIDEState *d)
>>>>>
>>>>>  static void via_ide_set_irq(void *opaque, int n, int level)
>>>>>  {
>>>>> -    PCIDevice *d = PCI_DEVICE(opaque);
>>>>> +    PCIIDEState *d = opaque;
>>>>>
>>>>>      if (level) {
>>>>> -        d->config[0x70 + n * 8] |= 0x80;
>>>>> +        d->parent_obj.config[0x70 + n * 8] |= 0x80;
>>>>>      } else {
>>>>> -        d->config[0x70 + n * 8] &= ~0x80;
>>>>> +        d->parent_obj.config[0x70 + n * 8] &= ~0x80;
>>>>>      }
>>>>
>>>> Better not to access parent_obj, so I'd rather keep the previous
>>>> code as it. The rest is OK, thanks. If you don't want to respin
>>>> I can fix and take via mips-next.
>>>
>>> Why not? If it's OK to access other fields why not parent_obj? That
>>> avoids the QOM cast PCI_DEVICE(opaque) or PCI_DEVICE(d) after this
>>> patch. We know it's a PCIIDEState and has PCIDevice parent_obj field
>>> because we set the opaque pointer when adding this callback so I think
>>> this works and is the less expensive way. But feel free to change it any
>>> way you like and use it that way. I'd keep it as it is.
>>
>> My understanding of QOM is we shouldn't access internal states that
>> way, because 1/ this makes object refactors harder and 2/ this is
>> not the style/example we want in the codebase, but it doesn't seem
>> documented, so Cc'ing Markus/Eduardo for confirmation.
>
> `PCI_DEVICE(d)` is preferred instead `of d.parent_obj` (parent_obj is
> just a QOM implementation detail).  If there are real performance
> reasons to avoid it, we need numbers to support that.
>
> Also, note that `OBJECT_CHECK(obj)` is just `return obj` if
> CONFIG_QOM_CAST_DEBUG is disabled.

I've tried to do some measurements but could be I did not come up with a 
good enough test (I was just trying to copy a few hundred megabytes and 
timed that) as I could not find any significant difference with or without 
QOM casts or even without this patch (so even caching func0 did not seem 
to make a difference). Could be there are other bigger bottlenecks 
elsewhere before this becomes critical so for now maybe just drop this 
patch and the similar one for USB (that is first and last patch in the 
series) and take the rest of the series only until somebody can do some 
better tests to see if this optimisation would help.

Gerd, Philippe which of you can take care of merging the remaining 
patches? That's still needed to fix USB interrupt on pegasos2.

Regards,
BALATON Zoltan