[PATCH 1/5] Recipes.ENRT.PerfTestMixins.SctpFirewallPerfTestMixin: move _get_flow_measurement_from_config() to base class

[Jan Tluka]

This method will be reused by other mixin classes.

Signed-off-by: Jan Tluka <jtluka(a)redhat.com&gt;
---
 lnst/Recipes/ENRT/PerfTestMixins/BasePerfTestTweakMixin.py  | 6 ++++++
 .../ENRT/PerfTestMixins/SctpFirewallPerfTestMixin.py        | 6 ------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lnst/Recipes/ENRT/PerfTestMixins/BasePerfTestTweakMixin.py
b/lnst/Recipes/ENRT/PerfTestMixins/BasePerfTestTweakMixin.py
index 380fc72e..5e008a68 100644
--- a/lnst/Recipes/ENRT/PerfTestMixins/BasePerfTestTweakMixin.py
+++ b/lnst/Recipes/ENRT/PerfTestMixins/BasePerfTestTweakMixin.py
@@ -1,3 +1,5 @@
+from lnst.RecipeCommon.Perf.Measurements.BaseFlowMeasurement import BaseFlowMeasurement
+
 class BasePerfTestTweakMixin(object):
     """
     This is a base class that defines common API for specific *perf test*
@@ -13,3 +15,7 @@ class BasePerfTestTweakMixin(object):
     def remove_perf_test_tweak(self, perf_config):
         # TODO: check if anything left in the perf_config.perf_test_tweak_config
         pass
+
+    def _get_flow_measurement_from_config(self, perf_config):
+        flow_measurements = [ m for m in perf_config.measurements if isinstance(m,
BaseFlowMeasurement) ]
+        return flow_measurements[0]
diff --git a/lnst/Recipes/ENRT/PerfTestMixins/SctpFirewallPerfTestMixin.py
b/lnst/Recipes/ENRT/PerfTestMixins/SctpFirewallPerfTestMixin.py
index 6c6cc416..c0520405 100644
--- a/lnst/Recipes/ENRT/PerfTestMixins/SctpFirewallPerfTestMixin.py
+++ b/lnst/Recipes/ENRT/PerfTestMixins/SctpFirewallPerfTestMixin.py
@@ -1,13 +1,7 @@
 from lnst.Controller.RecipeResults import ResultLevel
 from lnst.Recipes.ENRT.PerfTestMixins import BasePerfTestTweakMixin
-from lnst.RecipeCommon.Perf.Measurements.BaseFlowMeasurement import BaseFlowMeasurement
 
 class SctpFirewallPerfTestMixin(BasePerfTestTweakMixin):
-
-    def _get_flow_measurement_from_config(self, perf_config):
-        flow_measurements = [ m for m in perf_config.measurements if isinstance(m,
BaseFlowMeasurement) ]
-        return flow_measurements[0]
-
     def apply_perf_test_tweak(self, perf_config):
         super().apply_perf_test_tweak(perf_config)
 
-- 
2.21.3

[PATCH 2/5] Recipes.ENRT.PerfTestMixins: add DisableIdleStatesMixin

[Jan Tluka]

This mixin class provides recipe parameter 'disable_idlestates' that can be
used to control the CPU idle states before running a performance test through
BaseEnrtRecipe.do_perf_tests().

The value of the parameter is passed as the latency argument (-D) of the
'cpupower idle-set' that will disable all idle states with a equal or higher
latency than the specified value.

If the value is 0 this will effectively disable all CPU idle states.

Signed-off-by: Jan Tluka <jtluka(a)redhat.com&gt;
---
 .../PerfTestMixins/DisableIdleStatesMixin.py  | 42 +++++++++++++++++++
 lnst/Recipes/ENRT/PerfTestMixins/__init__.py  |  1 +
 2 files changed, 43 insertions(+)
 create mode 100644 lnst/Recipes/ENRT/PerfTestMixins/DisableIdleStatesMixin.py

diff --git a/lnst/Recipes/ENRT/PerfTestMixins/DisableIdleStatesMixin.py
b/lnst/Recipes/ENRT/PerfTestMixins/DisableIdleStatesMixin.py
new file mode 100644
index 00000000..4281016f
--- /dev/null
+++ b/lnst/Recipes/ENRT/PerfTestMixins/DisableIdleStatesMixin.py
@@ -0,0 +1,42 @@
+from lnst.Common.Parameters import IntParam
+from lnst.Recipes.ENRT.PerfTestMixins import BasePerfTestTweakMixin
+
+class DisableIdleStatesMixin(BasePerfTestTweakMixin):
+    disable_idlestates = IntParam(default=-1)
+
+    def generate_perf_test_tweak_description(self, perf_config):
+        description = super().generate_perf_test_tweak_description(perf_config)
+        latency = self.params.disable_idlestates
+        tweak_config = perf_config.perf_test_tweak_config
+        if 'idlestates' in tweak_config:
+            for host in tweak_config['idlestates']['hosts']:
+                desc = "disabled idle states with latency higher than {} "\
+                        "on host {}".format(latency, host.hostid)
+                description.append(desc)
+        else:
+            description.append("skipped disabling idle states")
+
+        return description
+
+    def apply_perf_test_tweak(self, perf_config):
+        super().apply_perf_test_tweak(perf_config)
+        latency = self.params.disable_idlestates
+        if latency != -1:
+            tweak_config = perf_config.perf_test_tweak_config
+            tweak_config['idlestates'] = {'hosts': []}
+
+            flow_measurement = self._get_flow_measurement_from_config(perf_config)
+            flow = flow_measurement.conf[0]
+            for host in [flow.generator, flow.receiver]:
+                # TODO: save previous state
+                host.run("cpupower idle-set -D {}".format(latency))
+                tweak_config['idlestates']['hosts'].append(host)
+
+    def remove_perf_test_tweak(self, perf_config):
+        tweak_config = perf_config.perf_test_tweak_config
+        if 'idlestates' in tweak_config:
+            for host in tweak_config['idlestates']['hosts']:
+                host.run("cpupower idle-set -E")
+            del tweak_config['idlestates']
+
+        super().remove_perf_test_tweak(perf_config)
diff --git a/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
b/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
index 9f320118..fc5b4747 100644
--- a/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
+++ b/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
@@ -1,2 +1,3 @@
 from lnst.Recipes.ENRT.PerfTestMixins.BasePerfTestTweakMixin import
BasePerfTestTweakMixin
 from lnst.Recipes.ENRT.PerfTestMixins.SctpFirewallPerfTestMixin import
SctpFirewallPerfTestMixin
+from lnst.Recipes.ENRT.PerfTestMixins.DisableIdleStatesMixin import
DisableIdleStatesMixin
-- 
2.21.3

[PATCH 2/5] Recipes.ENRT.PerfTestMixins: add DisableIdleStatesMixin

[Ondrej Lichtner]

On Mon, Oct 12, 2020 at 07:15:12PM +0200, Jan Tluka wrote:
>  This mixin class provides recipe parameter
> 'disable_idlestates' that can be
>  used to control the CPU idle states before running a performance test through
>  BaseEnrtRecipe.do_perf_tests().
>  
>  The value of the parameter is passed as the latency argument (-D) of the
>  'cpupower idle-set' that will disable all idle states with a equal or higher
>  latency than the specified value.
>  
>  If the value is 0 this will effectively disable all CPU idle states.
>  
>  Signed-off-by: Jan Tluka <jtluka(a)redhat.com&gt;
>  ---
>   .../PerfTestMixins/DisableIdleStatesMixin.py  | 42 +++++++++++++++++++
>   lnst/Recipes/ENRT/PerfTestMixins/__init__.py  |  1 +
>   2 files changed, 43 insertions(+)
>   create mode 100644 lnst/Recipes/ENRT/PerfTestMixins/DisableIdleStatesMixin.py
>  
>  diff --git a/lnst/Recipes/ENRT/PerfTestMixins/DisableIdleStatesMixin.py
> b/lnst/Recipes/ENRT/PerfTestMixins/DisableIdleStatesMixin.py
>  new file mode 100644
>  index 00000000..4281016f
>  --- /dev/null
>  +++ b/lnst/Recipes/ENRT/PerfTestMixins/DisableIdleStatesMixin.py
>  @@ -0,0 +1,42 @@
>  +from lnst.Common.Parameters import IntParam
>  +from lnst.Recipes.ENRT.PerfTestMixins import BasePerfTestTweakMixin
>  +
>  +class DisableIdleStatesMixin(BasePerfTestTweakMixin):
>  +    disable_idlestates = IntParam(default=-1) 
You don't need to set a "special" value to indicate that the parameter
wasn't set.

If the parameter is not mandatory and you don't provide a value for it
on Recipe initialization, the parameter isn't available during test
execution, e.g.:

class Recipe():
    optional_param = IntParam(mandatory=False)

    def test(self):
        if "optional_param" in self.params:
            configure(self.params.optional_param)

>  +
>  +    def generate_perf_test_tweak_description(self, perf_config):
>  +        description = super().generate_perf_test_tweak_description(perf_config)
>  +        latency = self.params.disable_idlestates
>  +        tweak_config = perf_config.perf_test_tweak_config
>  +        if 'idlestates' in tweak_config:
>  +            for host in tweak_config['idlestates']['hosts']:
>  +                desc = "disabled idle states with latency higher than {} "\
>  +                        "on host {}".format(latency, host.hostid)
>  +                description.append(desc)
>  +        else:
>  +            description.append("skipped disabling idle states")
>  +
>  +        return description
>  +
>  +    def apply_perf_test_tweak(self, perf_config):
>  +        super().apply_perf_test_tweak(perf_config)
>  +        latency = self.params.disable_idlestates 
I'm not sure about the name of the parameter... why call it
"disable_idlestates" when in reality it refers to the latency to be
used? shouldn't it somehow be reflected in the name of the parameter
itself?

>  +        if latency != -1:
>  +            tweak_config = perf_config.perf_test_tweak_config
>  +            tweak_config['idlestates'] = {'hosts': []}
>  +
>  +            flow_measurement = self._get_flow_measurement_from_config(perf_config)
>  +            flow = flow_measurement.conf[0]
>  +            for host in [flow.generator, flow.receiver]: 
I'm wondering how this will work for Virtual recipes, the generator and
receiver in those cases refer to the guest vms, can we also disable
idlestates for virtual cores?

Should we consider disabling them for the hypervisor hosts?

-Ondrej

>  +                # TODO: save previous state
>  +                host.run("cpupower idle-set -D {}".format(latency))
>  +                tweak_config['idlestates']['hosts'].append(host)
>  +
>  +    def remove_perf_test_tweak(self, perf_config):
>  +        tweak_config = perf_config.perf_test_tweak_config
>  +        if 'idlestates' in tweak_config:
>  +            for host in tweak_config['idlestates']['hosts']:
>  +                host.run("cpupower idle-set -E")
>  +            del tweak_config['idlestates']
>  +
>  +        super().remove_perf_test_tweak(perf_config)
>  diff --git a/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
> b/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
>  index 9f320118..fc5b4747 100644
>  --- a/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
>  +++ b/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
>  @@ -1,2 +1,3 @@
>   from lnst.Recipes.ENRT.PerfTestMixins.BasePerfTestTweakMixin import
> BasePerfTestTweakMixin
>   from lnst.Recipes.ENRT.PerfTestMixins.SctpFirewallPerfTestMixin import
> SctpFirewallPerfTestMixin
>  +from lnst.Recipes.ENRT.PerfTestMixins.DisableIdleStatesMixin import
> DisableIdleStatesMixin
>  -- 
>  2.21.3
>  _______________________________________________
>  LNST-developers mailing list -- lnst-developers(a)lists.fedorahosted.org
>  To unsubscribe send an email to lnst-developers-leave(a)lists.fedorahosted.org
>  Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
>  List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
>  List Archives:
> https://lists.fedorahosted.org/archives/list/[e-mail]...

[PATCH 2/5] Recipes.ENRT.PerfTestMixins: add DisableIdleStatesMixin

[Jan Tluka]

Tue, Oct 13, 2020 at 10:21:27AM CEST, olichtne(a)redhat.com wrote:
> On Mon, Oct 12, 2020 at 07:15:12PM +0200, Jan Tluka wrote:
> > This mixin class provides recipe parameter 'disable_idlestates' that can be
> > used to control the CPU idle states before running a performance test through
> > BaseEnrtRecipe.do_perf_tests().
> > 
> > The value of the parameter is passed as the latency argument (-D) of the
> > 'cpupower idle-set' that will disable all idle states with a equal or higher
> > latency than the specified value.
> > 
> > If the value is 0 this will effectively disable all CPU idle states.
> > 
> > Signed-off-by: Jan Tluka <jtluka(a)redhat.com&gt;
> > ---
> >  .../PerfTestMixins/DisableIdleStatesMixin.py  | 42 +++++++++++++++++++
> >  lnst/Recipes/ENRT/PerfTestMixins/__init__.py  |  1 +
> >  2 files changed, 43 insertions(+)
> >  create mode 100644 lnst/Recipes/ENRT/PerfTestMixins/DisableIdleStatesMixin.py
> > 
> > diff --git a/lnst/Recipes/ENRT/PerfTestMixins/DisableIdleStatesMixin.py
> b/lnst/Recipes/ENRT/PerfTestMixins/DisableIdleStatesMixin.py
> > new file mode 100644
> > index 00000000..4281016f
> > --- /dev/null
> > +++ b/lnst/Recipes/ENRT/PerfTestMixins/DisableIdleStatesMixin.py
> > @@ -0,0 +1,42 @@
> > +from lnst.Common.Parameters import IntParam
> > +from lnst.Recipes.ENRT.PerfTestMixins import BasePerfTestTweakMixin
> > +
> > +class DisableIdleStatesMixin(BasePerfTestTweakMixin):
> > +    disable_idlestates = IntParam(default=-1)
>
> You don't need to set a "special" value to indicate that the parameter
> wasn't set.
>
> If the parameter is not mandatory and you don't provide a value for it
> on Recipe initialization, the parameter isn't available during test
> execution, e.g.:
>
> class Recipe():
>     optional_param = IntParam(mandatory=False)
>
>     def test(self):
>         if "optional_param" in self.params:
>             configure(self.params.optional_param)
>  
I'll fix this.

> > +
> > +    def generate_perf_test_tweak_description(self, perf_config):
> > +        description = super().generate_perf_test_tweak_description(perf_config)
> > +        latency = self.params.disable_idlestates
> > +        tweak_config = perf_config.perf_test_tweak_config
> > +        if 'idlestates' in tweak_config:
> > +            for host in tweak_config['idlestates']['hosts']:
> > +                desc = "disabled idle states with latency higher than {}
> "\
> > +                        "on host {}".format(latency, host.hostid)
> > +                description.append(desc)
> > +        else:
> > +            description.append("skipped disabling idle states")
> > +
> > +        return description
> > +
> > +    def apply_perf_test_tweak(self, perf_config):
> > +        super().apply_perf_test_tweak(perf_config)
> > +        latency = self.params.disable_idlestates
>
> I'm not sure about the name of the parameter... why call it
> "disable_idlestates" when in reality it refers to the latency to be
> used? shouldn't it somehow be reflected in the name of the parameter
> itself?
>  
Yeah, probably 'minimal_idlestates_latency' is better.

> > +        if latency != -1:
> > +            tweak_config = perf_config.perf_test_tweak_config
> > +            tweak_config['idlestates'] = {'hosts': []}
> > +
> > +            flow_measurement = self._get_flow_measurement_from_config(perf_config)
> > +            flow = flow_measurement.conf[0]
> > +            for host in [flow.generator, flow.receiver]:
>
> I'm wondering how this will work for Virtual recipes, the generator and
> receiver in those cases refer to the guest vms, can we also disable
> idlestates for virtual cores? 
No idea. I'll have to check.

> Should we consider disabling them for the hypervisor hosts? 
Very likely. I'll work on enhancing this and send v2.

> -Ondrej
>
> > +                # TODO: save previous state
> > +                host.run("cpupower idle-set -D {}".format(latency))
> > +                tweak_config['idlestates']['hosts'].append(host)
> > +
> > +    def remove_perf_test_tweak(self, perf_config):
> > +        tweak_config = perf_config.perf_test_tweak_config
> > +        if 'idlestates' in tweak_config:
> > +            for host in tweak_config['idlestates']['hosts']:
> > +                host.run("cpupower idle-set -E")
> > +            del tweak_config['idlestates']
> > +
> > +        super().remove_perf_test_tweak(perf_config)
> > diff --git a/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
> b/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
> > index 9f320118..fc5b4747 100644
> > --- a/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
> > +++ b/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
> > @@ -1,2 +1,3 @@
> >  from lnst.Recipes.ENRT.PerfTestMixins.BasePerfTestTweakMixin import
> BasePerfTestTweakMixin
> >  from lnst.Recipes.ENRT.PerfTestMixins.SctpFirewallPerfTestMixin import
> SctpFirewallPerfTestMixin
> > +from lnst.Recipes.ENRT.PerfTestMixins.DisableIdleStatesMixin import
> DisableIdleStatesMixin
> > -- 
> > 2.21.3
> > _______________________________________________
> > LNST-developers mailing list -- lnst-developers(a)lists.fedorahosted.org
> > To unsubscribe send an email to lnst-developers-leave(a)lists.fedorahosted.org
> > Fedora Code of Conduct:
> https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> > List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> > List Archives:
> https://lists.fedorahosted.org/archives/list/[e-mail]...

[PATCH 3/5] Recipes.ENRT.PerfTestMixins: add DisableTurboboostMixin

[Jan Tluka]

This mixin class provides recipe parameter 'disable_turboboost' that can be
used to disable CPU turboboost before running a performance test through
BaseEnrtRecipe.do_perf_tests().

It uses intel_pstate sysfs interface to disable the CPU feature and so
it is usable only by systems with Intel CPUs.

To use it pass 'disable_turboost=True' parameter when creating an instance of
BaseEnrtRecipe.

Signed-off-by: Jan Tluka <jtluka(a)redhat.com&gt;
---
 .../PerfTestMixins/DisableTurboboostMixin.py  | 55 +++++++++++++++++++
 lnst/Recipes/ENRT/PerfTestMixins/__init__.py  |  1 +
 2 files changed, 56 insertions(+)
 create mode 100644 lnst/Recipes/ENRT/PerfTestMixins/DisableTurboboostMixin.py

diff --git a/lnst/Recipes/ENRT/PerfTestMixins/DisableTurboboostMixin.py
b/lnst/Recipes/ENRT/PerfTestMixins/DisableTurboboostMixin.py
new file mode 100644
index 00000000..8beec018
--- /dev/null
+++ b/lnst/Recipes/ENRT/PerfTestMixins/DisableTurboboostMixin.py
@@ -0,0 +1,55 @@
+from lnst.Common.Parameters import BoolParam
+from lnst.Recipes.ENRT.PerfTestMixins import BasePerfTestTweakMixin
+
+class DisableTurboboostMixin(BasePerfTestTweakMixin):
+    disable_turboboost = BoolParam(default=False)
+
+    def generate_perf_test_tweak_description(self, perf_config):
+        description = super().generate_perf_test_tweak_description(perf_config)
+        tweak_config = perf_config.perf_test_tweak_config
+        if 'turboboost' in tweak_config:
+            for host, host_data in tweak_config['turboboost'].items():
+                description.append(host_data['desc'])
+        else:
+            description.append("skipped disabling turboboost through
intel_pstate")
+
+        return description
+
+    def _is_turboboost_supported(self, host):
+        file_check = host.run("ls
/sys/devices/system/cpu/intel_pstate/no_turbo")
+        return file_check.passed
+
+    def apply_perf_test_tweak(self, perf_config):
+        super().apply_perf_test_tweak(perf_config)
+        if self.params.disable_turboboost:
+            tweak_config = perf_config.perf_test_tweak_config
+            tweak_config['turboboost'] = {}
+
+            flow_measurement = self._get_flow_measurement_from_config(perf_config)
+            flow = flow_measurement.conf[0]
+            for host in [flow.generator, flow.receiver]:
+                if self._is_turboboost_supported(host):
+                    # TODO: save previous state
+                    host.run("echo 1 >
/sys/devices/system/cpu/intel_pstate/no_turbo")
+                    desc = "disabled turboboost through intel_pstate on
{}".format(host.hostid)
+                    turboboost_status = 'disabled'
+                else:
+                    desc = "warning: user requested to disable turboboost "\
+                            "through intel_pstate but the sysfs file is not
available "\
+                            "on host {}".format(host.hostid)
+                    turboboost_status = 'unsupported'
+                tweak_config['turboboost'][host] = {
+                        'desc': desc,
+                        'status': turboboost_status,
+                        }
+
+    def remove_perf_test_tweak(self, perf_config):
+        tweak_config = perf_config.perf_test_tweak_config
+        if 'turboboost' in tweak_config:
+            for host, host_data in tweak_config['turboboost'].items():
+                if host_data['status'] == 'disabled':
+                    host.run("echo 0 >
/sys/devices/system/cpu/intel_pstate/no_turbo")
+            
+            del tweak_config['turboboost']
+
+        super().remove_perf_test_tweak(perf_config)
diff --git a/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
b/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
index fc5b4747..cbc9eda8 100644
--- a/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
+++ b/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
@@ -1,3 +1,4 @@
 from lnst.Recipes.ENRT.PerfTestMixins.BasePerfTestTweakMixin import
BasePerfTestTweakMixin
 from lnst.Recipes.ENRT.PerfTestMixins.SctpFirewallPerfTestMixin import
SctpFirewallPerfTestMixin
 from lnst.Recipes.ENRT.PerfTestMixins.DisableIdleStatesMixin import
DisableIdleStatesMixin
+from lnst.Recipes.ENRT.PerfTestMixins.DisableTurboboostMixin import
DisableTurboboostMixin
-- 
2.21.3

[PATCH 3/5] Recipes.ENRT.PerfTestMixins: add DisableTurboboostMixin

[Ondrej Lichtner]

On Mon, Oct 12, 2020 at 07:15:13PM +0200, Jan Tluka wrote:
>  This mixin class provides recipe parameter
> 'disable_turboboost' that can be
>  used to disable CPU turboboost before running a performance test through
>  BaseEnrtRecipe.do_perf_tests().
>  
>  It uses intel_pstate sysfs interface to disable the CPU feature and so
>  it is usable only by systems with Intel CPUs.
>  
>  To use it pass 'disable_turboost=True' parameter when creating an instance of
>  BaseEnrtRecipe.
>  
>  Signed-off-by: Jan Tluka <jtluka(a)redhat.com&gt;
>  ---
>   .../PerfTestMixins/DisableTurboboostMixin.py  | 55 +++++++++++++++++++
>   lnst/Recipes/ENRT/PerfTestMixins/__init__.py  |  1 +
>   2 files changed, 56 insertions(+)
>   create mode 100644 lnst/Recipes/ENRT/PerfTestMixins/DisableTurboboostMixin.py
>  
>  diff --git a/lnst/Recipes/ENRT/PerfTestMixins/DisableTurboboostMixin.py
> b/lnst/Recipes/ENRT/PerfTestMixins/DisableTurboboostMixin.py
>  new file mode 100644
>  index 00000000..8beec018
>  --- /dev/null
>  +++ b/lnst/Recipes/ENRT/PerfTestMixins/DisableTurboboostMixin.py
>  @@ -0,0 +1,55 @@
>  +from lnst.Common.Parameters import BoolParam
>  +from lnst.Recipes.ENRT.PerfTestMixins import BasePerfTestTweakMixin
>  +
>  +class DisableTurboboostMixin(BasePerfTestTweakMixin):
>  +    disable_turboboost = BoolParam(default=False)
>  +
>  +    def generate_perf_test_tweak_description(self, perf_config):
>  +        description = super().generate_perf_test_tweak_description(perf_config)
>  +        tweak_config = perf_config.perf_test_tweak_config
>  +        if 'turboboost' in tweak_config:
>  +            for host, host_data in tweak_config['turboboost'].items():
>  +                description.append(host_data['desc'])
>  +        else:
>  +            description.append("skipped disabling turboboost through
> intel_pstate")
>  +
>  +        return description
>  +
>  +    def _is_turboboost_supported(self, host):
>  +        file_check = host.run("ls
> /sys/devices/system/cpu/intel_pstate/no_turbo")
>  +        return file_check.passed
>  +
>  +    def apply_perf_test_tweak(self, perf_config):
>  +        super().apply_perf_test_tweak(perf_config)
>  +        if self.params.disable_turboboost:
>  +            tweak_config = perf_config.perf_test_tweak_config
>  +            tweak_config['turboboost'] = {}
>  +
>  +            flow_measurement = self._get_flow_measurement_from_config(perf_config)
>  +            flow = flow_measurement.conf[0]
>  +            for host in [flow.generator, flow.receiver]: 
Same comment as for previous patch - how should this be handled for
Virtual recipes?

maybe a "disable_turboboost_host_list" property could work for these
cases?

>  +                if self._is_turboboost_supported(host):
>  +                    # TODO: save previous state
>  +                    host.run("echo 1 >
> /sys/devices/system/cpu/intel_pstate/no_turbo")
>  +                    desc = "disabled turboboost through intel_pstate on
> {}".format(host.hostid)
>  +                    turboboost_status = 'disabled'
>  +                else:
>  +                    desc = "warning: user requested to disable turboboost "\
>  +                            "through intel_pstate but the sysfs file is not
> available "\
>  +                            "on host {}".format(host.hostid)
>  +                    turboboost_status = 'unsupported'
>  +                tweak_config['turboboost'][host] = {
>  +                        'desc': desc,
>  +                        'status': turboboost_status,
>  +                        }
>  +
>  +    def remove_perf_test_tweak(self, perf_config):
>  +        tweak_config = perf_config.perf_test_tweak_config
>  +        if 'turboboost' in tweak_config:
>  +            for host, host_data in tweak_config['turboboost'].items():
>  +                if host_data['status'] == 'disabled':
>  +                    host.run("echo 0 >
> /sys/devices/system/cpu/intel_pstate/no_turbo")
>  +             
whitespace here

-Ondrej

>  +            del tweak_config['turboboost']
>  +
>  +        super().remove_perf_test_tweak(perf_config)
>  diff --git a/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
> b/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
>  index fc5b4747..cbc9eda8 100644
>  --- a/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
>  +++ b/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
>  @@ -1,3 +1,4 @@
>   from lnst.Recipes.ENRT.PerfTestMixins.BasePerfTestTweakMixin import
> BasePerfTestTweakMixin
>   from lnst.Recipes.ENRT.PerfTestMixins.SctpFirewallPerfTestMixin import
> SctpFirewallPerfTestMixin
>   from lnst.Recipes.ENRT.PerfTestMixins.DisableIdleStatesMixin import
> DisableIdleStatesMixin
>  +from lnst.Recipes.ENRT.PerfTestMixins.DisableTurboboostMixin import
> DisableTurboboostMixin
>  -- 
>  2.21.3
>  _______________________________________________
>  LNST-developers mailing list -- lnst-developers(a)lists.fedorahosted.org
>  To unsubscribe send an email to lnst-developers-leave(a)lists.fedorahosted.org
>  Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
>  List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
>  List Archives:
> https://lists.fedorahosted.org/archives/list/[e-mail]...

[PATCH 3/5] Recipes.ENRT.PerfTestMixins: add DisableTurboboostMixin

[Jan Tluka]

Tue, Oct 13, 2020 at 10:27:08AM CEST, olichtne(a)redhat.com wrote:
> On Mon, Oct 12, 2020 at 07:15:13PM +0200, Jan Tluka wrote:
> > This mixin class provides recipe parameter 'disable_turboboost' that can be
> > used to disable CPU turboboost before running a performance test through
> > BaseEnrtRecipe.do_perf_tests().
> > 
> > It uses intel_pstate sysfs interface to disable the CPU feature and so
> > it is usable only by systems with Intel CPUs.
> > 
> > To use it pass 'disable_turboost=True' parameter when creating an instance
> of
> > BaseEnrtRecipe.
> > 
> > Signed-off-by: Jan Tluka <jtluka(a)redhat.com&gt;
> > ---
> >  .../PerfTestMixins/DisableTurboboostMixin.py  | 55 +++++++++++++++++++
> >  lnst/Recipes/ENRT/PerfTestMixins/__init__.py  |  1 +
> >  2 files changed, 56 insertions(+)
> >  create mode 100644 lnst/Recipes/ENRT/PerfTestMixins/DisableTurboboostMixin.py
> > 
> > diff --git a/lnst/Recipes/ENRT/PerfTestMixins/DisableTurboboostMixin.py
> b/lnst/Recipes/ENRT/PerfTestMixins/DisableTurboboostMixin.py
> > new file mode 100644
> > index 00000000..8beec018
> > --- /dev/null
> > +++ b/lnst/Recipes/ENRT/PerfTestMixins/DisableTurboboostMixin.py
> > @@ -0,0 +1,55 @@
> > +from lnst.Common.Parameters import BoolParam
> > +from lnst.Recipes.ENRT.PerfTestMixins import BasePerfTestTweakMixin
> > +
> > +class DisableTurboboostMixin(BasePerfTestTweakMixin):
> > +    disable_turboboost = BoolParam(default=False)
> > +
> > +    def generate_perf_test_tweak_description(self, perf_config):
> > +        description = super().generate_perf_test_tweak_description(perf_config)
> > +        tweak_config = perf_config.perf_test_tweak_config
> > +        if 'turboboost' in tweak_config:
> > +            for host, host_data in tweak_config['turboboost'].items():
> > +                description.append(host_data['desc'])
> > +        else:
> > +            description.append("skipped disabling turboboost through
> intel_pstate")
> > +
> > +        return description
> > +
> > +    def _is_turboboost_supported(self, host):
> > +        file_check = host.run("ls
> /sys/devices/system/cpu/intel_pstate/no_turbo")
> > +        return file_check.passed
> > +
> > +    def apply_perf_test_tweak(self, perf_config):
> > +        super().apply_perf_test_tweak(perf_config)
> > +        if self.params.disable_turboboost:
> > +            tweak_config = perf_config.perf_test_tweak_config
> > +            tweak_config['turboboost'] = {}
> > +
> > +            flow_measurement = self._get_flow_measurement_from_config(perf_config)
> > +            flow = flow_measurement.conf[0]
> > +            for host in [flow.generator, flow.receiver]:
>
> Same comment as for previous patch - how should this be handled for
> Virtual recipes?
>
> maybe a "disable_turboboost_host_list" property could work for these
> cases?
>  
Generally it is a good idea, I'm just wondering what the user should 
pass as the argument value.

I guess they should be strings matching hosts in self.matched.*.

So for example in case of VirtualBridgeVlansOverBondRecipe, which
defines:

    host1 = HostReq()
    ...
    host2 = HostReq()

this could be:

    disable_turboboost_host_list=['host1, host2']

Or is there a better way how to deal with this? I believe there's no
other test machine representation than that.

> > +                if self._is_turboboost_supported(host):
> > +                    # TODO: save previous state
> > +                    host.run("echo 1 >
> /sys/devices/system/cpu/intel_pstate/no_turbo")
> > +                    desc = "disabled turboboost through intel_pstate on
> {}".format(host.hostid)
> > +                    turboboost_status = 'disabled'
> > +                else:
> > +                    desc = "warning: user requested to disable turboboost
> "\
> > +                            "through intel_pstate but the sysfs file is not
> available "\
> > +                            "on host {}".format(host.hostid)
> > +                    turboboost_status = 'unsupported'
> > +                tweak_config['turboboost'][host] = {
> > +                        'desc': desc,
> > +                        'status': turboboost_status,
> > +                        }
> > +
> > +    def remove_perf_test_tweak(self, perf_config):
> > +        tweak_config = perf_config.perf_test_tweak_config
> > +        if 'turboboost' in tweak_config:
> > +            for host, host_data in tweak_config['turboboost'].items():
> > +                if host_data['status'] == 'disabled':
> > +                    host.run("echo 0 >
> /sys/devices/system/cpu/intel_pstate/no_turbo")
> > +            
>
> whitespace here 
Thanks for catching this!

-Jan

> -Ondrej
>
> > +            del tweak_config['turboboost']
> > +
> > +        super().remove_perf_test_tweak(perf_config)
> > diff --git a/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
> b/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
> > index fc5b4747..cbc9eda8 100644
> > --- a/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
> > +++ b/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
> > @@ -1,3 +1,4 @@
> >  from lnst.Recipes.ENRT.PerfTestMixins.BasePerfTestTweakMixin import
> BasePerfTestTweakMixin
> >  from lnst.Recipes.ENRT.PerfTestMixins.SctpFirewallPerfTestMixin import
> SctpFirewallPerfTestMixin
> >  from lnst.Recipes.ENRT.PerfTestMixins.DisableIdleStatesMixin import
> DisableIdleStatesMixin
> > +from lnst.Recipes.ENRT.PerfTestMixins.DisableTurboboostMixin import
> DisableTurboboostMixin
> > -- 
> > 2.21.3
> > _______________________________________________
> > LNST-developers mailing list -- lnst-developers(a)lists.fedorahosted.org
> > To unsubscribe send an email to lnst-developers-leave(a)lists.fedorahosted.org
> > Fedora Code of Conduct:
> https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> > List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> > List Archives:
> https://lists.fedorahosted.org/archives/list/[e-mail]...

[PATCH 3/5] Recipes.ENRT.PerfTestMixins: add DisableTurboboostMixin

[Ondrej Lichtner]

On Wed, Oct 14, 2020 at 11:26:29AM +0200, Jan Tluka wrote:
>  Tue, Oct 13, 2020 at 10:27:08AM CEST, olichtne(a)redhat.com wrote:
>  >On Mon, Oct 12, 2020 at 07:15:13PM +0200, Jan Tluka wrote:
>  >> +    def apply_perf_test_tweak(self, perf_config):
>  >> +        super().apply_perf_test_tweak(perf_config)
>  >> +        if self.params.disable_turboboost:
>  >> +            tweak_config = perf_config.perf_test_tweak_config
>  >> +            tweak_config['turboboost'] = {}
>  >> +
>  >> +            flow_measurement =
> self._get_flow_measurement_from_config(perf_config)
>  >> +            flow = flow_measurement.conf[0]
>  >> +            for host in [flow.generator, flow.receiver]:
>  >
>  >Same comment as for previous patch - how should this be handled for
>  >Virtual recipes?
>  >
>  >maybe a "disable_turboboost_host_list" property could work for these
>  >cases?
>  >
>  
>  Generally it is a good idea, I'm just wondering what the user should 
>  pass as the argument value.
>  
>  I guess they should be strings matching hosts in self.matched.*.
>  
>  So for example in case of VirtualBridgeVlansOverBondRecipe, which
>  defines:
>  
>      host1 = HostReq()
>      ...
>      host2 = HostReq()
>  
>  this could be:
>  
>      disable_turboboost_host_list=['host1, host2']
>  
>  Or is there a better way how to deal with this? I believe there's no
>  other test machine representation than that. 
Why not the objects from the self.matched.* directly? e.g.:

@property
def disable_turboboost_host_list(self):
    return [self.matched.m1, self.matched.m2, ...]


These are the same object references as flow.generator, but self.matched
contains all of them, and the property can select the relevant ones.

-Ondrej

[PATCH 3/5] Recipes.ENRT.PerfTestMixins: add DisableTurboboostMixin

[Jan Tluka]

Wed, Oct 14, 2020 at 11:53:11AM CEST, olichtne(a)redhat.com wrote:
> On Wed, Oct 14, 2020 at 11:26:29AM +0200, Jan Tluka wrote:
> > Tue, Oct 13, 2020 at 10:27:08AM CEST, olichtne(a)redhat.com wrote:
> > >On Mon, Oct 12, 2020 at 07:15:13PM +0200, Jan Tluka wrote:
> > >> +    def apply_perf_test_tweak(self, perf_config):
> > >> +        super().apply_perf_test_tweak(perf_config)
> > >> +        if self.params.disable_turboboost:
> > >> +            tweak_config = perf_config.perf_test_tweak_config
> > >> +            tweak_config['turboboost'] = {}
> > >> +
> > >> +            flow_measurement =
> self._get_flow_measurement_from_config(perf_config)
> > >> +            flow = flow_measurement.conf[0]
> > >> +            for host in [flow.generator, flow.receiver]:
> > >
> > >Same comment as for previous patch - how should this be handled for
> > >Virtual recipes?
> > >
> > >maybe a "disable_turboboost_host_list" property could work for these
> > >cases?
> > >
> > 
> > Generally it is a good idea, I'm just wondering what the user should 
> > pass as the argument value.
> > 
> > I guess they should be strings matching hosts in self.matched.*.
> > 
> > So for example in case of VirtualBridgeVlansOverBondRecipe, which
> > defines:
> > 
> >     host1 = HostReq()
> >     ...
> >     host2 = HostReq()
> > 
> > this could be:
> > 
> >     disable_turboboost_host_list=['host1, host2']
> > 
> > Or is there a better way how to deal with this? I believe there's no
> > other test machine representation than that.
>
> Why not the objects from the self.matched.* directly? e.g.:
>
> @property
> def disable_turboboost_host_list(self):
>     return [self.matched.m1, self.matched.m2, ...]
>
>
> These are the same object references as flow.generator, but self.matched
> contains all of them, and the property can select the relevant ones.
>
> -Ondrej 
My question was about how to pass this as a recipe parameter while
creating the instance.

r = VirtualBridgeVlansOverBondRecipe(
    disable_turboboost_host_list=['host1, host2']
    )

But solving this through a property is better.

-Jan

[PATCH 3/5] Recipes.ENRT.PerfTestMixins: add DisableTurboboostMixin

[Ondrej Lichtner]

On Wed, Oct 14, 2020 at 12:10:46PM +0200, Jan Tluka wrote:
>  Wed, Oct 14, 2020 at 11:53:11AM CEST, olichtne(a)redhat.com wrote:
>  >On Wed, Oct 14, 2020 at 11:26:29AM +0200, Jan Tluka wrote:
>  >> Tue, Oct 13, 2020 at 10:27:08AM CEST, olichtne(a)redhat.com wrote:
>  >> >On Mon, Oct 12, 2020 at 07:15:13PM +0200, Jan Tluka wrote:
>  >> >> +    def apply_perf_test_tweak(self, perf_config):
>  >> >> +        super().apply_perf_test_tweak(perf_config)
>  >> >> +        if self.params.disable_turboboost:
>  >> >> +            tweak_config = perf_config.perf_test_tweak_config
>  >> >> +            tweak_config['turboboost'] = {}
>  >> >> +
>  >> >> +            flow_measurement =
> self._get_flow_measurement_from_config(perf_config)
>  >> >> +            flow = flow_measurement.conf[0]
>  >> >> +            for host in [flow.generator, flow.receiver]:
>  >> >
>  >> >Same comment as for previous patch - how should this be handled for
>  >> >Virtual recipes?
>  >> >
>  >> >maybe a "disable_turboboost_host_list" property could work for
> these
>  >> >cases?
>  >> >
>  >> 
>  >> Generally it is a good idea, I'm just wondering what the user should 
>  >> pass as the argument value.
>  >> 
>  >> I guess they should be strings matching hosts in self.matched.*.
>  >> 
>  >> So for example in case of VirtualBridgeVlansOverBondRecipe, which
>  >> defines:
>  >> 
>  >>     host1 = HostReq()
>  >>     ...
>  >>     host2 = HostReq()
>  >> 
>  >> this could be:
>  >> 
>  >>     disable_turboboost_host_list=['host1, host2']
>  >> 
>  >> Or is there a better way how to deal with this? I believe there's no
>  >> other test machine representation than that.
>  >
>  >Why not the objects from the self.matched.* directly? e.g.:
>  >
>  >@property
>  >def disable_turboboost_host_list(self):
>  >    return [self.matched.m1, self.matched.m2, ...]
>  >
>  >
>  >These are the same object references as flow.generator, but self.matched
>  >contains all of them, and the property can select the relevant ones.
>  >
>  >-Ondrej
>  
>  My question was about how to pass this as a recipe parameter while
>  creating the instance.
>  
>  r = VirtualBridgeVlansOverBondRecipe(
>      disable_turboboost_host_list=['host1, host2']
>      )
>  
>  But solving this through a property is better.
>  
>  -Jan 
Ah, not sure if I would expose that as a parameter, maybe just a boolean
of disable/enable turboboosts, but "which hosts" I would keep on the
implementation of the property itself.

-Ondrej

[PATCH 4/5] Recipes.ENRT.PerfTestMixins: add CommonPerfTestTweakMixin

[Jan Tluka]

This class is a joint of SctpFirewallPerfTestMixin, DisableIdleStatesMixin
and DisableTurboboostMixin classes for a user convenience.

Signed-off-by: Jan Tluka <jtluka(a)redhat.com&gt;
---
 .../ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py      | 9 +++++++++
 lnst/Recipes/ENRT/PerfTestMixins/__init__.py             | 1 +
 2 files changed, 10 insertions(+)
 create mode 100644 lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py

diff --git a/lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
b/lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
new file mode 100644
index 00000000..3efff9f7
--- /dev/null
+++ b/lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
@@ -0,0 +1,9 @@
+from lnst.Recipes.ENRT.PerfTestMixins import (
+        SctpFirewallPerfTestMixin,
+        DisableIdleStatesMixin,
+        DisableTurboboostMixin,
+        )
+
+class CommonPerfTestTweakMixin(SctpFirewallPerfTestMixin, DisableTurboboostMixin,
+        DisableIdleStatesMixin):
+    pass
diff --git a/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
b/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
index cbc9eda8..431135ac 100644
--- a/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
+++ b/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
@@ -2,3 +2,4 @@ from lnst.Recipes.ENRT.PerfTestMixins.BasePerfTestTweakMixin import
BasePerfTest
 from lnst.Recipes.ENRT.PerfTestMixins.SctpFirewallPerfTestMixin import
SctpFirewallPerfTestMixin
 from lnst.Recipes.ENRT.PerfTestMixins.DisableIdleStatesMixin import
DisableIdleStatesMixin
 from lnst.Recipes.ENRT.PerfTestMixins.DisableTurboboostMixin import
DisableTurboboostMixin
+from lnst.Recipes.ENRT.PerfTestMixins.CommonPerfTestTweakMixin import
CommonPerfTestTweakMixin
-- 
2.21.3

[PATCH 4/5] Recipes.ENRT.PerfTestMixins: add CommonPerfTestTweakMixin

[Ondrej Lichtner]

On Mon, Oct 12, 2020 at 07:15:14PM +0200, Jan Tluka wrote:
>  This class is a joint of SctpFirewallPerfTestMixin,
> DisableIdleStatesMixin
>  and DisableTurboboostMixin classes for a user convenience.
>  
>  Signed-off-by: Jan Tluka <jtluka(a)redhat.com&gt;
>  ---
>   .../ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py      | 9 +++++++++
>   lnst/Recipes/ENRT/PerfTestMixins/__init__.py             | 1 +
>   2 files changed, 10 insertions(+)
>   create mode 100644 lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
>  
>  diff --git a/lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
> b/lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
>  new file mode 100644
>  index 00000000..3efff9f7
>  --- /dev/null
>  +++ b/lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
>  @@ -0,0 +1,9 @@
>  +from lnst.Recipes.ENRT.PerfTestMixins import (
>  +        SctpFirewallPerfTestMixin,
>  +        DisableIdleStatesMixin,
>  +        DisableTurboboostMixin,
>  +        )
>  +
>  +class CommonPerfTestTweakMixin(SctpFirewallPerfTestMixin, DisableTurboboostMixin,
>  +        DisableIdleStatesMixin):
>  +    pass 
I'm not sure if the DisableIdleStatesMixin and DisableTurboboostMixin
are actualy perf tweaks to be honest. To me they feel a little bit
different than the SctpFirewallPerfTestMixin, I think I would probably
have put them as generic Subconfiguration mixins.

In the context of adding a "*host_list" I think it also makes a little
more sense as the mixins no longer need to access the Flow measurement
object anymore.

What do you think?

-Ondrej

>  diff --git a/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
> b/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
>  index cbc9eda8..431135ac 100644
>  --- a/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
>  +++ b/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
>  @@ -2,3 +2,4 @@ from lnst.Recipes.ENRT.PerfTestMixins.BasePerfTestTweakMixin import
> BasePerfTest
>   from lnst.Recipes.ENRT.PerfTestMixins.SctpFirewallPerfTestMixin import
> SctpFirewallPerfTestMixin
>   from lnst.Recipes.ENRT.PerfTestMixins.DisableIdleStatesMixin import
> DisableIdleStatesMixin
>   from lnst.Recipes.ENRT.PerfTestMixins.DisableTurboboostMixin import
> DisableTurboboostMixin
>  +from lnst.Recipes.ENRT.PerfTestMixins.CommonPerfTestTweakMixin import
> CommonPerfTestTweakMixin
>  -- 
>  2.21.3
>  _______________________________________________
>  LNST-developers mailing list -- lnst-developers(a)lists.fedorahosted.org
>  To unsubscribe send an email to lnst-developers-leave(a)lists.fedorahosted.org
>  Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
>  List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
>  List Archives:
> https://lists.fedorahosted.org/archives/list/[e-mail]...

[PATCH 4/5] Recipes.ENRT.PerfTestMixins: add CommonPerfTestTweakMixin

[Jan Tluka]

Tue, Oct 13, 2020 at 10:28:58AM CEST, olichtne(a)redhat.com wrote:
> On Mon, Oct 12, 2020 at 07:15:14PM +0200, Jan Tluka wrote:
> > This class is a joint of SctpFirewallPerfTestMixin, DisableIdleStatesMixin
> > and DisableTurboboostMixin classes for a user convenience.
> > 
> > Signed-off-by: Jan Tluka <jtluka(a)redhat.com&gt;
> > ---
> >  .../ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py      | 9 +++++++++
> >  lnst/Recipes/ENRT/PerfTestMixins/__init__.py             | 1 +
> >  2 files changed, 10 insertions(+)
> >  create mode 100644 lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
> > 
> > diff --git a/lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
> b/lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
> > new file mode 100644
> > index 00000000..3efff9f7
> > --- /dev/null
> > +++ b/lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
> > @@ -0,0 +1,9 @@
> > +from lnst.Recipes.ENRT.PerfTestMixins import (
> > +        SctpFirewallPerfTestMixin,
> > +        DisableIdleStatesMixin,
> > +        DisableTurboboostMixin,
> > +        )
> > +
> > +class CommonPerfTestTweakMixin(SctpFirewallPerfTestMixin, DisableTurboboostMixin,
> > +        DisableIdleStatesMixin):
> > +    pass
>
> I'm not sure if the DisableIdleStatesMixin and DisableTurboboostMixin
> are actualy perf tweaks to be honest. To me they feel a little bit
> different than the SctpFirewallPerfTestMixin, I think I would probably
> have put them as generic Subconfiguration mixins.
>
> In the context of adding a "*host_list" I think it also makes a little
> more sense as the mixins no longer need to access the Flow measurement
> object anymore.
>
> What do you think?
>
> -Ondrej
>  
I think that these mixins are required only for the performance test so
it kind of makes sense to have it isolated just for this.

But I have no problem with moving this to subconfig level.

-Jan

[PATCH 4/5] Recipes.ENRT.PerfTestMixins: add CommonPerfTestTweakMixin

[Ondrej Lichtner]

On Wed, Oct 14, 2020 at 11:07:55AM +0200, Jan Tluka wrote:
>  Tue, Oct 13, 2020 at 10:28:58AM CEST, olichtne(a)redhat.com wrote:
>  >On Mon, Oct 12, 2020 at 07:15:14PM +0200, Jan Tluka wrote:
>  >> This class is a joint of SctpFirewallPerfTestMixin, DisableIdleStatesMixin
>  >> and DisableTurboboostMixin classes for a user convenience.
>  >> 
>  >> Signed-off-by: Jan Tluka <jtluka(a)redhat.com&gt;
>  >> ---
>  >>  .../ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py      | 9 +++++++++
>  >>  lnst/Recipes/ENRT/PerfTestMixins/__init__.py             | 1 +
>  >>  2 files changed, 10 insertions(+)
>  >>  create mode 100644
> lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
>  >> 
>  >> diff --git a/lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
> b/lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
>  >> new file mode 100644
>  >> index 00000000..3efff9f7
>  >> --- /dev/null
>  >> +++ b/lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
>  >> @@ -0,0 +1,9 @@
>  >> +from lnst.Recipes.ENRT.PerfTestMixins import (
>  >> +        SctpFirewallPerfTestMixin,
>  >> +        DisableIdleStatesMixin,
>  >> +        DisableTurboboostMixin,
>  >> +        )
>  >> +
>  >> +class CommonPerfTestTweakMixin(SctpFirewallPerfTestMixin,
> DisableTurboboostMixin,
>  >> +        DisableIdleStatesMixin):
>  >> +    pass
>  >
>  >I'm not sure if the DisableIdleStatesMixin and DisableTurboboostMixin
>  >are actualy perf tweaks to be honest. To me they feel a little bit
>  >different than the SctpFirewallPerfTestMixin, I think I would probably
>  >have put them as generic Subconfiguration mixins.
>  >
>  >In the context of adding a "*host_list" I think it also makes a little
>  >more sense as the mixins no longer need to access the Flow measurement
>  >object anymore.
>  >
>  >What do you think?
>  >
>  >-Ondrej
>  >
>  
>  I think that these mixins are required only for the performance test so
>  it kind of makes sense to have it isolated just for this.
>  
>  But I have no problem with moving this to subconfig level.
>  
>  -Jan 
I guess that's true, they shouldn't affect the functionality or
connectivity.

I was thinking from the opposite view - these mixins don't require any
of the additional context provided by the actual perf configuration -
which means that they're going to be applied and removed for each perf
test, but in exactly the same way.

On the other hand the SctpFirewallPerfTestMixin is applied based on
specifics defined by the flow.

This means that moving the Disable* mixins to the subconfig level will
"save" some amount of apply-remove cycles that were spent for the
multiple flow combinations generated.

-Ondrej

[PATCH 4/5] Recipes.ENRT.PerfTestMixins: add CommonPerfTestTweakMixin

[Jan Tluka]

Wed, Oct 14, 2020 at 12:03:29PM CEST, olichtne(a)redhat.com wrote:
> On Wed, Oct 14, 2020 at 11:07:55AM +0200, Jan Tluka wrote:
> > Tue, Oct 13, 2020 at 10:28:58AM CEST, olichtne(a)redhat.com wrote:
> > >On Mon, Oct 12, 2020 at 07:15:14PM +0200, Jan Tluka wrote:
> > >> This class is a joint of SctpFirewallPerfTestMixin, DisableIdleStatesMixin
> > >> and DisableTurboboostMixin classes for a user convenience.
> > >> 
> > >> Signed-off-by: Jan Tluka <jtluka(a)redhat.com&gt;
> > >> ---
> > >>  .../ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py      | 9 +++++++++
> > >>  lnst/Recipes/ENRT/PerfTestMixins/__init__.py             | 1 +
> > >>  2 files changed, 10 insertions(+)
> > >>  create mode 100644
> lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
> > >> 
> > >> diff --git a/lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
> b/lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
> > >> new file mode 100644
> > >> index 00000000..3efff9f7
> > >> --- /dev/null
> > >> +++ b/lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
> > >> @@ -0,0 +1,9 @@
> > >> +from lnst.Recipes.ENRT.PerfTestMixins import (
> > >> +        SctpFirewallPerfTestMixin,
> > >> +        DisableIdleStatesMixin,
> > >> +        DisableTurboboostMixin,
> > >> +        )
> > >> +
> > >> +class CommonPerfTestTweakMixin(SctpFirewallPerfTestMixin,
> DisableTurboboostMixin,
> > >> +        DisableIdleStatesMixin):
> > >> +    pass
> > >
> > >I'm not sure if the DisableIdleStatesMixin and DisableTurboboostMixin
> > >are actualy perf tweaks to be honest. To me they feel a little bit
> > >different than the SctpFirewallPerfTestMixin, I think I would probably
> > >have put them as generic Subconfiguration mixins.
> > >
> > >In the context of adding a "*host_list" I think it also makes a little
> > >more sense as the mixins no longer need to access the Flow measurement
> > >object anymore.
> > >
> > >What do you think?
> > >
> > >-Ondrej
> > >
> > 
> > I think that these mixins are required only for the performance test so
> > it kind of makes sense to have it isolated just for this.
> > 
> > But I have no problem with moving this to subconfig level.
> > 
> > -Jan
>
> I guess that's true, they shouldn't affect the functionality or
> connectivity.
>
> I was thinking from the opposite view - these mixins don't require any
> of the additional context provided by the actual perf configuration -
> which means that they're going to be applied and removed for each perf
> test, but in exactly the same way.
>
> On the other hand the SctpFirewallPerfTestMixin is applied based on
> specifics defined by the flow.
>
> This means that moving the Disable* mixins to the subconfig level will
> "save" some amount of apply-remove cycles that were spent for the
> multiple flow combinations generated.
>
> -Ondrej
>  
More thoughts on this.

Just to be clear, are you proposing to change the mixins to inherit from
the BaseSubConfigMixin? Or should this be a different concept?

Secondly, if we want to save the apply-remove cycles, it would make more
sense to move this between the test_wide configuration and
sub_configuration. With this there will be just one apply-remove cycle.

-Jan

[PATCH 4/5] Recipes.ENRT.PerfTestMixins: add CommonPerfTestTweakMixin

[Ondrej Lichtner]

On Wed, Oct 14, 2020 at 02:58:53PM +0200, Jan Tluka wrote:
>  Wed, Oct 14, 2020 at 12:03:29PM CEST, olichtne(a)redhat.com wrote:
>  >On Wed, Oct 14, 2020 at 11:07:55AM +0200, Jan Tluka wrote:
>  >> Tue, Oct 13, 2020 at 10:28:58AM CEST, olichtne(a)redhat.com wrote:
>  >> >On Mon, Oct 12, 2020 at 07:15:14PM +0200, Jan Tluka wrote:
>  >> >> This class is a joint of SctpFirewallPerfTestMixin,
> DisableIdleStatesMixin
>  >> >> and DisableTurboboostMixin classes for a user convenience.
>  >> >> 
>  >> >> Signed-off-by: Jan Tluka <jtluka(a)redhat.com&gt;
>  >> >> ---
>  >> >>  .../ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py      | 9
> +++++++++
>  >> >>  lnst/Recipes/ENRT/PerfTestMixins/__init__.py             | 1 +
>  >> >>  2 files changed, 10 insertions(+)
>  >> >>  create mode 100644
> lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
>  >> >> 
>  >> >> diff --git
> a/lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
> b/lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
>  >> >> new file mode 100644
>  >> >> index 00000000..3efff9f7
>  >> >> --- /dev/null
>  >> >> +++ b/lnst/Recipes/ENRT/PerfTestMixins/CommonPerfTestTweakMixin.py
>  >> >> @@ -0,0 +1,9 @@
>  >> >> +from lnst.Recipes.ENRT.PerfTestMixins import (
>  >> >> +        SctpFirewallPerfTestMixin,
>  >> >> +        DisableIdleStatesMixin,
>  >> >> +        DisableTurboboostMixin,
>  >> >> +        )
>  >> >> +
>  >> >> +class CommonPerfTestTweakMixin(SctpFirewallPerfTestMixin,
> DisableTurboboostMixin,
>  >> >> +        DisableIdleStatesMixin):
>  >> >> +    pass
>  >> >
>  >> >I'm not sure if the DisableIdleStatesMixin and DisableTurboboostMixin
>  >> >are actualy perf tweaks to be honest. To me they feel a little bit
>  >> >different than the SctpFirewallPerfTestMixin, I think I would probably
>  >> >have put them as generic Subconfiguration mixins.
>  >> >
>  >> >In the context of adding a "*host_list" I think it also makes a
> little
>  >> >more sense as the mixins no longer need to access the Flow measurement
>  >> >object anymore.
>  >> >
>  >> >What do you think?
>  >> >
>  >> >-Ondrej
>  >> >
>  >> 
>  >> I think that these mixins are required only for the performance test so
>  >> it kind of makes sense to have it isolated just for this.
>  >> 
>  >> But I have no problem with moving this to subconfig level.
>  >> 
>  >> -Jan
>  >
>  >I guess that's true, they shouldn't affect the functionality or
>  >connectivity.
>  >
>  >I was thinking from the opposite view - these mixins don't require any
>  >of the additional context provided by the actual perf configuration -
>  >which means that they're going to be applied and removed for each perf
>  >test, but in exactly the same way.
>  >
>  >On the other hand the SctpFirewallPerfTestMixin is applied based on
>  >specifics defined by the flow.
>  >
>  >This means that moving the Disable* mixins to the subconfig level will
>  >"save" some amount of apply-remove cycles that were spent for the
>  >multiple flow combinations generated.
>  >
>  >-Ondrej
>  >
>  
>  More thoughts on this.
>  
>  Just to be clear, are you proposing to change the mixins to inherit from
>  the BaseSubConfigMixin? Or should this be a different concept?
>  
>  Secondly, if we want to save the apply-remove cycles, it would make more
>  sense to move this between the test_wide configuration and
>  sub_configuration. With this there will be just one apply-remove cycle.
>  
>  -Jan 
Talked about this offline, yes, I believe moving this to inherit from
BaseSubConfigMixin is better.

Introducing some intermediate mixin loop between test_wide_configuration
and sub_configuration may be an interesting idea but I think we should
follow up on that later.

-Ondrej

[PATCH 5/5] Recipes.ENRT.BaseEnrtRecipe: inherit from CommonPerfTestTweakMixin

[Jan Tluka]

SctpFirewallPerfTest mixin is replaced with CommonPerfTestTweakMixin to include
additional mixins in much simpler way.

Signed-off-by: Jan Tluka <jtluka(a)redhat.com&gt;
---
 lnst/Recipes/ENRT/BaseEnrtRecipe.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lnst/Recipes/ENRT/BaseEnrtRecipe.py b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
index 4d0c93c8..a5296e94 100644
--- a/lnst/Recipes/ENRT/BaseEnrtRecipe.py
+++ b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
@@ -13,7 +13,7 @@ from lnst.Common.Parameters import (
 from lnst.Common.IpAddress import AF_INET, AF_INET6
 
 from lnst.Recipes.ENRT.ConfigMixins.BaseSubConfigMixin import BaseSubConfigMixin
-from lnst.Recipes.ENRT.PerfTestMixins import SctpFirewallPerfTestMixin
+from lnst.Recipes.ENRT.PerfTestMixins import CommonPerfTestTweakMixin
 
 from lnst.RecipeCommon.Ping.Recipe import PingTestAndEvaluate, PingConf
 from lnst.RecipeCommon.Perf.Recipe import Recipe as PerfRecipe
@@ -33,7 +33,7 @@ class EnrtConfiguration(object):
     """
     pass
 
-class BaseEnrtRecipe(SctpFirewallPerfTestMixin, BaseSubConfigMixin,
+class BaseEnrtRecipe(CommonPerfTestTweakMixin, BaseSubConfigMixin,
         PingTestAndEvaluate, PerfRecipe):
     """Base Recipe class for the ENRT recipe package
 
-- 
2.21.3