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.

v3: * update docs/ after renaming the class

Signed-off-by: Jan Tluka jtluka@redhat.com --- .../config_mixin_classes/disable_pause_frames_mixin.rst | 6 ------ docs/source/config_mixin_classes/pause_frames_mixin.rst | 6 ++++++ docs/source/config_mixins.rst | 2 +- lnst/Recipes/ENRT/ConfigMixins/CommonHWSubConfigMixin.py | 6 +++--- ...seFramesHWConfigMixin.py => PauseFramesHWConfigMixin.py} | 2 +- 5 files changed, 11 insertions(+), 11 deletions(-) delete mode 100644 docs/source/config_mixin_classes/disable_pause_frames_mixin.rst create mode 100644 docs/source/config_mixin_classes/pause_frames_mixin.rst rename lnst/Recipes/ENRT/ConfigMixins/{DisablePauseFramesHWConfigMixin.py => PauseFramesHWConfigMixin.py} (95%)

diff --git a/docs/source/config_mixin_classes/disable_pause_frames_mixin.rst b/docs/source/config_mixin_classes/disable_pause_frames_mixin.rst deleted file mode 100644 index edd649fc..00000000 --- a/docs/source/config_mixin_classes/disable_pause_frames_mixin.rst +++ /dev/null @@ -1,6 +0,0 @@ -DisablePauseFramesHWConfigMixin -=============================== - -.. autoclass:: lnst.Recipes.ENRT.ConfigMixins.DisablePauseFramesHWConfigMixin.DisablePauseFramesHWConfigMixin - :members: - :show-inheritance: diff --git a/docs/source/config_mixin_classes/pause_frames_mixin.rst b/docs/source/config_mixin_classes/pause_frames_mixin.rst new file mode 100644 index 00000000..95b95d10 --- /dev/null +++ b/docs/source/config_mixin_classes/pause_frames_mixin.rst @@ -0,0 +1,6 @@ +PauseFramesHWConfigMixin +=============================== + +.. autoclass:: lnst.Recipes.ENRT.ConfigMixins.PauseFramesHWConfigMixin.PauseFramesHWConfigMixin + :members: + :show-inheritance: diff --git a/docs/source/config_mixins.rst b/docs/source/config_mixins.rst index 7264eb9e..8558fc95 100644 --- a/docs/source/config_mixins.rst +++ b/docs/source/config_mixins.rst @@ -11,4 +11,4 @@ ENRT Config Mixins config_mixin_classes/coalescing_mixin config_mixin_classes/dev_interrupt_mixin config_mixin_classes/parallel_stream_qdisc_mixin - config_mixin_classes/disable_pause_frames_mixin + config_mixin_classes/pause_frames_mixin 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

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 jtluka@redhat.com --- 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):

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, )

v3: * keep and adjust the hw_deconfig() method

Signed-off-by: Jan Tluka jtluka@redhat.com --- lnst/Recipes/ENRT/BondRecipe.py | 2 +- .../ConfigMixins/PauseFramesHWConfigMixin.py | 43 ++++++++++++------- lnst/Recipes/ENRT/SimpleNetworkRecipe.py | 2 +- 3 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/lnst/Recipes/ENRT/BondRecipe.py b/lnst/Recipes/ENRT/BondRecipe.py index 2d8c71ac..620a0f4b 100644 --- a/lnst/Recipes/ENRT/BondRecipe.py +++ b/lnst/Recipes/ENRT/BondRecipe.py @@ -241,7 +241,7 @@ class BondRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin, self.matched.host2.eth0]

@property - def no_pause_frames_dev_list(self): + def pause_frames_dev_list(self): """ The `parallel_stream_qdisc_hw_config_dev_list` property value for this scenario is a list containing the matched physical devices used to create diff --git a/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py b/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py index 74a1d348..389a5426 100644 --- a/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py +++ b/lnst/Recipes/ENRT/ConfigMixins/PauseFramesHWConfigMixin.py @@ -1,41 +1,54 @@ 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)) + 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 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)) + for param in ["rx_pause_frames", "tx_pause_frames"]: + self._deconfigure_dev_attribute( + config, + self.pause_frames_dev_list, + param + )

super().hw_deconfig(config)

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

All HWSubConfigMixins must have an implementation of hw_deconfig() method with at least super() call to let other mixins do their part in collaborative inheritance. Some of the HWConfigMixins did not have the method implemented.

In addition to the super().hw_deconfig() call: * the CoalescingHWConfigMixin includes the restoring the original adaptive coalescing setting * the MTUHWConfigMixin includes restoring the original MTU value * the ParallelStreamQDiscHWConfigMixin deletes the root qdisc to restore the default qdisc of the previously configured devices

Signed-off-by: Jan Tluka jtluka@redhat.com --- .../ENRT/ConfigMixins/CoalescingHWConfigMixin.py | 10 ++++++++++ lnst/Recipes/ENRT/ConfigMixins/MTUHWConfigMixin.py | 7 +++++++ .../ConfigMixins/ParallelStreamQDiscHWConfigMixin.py | 10 ++++++++++ 3 files changed, 27 insertions(+)

diff --git a/lnst/Recipes/ENRT/ConfigMixins/CoalescingHWConfigMixin.py b/lnst/Recipes/ENRT/ConfigMixins/CoalescingHWConfigMixin.py index f3470234..0c259b13 100644 --- a/lnst/Recipes/ENRT/ConfigMixins/CoalescingHWConfigMixin.py +++ b/lnst/Recipes/ENRT/ConfigMixins/CoalescingHWConfigMixin.py @@ -42,6 +42,16 @@ class CoalescingHWConfigMixin(BaseHWConfigMixin): param_value )

+ def hw_deconfig(self, config): + for feature in ['adaptive_rx_coalescing', 'adaptive_tx_coalescing']: + self._deconfigure_dev_attribute( + config, + self.coalescing_hw_config_dev_list, + feature, + ) + + super().hw_deconfig(config) + def describe_hw_config(self, config): desc = super().describe_hw_config(config) for param in ["adaptive_rx_coalescing", "adaptive_tx_coalescing"]: diff --git a/lnst/Recipes/ENRT/ConfigMixins/MTUHWConfigMixin.py b/lnst/Recipes/ENRT/ConfigMixins/MTUHWConfigMixin.py index e24ec320..fff0c2e0 100644 --- a/lnst/Recipes/ENRT/ConfigMixins/MTUHWConfigMixin.py +++ b/lnst/Recipes/ENRT/ConfigMixins/MTUHWConfigMixin.py @@ -32,6 +32,13 @@ class MTUHWConfigMixin(BaseHWConfigMixin): config, self.mtu_hw_config_dev_list, "mtu", mtu_value )

+ def hw_deconfig(self, config): + self._deconfigure_dev_attribute( + config, self.mtu_hw_config_dev_list, "mtu" + ) + + super().hw_deconfig(config) + def describe_hw_config(self, config): desc = super().describe_hw_config(config) return desc + self._describe_dev_attribute(config, "mtu") diff --git a/lnst/Recipes/ENRT/ConfigMixins/ParallelStreamQDiscHWConfigMixin.py b/lnst/Recipes/ENRT/ConfigMixins/ParallelStreamQDiscHWConfigMixin.py index 63b697be..7ec0d6b1 100644 --- a/lnst/Recipes/ENRT/ConfigMixins/ParallelStreamQDiscHWConfigMixin.py +++ b/lnst/Recipes/ENRT/ConfigMixins/ParallelStreamQDiscHWConfigMixin.py @@ -32,6 +32,16 @@ class ParallelStreamQDiscHWConfigMixin(BaseHWConfigMixin): dev.host.run("tc qdisc replace dev %s root mq" % dev.name) hw_config["parallel_stream_devs"].append(dev)

+ def hw_deconfig(self, config): + hw_config = config.hw_config + parallel_devs = hw_config.get("parallel_stream_devs", None) + if parallel_devs is not None: + for dev in parallel_devs: + dev.host.run("tc qdisc del dev %s root" % dev.name) + del hw_config["parallel_stream_devs"] + + super().hw_deconfig(config) + def describe_hw_config(self, config): desc = super().describe_hw_config(config)

Signed-off-by: Jan Tluka jtluka@redhat.com --- lnst/Recipes/ENRT/BondRecipe.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lnst/Recipes/ENRT/BondRecipe.py b/lnst/Recipes/ENRT/BondRecipe.py index 612da60e..abad65a9 100644 --- a/lnst/Recipes/ENRT/BondRecipe.py +++ b/lnst/Recipes/ENRT/BondRecipe.py @@ -251,7 +251,8 @@ class BondRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin, | host2.eth0

For detailed explanation of this property see - :any:`class DisablePauseFramesHWConfigMixin`. + :any:`PauseFramesHWConfigMixin` and + :any:`PauseFramesHWConfigMixin.pause_frames_dev_list`. """ return [self.matched.host1.eth0, self.matched.host1.eth1, self.matched.host2.eth0]