| Message ID | 20211018014059.13E65746353@zero.eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | via-ide: Avoid expensive operations in irq handler | expand |
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]; >
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]; >> > >
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]; >>> >> >>
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.
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
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; ... }
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
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
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];
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(-)