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

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- 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