[PATCH] Fixed reversed perf flow

From: Perry Gagne

redefined the perf_reverse parameter.

Added mixin for recipes that which to be able to run their flows
in reverse.

Signed-off-by: Perry Gagne 
---
 lnst/Recipes/ENRT/BaseEnrtRecipe.py           | 61 ++++++++-----------
 .../ConfigMixins/PerfReversibleFlowMixin.py   | 36 +++++++++++
 lnst/Recipes/ENRT/TeamRecipe.py               |  8 +--
 3 files changed, 64 insertions(+), 41 deletions(-)
 create mode 100644 lnst/Recipes/ENRT/ConfigMixins/PerfReversibleFlowMixin.py

diff --git a/lnst/Recipes/ENRT/BaseEnrtRecipe.py b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
index f7bb588..6a287ad 100644
--- a/lnst/Recipes/ENRT/BaseEnrtRecipe.py
+++ b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
@@ -143,13 +143,6 @@ class BaseEnrtRecipe(BaseSubConfigMixin, PingTestAndEvaluate, PerfRecipe):
         separate performance measurement.
     :type perf_msg_sizes: List[Int] (default [123])

-    :param perf_reverse:
-        Parameter used by the :any:`generate_flow_combinations` generator. To
-        specify that both directions between the endpoints of a network flow
-        should be measured for the same test.
-    :type perf_reverse: :any:`BoolParam` (default False)
-
-
     :param net_perf_tool:
         Parameter used by the :any:`generate_perf_configurations` generator to
         create a PerfRecipeConf object.
@@ -185,7 +178,6 @@ class BaseEnrtRecipe(BaseSubConfigMixin, PingTestAndEvaluate, PerfRecipe):
     perf_iterations = IntParam(default=5)
     perf_parallel_streams = IntParam(default=1)
     perf_msg_sizes = ListParam(default=[123])
-    perf_reverse = BoolParam(default=False)

net_perf_tool = Param(default=IperfFlowMeasurement)
     cpu_perf_tool = Param(default=StatCPUMeasurement)
@@ -499,22 +491,29 @@ class BaseEnrtRecipe(BaseSubConfigMixin, PingTestAndEvaluate, PerfRecipe):

for perf_test in self.params.perf_tests:
                     for size in self.params.perf_msg_sizes:
-                        flow = PerfFlow(
-                                type = perf_test,
-                                generator = client_nic.netns,
-                                generator_bind = client_bind,
-                                receiver = server_nic.netns,
-                                receiver_bind = server_bind,
-                                msg_size = size,
-                                duration = self.params.perf_duration,
-                                parallel_streams = self.params.perf_parallel_streams,
-                                cpupin = self.params.perf_tool_cpu if "perf_tool_cpu" in self.params else None
-                                )
-                        yield [flow]
-
-                        if self.params.perf_reverse:
-                            reverse_flow = self._create_reverse_flow(flow)
-                            yield [reverse_flow]
+                        yield [self._create_perf_flow(perf_test,
+                                                      client_nic,
+                                                      client_bind,
+                                                      server_nic,
+                                                      server_bind,
+                                                      size,
+                                                      )]
+
+    def _create_perf_flow(self, perf_test, client_nic, client_bind, server_nic,
+                          server_bind, msg_size) -> PerfFlow:
+        """
+        Wrapper to create a PerfFlow. Mixins that want to change this behavior (for example, to reverse the direction)
+        can override this method as an alternative to overriding :any:`generate_flow_combinations`
+        """
+        cpupin = self.params.perf_tool_cpu if "perf_tool_cpu" in self.params else None
+        return PerfFlow(type=perf_test,
+                        generator=client_nic.netns, generator_bind=client_bind,
+                        receiver=server_nic.netns, receiver_bind=server_bind,
+                        msg_size=msg_size,
+                        duration=self.params.perf_duration,
+                        parallel_streams=self.params.perf_parallel_streams,
+                        cpupin=cpupin,
+                        )

def generate_perf_endpoints(self, config):
         """Generator for perf endpoints
@@ -558,19 +557,7 @@ class BaseEnrtRecipe(BaseSubConfigMixin, PingTestAndEvaluate, PerfRecipe):

self.ctl.wait_for_condition(condition, timeout=5)

-    def _create_reverse_flow(self, flow):
-        rev_flow = PerfFlow(
-                    type = flow.type,
-                    generator = flow.receiver,
-                    generator_bind = flow.receiver_bind,
-                    receiver = flow.generator,
-                    receiver_bind = flow.generator_bind,
-                    msg_size = flow.msg_size,
-                    duration = flow.duration,
-                    parallel_streams = flow.parallel_streams,
-                    cpupin = flow.cpupin
-                    )
-        return rev_flow
+

def _create_reverse_ping(self, pconf):
         return PingConf(
diff --git a/lnst/Recipes/ENRT/ConfigMixins/PerfReversibleFlowMixin.py b/lnst/Recipes/ENRT/ConfigMixins/PerfReversibleFlowMixin.py
new file mode 100644
index 0000000..a5bcfad
--- /dev/null
+++ b/lnst/Recipes/ENRT/ConfigMixins/PerfReversibleFlowMixin.py
@@ -0,0 +1,36 @@
+from lnst.Common.Parameters import BoolParam
+from lnst.RecipeCommon.Perf.Measurements import Flow as PerfFlow
+
+
+class PerfReversibleFlowMixin(object):
+    """ Mixin class for reversing the performance test flows
+
+    This only really makes sense for recipes that have asymmetric endpoints.
+
+    For example:
+
+    SimpleNetworkRecipe is symmetrical since both endpoints are of the same type
+    (both plain interfaces).
+
+    TeamRecipe is asymmetrical because one endpoint is a team device and the
+     other is a plain interface.
+
+    So TeamRecipe could use this mixin to indicate that is can be reversed.
+
+    This can be controlled by the perf_reverse parameter:
+
+    :param perf_reverse:
+        Parameter used by the :any:`generate_flow_combinations` generator. To
+        specify that the flow of traffic between the endpoints should be reversed.
+    :type perf_reverse: :any:`BoolParam` (default False)
+    """
+    perf_reverse = BoolParam(default=False)
+
+    def _create_perf_flow(self, perf_test, client_nic, client_bind, server_nic,
+                          server_bind, msg_size) -> PerfFlow:
+        if self.params.perf_reverse:
+            return super()._create_perf_flow(perf_test, server_nic, server_bind,
+                                             client_nic, client_bind, msg_size)
+        else:
+            return super()._create_perf_flow(perf_test, client_nic, client_bind,
+                                             server_nic, server_bind, msg_size)
diff --git a/lnst/Recipes/ENRT/TeamRecipe.py b/lnst/Recipes/ENRT/TeamRecipe.py
index e97a10b..b8233c8 100644
--- a/lnst/Recipes/ENRT/TeamRecipe.py
+++ b/lnst/Recipes/ENRT/TeamRecipe.py
@@ -6,11 +6,13 @@ from lnst.Recipes.ENRT.ConfigMixins.OffloadSubConfigMixin import (
     OffloadSubConfigMixin)
 from lnst.Recipes.ENRT.ConfigMixins.CommonHWSubConfigMixin import (
     CommonHWSubConfigMixin)
+from lnst.Recipes.ENRT.ConfigMixins.PerfReversibleFlowMixin import (
+    PerfReversibleFlowMixin)
 from lnst.RecipeCommon.Ping.PingEndpoints import PingEndpoints
 from lnst.Devices import TeamDevice

-class TeamRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
+class TeamRecipe(PerfReversibleFlowMixin, CommonHWSubConfigMixin, OffloadSubConfigMixin,
     BaseEnrtRecipe):
     host1 = HostReq()
     host1.eth0 = DeviceReq(label="tnet", driver=RecipeParam("driver"))
@@ -25,7 +27,6 @@ class TeamRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
         dict(gro="on", gso="off", tso="off", tx="on"),
         dict(gro="on", gso="on", tso="off", tx="off")))

-    perf_reverse = BoolParam(default=True)
     runner_name = StrParam(mandatory=True)

def test_wide_configuration(self):
@@ -88,8 +89,7 @@ class TeamRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
         ]

def generate_perf_endpoints(self, config):
-        return [(self.matched.host1.team0, self.matched.host2.eth0),
-            (self.matched.host2.eth0, self.matched.host1.team0)]
+        return [(self.matched.host1.team0, self.matched.host2.eth0)]

@property
     def offload_nics(self):

[Ondrej Lichtner]

On Mon, May 18, 2020 at 04:16:21PM -0400, [e-mail] wrote:
> From: Perry Gagne
>
> redefined the perf_reverse parameter.
>
> Added mixin for recipes that which to be able to run their flows
> in reverse.
>
> Signed-off-by: Perry Gagne

Looks fine, just a generic comment to the commit message - we usually
like to add some information about the location of the changes into the
header of the commit message, e.g. "ENRT.Recipes:" so we can see the
context at a glance.

Similarly a more detailed explanation of the changes would also be
useful, in this case the changes are documented with docstrings so it's
not a big deal, but again... being able to understand the changes, and
the motivation behind those changes, from just the commit description
can be often very helpful.

Anyway, pushed to github master.

-Ondrej

[Jan Tluka]

Mon, May 18, 2020 at 10:16:21PM CEST, [e-mail] wrote:
> From: Perry Gagne
>
> redefined the perf_reverse parameter.
>
> Added mixin for recipes that which to be able to run their flows
> in reverse.
> Signed-off-by: Perry Gagne
>
> lnst/Recipes/ENRT/BaseEnrtRecipe.py           | 61 ++++++++-----------
> .../ConfigMixins/PerfReversibleFlowMixin.py   | 36 +++++++++++
> lnst/Recipes/ENRT/TeamRecipe.py               |  8 +--
> 3 files changed, 64 insertions(+), 41 deletions(-)
> create mode 100644 lnst/Recipes/ENRT/ConfigMixins/PerfReversibleFlowMixin.py
>
> diff --git a/lnst/Recipes/ENRT/BaseEnrtRecipe.py b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
> index f7bb588..6a287ad 100644
> --- a/lnst/Recipes/ENRT/BaseEnrtRecipe.py
> +++ b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
> @@ -143,13 +143,6 @@ class BaseEnrtRecipe(BaseSubConfigMixin, PingTestAndEvaluate, PerfRecipe):
>         separate performance measurement.
>     :type perf_msg_sizes: List[Int] (default [123])
> :param perf_reverse:
>    Parameter used by the :any:`generate_flow_combinations` generator. To
>    specify that both directions between the endpoints of a network flow
>    should be measured for the same test.
> :type perf_reverse: :any:`BoolParam` (default False)
> :param net_perf_tool:
>       Parameter used by the :any:`generate_perf_configurations` generator to
>       create a PerfRecipeConf object.
>
> @@ -185,7 +178,6 @@ class BaseEnrtRecipe(BaseSubConfigMixin, PingTestAndEvaluate, PerfRecipe):
>     perf_iterations = IntParam(default=5)
>     perf_parallel_streams = IntParam(default=1)
>     perf_msg_sizes = ListParam(default=[123])
>
> perf_reverse = BoolParam(default=False)
>
> net_perf_tool = Param(default=IperfFlowMeasurement)
> cpu_perf_tool = Param(default=StatCPUMeasurement)
>
> @@ -499,22 +491,29 @@ class BaseEnrtRecipe(BaseSubConfigMixin, PingTestAndEvaluate, PerfRecipe):
>             for perf_test in self.params.perf_tests:
>                 for size in self.params.perf_msg_sizes:
>                    flow = PerfFlow(
>                            type = perf_test,
>                            generator = client_nic.netns,
>                            generator_bind = client_bind,
>                            receiver = server_nic.netns,
>                            receiver_bind = server_bind,
>                            msg_size = size,
>                            duration = self.params.perf_duration,
>                            parallel_streams = self.params.perf_parallel_streams,
>                            cpupin = self.params.perf_tool_cpu if "perf_tool_cpu" in self.params else None
>                            )
>                    yield [flow]
>                    if self.params.perf_reverse:
>                        reverse_flow = self._create_reverse_flow(flow)
>                        yield [reverse_flow]
>                    yield [self._create_perf_flow(perf_test,
>                                                  client_nic,
>                                                  client_bind,
>                                                  server_nic,
>                                                  server_bind,
>                                                  size,
>                                                  )]
>
> def _create_perf_flow(self, perf_test, client_nic, client_bind, server_nic,
>                      server_bind, msg_size) -> PerfFlow:
>    """
>    Wrapper to create a PerfFlow. Mixins that want to change this behavior (for example, to reverse the direction)
>    can override this method as an alternative to overriding :any:`generate_flow_combinations`
>    """
>    cpupin = self.params.perf_tool_cpu if "perf_tool_cpu" in self.params else None
>    return PerfFlow(type=perf_test,
>                    generator=client_nic.netns, generator_bind=client_bind,
>                    receiver=server_nic.netns, receiver_bind=server_bind,
>                    msg_size=msg_size,
>                    duration=self.params.perf_duration,
>                    parallel_streams=self.params.perf_parallel_streams,
>                    cpupin=cpupin,
>                    )
>
> def generate_perf_endpoints(self, config):
>       """Generator for perf endpoints
>
> @@ -558,19 +557,7 @@ class BaseEnrtRecipe(BaseSubConfigMixin, PingTestAndEvaluate, PerfRecipe):
>     self.ctl.wait_for_condition(condition, timeout=5)
>
> def _create_reverse_flow(self, flow):
>    rev_flow = PerfFlow(
>                type = flow.type,
>                generator = flow.receiver,
>                generator_bind = flow.receiver_bind,
>                receiver = flow.generator,
>                receiver_bind = flow.generator_bind,
>                msg_size = flow.msg_size,
>                duration = flow.duration,
>                parallel_streams = flow.parallel_streams,
>                cpupin = flow.cpupin
>                )
>    return rev_flow
>
> def _create_reverse_ping(self, pconf):
>       return PingConf(
>
> diff --git a/lnst/Recipes/ENRT/ConfigMixins/PerfReversibleFlowMixin.py b/lnst/Recipes/ENRT/ConfigMixins/PerfReversibleFlowMixin.py
> new file mode 100644
> index 0000000..a5bcfad
> --- /dev/null
> +++ b/lnst/Recipes/ENRT/ConfigMixins/PerfReversibleFlowMixin.py
> @@ -0,0 +1,36 @@
> +from lnst.Common.Parameters import BoolParam
> +from lnst.RecipeCommon.Perf.Measurements import Flow as PerfFlow
>
> +class PerfReversibleFlowMixin(object):
> """ Mixin class for reversing the performance test flows
> This only really makes sense for recipes that have asymmetric endpoints.
> For example:
> SimpleNetworkRecipe is symmetrical since both endpoints are of the same type
> (both plain interfaces).
> TeamRecipe is asymmetrical because one endpoint is a team device and the
> other is a plain interface.
> So TeamRecipe could use this mixin to indicate that is can be reversed.
> This can be controlled by the perf_reverse parameter:
> :param perf_reverse:
>    Parameter used by the :any:`generate_flow_combinations` generator. To
>    specify that the flow of traffic between the endpoints should be reversed.
> :type perf_reverse: :any:`BoolParam` (default False)
> """
> perf_reverse = BoolParam(default=False)
> def _create_perf_flow(self, perf_test, client_nic, client_bind, server_nic,
>                      server_bind, msg_size) -> PerfFlow:
>    if self.params.perf_reverse:
>        return super()._create_perf_flow(perf_test, server_nic, server_bind,
>                                         client_nic, client_bind, msg_size)
>    else:
>        return super()._create_perf_flow(perf_test, client_nic, client_bind,
>                                         server_nic, server_bind, msg_size)
>
> diff --git a/lnst/Recipes/ENRT/TeamRecipe.py b/lnst/Recipes/ENRT/TeamRecipe.py
> index e97a10b..b8233c8 100644
> --- a/lnst/Recipes/ENRT/TeamRecipe.py
> +++ b/lnst/Recipes/ENRT/TeamRecipe.py
> @@ -6,11 +6,13 @@ from lnst.Recipes.ENRT.ConfigMixins.OffloadSubConfigMixin import (
>     OffloadSubConfigMixin)
> from lnst.Recipes.ENRT.ConfigMixins.CommonHWSubConfigMixin import (
>     CommonHWSubConfigMixin)
> +from lnst.Recipes.ENRT.ConfigMixins.PerfReversibleFlowMixin import (
> PerfReversibleFlowMixin)
>
> from lnst.RecipeCommon.Ping.PingEndpoints import PingEndpoints
> from lnst.Devices import TeamDevice
>
> -class TeamRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
> +class TeamRecipe(PerfReversibleFlowMixin, CommonHWSubConfigMixin, OffloadSubConfigMixin,
>     BaseEnrtRecipe):
>     host1 = HostReq()
>     host1.eth0 = DeviceReq(label="tnet", driver=RecipeParam("driver"))
> @@ -25,7 +27,6 @@ class TeamRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
>         dict(gro="on", gso="off", tso="off", tx="on"),
>         dict(gro="on", gso="on", tso="off", tx="off")))
>
> perf_reverse = BoolParam(default=True)
> runner_name = StrParam(mandatory=True)
>
> def test_wide_configuration(self):
>
> @@ -88,8 +89,7 @@ class TeamRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
>         ]
> def generate_perf_endpoints(self, config):
>    return [(self.matched.host1.team0, self.matched.host2.eth0),
>        (self.matched.host2.eth0, self.matched.host1.team0)]
>    return [(self.matched.host1.team0, self.matched.host2.eth0)]

Just one more note. The change above is needed for few other tests, too.
Along with adding the PerfReversibleMixin.

In particular:
    DoubleTeamRecipe.py
    TeamVsBondRecipe.py

-Jan
> @property
> def offload_nics(self):
>
> -- 
> 2.26.2
> _______________________________________________
> LNST-developers mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]
> 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]...