[PATCH v2 1/7] Recipes.ENRT.ConfigMixins: remove logic from configure_dev_attribute

[Jan Tluka]

The configure_dev_attribute should do whatever caller wants and the
logic should be move from this method to caller level.

Signed-off-by: Jan Tluka 
---
 .../ENRT/ConfigMixins/BaseHWConfigMixin.py    | 42 ++++++++++---------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/lnst/Recipes/ENRT/ConfigMixins/BaseHWConfigMixin.py b/lnst/Recipes/ENRT/ConfigMixins/BaseHWConfigMixin.py
index 633a0fe6..29854ee6 100644
--- a/lnst/Recipes/ENRT/ConfigMixins/BaseHWConfigMixin.py
+++ b/lnst/Recipes/ENRT/ConfigMixins/BaseHWConfigMixin.py
@@ -10,29 +10,31 @@ class BaseHWConfigMixin(object):

def _configure_dev_attribute(self, config, dev_list, attr_name, value):
         hw_config = config.hw_config
-        if value:
-            attr_cfg = hw_config[attr_name + "_configuration"] = {}
-            for dev in dev_list:
-                attr_cfg[dev] = {}
-                attr_cfg[dev]["original"] = getattr(dev, attr_name)
-                setattr(dev, attr_name, value)
-                attr_cfg[dev]["configured"] = getattr(dev, attr_name)
+        attr_cfg = hw_config[attr_name + "_configuration"] = {}
+        for dev in dev_list:
+            attr_cfg[dev] = {}
+            attr_cfg[dev]["original"] = getattr(dev, attr_name)
+            setattr(dev, attr_name, value)
+            attr_cfg[dev]["configured"] = getattr(dev, attr_name)

def _describe_dev_attribute(self, config, attr_name):
         hw_config = config.hw_config
         res = []
-        attr = hw_config.get(attr_name + "_configuration", None)
-        if attr:
-            for dev, info in attr.items():
-                res.append(
-                    "{}.{}.{} configured to {}, original value {}".format(
-                        dev.host.hostid,
-                        dev.name,
-                        attr_name,
-                        info["configured"],
-                        info["original"],
-                    )
-                )
-        else:
+        try:
+            attr = hw_config[attr_name + "_configuration"]
+        except:
             res.append("{} configuration skipped.".format(attr_name))
+            return res
+
+        for dev, info in attr.items():
+            res.append(
+                "{}.{}.{} configured to {}, original value {}".format(
+                    dev.host.hostid,
+                    dev.name,
+                    attr_name,
+                    info["configured"],
+                    info["original"],
+                )
+            )
+
         return res

[PATCH v2 2/7] Recipes.ENRT.ConfigMixins: check the value before calling configure_dev_attribute

[Jan Tluka]

This patch adds checks in CoalescingHWConfigMixin and MTUHWConfigMixin of
the individual mixin parameters and skips their configuration if they are None.

Signed-off-by: Jan Tluka 
---
 .../ConfigMixins/CoalescingHWConfigMixin.py   | 21 ++++++++-----------
 .../ENRT/ConfigMixins/MTUHWConfigMixin.py     |  8 ++++---
 2 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/lnst/Recipes/ENRT/ConfigMixins/CoalescingHWConfigMixin.py b/lnst/Recipes/ENRT/ConfigMixins/CoalescingHWConfigMixin.py
index 0e8807af..affad398 100644
--- a/lnst/Recipes/ENRT/ConfigMixins/CoalescingHWConfigMixin.py
+++ b/lnst/Recipes/ENRT/ConfigMixins/CoalescingHWConfigMixin.py
@@ -32,18 +32,15 @@ class CoalescingHWConfigMixin(BaseHWConfigMixin):
     def hw_config(self, config):
         super().hw_config(config)

-        self._configure_dev_attribute(
-            config,
-            self.coalescing_hw_config_dev_list,
-            "adaptive_rx_coalescing",
-            getattr(self.params, "adaptive_rx_coalescing", None),
-        )
-        self._configure_dev_attribute(
-            config,
-            self.coalescing_hw_config_dev_list,
-            "adaptive_tx_coalescing",
-            getattr(self.params, "adaptive_tx_coalescing", None),
-        )
+        for param in ["adaptive_rx_coalescing", "adaptive_tx_coalescing"]:
+            param_value = getattr(self.params, param, None)
+            if param_value is not None:
+                self._configure_dev_attribute(
+                    config,
+                    self.coalescing_hw_config_dev_list,
+                    param,
+                    param_value
+                )

def describe_hw_config(self, config):
         desc = super().describe_hw_config(config)
diff --git a/lnst/Recipes/ENRT/ConfigMixins/MTUHWConfigMixin.py b/lnst/Recipes/ENRT/ConfigMixins/MTUHWConfigMixin.py
index cbd7d62e..e24ec320 100644
--- a/lnst/Recipes/ENRT/ConfigMixins/MTUHWConfigMixin.py
+++ b/lnst/Recipes/ENRT/ConfigMixins/MTUHWConfigMixin.py
@@ -26,9 +26,11 @@ class MTUHWConfigMixin(BaseHWConfigMixin):
     def hw_config(self, config):
         super().hw_config(config)

-        self._configure_dev_attribute(
-            config, self.mtu_hw_config_dev_list, "mtu", getattr(self.params, "mtu", None)
-        )
+        mtu_value = getattr(self.params, "mtu", None)
+        if mtu_value is not None:
+            self._configure_dev_attribute(
+                config, self.mtu_hw_config_dev_list, "mtu", mtu_value
+            )

def describe_hw_config(self, config):
         desc = super().describe_hw_config(config)

[PATCH v2 3/7] Recipes.ENRT.ConfigMixins: rename DisablePauseFramesHWConfigMixin

[Jan Tluka]

Another patch dependent on this one will follow to make pause frames
configurable through a test parameter explicitly. Therefore the name
of the class should not contain a specific state of the pause setting.

Signed-off-by: Jan Tluka 
---
 lnst/Recipes/ENRT/ConfigMixins/CommonHWSubConfigMixin.py    | 6 +++---
 ...seFramesHWConfigMixin.py => PauseFramesHWConfigMixin.py} | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)
 rename lnst/Recipes/ENRT/ConfigMixins/{DisablePauseFramesHWConfigMixin.py => PauseFramesHWConfigMixin.py} (95%)

diff --git a/lnst/Recipes/ENRT/ConfigMixins/CommonHWSubConfigMixin.py b/lnst/Recipes/ENRT/ConfigMixins/CommonHWSubConfigMixin.py
index f1f9599d..d119381f 100644
--- a/lnst/Recipes/ENRT/ConfigMixins/CommonHWSubConfigMixin.py
+++ b/lnst/Recipes/ENRT/ConfigMixins/CommonHWSubConfigMixin.py
@@ -8,14 +8,14 @@ from lnst.Recipes.ENRT.ConfigMixins.CoalescingHWConfigMixin import (
     CoalescingHWConfigMixin,
 )
 from lnst.Recipes.ENRT.ConfigMixins.MTUHWConfigMixin import MTUHWConfigMixin
-from lnst.Recipes.ENRT.ConfigMixins.DisablePauseFramesHWConfigMixin import (
-    DisablePauseFramesHWConfigMixin,
+from lnst.Recipes.ENRT.ConfigMixins.PauseFramesHWConfigMixin import (
+    PauseFramesHWConfigMixin,
 )
 from lnst.Recipes.ENRT.ConfigMixins.BaseSubConfigMixin import BaseSubConfigMixin

class CommonHWSubConfigMixin(
-    DisablePauseFramesHWConfigMixin,
+    PauseFramesHWConfigMixin,
     ParallelStreamQDiscHWConfigMixin,
     DevInterruptHWConfigMixin,
     CoalescingHWConfigMixin,
diff --git a/lnst/Recipes/ENRT/ConfigMixins/DisablePauseFramesHWConfigMixin.py b/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py
similarity index 95%
rename from lnst/Recipes/ENRT/ConfigMixins/DisablePauseFramesHWConfigMixin.py
rename to lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py
index a08e324e..74a1d348 100644
--- a/lnst/Recipes/ENRT/ConfigMixins/DisablePauseFramesHWConfigMixin.py
+++ b/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py
@@ -2,7 +2,7 @@ from time import sleep
 from lnst.Recipes.ENRT.ConfigMixins.BaseHWConfigMixin import BaseHWConfigMixin

-class DisablePauseFramesHWConfigMixin(BaseHWConfigMixin):
+class PauseFramesHWConfigMixin(BaseHWConfigMixin):
     """
     This class is an extension to the :any:`BaseEnrtRecipe` class to turn off
     the Ethernet pause frames on the devices defined by

[PATCH v2 4/7] Common.ExecCmd: add retval getter to ExecCmdFail

[Jan Tluka]

Signed-off-by: Jan Tluka 
---
 lnst/Common/ExecCmd.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lnst/Common/ExecCmd.py b/lnst/Common/ExecCmd.py
index c6c13b3f..7b40ef4a 100644
--- a/lnst/Common/ExecCmd.py
+++ b/lnst/Common/ExecCmd.py
@@ -37,6 +37,9 @@ class ExecCmdFail(LnstError):
     def get_stdout(self):
         return self._stdout

+    def get_retval(self):
+        return self._retval
+
     def __str__(self):
         retval = ""
         stderr = ""

[PATCH v2 5/7] Devices.Device: add pause frames properties and methods

[Jan Tluka]

The Device class now provides following properties:
rx_pause_frames
tx_pause_frames

The property value is True if the pause frame setting is on or False
if it is turned off. The same applies when setting the property value.

The configuration of the pause frames is automatically restored to the
state before running a recipe.

Version 2:

Ethtool returns non-zero error code when the pause frames have the same
values as the values to be configured. To handle this I added a return
code check for this specific case.

Signed-off-by: Jan Tluka 
---
 lnst/Devices/Device.py | 109 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 108 insertions(+), 1 deletion(-)

diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py
index 112a9276..30b82f06 100644
--- a/lnst/Devices/Device.py
+++ b/lnst/Devices/Device.py
@@ -16,11 +16,12 @@ import ethtool
 import pyroute2
 import logging
 import pprint
+import time
 from abc import ABCMeta
 from pyroute2.netlink.rtnl import ifinfmsg
 from lnst.Common.Logs import log_exc_traceback
 from lnst.Common.NetUtils import normalize_hwaddr
-from lnst.Common.ExecCmd import exec_cmd
+from lnst.Common.ExecCmd import exec_cmd, ExecCmdFail
 from lnst.Common.DeviceError import DeviceError, DeviceDeleted, DeviceDisabled
 from lnst.Common.DeviceError import DeviceConfigError, DeviceConfigValueError
 from lnst.Common.DeviceError import DeviceFeatureNotSupported
@@ -242,6 +243,13 @@ class Device(object, metaclass=DeviceMeta):
         if_data["adaptive_rx_coalescing"] = ad_rx_coal
         if_data["adaptive_tx_coalescing"] = ad_tx_coal

+        try:
+            rx_pause, tx_pause = self._read_pause_frames()
+        except DeviceError:
+            rx_pause, tx_pause = None, None
+        if_data["rx_pause"] = rx_pause
+        if_data["tx_pause"] = tx_pause
+
         return if_data

def _vars(self):
@@ -289,6 +297,16 @@ class Device(object, metaclass=DeviceMeta):
         self._cleanup_data["adaptive_rx_coalescing"] = ad_rx_coal
         self._cleanup_data["adaptive_tx_coalescing"] = ad_tx_coal

+        try:
+            rx_pause, tx_pause = self._read_pause_frames()
+        except DeviceError:
+            rx_pause, tx_pause = None, None
+        self._cleanup_data.update(
+                {
+                    "rx_pause": rx_pause,
+                    "tx_pause": tx_pause
+                })
+
     def restore_original_data(self):
         """Restores initial configuration from stored values"""
         if not self._cleanup_data:
@@ -309,6 +327,11 @@ class Device(object, metaclass=DeviceMeta):
         except DeviceError:
             pass

+        # the device needs to be up to configure the pause frame settings
+        self.up()
+        self.restore_pause_frames()
+        self.down()
+
         self._cleanup_data = None

def _create(self):
@@ -654,6 +677,90 @@ class Device(object, metaclass=DeviceMeta):
         if (rx_val, tx_val) != (None, None):
             self._write_adaptive_coalescing(rx_val, tx_val)

+    @property
+    def rx_pause_frames(self):
+        return self._read_pause_frames()[0]
+
+    @rx_pause_frames.setter
+    def rx_pause_frames(self, value):
+        self._write_pause_frames(value, None)
+
+    @property
+    def tx_pause_frames(self):
+        return self._read_pause_frames()[1]
+
+    @tx_pause_frames.setter
+    def tx_pause_frames(self, value):
+        self._write_pause_frames(None, value)
+
+    def _read_pause_frames(self):
+        try:
+            res, _ = exec_cmd("ethtool -a %s" % self.name)
+        except:
+            raise DeviceFeatureNotSupported(
+                "No values for pause frames of %s." % self.name
+                )
+
+        # TODO: add autonegotiate
+        pause_settings = []
+        regex = "(RX|TX):.*(on|off)"
+
+        for line in res.split('\n'):
+            m = re.search(regex, line)
+            if m:
+                setting = True if m.group(2) == 'on' else False
+                pause_settings.append(setting)
+
+        if len(pause_settings) != 2:
+            raise Exception("Could not fetch pause frame settings. %s" % res)
+
+        return pause_settings
+
+    def _write_pause_frames(self, rx_val, tx_val):
+        ethtool_cmd = "ethtool -A {}".format(self.name)
+        ethtool_opts = ""
+
+        for feature, value in [('rx', rx_val), ('tx', tx_val)]:
+            if value is None:
+                continue
+
+            ethtool_opts += " {} {}".format(feature, 'on' if value else 'off')
+
+        if len(ethtool_opts) == 0:
+            return
+
+        try:
+            exec_cmd(ethtool_cmd + ethtool_opts)
+        except ExecCmdFail as e:
+            if e.get_retval() == 79:
+                raise DeviceConfigError(
+                    "Could not modify pause settings for %s." % self.name
+                )
+
+        timeout=5
+        while timeout > 0:
+            if self._pause_frames_match(rx_val, tx_val):
+                break
+            time.sleep(1)
+            timeout -= 1
+
+        if timeout == 0:
+            raise DeviceError("Pause frames not set!")
+
+    def _pause_frames_match(self, rx_expected, tx_expected):
+        rx_value, tx_value = self._read_pause_frames()
+        if ((rx_expected is not None and rx_expected != rx_value) or
+                (tx_expected is not None and tx_expected != tx_value)):
+            return False
+
+        return True
+
+    def restore_pause_frames(self):
+        rx_val = self._cleanup_data["rx_pause"]
+        tx_val = self._cleanup_data["tx_pause"]
+        if (rx_val, tx_val) != (None, None):
+            self._write_pause_frames(rx_val, tx_val)
+
     #TODO implement proper Route objects
     #consider the same as with tc?
     # def route_add(self, dest):

[PATCH v2 6/7] Recipes.ENRT.ConfigMixins.PauseFramesHWConfigMixin: configure pause frames through parameters

[Jan Tluka]

The mixin now allows to configure any pause frames configuration
instead of a single option to turn them off.

For example, to turn the pause frames off:

from lnst.Recipes.ENRT import SimpleNetworkRecipe
recipe = SimpleNetworkRecipe(
    rx_pause_frames=False,
    tx_pause_frames=False,
    )

Signed-off-by: Jan Tluka 
---
 lnst/Recipes/ENRT/BondRecipe.py               |  2 +-
 .../ConfigMixins/PauseFramesHWConfigMixin.py  | 41 ++++++++++---------
 lnst/Recipes/ENRT/SimpleNetworkRecipe.py      |  2 +-
 3 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/lnst/Recipes/ENRT/BondRecipe.py b/lnst/Recipes/ENRT/BondRecipe.py
index 16b80e90..d5c48a8b 100644
--- a/lnst/Recipes/ENRT/BondRecipe.py
+++ b/lnst/Recipes/ENRT/BondRecipe.py
@@ -113,6 +113,6 @@ class BondRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
             self.matched.host2.eth0]

@property
-    def no_pause_frames_dev_list(self):
+    def pause_frames_dev_list(self):
         return [self.matched.host1.eth0, self.matched.host1.eth1,
                 self.matched.host2.eth0]
diff --git a/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py b/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py
index 74a1d348..d72e2463 100644
--- a/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py
+++ b/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py
@@ -1,41 +1,44 @@
 from time import sleep
+from lnst.Common.Parameters import BoolParam
 from lnst.Recipes.ENRT.ConfigMixins.BaseHWConfigMixin import BaseHWConfigMixin

class PauseFramesHWConfigMixin(BaseHWConfigMixin):
     """
-    This class is an extension to the :any:`BaseEnrtRecipe` class to turn off
+    This class is an extension to the :any:`BaseEnrtRecipe` class to configure
     the Ethernet pause frames on the devices defined by
-    the :attr:`no_pause_frames_dev_list` property.
+    the :attr:`pause_frames_dev_list` property.
     """

+    rx_pause_frames = BoolParam(mandatory=False)
+    tx_pause_frames = BoolParam(mandatory=False)
+
     @property
-    def no_pause_frames_dev_list(self):
+    def pause_frames_dev_list(self):
         """
         The value of this property is a list of devices for which the pause
-        frames should be disabled. It has to be defined by a derived class.
+        frames should be configured. It has to be defined by a derived class.
         """
         return []

def hw_config(self, config):
         super().hw_config(config)

-        for dev in self.no_pause_frames_dev_list:
-            dev.host.run("ethtool -A {} rx off tx off".format(dev.name))
-            sleep(1)
-            dev.host.run("ethtool -a {}".format(dev.name))
-
-    def hw_deconfig(self, config):
-        for dev in self.no_pause_frames_dev_list:
-            dev.host.run("ethtool -A {} rx on tx on".format(dev.name))
-
-        super().hw_deconfig(config)
+        for param in ["rx_pause_frames", "tx_pause_frames"]:
+            param_value = getattr(self.params, param, None)
+            if param_value is not None:
+                self._configure_dev_attribute(
+                    config,
+                    self.pause_frames_dev_list,
+                    param,
+                    param_value
+                )

def describe_hw_config(self, config):
         desc = super().describe_hw_config(config)
-        desc += [
-            "Pause frames disabled for: {}".format(
-                self.no_pause_frames_dev_list
-            )
-        ]
+        for param in ["rx_pause_frames", "tx_pause_frames"]:
+            desc.extend(
+                    self._describe_dev_attribute(config, param)
+                    )
+
         return desc
diff --git a/lnst/Recipes/ENRT/SimpleNetworkRecipe.py b/lnst/Recipes/ENRT/SimpleNetworkRecipe.py
index 508afdc0..506cbdaa 100644
--- a/lnst/Recipes/ENRT/SimpleNetworkRecipe.py
+++ b/lnst/Recipes/ENRT/SimpleNetworkRecipe.py
@@ -112,7 +112,7 @@ class SimpleNetworkRecipe(
         return [(self.matched.host1.eth0, self.matched.host2.eth0)]

@property
-    def no_pause_frames_dev_list(self):
+    def pause_frames_dev_list(self):
         return [self.matched.host1.eth0, self.matched.host2.eth0]

@property

[PATCH v2 6/7] Recipes.ENRT.ConfigMixins.PauseFramesHWConfigMixin: configure pause frames through parameters

[Ondrej Lichtner]

On Thu, May 07, 2020 at 11:05:07AM +0200, Jan Tluka wrote:
> The mixin now allows to configure any pause frames configuration
> instead of a single option to turn them off.
>
> For example, to turn the pause frames off:
>
> from lnst.Recipes.ENRT import SimpleNetworkRecipe
> recipe = SimpleNetworkRecipe(
>     rx_pause_frames=False,
>     tx_pause_frames=False,
>     )
> Signed-off-by: Jan Tluka
>
> lnst/Recipes/ENRT/BondRecipe.py               |  2 +-
>  .../ConfigMixins/PauseFramesHWConfigMixin.py  | 41 ++++++++++---------
>  lnst/Recipes/ENRT/SimpleNetworkRecipe.py      |  2 +-
>  3 files changed, 24 insertions(+), 21 deletions(-)
>
> diff --git a/lnst/Recipes/ENRT/BondRecipe.py b/lnst/Recipes/ENRT/BondRecipe.py
> index 16b80e90..d5c48a8b 100644
> --- a/lnst/Recipes/ENRT/BondRecipe.py
> +++ b/lnst/Recipes/ENRT/BondRecipe.py
> @@ -113,6 +113,6 @@ class BondRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
>              self.matched.host2.eth0]
>  @property
> def no_pause_frames_dev_list(self):
> def pause_frames_dev_list(self):
>     return [self.matched.host1.eth0, self.matched.host1.eth1,
>             self.matched.host2.eth0]
>
> diff --git a/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py b/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py
> index 74a1d348..d72e2463 100644
> --- a/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py
> +++ b/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py
> @@ -1,41 +1,44 @@
>  from time import sleep
> +from lnst.Common.Parameters import BoolParam
>  from lnst.Recipes.ENRT.ConfigMixins.BaseHWConfigMixin import BaseHWConfigMixin
>
> class PauseFramesHWConfigMixin(BaseHWConfigMixin):
>      """
> This class is an extension to the :any:`BaseEnrtRecipe` class to turn off
> This class is an extension to the :any:`BaseEnrtRecipe` class to configure
> the Ethernet pause frames on the devices defined by
> the :attr:`no_pause_frames_dev_list` property.
>
> the :attr:`pause_frames_dev_list` property.
> """
>
> rx_pause_frames = BoolParam(mandatory=False)
>
> tx_pause_frames = BoolParam(mandatory=False)
>
> @property
> def no_pause_frames_dev_list(self):
> def pause_frames_dev_list(self):
>     """
>     The value of this property is a list of devices for which the pause
>    frames should be disabled. It has to be defined by a derived class.
>    frames should be configured. It has to be defined by a derived class.
>    """
>    return []
>
> def hw_config(self, config):
>        super().hw_config(config)
>    for dev in self.no_pause_frames_dev_list:
>        dev.host.run("ethtool -A {} rx off tx off".format(dev.name))
>        sleep(1)
>        dev.host.run("ethtool -a {}".format(dev.name))
> def hw_deconfig(self, config):
>    for dev in self.no_pause_frames_dev_list:
>        dev.host.run("ethtool -A {} rx on tx on".format(dev.name))
>    super().hw_deconfig(config)

I'm not sure about removing this method. I assume you removed it because
the previous patch implements pause frame configuration in the Device
class such that it will be deconfigured with the Device deconfiguration?

But, this is a SubConfig mixin class which means that theoretically you
could have multiple sub config loop iterations where each could define
it's own configuration for pause frames. Even though the value will
always be overwritten I think it may make more sense to have it in the
"comple" form of running both configuration and deconfiguration for this
setting.

What do you think?
>    for param in ["rx_pause_frames", "tx_pause_frames"]:
>        param_value = getattr(self.params, param, None)
>        if param_value is not None:
>            self._configure_dev_attribute(
>                config,
>                self.pause_frames_dev_list,
>                param,
>                param_value
>            )
>
> def describe_hw_config(self, config):
>        desc = super().describe_hw_config(config)
>    desc += [
>        "Pause frames disabled for: {}".format(
>            self.no_pause_frames_dev_list
>        )
>    ]
>    for param in ["rx_pause_frames", "tx_pause_frames"]:
>        desc.extend(
>                self._describe_dev_attribute(config, param)
>                )
>    return desc
>
> diff --git a/lnst/Recipes/ENRT/SimpleNetworkRecipe.py b/lnst/Recipes/ENRT/SimpleNetworkRecipe.py
> index 508afdc0..506cbdaa 100644
> --- a/lnst/Recipes/ENRT/SimpleNetworkRecipe.py
> +++ b/lnst/Recipes/ENRT/SimpleNetworkRecipe.py
> @@ -112,7 +112,7 @@ class SimpleNetworkRecipe(
>          return [(self.matched.host1.eth0, self.matched.host2.eth0)]
>  @property
> def no_pause_frames_dev_list(self):
>
> def pause_frames_dev_list(self):
>     return [self.matched.host1.eth0, self.matched.host2.eth0]
>
> @property
>
> -- 
> 2.21.1
> _______________________________________________
> 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]...

[PATCH v2 6/7] Recipes.ENRT.ConfigMixins.PauseFramesHWConfigMixin: configure pause frames through parameters

[Jan Tluka]

Wed, May 13, 2020 at 11:51:45AM CEST, [e-mail] wrote:
> On Thu, May 07, 2020 at 11:05:07AM +0200, Jan Tluka wrote:
> > The mixin now allows to configure any pause frames configuration
> > instead of a single option to turn them off.
> >
> > For example, to turn the pause frames off:
> >
> > from lnst.Recipes.ENRT import SimpleNetworkRecipe
> > recipe = SimpleNetworkRecipe(
> >     rx_pause_frames=False,
> >     tx_pause_frames=False,
> >     )
> > Signed-off-by: Jan Tluka
> >
> > lnst/Recipes/ENRT/BondRecipe.py               |  2 +-
> >  .../ConfigMixins/PauseFramesHWConfigMixin.py  | 41 ++++++++++---------
> >  lnst/Recipes/ENRT/SimpleNetworkRecipe.py      |  2 +-
> >  3 files changed, 24 insertions(+), 21 deletions(-)
> >
> > diff --git a/lnst/Recipes/ENRT/BondRecipe.py b/lnst/Recipes/ENRT/BondRecipe.py
> > index 16b80e90..d5c48a8b 100644
> > --- a/lnst/Recipes/ENRT/BondRecipe.py
> > +++ b/lnst/Recipes/ENRT/BondRecipe.py
> > @@ -113,6 +113,6 @@ class BondRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
> >              self.matched.host2.eth0]
> >  @property
> > def no_pause_frames_dev_list(self):
> > def pause_frames_dev_list(self):
> >     return [self.matched.host1.eth0, self.matched.host1.eth1,
> >             self.matched.host2.eth0]
> >
> > diff --git a/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py b/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py
> > index 74a1d348..d72e2463 100644
> > --- a/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py
> > +++ b/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py
> > @@ -1,41 +1,44 @@
> >  from time import sleep
> > +from lnst.Common.Parameters import BoolParam
> >  from lnst.Recipes.ENRT.ConfigMixins.BaseHWConfigMixin import BaseHWConfigMixin
> >
> > class PauseFramesHWConfigMixin(BaseHWConfigMixin):
> >      """
> > This class is an extension to the :any:`BaseEnrtRecipe` class to turn off
> > This class is an extension to the :any:`BaseEnrtRecipe` class to configure
> > the Ethernet pause frames on the devices defined by
> > the :attr:`no_pause_frames_dev_list` property.
> >
> > the :attr:`pause_frames_dev_list` property.
> > """
> >
> > rx_pause_frames = BoolParam(mandatory=False)
> >
> > tx_pause_frames = BoolParam(mandatory=False)
> >
> > @property
> > def no_pause_frames_dev_list(self):
> > def pause_frames_dev_list(self):
> >     """
> >     The value of this property is a list of devices for which the pause
> >    frames should be disabled. It has to be defined by a derived class.
> >    frames should be configured. It has to be defined by a derived class.
> >    """
> >    return []
> >
> > def hw_config(self, config):
> >        super().hw_config(config)
> >    for dev in self.no_pause_frames_dev_list:
> >        dev.host.run("ethtool -A {} rx off tx off".format(dev.name))
> >        sleep(1)
> >        dev.host.run("ethtool -a {}".format(dev.name))
> > def hw_deconfig(self, config):
> >    for dev in self.no_pause_frames_dev_list:
> >        dev.host.run("ethtool -A {} rx on tx on".format(dev.name))
> >    super().hw_deconfig(config)
>
> I'm not sure about removing this method. I assume you removed it because
> the previous patch implements pause frame configuration in the Device
> class such that it will be deconfigured with the Device deconfiguration?
>
> But, this is a SubConfig mixin class which means that theoretically you
> could have multiple sub config loop iterations where each could define
> it's own configuration for pause frames. Even though the value will
> always be overwritten I think it may make more sense to have it in the
> "comple" form of running both configuration and deconfiguration for this
> setting.
>
> What do you think?

I think it should not be removed. I can't remember why I'd want to
remove this. I'll think about this more, but likely I will send a v3
patchset.
> >    for param in ["rx_pause_frames", "tx_pause_frames"]:
> >        param_value = getattr(self.params, param, None)
> >        if param_value is not None:
> >            self._configure_dev_attribute(
> >                config,
> >                self.pause_frames_dev_list,
> >                param,
> >                param_value
> >            )
> >
> > def describe_hw_config(self, config):
> >        desc = super().describe_hw_config(config)
> >    desc += [
> >        "Pause frames disabled for: {}".format(
> >            self.no_pause_frames_dev_list
> >        )
> >    ]
> >    for param in ["rx_pause_frames", "tx_pause_frames"]:
> >        desc.extend(
> >                self._describe_dev_attribute(config, param)
> >                )
> >    return desc
> >
> > diff --git a/lnst/Recipes/ENRT/SimpleNetworkRecipe.py b/lnst/Recipes/ENRT/SimpleNetworkRecipe.py
> > index 508afdc0..506cbdaa 100644
> > --- a/lnst/Recipes/ENRT/SimpleNetworkRecipe.py
> > +++ b/lnst/Recipes/ENRT/SimpleNetworkRecipe.py
> > @@ -112,7 +112,7 @@ class SimpleNetworkRecipe(
> >          return [(self.matched.host1.eth0, self.matched.host2.eth0)]
> >  @property
> > def no_pause_frames_dev_list(self):
> >
> > def pause_frames_dev_list(self):
> >     return [self.matched.host1.eth0, self.matched.host2.eth0]
> >
> > @property
> >
> > -- 
> > 2.21.1
> > _______________________________________________
> > 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]...

[PATCH v2 6/7] Recipes.ENRT.ConfigMixins.PauseFramesHWConfigMixin: configure pause frames through parameters

[Jan Tluka]

Wed, May 13, 2020 at 01:42:23PM CEST, [e-mail] wrote:
> Wed, May 13, 2020 at 11:51:45AM CEST, [e-mail] wrote:
> > On Thu, May 07, 2020 at 11:05:07AM +0200, Jan Tluka wrote:
> > > The mixin now allows to configure any pause frames configuration
> > > instead of a single option to turn them off.
> > >
> > > For example, to turn the pause frames off:
> > >
> > > from lnst.Recipes.ENRT import SimpleNetworkRecipe
> > > recipe = SimpleNetworkRecipe(
> > >     rx_pause_frames=False,
> > >     tx_pause_frames=False,
> > >     )
> > > Signed-off-by: Jan Tluka
> > >
> > > lnst/Recipes/ENRT/BondRecipe.py               |  2 +-
> > >  .../ConfigMixins/PauseFramesHWConfigMixin.py  | 41 ++++++++++---------
> > >  lnst/Recipes/ENRT/SimpleNetworkRecipe.py      |  2 +-
> > >  3 files changed, 24 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/lnst/Recipes/ENRT/BondRecipe.py b/lnst/Recipes/ENRT/BondRecipe.py
> > > index 16b80e90..d5c48a8b 100644
> > > --- a/lnst/Recipes/ENRT/BondRecipe.py
> > > +++ b/lnst/Recipes/ENRT/BondRecipe.py
> > > @@ -113,6 +113,6 @@ class BondRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
> > >              self.matched.host2.eth0]
> > >  @property
> > > def no_pause_frames_dev_list(self):
> > > def pause_frames_dev_list(self):
> > >     return [self.matched.host1.eth0, self.matched.host1.eth1,
> > >             self.matched.host2.eth0]
> > >
> > > diff --git a/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py b/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py
> > > index 74a1d348..d72e2463 100644
> > > --- a/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py
> > > +++ b/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py
> > > @@ -1,41 +1,44 @@
> > >  from time import sleep
> > > +from lnst.Common.Parameters import BoolParam
> > >  from lnst.Recipes.ENRT.ConfigMixins.BaseHWConfigMixin import BaseHWConfigMixin
> > >
> > > class PauseFramesHWConfigMixin(BaseHWConfigMixin):
> > >      """
> > > This class is an extension to the :any:`BaseEnrtRecipe` class to turn off
> > > This class is an extension to the :any:`BaseEnrtRecipe` class to configure
> > > the Ethernet pause frames on the devices defined by
> > > the :attr:`no_pause_frames_dev_list` property.
> > >
> > > the :attr:`pause_frames_dev_list` property.
> > > """
> > >
> > > rx_pause_frames = BoolParam(mandatory=False)
> > >
> > > tx_pause_frames = BoolParam(mandatory=False)
> > >
> > > @property
> > > def no_pause_frames_dev_list(self):
> > > def pause_frames_dev_list(self):
> > >     """
> > >     The value of this property is a list of devices for which the pause
> > >    frames should be disabled. It has to be defined by a derived class.
> > >    frames should be configured. It has to be defined by a derived class.
> > >    """
> > >    return []
> > >
> > > def hw_config(self, config):
> > >        super().hw_config(config)
> > >    for dev in self.no_pause_frames_dev_list:
> > >        dev.host.run("ethtool -A {} rx off tx off".format(dev.name))
> > >        sleep(1)
> > >        dev.host.run("ethtool -a {}".format(dev.name))
> > > def hw_deconfig(self, config):
> > >    for dev in self.no_pause_frames_dev_list:
> > >        dev.host.run("ethtool -A {} rx on tx on".format(dev.name))
> > >    super().hw_deconfig(config)
> >
> > I'm not sure about removing this method. I assume you removed it because
> > the previous patch implements pause frame configuration in the Device
> > class such that it will be deconfigured with the Device deconfiguration?
> >
> > But, this is a SubConfig mixin class which means that theoretically you
> > could have multiple sub config loop iterations where each could define
> > it's own configuration for pause frames. Even though the value will
> > always be overwritten I think it may make more sense to have it in the
> > "comple" form of running both configuration and deconfiguration for this
> > setting.
> >
> > What do you think?
>
> I think it should not be removed. I can't remember why I'd want to
> remove this. I'll think about this more, but likely I will send a v3
> patchset.

So, I think I removed it based on the similar approach of
CoalescingHWConfigMixin class. This (and few others) do not have it
defined either.

Likely the other classes should be fixed, too.

-Jan
> > >    for param in ["rx_pause_frames", "tx_pause_frames"]:
> > >        param_value = getattr(self.params, param, None)
> > >        if param_value is not None:
> > >            self._configure_dev_attribute(
> > >                config,
> > >                self.pause_frames_dev_list,
> > >                param,
> > >                param_value
> > >            )
> > >
> > > def describe_hw_config(self, config):
> > >        desc = super().describe_hw_config(config)
> > >    desc += [
> > >        "Pause frames disabled for: {}".format(
> > >            self.no_pause_frames_dev_list
> > >        )
> > >    ]
> > >    for param in ["rx_pause_frames", "tx_pause_frames"]:
> > >        desc.extend(
> > >                self._describe_dev_attribute(config, param)
> > >                )
> > >    return desc
> > >
> > > diff --git a/lnst/Recipes/ENRT/SimpleNetworkRecipe.py b/lnst/Recipes/ENRT/SimpleNetworkRecipe.py
> > > index 508afdc0..506cbdaa 100644
> > > --- a/lnst/Recipes/ENRT/SimpleNetworkRecipe.py
> > > +++ b/lnst/Recipes/ENRT/SimpleNetworkRecipe.py
> > > @@ -112,7 +112,7 @@ class SimpleNetworkRecipe(
> > >          return [(self.matched.host1.eth0, self.matched.host2.eth0)]
> > >  @property
> > > def no_pause_frames_dev_list(self):
> > >
> > > def pause_frames_dev_list(self):
> > >     return [self.matched.host1.eth0, self.matched.host2.eth0]
> > >
> > > @property
> > >
> > > -- 
> > > 2.21.1
> > > _______________________________________________
> > > 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]...
>
> 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]...

[PATCH v2 6/7] Recipes.ENRT.ConfigMixins.PauseFramesHWConfigMixin: configure pause frames through parameters

[Jan Tluka]

Wed, May 13, 2020 at 01:59:25PM CEST, [e-mail] wrote:
> Wed, May 13, 2020 at 01:42:23PM CEST, [e-mail] wrote:
> > Wed, May 13, 2020 at 11:51:45AM CEST, [e-mail] wrote:
> > > On Thu, May 07, 2020 at 11:05:07AM +0200, Jan Tluka wrote:
> > > > def hw_deconfig(self, config):
> > > >    for dev in self.no_pause_frames_dev_list:
> > > >        dev.host.run("ethtool -A {} rx on tx on".format(dev.name))
> > > >    super().hw_deconfig(config)
> > >
> > > I'm not sure about removing this method. I assume you removed it because
> > > the previous patch implements pause frame configuration in the Device
> > > class such that it will be deconfigured with the Device deconfiguration?
> > >
> > > But, this is a SubConfig mixin class which means that theoretically you
> > > could have multiple sub config loop iterations where each could define
> > > it's own configuration for pause frames. Even though the value will
> > > always be overwritten I think it may make more sense to have it in the
> > > "comple" form of running both configuration and deconfiguration for this
> > > setting.
> > >
> > > What do you think?
> >
> > I think it should not be removed. I can't remember why I'd want to
> > remove this. I'll think about this more, but likely I will send a v3
> > patchset.
>
> So, I think I removed it based on the similar approach of
> CoalescingHWConfigMixin class. This (and few others) do not have it
> defined either.
>
> Likely the other classes should be fixed, too.
>
> -Jan

More thoughts.

Why does CoalescingHWConfigMixin save the coalescing status in the
Device class rather than doing this through hw_config and hw_deconfig?

With the current implementation the deconfig part behaves more like
a Testwide deconfiguration rather than Sub deconfiguration.

So, this should be fixed as well.

Saving a device feature status is ok, but each hw_deconfig should
call:
    dev.restore_feature()
    ...
    super().hw_deconfig()

-Jan

[PATCH v2 6/7] Recipes.ENRT.ConfigMixins.PauseFramesHWConfigMixin: configure pause frames through parameters

[Ondrej Lichtner]

On Wed, May 13, 2020 at 02:49:48PM +0200, Jan Tluka wrote:
> Wed, May 13, 2020 at 01:59:25PM CEST, [e-mail] wrote:
> > Wed, May 13, 2020 at 01:42:23PM CEST, [e-mail] wrote:
> > > Wed, May 13, 2020 at 11:51:45AM CEST, [e-mail] wrote:
> > > > On Thu, May 07, 2020 at 11:05:07AM +0200, Jan Tluka wrote:
> > > > > def hw_deconfig(self, config):
> > > > >    for dev in self.no_pause_frames_dev_list:
> > > > >        dev.host.run("ethtool -A {} rx on tx on".format(dev.name))
> > > > >    super().hw_deconfig(config)
> > > >
> > > > I'm not sure about removing this method. I assume you removed it because
> > > > the previous patch implements pause frame configuration in the Device
> > > > class such that it will be deconfigured with the Device deconfiguration?
> > > >
> > > > But, this is a SubConfig mixin class which means that theoretically you
> > > > could have multiple sub config loop iterations where each could define
> > > > it's own configuration for pause frames. Even though the value will
> > > > always be overwritten I think it may make more sense to have it in the
> > > > "comple" form of running both configuration and deconfiguration for this
> > > > setting.
> > > >
> > > > What do you think?
> > >
> > > I think it should not be removed. I can't remember why I'd want to
> > > remove this. I'll think about this more, but likely I will send a v3
> > > patchset.
> >
> > So, I think I removed it based on the similar approach of
> > CoalescingHWConfigMixin class. This (and few others) do not have it
> > defined either.
> >
> > Likely the other classes should be fixed, too.

Yeah, good point, I didn't realize that the others used this approach, I
think all of these subconfig classes should have both the config and the
deconfig methods implemented and they should be running with every
subconfig loop iteration...
> > -Jan
>
> More thoughts.
>
> Why does CoalescingHWConfigMixin save the coalescing status in the
> Device class rather than doing this through hw_config and hw_deconfig?

Not sure what you mean by that, CoalescingHWConfigMixin class uses the
_configure_dev_attribute() method which also stores the original value
in the config object for the Recipe.

It's also stored in the Device class so that the Slave can do cleanup
after a recipe is finished in case the recipe didn't do proper cleanup
(either due to incorrect implementation or due to a crash).
> With the current implementation the deconfig part behaves more like
> a Testwide deconfiguration rather than Sub deconfiguration.
>
> So, this should be fixed as well.
>
> Saving a device feature status is ok, but each hw_deconfig should
> call:
>     dev.restore_feature()
>     ...
>     super().hw_deconfig()

yes, I think that's good.

-Ondrej

[PATCH v2 7/7] Recipes.ENRT.ConfigMixins: simplify describe_hw_config code

[Jan Tluka]

Signed-off-by: Jan Tluka 
---
 .../ENRT/ConfigMixins/CoalescingHWConfigMixin.py       | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/lnst/Recipes/ENRT/ConfigMixins/CoalescingHWConfigMixin.py b/lnst/Recipes/ENRT/ConfigMixins/CoalescingHWConfigMixin.py
index affad398..f3470234 100644
--- a/lnst/Recipes/ENRT/ConfigMixins/CoalescingHWConfigMixin.py
+++ b/lnst/Recipes/ENRT/ConfigMixins/CoalescingHWConfigMixin.py
@@ -44,10 +44,8 @@ class CoalescingHWConfigMixin(BaseHWConfigMixin):

def describe_hw_config(self, config):
         desc = super().describe_hw_config(config)
-        desc.extend(
-            self._describe_dev_attribute(config, "adaptive_rx_coalescing")
-        )
-        desc.extend(
-            self._describe_dev_attribute(config, "adaptive_tx_coalescing")
-        )
+        for param in ["adaptive_rx_coalescing", "adaptive_tx_coalescing"]:
+            desc.extend(
+                self._describe_dev_attribute(config, param)
+            )
         return desc

[Ondrej Lichtner]

On Thu, May 07, 2020 at 11:05:02AM +0200, Jan Tluka wrote:
> The configure_dev_attribute should do whatever caller wants and the
> logic should be move from this method to caller level.
>
> Signed-off-by: Jan Tluka

The rest of the patchset is fine, I'll wait for v3 wrt. the hw_deconfig
changes.

-Ondrej