hw/ppc/spapr.c | 13 +- hw/ppc/spapr_drc.c | 404 ++++++++++++++++++++++++++++++++++++++------- hw/ppc/spapr_events.c | 10 +- hw/ppc/spapr_pci.c | 4 +- hw/ppc/spapr_rtas.c | 304 ---------------------------------- hw/ppc/trace-events | 2 - include/hw/ppc/spapr_drc.h | 9 +- 7 files changed, 355 insertions(+), 391 deletions(-)
The code managing DRCs[0] has quite a few things that are more
complicated than they need to be. In particular the object
representing a DRC has a bunch of method pointers, despite the fact
that there are currently no subclasses, and even if there were the
method implementations would be unlikely to differ.
This appears to be a misguided attempt to "abstract" or hide things in
a way which is bureaucraticl, rather than meaningful. We may have an
object model, but we don't have to adopt Java's kingdom-of-nouns
nonsense[1].
This series makes a start on simplifying things. There's still plenty
more, but you have to start somewhere.
[0] "Dynamic Reconfiguration Connectors" a firmware abstraction used
in hotplug operations
[1] https://steve-yegge.blogspot.com.au/2006/03/execution-in-kingdom-of-nouns.html
David Gibson (4):
spapr: Move DRC RTAS calls into spapr_drc.c
spapr: Abolish DRC get_fdt method
spapr: Abolish DRC set_configured method
spapr: Make DRC get_index and get_type methods into plain functions
hw/ppc/spapr.c | 13 +-
hw/ppc/spapr_drc.c | 404 ++++++++++++++++++++++++++++++++++++++-------
hw/ppc/spapr_events.c | 10 +-
hw/ppc/spapr_pci.c | 4 +-
hw/ppc/spapr_rtas.c | 304 ----------------------------------
hw/ppc/trace-events | 2 -
include/hw/ppc/spapr_drc.h | 9 +-
7 files changed, 355 insertions(+), 391 deletions(-)
--
2.9.4
On Thu, Jun 01, 2017 at 11:52:14AM +1000, David Gibson wrote: > The code managing DRCs[0] has quite a few things that are more > complicated than they need to be. In particular the object > representing a DRC has a bunch of method pointers, despite the fact > that there are currently no subclasses, and even if there were the > method implementations would be unlikely to differ. So you are getting rid of a few methods. How about other methods ? Specially attach and detach which have incorporated all the logic needed to handle logical and physical DRs into their implementations ? Guess these will be follow in subequent parts ? Regards, Bharata.
On Thu, Jun 01, 2017 at 09:36:46AM +0530, Bharata B Rao wrote: > On Thu, Jun 01, 2017 at 11:52:14AM +1000, David Gibson wrote: > > The code managing DRCs[0] has quite a few things that are more > > complicated than they need to be. In particular the object > > representing a DRC has a bunch of method pointers, despite the fact > > that there are currently no subclasses, and even if there were the > > method implementations would be unlikely to differ. > > So you are getting rid of a few methods. How about other methods ? > Specially attach and detach which have incorporated all the logic needed > to handle logical and physical DRs into their implementations ? > > Guess these will be follow in subequent parts ? That's the plan. In fact I already have patches underway, these are just the ones that are polished enough to post so far. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
Quoting Bharata B Rao (2017-05-31 23:06:46) > On Thu, Jun 01, 2017 at 11:52:14AM +1000, David Gibson wrote: > > The code managing DRCs[0] has quite a few things that are more > > complicated than they need to be. In particular the object > > representing a DRC has a bunch of method pointers, despite the fact > > that there are currently no subclasses, and even if there were the > > method implementations would be unlikely to differ. > > So you are getting rid of a few methods. How about other methods ? > Specially attach and detach which have incorporated all the logic needed > to handle logical and physical DRs into their implementations ? I would avoid any methods that incorporate special-casing for physical vs. logical DRCs, since that seems like a good logical starting point for moving to 'physical'/'logical' DRC sub-classes to help simplify the increasingly complicated state-tracking. I also don't think we should expose DRC internal fields to outside callers (which attach/detach would involve). This series does that to some extent with the RTAS calls, but since those are now moved to spapr_drc.c it makes more sense. > > Guess these will be follow in subequent parts ? > > Regards, > Bharata. > >
On Wed, May 31, 2017 at 11:25:41PM -0500, Michael Roth wrote: > Quoting Bharata B Rao (2017-05-31 23:06:46) > > On Thu, Jun 01, 2017 at 11:52:14AM +1000, David Gibson wrote: > > > The code managing DRCs[0] has quite a few things that are more > > > complicated than they need to be. In particular the object > > > representing a DRC has a bunch of method pointers, despite the fact > > > that there are currently no subclasses, and even if there were the > > > method implementations would be unlikely to differ. > > > > So you are getting rid of a few methods. How about other methods ? > > Specially attach and detach which have incorporated all the logic needed > > to handle logical and physical DRs into their implementations ? > > I would avoid any methods that incorporate special-casing for > physical vs. logical DRCs, since that seems like a good logical > starting point for moving to 'physical'/'logical' DRC > sub-classes to help simplify the increasingly complicated > state-tracking. Right, I'm looking at making subclasses for each of the DRC types. Possibly with intermediate subclasses for physical vs. logical, we'll see how it works out. > I also don't think we should expose DRC internal fields to > outside callers (which attach/detach would involve). Well.. just changing attach/detach to plain functions instead of methods wouldn't break that. > This > series does that to some extent with the RTAS calls, but > since those are now moved to spapr_drc.c it makes more sense. Right - the semantics of the RTAS calls are tied closely to the DRC semantics, so I don't think there's any point considering the RTAS calls to be "outside" the DRC code itself. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On 06/01/2017 02:30 AM, David Gibson wrote: > On Wed, May 31, 2017 at 11:25:41PM -0500, Michael Roth wrote: >> Quoting Bharata B Rao (2017-05-31 23:06:46) >>> On Thu, Jun 01, 2017 at 11:52:14AM +1000, David Gibson wrote: >>>> The code managing DRCs[0] has quite a few things that are more >>>> complicated than they need to be. In particular the object >>>> representing a DRC has a bunch of method pointers, despite the fact >>>> that there are currently no subclasses, and even if there were the >>>> method implementations would be unlikely to differ. >>> So you are getting rid of a few methods. How about other methods ? >>> Specially attach and detach which have incorporated all the logic needed >>> to handle logical and physical DRs into their implementations ? >> I would avoid any methods that incorporate special-casing for >> physical vs. logical DRCs, since that seems like a good logical >> starting point for moving to 'physical'/'logical' DRC >> sub-classes to help simplify the increasingly complicated >> state-tracking. > Right, I'm looking at making subclasses for each of the DRC types. > Possibly with intermediate subclasses for physical vs. logical, we'll > see how it works out. Back in the DRC migration patch series I talked with Mike about refactoring the DRC code in such fashion (physical DRC and logical DRC). But first I would implement some kind of unit testing in this code to avoid breaking too much stuff during this refactoring. I am not sure about the effort to implementing unit test in the current DRC code. This series is simplifying the DRC code, making it more minimalist and possibly easier to be tested. In the end it would be a first step towards unit testing. However, there is the issue of backward compatibility. I fear this DRC refactoring of Logical/Physical DRC would be too drastic to maintain such compatibility (assuming that it is not already broken). If this refactor goes live only in 2.11 then we will have a hard time to migrate from 2.11 to 2.10. All that said, I believe we can live without unit testing for a little longer and if we're going for this Physical/DRC refactoring, we need to push it for 2.10. We can think about unit test later with the refactored code. Feel free to send to me any unfinished/beta DRC refactoring code you might be working on and want tested. I can help in the refactoring too, just let me know. Daniel > >> I also don't think we should expose DRC internal fields to >> outside callers (which attach/detach would involve). > Well.. just changing attach/detach to plain functions instead of > methods wouldn't break that. > >> This >> series does that to some extent with the RTAS calls, but >> since those are now moved to spapr_drc.c it makes more sense. > Right - the semantics of the RTAS calls are tied closely to the DRC > semantics, so I don't think there's any point considering the RTAS > calls to be "outside" the DRC code itself. >
On Thu, Jun 01, 2017 at 12:41:40PM -0300, Daniel Henrique Barboza wrote: > > > On 06/01/2017 02:30 AM, David Gibson wrote: > > On Wed, May 31, 2017 at 11:25:41PM -0500, Michael Roth wrote: > > > Quoting Bharata B Rao (2017-05-31 23:06:46) > > > > On Thu, Jun 01, 2017 at 11:52:14AM +1000, David Gibson wrote: > > > > > The code managing DRCs[0] has quite a few things that are more > > > > > complicated than they need to be. In particular the object > > > > > representing a DRC has a bunch of method pointers, despite the fact > > > > > that there are currently no subclasses, and even if there were the > > > > > method implementations would be unlikely to differ. > > > > So you are getting rid of a few methods. How about other methods ? > > > > Specially attach and detach which have incorporated all the logic needed > > > > to handle logical and physical DRs into their implementations ? > > > I would avoid any methods that incorporate special-casing for > > > physical vs. logical DRCs, since that seems like a good logical > > > starting point for moving to 'physical'/'logical' DRC > > > sub-classes to help simplify the increasingly complicated > > > state-tracking. > > Right, I'm looking at making subclasses for each of the DRC types. > > Possibly with intermediate subclasses for physical vs. logical, we'll > > see how it works out. > > Back in the DRC migration patch series I talked with Mike about refactoring > the DRC code in such fashion (physical DRC and logical DRC). But first I > would > implement some kind of unit testing in this code to avoid breaking too much > stuff during this refactoring. So, I'd love to have good unit tests, but everything takes time. > I am not sure about the effort to implementing unit test in the > current DRC code. This series is simplifying the DRC code, making > it more minimalist and possibly easier to be tested. In the end it > would be a first step towards unit testing. ..and as you say, extra complexity in the code makes testing and reliability harder. > > However, there is the issue of backward compatibility. I fear this DRC > refactoring > of Logical/Physical DRC would be too drastic to maintain such compatibility > (assuming that it is not already broken). If this refactor goes live only in > 2.11 then > we will have a hard time to migrate from 2.11 to 2.10. Right such a rework could break migration. > All that said, I believe we can live without unit testing for a little > longer and if > we're going for this Physical/DRC refactoring, we need to push it for 2.10. > We can > think about unit test later with the refactored code. Feel free to send to > me any > unfinished/beta DRC refactoring code you might be working on and want > tested. I can help in the refactoring too, just let me know. So like you I think getting it into 2.10 would be a good idea, before we have any released version with DRC migration to break. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On 05/31/2017 10:52 PM, David Gibson wrote: > The code managing DRCs[0] has quite a few things that are more > complicated than they need to be. In particular the object > representing a DRC has a bunch of method pointers, despite the fact > that there are currently no subclasses, and even if there were the > method implementations would be unlikely to differ. > > This appears to be a misguided attempt to "abstract" or hide things in > a way which is bureaucraticl, rather than meaningful. We may have an > object model, but we don't have to adopt Java's kingdom-of-nouns > nonsense[1]. > > This series makes a start on simplifying things. There's still plenty > more, but you have to start somewhere. > > [0] "Dynamic Reconfiguration Connectors" a firmware abstraction used > in hotplug operations > [1] https://steve-yegge.blogspot.com.au/2006/03/execution-in-kingdom-of-nouns.html > > David Gibson (4): > spapr: Move DRC RTAS calls into spapr_drc.c > spapr: Abolish DRC get_fdt method > spapr: Abolish DRC set_configured method > spapr: Make DRC get_index and get_type methods into plain functions > > hw/ppc/spapr.c | 13 +- > hw/ppc/spapr_drc.c | 404 ++++++++++++++++++++++++++++++++++++++------- > hw/ppc/spapr_events.c | 10 +- > hw/ppc/spapr_pci.c | 4 +- > hw/ppc/spapr_rtas.c | 304 ---------------------------------- > hw/ppc/trace-events | 2 - > include/hw/ppc/spapr_drc.h | 9 +- > 7 files changed, 355 insertions(+), 391 deletions(-) > Tested-by: Daniel Barboza <danielhb@linux.vnet.ibm.com>
On Thu, Jun 01, 2017 at 11:52:14AM +1000, David Gibson wrote: > The code managing DRCs[0] has quite a few things that are more > complicated than they need to be. In particular the object > representing a DRC has a bunch of method pointers, despite the fact > that there are currently no subclasses, and even if there were the > method implementations would be unlikely to differ. > > This appears to be a misguided attempt to "abstract" or hide things in > a way which is bureaucraticl, rather than meaningful. We may have an > object model, but we don't have to adopt Java's kingdom-of-nouns > nonsense[1]. > > This series makes a start on simplifying things. There's still plenty > more, but you have to start somewhere. > > [0] "Dynamic Reconfiguration Connectors" a firmware abstraction used > in hotplug operations > [1] > https://steve-yegge.blogspot.com.au/2006/03/execution-in-kingdom-of-nouns.html I've had enough acks that I've merged this series (with minor corrections) into ppc-for-2.10. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
© 2016 - 2024 Red Hat, Inc.