[PATCH 1/6] 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 <jtluka(a)redhat.com&gt;
---
 .../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
-- 
2.21.1

[PATCH 2/6] 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 <jtluka(a)redhat.com&gt;
---
 .../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)
-- 
2.21.1

[PATCH 3/6] 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 <jtluka(a)redhat.com&gt;
---
 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
-- 
2.21.1

[PATCH 4/6] 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.

Signed-off-by: Jan Tluka <jtluka(a)redhat.com&gt;
---
 lnst/Devices/Device.py | 106 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py
index 112a9276..608c8412 100644
--- a/lnst/Devices/Device.py
+++ b/lnst/Devices/Device.py
@@ -16,6 +16,7 @@ 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
@@ -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,89 @@ 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:
+            raise DeviceFeatureNotSupported(
+                "Not allowed to 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):
-- 
2.21.1

[PATCH 5/6] 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 <jtluka(a)redhat.com&gt;
---
 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
-- 
2.21.1

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

[Jan Tluka]

Signed-off-by: Jan Tluka <jtluka(a)redhat.com&gt;
---
 .../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
-- 
2.21.1

[Jan Tluka]

Mon, May 04, 2020 at 04:21:59PM CEST, jtluka(a)redhat.com 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 <jtluka(a)redhat.com&gt;
> --- 
Please ignore this patchset, a v2 is available.

-Jan