Re: [PATCH v2] rteval: Add measurement and load location to run report

[John Kacur <jkacur@redhat.com>]

On Fri, 5 Aug 2022, Leah Leshchinsky wrote:

> On Fri, Aug 05, 2022 at 12:32:32PM -0400, John Kacur wrote:
> > 
> > 
> > On Fri, 5 Aug 2022, Leah Leshchinsky wrote:
> > 
> > > On Thu, Aug 04, 2022 at 11:48:07AM -0400, Leah Leshchinsky wrote:
> > > > The run report produced at the end of a run does not contain information
> > > > on load and measurement thread locations.
> > > > 
> > > > Adjust MakeReport() functions of LoadModules and MeasurementModules
> > > > class so that new properties with number of loads and cpu information
> > > > are added to the XML report and can be read by rteval_text.xsl.
> > > > 
> > > > Signed-off-by: Leah Leshchinsky <lleshchi@redhat.com>
> > > > 
> > > > diff --git a/rteval/modules/loads/__init__.py b/rteval/modules/loads/__init__.py
> > > > index 2c2105efa964..5e5a5d85b616 100644
> > > > --- a/rteval/modules/loads/__init__.py
> > > > +++ b/rteval/modules/loads/__init__.py
> > > > @@ -30,6 +30,7 @@ import libxml2
> > > >  from rteval.Log import Log
> > > >  from rteval.rtevalConfig import rtevalCfgSection
> > > >  from rteval.modules import RtEvalModules, rtevalModulePrototype
> > > > +from rteval.systopology import collapse_cpulist, SysTopology
> > > >  
> > > >  class LoadThread(rtevalModulePrototype):
> > > >      def __init__(self, name, config, logger=None):
> > > > @@ -131,6 +132,12 @@ class LoadModules(RtEvalModules):
> > > >      def MakeReport(self):
> > > >          rep_n = RtEvalModules.MakeReport(self)
> > > >          rep_n.newProp("load_average", str(self.GetLoadAvg()))
> > > > +        rep_n.newProp("loads", str(self.ModulesLoaded()))
> > > > +        cpulist = self._cfg.GetSection(self._module_config).cpulist
> > > > +        if not cpulist:
> > > > +            st = SysTopology()
> > > > +            cpulist = collapse_cpulist(st.online_cpus())
> > > > +        rep_n.newProp("loadcpus", cpulist)
> > > >  
> > > >          return rep_n
> > > >  
> > > > diff --git a/rteval/modules/measurement/__init__.py b/rteval/modules/measurement/__init__.py
> > > > index 318248bd7e35..371fc5d08f44 100644
> > > > --- a/rteval/modules/measurement/__init__.py
> > > > +++ b/rteval/modules/measurement/__init__.py
> > > > @@ -24,7 +24,7 @@
> > > >  
> > > >  import libxml2
> > > >  from rteval.modules import RtEvalModules, ModuleContainer
> > > > -
> > > > +from rteval.systopology import collapse_cpulist, SysTopology
> > > >  
> > > >  class MeasurementProfile(RtEvalModules):
> > > >      """Keeps and controls all the measurement modules with the same measurement profile"""
> > > > @@ -189,6 +189,12 @@ measurement profiles, based on their characteristics"""
> > > >  
> > > >          # Get the reports from all meaurement modules in all measurement profiles
> > > >          rep_n = libxml2.newNode("Measurements")
> > > > +        cpulist = self.__cfg.GetSection("measurement").cpulist
> > > > +        if not cpulist:
> > > > +            st = SysTopology()
> > > > +            cpulist = collapse_cpulist(st.online_cpus())
> > > > +        rep_n.newProp("measurecpus", cpulist)
> > > > +
> > > >          for mp in self.__measureprofiles:
> > > >              mprep_n = mp.MakeReport()
> > > >              if mprep_n:
> > > > diff --git a/rteval/rteval_text.xsl b/rteval/rteval_text.xsl
> > > > index c40063e3dd19..7ecfac6b6140 100644
> > > > --- a/rteval/rteval_text.xsl
> > > > +++ b/rteval/rteval_text.xsl
> > > > @@ -13,6 +13,14 @@
> > > >      <xsl:value-of select="run_info/date"/><xsl:text> </xsl:text><xsl:value-of select="run_info/time"/>
> > > >      <xsl:text>&#10;</xsl:text>
> > > >  
> > > > +    <xsl:text>   Loads:        </xsl:text>
> > > > +    <xsl:value-of select="loads/@loads"/><xsl:text> loads run on cores </xsl:text><xsl:value-of select="loads/@loadcpus"/>
> > > > +    <xsl:text>&#10;</xsl:text>
> > > > +
> > > > +    <xsl:text>   Measurement:  </xsl:text>
> > > > +    <xsl:text>measurement threads run on cores </xsl:text><xsl:value-of select="Measurements/@measurecpus"/>
> > > > +    <xsl:text>&#10;</xsl:text>
> > > > +
> > > >      <xsl:text>   Run time:     </xsl:text>
> > > >      <xsl:value-of select="run_info/@days"/><xsl:text> days </xsl:text>
> > > >      <xsl:value-of select="run_info/@hours"/><xsl:text>h </xsl:text>
> > > > -- 
> > > > 2.31.1
> > > > 
> > > 
> > > Hi John,
> > > 
> > > I do believe the cpulist variable can be empty. I was able to test this by running rteval without specifying and load and measurement cpus.
> > 
> > Ok, you are correct, cpulist can be empty if the following are not set
> > --loads-cpulist=LIST
> > --measurement-cpulist=LIST
> > 
> > So you do need some code to initialize the list of it is empty.
> > But there are still problems
> > 
> > For example if you shut off cpu 3
> > su -c 'echo 0 > /sys/devices/system/cpu/cpu3/online '
> > 
> > And then request measurement on cpus 0-5
> > su -c './rteval-cmd -d5s --measurement-cpulist=0-5'
> > 
> > The report should say 
> > Measurement:  measurement threads run on cores  0-2,4,5
> > Loads:        3 loads run on cores 6-11 (the inverse)
> > but instead with your patch it says
> > Loads:        3 loads run on cores 1,2,4,6,7,8,9,10,11
> > Measurement:  measurement threads run on cores 0-5
> > 
> > Now there are problems that exist even before your patch, so for example, 
> > the initial info to the screen not included in the report doesn't consider 
> > offline cpus. (We should fix that too of course, but we can do this 
> > stepwise)
> > 
> > And it's also possible that I have introduced some new problems with the 
> > changes to systopology that will hopefully make things simpler in the long 
> > run but may be interferring with your code. So, there are many things to 
> > untangle here.
> > 
> > > 
> > > This patch is attempting to replicate the behavior found in rteval/__init.py lines 187-201, which also finds the cpulist using
> > >             cpulist = self._loadmods._cfg.GetSection("loads").cpulist
> > > and if the cpulist is empty, defaults to reporting the online cpus. I assume this is because the rteval program uses all available cpus
> > > to run measurement and load threads if no specific cpus are specified.
> > > 
> > > Please let me know if I am misunderstanding any of the above.
> > > 
> > > I will investigate why the xsl code is not working for you. It seems to be working for me at the moment.
> > 
> > 
> > > 
> > > Thank you for the review,
> > > Leah
> > > 
> > > 
> > Thanks Leah
> > 
> > John
> > 
> 
> Hi John,
> 
> > The report should say
> > Measurement:  measurement threads run on cores  0-2,4,5
> > Loads:        3 loads run on cores 6-11 (the inverse)
> > but instead with your patch it says
> > Loads:        3 loads run on cores 1,2,4,6,7,8,9,10,11
> > Measurement:  measurement threads run on cores 0-5
> 
> Now that I'm looking at the rteval program prior to my changes, I see that this issue existed in the initial pre-report posted to stdout.
> This seems to be a separate bug in the way that 'self._loadmods._cfg.GetSection("loads").cpulist' and self.__cfg.GetSection("measurement").cpulist are calculated.
> 
> When I test your example on the rteval main branch, this is the result:
> 
> su -c 'echo 0 > /sys/devices/system/cpu/cpu3/online '
> su -c './rteval-cmd -d5s --measurement-cpulist=0-5'
> 
> started 3 loads on cores 1,2,4,6,7
> started measurement threads on cores 0-5
> 
> Should we file this as a separate bug, since the scope of it goes beyond my current patch?
> 
> Thanks,
> Leah
> 

Please send a separate patch to fix this.

John