Wed, May 13, 2020 at 01:42:23PM CEST, jtluka(a)redhat.com wrote:
Wed, May 13, 2020 at 11:51:45AM CEST, olichtne(a)redhat.com 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 <jtluka(a)redhat.com>
>> ---
>> 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 -- lnst-developers(a)lists.fedorahosted.org
>> To unsubscribe send an email to lnst-developers-leave(a)lists.fedorahosted.org
>> Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
>> List Guidelines:
https://fedoraproject.org/wiki/Mailing_list_guidelines
>> List Archives:
https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedora...
_______________________________________________
LNST-developers mailing list -- lnst-developers(a)lists.fedorahosted.org
To unsubscribe send an email to lnst-developers-leave(a)lists.fedorahosted.org
Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines:
https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives:
https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedora...