| Message ID | 20181105110313.29312-3-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x/zpci: some hotplug handler cleanups | expand |
On Mon, 5 Nov 2018 12:03:11 +0100 David Hildenbrand <david@redhat.com> wrote: > We directly have it in our hands. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/s390x/s390-pci-bus.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > index 1eaae3aca6..68660eac74 100644 > --- a/hw/s390x/s390-pci-bus.c > +++ b/hw/s390x/s390-pci-bus.c > @@ -814,9 +814,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev) > static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > Error **errp) > { > + S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev); > PCIDevice *pdev = NULL; > S390PCIBusDevice *pbdev = NULL; > - S390pciState *s = s390_get_phb(); > > if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) { > BusState *bus; > @@ -924,11 +924,11 @@ static void s390_pcihost_timer_cb(void *opaque) > static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, > Error **errp) > { > + S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev); > PCIDevice *pci_dev = NULL; > PCIBus *bus; > int32_t devfn; > S390PCIBusDevice *pbdev = NULL; > - S390pciState *s = s390_get_phb(); > > if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) { > error_setg(errp, "PCI bridge hot unplug currently not supported"); Not sure whether that is an improvement (s390_get_phb() caches the value, and is called from multiple other places as well.)
On 05.11.18 12:21, Cornelia Huck wrote: > On Mon, 5 Nov 2018 12:03:11 +0100 > David Hildenbrand <david@redhat.com> wrote: > >> We directly have it in our hands. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> hw/s390x/s390-pci-bus.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c >> index 1eaae3aca6..68660eac74 100644 >> --- a/hw/s390x/s390-pci-bus.c >> +++ b/hw/s390x/s390-pci-bus.c >> @@ -814,9 +814,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev) >> static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev, >> Error **errp) >> { >> + S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev); >> PCIDevice *pdev = NULL; >> S390PCIBusDevice *pbdev = NULL; >> - S390pciState *s = s390_get_phb(); >> >> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) { >> BusState *bus; >> @@ -924,11 +924,11 @@ static void s390_pcihost_timer_cb(void *opaque) >> static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, >> Error **errp) >> { >> + S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev); >> PCIDevice *pci_dev = NULL; >> PCIBus *bus; >> int32_t devfn; >> S390PCIBusDevice *pbdev = NULL; >> - S390pciState *s = s390_get_phb(); >> >> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) { >> error_setg(errp, "PCI bridge hot unplug currently not supported"); > > Not sure whether that is an improvement (s390_get_phb() caches the > value, and is called from multiple other places as well.) > Looking up a variable that is directly passed as an argument doesn't look clean to me.
On 11/05/2018 12:37 PM, David Hildenbrand wrote: > On 05.11.18 12:21, Cornelia Huck wrote: >> On Mon, 5 Nov 2018 12:03:11 +0100 >> David Hildenbrand <david@redhat.com> wrote: >> >>> We directly have it in our hands. >>> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> --- >>> hw/s390x/s390-pci-bus.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c >>> index 1eaae3aca6..68660eac74 100644 >>> --- a/hw/s390x/s390-pci-bus.c >>> +++ b/hw/s390x/s390-pci-bus.c >>> @@ -814,9 +814,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev) >>> static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev, >>> Error **errp) >>> { >>> + S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev); >>> PCIDevice *pdev = NULL; >>> S390PCIBusDevice *pbdev = NULL; >>> - S390pciState *s = s390_get_phb(); >>> >>> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) { >>> BusState *bus; >>> @@ -924,11 +924,11 @@ static void s390_pcihost_timer_cb(void *opaque) >>> static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, >>> Error **errp) >>> { >>> + S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev); >>> PCIDevice *pci_dev = NULL; >>> PCIBus *bus; >>> int32_t devfn; >>> S390PCIBusDevice *pbdev = NULL; >>> - S390pciState *s = s390_get_phb(); >>> >>> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) { >>> error_setg(errp, "PCI bridge hot unplug currently not supported"); >> >> Not sure whether that is an improvement (s390_get_phb() caches the >> value, and is called from multiple other places as well.) >> > > Looking up a variable that is directly passed as an argument doesn't > look clean to me. I think there was a reason for this caching, namely that qom resolution can be quite expensive. For the hotplug case this obviously does not matter but for all the other cases it might. So do we really want to have different places use different methods?
On 05.11.18 12:40, Christian Borntraeger wrote: > > > On 11/05/2018 12:37 PM, David Hildenbrand wrote: >> On 05.11.18 12:21, Cornelia Huck wrote: >>> On Mon, 5 Nov 2018 12:03:11 +0100 >>> David Hildenbrand <david@redhat.com> wrote: >>> >>>> We directly have it in our hands. >>>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> hw/s390x/s390-pci-bus.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c >>>> index 1eaae3aca6..68660eac74 100644 >>>> --- a/hw/s390x/s390-pci-bus.c >>>> +++ b/hw/s390x/s390-pci-bus.c >>>> @@ -814,9 +814,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev) >>>> static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev, >>>> Error **errp) >>>> { >>>> + S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev); >>>> PCIDevice *pdev = NULL; >>>> S390PCIBusDevice *pbdev = NULL; >>>> - S390pciState *s = s390_get_phb(); >>>> >>>> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) { >>>> BusState *bus; >>>> @@ -924,11 +924,11 @@ static void s390_pcihost_timer_cb(void *opaque) >>>> static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, >>>> Error **errp) >>>> { >>>> + S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev); >>>> PCIDevice *pci_dev = NULL; >>>> PCIBus *bus; >>>> int32_t devfn; >>>> S390PCIBusDevice *pbdev = NULL; >>>> - S390pciState *s = s390_get_phb(); >>>> >>>> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) { >>>> error_setg(errp, "PCI bridge hot unplug currently not supported"); >>> >>> Not sure whether that is an improvement (s390_get_phb() caches the >>> value, and is called from multiple other places as well.) >>> >> >> Looking up a variable that is directly passed as an argument doesn't >> look clean to me. > > I think there was a reason for this caching, namely that qom resolution can > be quite expensive. For the hotplug case this obviously does not matter but > for all the other cases it might. So do we really want to have different > places use different methods? > Caching resolution is fine (as that is expensive), caching a downcast is as far as I remember not necessary. Especially, as you said, for hotplug handlers. Anyhow, if there are strong feelings to this change, I can drop this patch. There are certainly more important things to do in zPCI hotplug code.
On 11/5/18 6:50 AM, David Hildenbrand wrote: > On 05.11.18 12:40, Christian Borntraeger wrote: >> >> >> On 11/05/2018 12:37 PM, David Hildenbrand wrote: >>> On 05.11.18 12:21, Cornelia Huck wrote: >>>> On Mon, 5 Nov 2018 12:03:11 +0100 >>>> David Hildenbrand <david@redhat.com> wrote: >>>> >>>>> We directly have it in our hands. >>>>> >>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>> --- >>>>> hw/s390x/s390-pci-bus.c | 4 ++-- >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c >>>>> index 1eaae3aca6..68660eac74 100644 >>>>> --- a/hw/s390x/s390-pci-bus.c >>>>> +++ b/hw/s390x/s390-pci-bus.c >>>>> @@ -814,9 +814,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev) >>>>> static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev, >>>>> Error **errp) >>>>> { >>>>> + S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev); >>>>> PCIDevice *pdev = NULL; >>>>> S390PCIBusDevice *pbdev = NULL; >>>>> - S390pciState *s = s390_get_phb(); >>>>> >>>>> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) { >>>>> BusState *bus; >>>>> @@ -924,11 +924,11 @@ static void s390_pcihost_timer_cb(void *opaque) >>>>> static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, >>>>> Error **errp) >>>>> { >>>>> + S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev); >>>>> PCIDevice *pci_dev = NULL; >>>>> PCIBus *bus; >>>>> int32_t devfn; >>>>> S390PCIBusDevice *pbdev = NULL; >>>>> - S390pciState *s = s390_get_phb(); >>>>> >>>>> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) { >>>>> error_setg(errp, "PCI bridge hot unplug currently not supported"); >>>> >>>> Not sure whether that is an improvement (s390_get_phb() caches the >>>> value, and is called from multiple other places as well.) >>>> >>> >>> Looking up a variable that is directly passed as an argument doesn't >>> look clean to me. >> >> I think there was a reason for this caching, namely that qom resolution can >> be quite expensive. For the hotplug case this obviously does not matter but >> for all the other cases it might. So do we really want to have different >> places use different methods? >> > > Caching resolution is fine (as that is expensive), caching a downcast is > as far as I remember not necessary. Especially, as you said, for hotplug > handlers. > > Anyhow, if there are strong feelings to this change, I can drop this > patch. There are certainly more important things to do in zPCI hotplug code. > > Truthfully, I'm not in favor of one over the other. As long as the device handlers are consistent, I think either is fine. However, it would be nice if at some point during plug we cache the PHB somewhere. That would be some sort of best-of-both-worlds approach.
On Wed, 7 Nov 2018 15:28:31 -0500 Collin Walling <walling@linux.ibm.com> wrote: > On 11/5/18 6:50 AM, David Hildenbrand wrote: > > On 05.11.18 12:40, Christian Borntraeger wrote: > >> > >> > >> On 11/05/2018 12:37 PM, David Hildenbrand wrote: > >>> On 05.11.18 12:21, Cornelia Huck wrote: > >>>> On Mon, 5 Nov 2018 12:03:11 +0100 > >>>> David Hildenbrand <david@redhat.com> wrote: > >>>> > >>>>> We directly have it in our hands. > >>>>> > >>>>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>>>> --- > >>>>> hw/s390x/s390-pci-bus.c | 4 ++-- > >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > >>>>> index 1eaae3aca6..68660eac74 100644 > >>>>> --- a/hw/s390x/s390-pci-bus.c > >>>>> +++ b/hw/s390x/s390-pci-bus.c > >>>>> @@ -814,9 +814,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev) > >>>>> static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > >>>>> Error **errp) > >>>>> { > >>>>> + S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev); > >>>>> PCIDevice *pdev = NULL; > >>>>> S390PCIBusDevice *pbdev = NULL; > >>>>> - S390pciState *s = s390_get_phb(); > >>>>> > >>>>> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) { > >>>>> BusState *bus; > >>>>> @@ -924,11 +924,11 @@ static void s390_pcihost_timer_cb(void *opaque) > >>>>> static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, > >>>>> Error **errp) > >>>>> { > >>>>> + S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev); > >>>>> PCIDevice *pci_dev = NULL; > >>>>> PCIBus *bus; > >>>>> int32_t devfn; > >>>>> S390PCIBusDevice *pbdev = NULL; > >>>>> - S390pciState *s = s390_get_phb(); > >>>>> > >>>>> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) { > >>>>> error_setg(errp, "PCI bridge hot unplug currently not supported"); > >>>> > >>>> Not sure whether that is an improvement (s390_get_phb() caches the > >>>> value, and is called from multiple other places as well.) > >>>> > >>> > >>> Looking up a variable that is directly passed as an argument doesn't > >>> look clean to me. > >> > >> I think there was a reason for this caching, namely that qom resolution can > >> be quite expensive. For the hotplug case this obviously does not matter but > >> for all the other cases it might. So do we really want to have different > >> places use different methods? > >> > > > > Caching resolution is fine (as that is expensive), caching a downcast is > > as far as I remember not necessary. Especially, as you said, for hotplug > > handlers. Yes, the complete QOM cast was the expensive thing AFAIR. > > > > Anyhow, if there are strong feelings to this change, I can drop this > > patch. There are certainly more important things to do in zPCI hotplug code. > > > > > > Truthfully, I'm not in favor of one over the other. As long as the device handlers > are consistent, I think either is fine. I don't feel *that* strong about this change here, either :) Your call. > > However, it would be nice if at some point during plug we cache the PHB somewhere. > That would be some sort of best-of-both-worlds approach. Not sure if caching-from-a-downcast would be conceptionally clean. I'd vote for either taking this patch or dropping it completely.
On 08.11.18 12:07, Cornelia Huck wrote: > On Wed, 7 Nov 2018 15:28:31 -0500 > Collin Walling <walling@linux.ibm.com> wrote: > >> On 11/5/18 6:50 AM, David Hildenbrand wrote: >>> On 05.11.18 12:40, Christian Borntraeger wrote: >>>> >>>> >>>> On 11/05/2018 12:37 PM, David Hildenbrand wrote: >>>>> On 05.11.18 12:21, Cornelia Huck wrote: >>>>>> On Mon, 5 Nov 2018 12:03:11 +0100 >>>>>> David Hildenbrand <david@redhat.com> wrote: >>>>>> >>>>>>> We directly have it in our hands. >>>>>>> >>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>>>> --- >>>>>>> hw/s390x/s390-pci-bus.c | 4 ++-- >>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c >>>>>>> index 1eaae3aca6..68660eac74 100644 >>>>>>> --- a/hw/s390x/s390-pci-bus.c >>>>>>> +++ b/hw/s390x/s390-pci-bus.c >>>>>>> @@ -814,9 +814,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev) >>>>>>> static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev, >>>>>>> Error **errp) >>>>>>> { >>>>>>> + S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev); >>>>>>> PCIDevice *pdev = NULL; >>>>>>> S390PCIBusDevice *pbdev = NULL; >>>>>>> - S390pciState *s = s390_get_phb(); >>>>>>> >>>>>>> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) { >>>>>>> BusState *bus; >>>>>>> @@ -924,11 +924,11 @@ static void s390_pcihost_timer_cb(void *opaque) >>>>>>> static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, >>>>>>> Error **errp) >>>>>>> { >>>>>>> + S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev); >>>>>>> PCIDevice *pci_dev = NULL; >>>>>>> PCIBus *bus; >>>>>>> int32_t devfn; >>>>>>> S390PCIBusDevice *pbdev = NULL; >>>>>>> - S390pciState *s = s390_get_phb(); >>>>>>> >>>>>>> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) { >>>>>>> error_setg(errp, "PCI bridge hot unplug currently not supported"); >>>>>> >>>>>> Not sure whether that is an improvement (s390_get_phb() caches the >>>>>> value, and is called from multiple other places as well.) >>>>>> >>>>> >>>>> Looking up a variable that is directly passed as an argument doesn't >>>>> look clean to me. >>>> >>>> I think there was a reason for this caching, namely that qom resolution can >>>> be quite expensive. For the hotplug case this obviously does not matter but >>>> for all the other cases it might. So do we really want to have different >>>> places use different methods? >>>> >>> >>> Caching resolution is fine (as that is expensive), caching a downcast is >>> as far as I remember not necessary. Especially, as you said, for hotplug >>> handlers. > > Yes, the complete QOM cast was the expensive thing AFAIR. > >>> >>> Anyhow, if there are strong feelings to this change, I can drop this >>> patch. There are certainly more important things to do in zPCI hotplug code. >>> >>> >> >> Truthfully, I'm not in favor of one over the other. As long as the device handlers >> are consistent, I think either is fine. > > I don't feel *that* strong about this change here, either :) Your call. > >> >> However, it would be nice if at some point during plug we cache the PHB somewhere. >> That would be some sort of best-of-both-worlds approach. > > Not sure if caching-from-a-downcast would be conceptionally clean. I'd > vote for either taking this patch or dropping it completely. > Yes, caching at that point, I would consider premature optimization. I'll drag this patch along and let the maintainers decide what to do :) (later we should forward the actual value to e.g. event injection code, so we don't have to look it up multiple times during one call)
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c index 1eaae3aca6..68660eac74 100644 --- a/hw/s390x/s390-pci-bus.c +++ b/hw/s390x/s390-pci-bus.c @@ -814,9 +814,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev) static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { + S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev); PCIDevice *pdev = NULL; S390PCIBusDevice *pbdev = NULL; - S390pciState *s = s390_get_phb(); if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) { BusState *bus; @@ -924,11 +924,11 @@ static void s390_pcihost_timer_cb(void *opaque) static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { + S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev); PCIDevice *pci_dev = NULL; PCIBus *bus; int32_t devfn; S390PCIBusDevice *pbdev = NULL; - S390pciState *s = s390_get_phb(); if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) { error_setg(errp, "PCI bridge hot unplug currently not supported");
We directly have it in our hands. Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/s390x/s390-pci-bus.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)