From: Christos Sfakianakis csfakian@redhat.com
This patch set inlcudes modifications in the Device module and in some ENRT recipes that hanlde adaptive coalescence.
In the Device module, methods are added to read, modify and verify the coalescence settings of a target device.
In the ENRT recipes, the necessary calls are added into both configuration and deconfiguration phase in case coalescence needs to be disabled. This is in agreement with old phase1/2 recipes.
Due to the fact that the target devices are ethernet and we currenlty cannot filter them out of 2 arbitrary perf configuration endpoints, it was not possible to apply the calls in BaseEnrtRecipe.
Christos Sfakianakis (2): lnst.Devices.Device: handle coalesce lnst.Recipes.ENRT: use coalesce tuning
lnst/Devices/Device.py | 47 ++++++++++++++++++++++++ lnst/Recipes/ENRT/BondRecipe.py | 8 ++++ lnst/Recipes/ENRT/DoubleBondRecipe.py | 10 +++++ lnst/Recipes/ENRT/DoubleTeamRecipe.py | 10 +++++ lnst/Recipes/ENRT/SimplePerfRecipe.py | 8 ++++ lnst/Recipes/ENRT/TeamRecipe.py | 8 ++++ lnst/Recipes/ENRT/TeamVsBondRecipe.py | 10 +++++ lnst/Recipes/ENRT/VlansOverBondRecipe.py | 8 ++++ lnst/Recipes/ENRT/VlansOverTeamRecipe.py | 8 ++++ lnst/Recipes/ENRT/VlansRecipe.py | 8 ++++ 10 files changed, 125 insertions(+)
From: Christos Sfakianakis csfakian@redhat.com
Add methods for handling coalesce settings. Raise errors when these settings cannot be read or modified correctly.
Signed-off-by: Christos Sfakianakis csfakian@redhat.com --- lnst/Devices/Device.py | 47 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py index 20cddcf..6a04d1b 100644 --- a/lnst/Devices/Device.py +++ b/lnst/Devices/Device.py @@ -17,10 +17,12 @@ import pyroute2 import logging import pprint from abc import ABCMeta +from itertools import product 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.LnstError import LnstError from lnst.Common.DeviceError import DeviceError, DeviceDeleted, DeviceDisabled from lnst.Common.DeviceError import DeviceConfigError, DeviceConfigValueError from lnst.Common.IpAddress import ipaddress @@ -58,6 +60,7 @@ class Device(object): self._deleted = False
self._ip_addrs = [] + self._coalesce_settings = ['', '']
self._nl_update = {} self._bulk_enabled = False @@ -603,6 +606,50 @@ class Device(object): """disable automatic negotiation of speed for this device""" exec_cmd("ethtool -s %s autoneg off" % self.name)
+ def _read_coalesce(self): + try: + res = exec_cmd("ethtool -c %s" % self.name) + except: + raise DeviceError("Could not fetch coalesce settings of " + "%s." % self.name) + res = re.findall('\wX: (o[n|f]*)', res[0]) + if not tuple(res) in product(['off', 'on'], repeat=2): + raise LnstError("Invalid values for coalesce settings of " + "{} : {}".format(self.name, res)) + return res + + def _store_coalesce(self): + coal = self._read_coalesce() + self._coalesce_settings = coal + + def _write_coalesce(self, rx_val, tx_val): + if self._coalesce_settings == [rx_val, tx_val]: + return + try: + exec_cmd("ethtool -C %s adaptive-rx %s adaptive-tx %s" % + (self.name, rx_val, tx_val)) + except: + raise DeviceConfigError("Not allowed to modify coalesce " + "settings for %s." % self.name) + if self._read_coalesce != [rx_val, tx_val]: + raise LnstError("Could not apply coalesce settings for " + "%s effectively." % self.name) + + def _disable_coalesce(self): + self._store_coalesce() + self._write_coalesce('off', 'off') + + def _restore_coalesce(self): + rx_val = self._coalesce_settings[0] + tx_val = self._coalesce_settings[1] + self._write_coalesce(rx_val, tx_val) + + def disable_coalesce(self): + self._disable_coalesce() + + def restore_coalesce(self): + self._restore_coalesce() + #TODO implement proper Route objects #consider the same as with tc? # def route_add(self, dest):
On Tue, Feb 26, 2019 at 04:34:43PM +0100, csfakian@redhat.com wrote:
From: Christos Sfakianakis csfakian@redhat.com
Add methods for handling coalesce settings. Raise errors when these settings cannot be read or modified correctly.
Signed-off-by: Christos Sfakianakis csfakian@redhat.com
lnst/Devices/Device.py | 47 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py index 20cddcf..6a04d1b 100644 --- a/lnst/Devices/Device.py +++ b/lnst/Devices/Device.py @@ -17,10 +17,12 @@ import pyroute2 import logging import pprint from abc import ABCMeta +from itertools import product 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.LnstError import LnstError from lnst.Common.DeviceError import DeviceError, DeviceDeleted, DeviceDisabled from lnst.Common.DeviceError import DeviceConfigError, DeviceConfigValueError from lnst.Common.IpAddress import ipaddress @@ -58,6 +60,7 @@ class Device(object): self._deleted = False
self._ip_addrs = []
self._coalesce_settings = ['', ''] self._nl_update = {} self._bulk_enabled = False@@ -603,6 +606,50 @@ class Device(object): """disable automatic negotiation of speed for this device""" exec_cmd("ethtool -s %s autoneg off" % self.name)
- def _read_coalesce(self):
try:res = exec_cmd("ethtool -c %s" % self.name)except:raise DeviceError("Could not fetch coalesce settings of ""%s." % self.name)res = re.findall('\wX: (o[n|f]*)', res[0])
^^^^^^^ i think this can just be (on|off)
if not tuple(res) in product(['off', 'on'], repeat=2):raise LnstError("Invalid values for coalesce settings of ""{} : {}".format(self.name, res))
^^^ use DeviceError for exceptions from the Device class
So if I understand this correctly, the condition checks if all the strings found by the re.findall are "on" or "off", and that there's exactly two of them. But the regex already ensures that it's only "on" or "off" (with my modification at least). And you can ensure the "exactly two values" by modifying the regex a bit more:
"Adaptive RX: (on|off) TX: (on|off)"
then I'd just use re.search and return the result.groups().
In case the regex search fails, then throw the exception.
return res- def _store_coalesce(self):
coal = self._read_coalesce()self._coalesce_settings = coal- def _write_coalesce(self, rx_val, tx_val):
if self._coalesce_settings == [rx_val, tx_val]:
Depending on if you store a tuple or a list into the _coalesce_settings, this comparison might always fail, tuple never equals a list... just a note in case you rewrite the read method with match.groups() which returns a tuple...
returntry:exec_cmd("ethtool -C %s adaptive-rx %s adaptive-tx %s" %(self.name, rx_val, tx_val))except:raise DeviceConfigError("Not allowed to modify coalesce ""settings for %s." % self.name)if self._read_coalesce != [rx_val, tx_val]:
^^^ this is just a reference to a method, you're missing () to actually call it so this condition is always True
raise LnstError("Could not apply coalesce settings for ""%s effectively." % self.name)
DeviceError instead of LnstError, DeviceConfigError is also appropriate here I think
- def _disable_coalesce(self):
self._store_coalesce()self._write_coalesce('off', 'off')- def _restore_coalesce(self):
rx_val = self._coalesce_settings[0]tx_val = self._coalesce_settings[1]self._write_coalesce(rx_val, tx_val)- def disable_coalesce(self):
self._disable_coalesce()
Does it make sense to also have an enable_coalesce method for completeness?
- def restore_coalesce(self):
self._restore_coalesce()- #TODO implement proper Route objects #consider the same as with tc? # def route_add(self, dest):
-- 2.17.1 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahos...
----- Original Message -----
From: "Ondrej Lichtner" olichtne@redhat.com To: csfakian@redhat.com Cc: lnst-developers@lists.fedorahosted.org Sent: Wednesday, February 27, 2019 12:39:08 PM Subject: Re: [PATCH-next 1/2] lnst.Devices.Device: handle coalesce
On Tue, Feb 26, 2019 at 04:34:43PM +0100, csfakian@redhat.com wrote:
From: Christos Sfakianakis csfakian@redhat.com
Add methods for handling coalesce settings. Raise errors when these settings cannot be read or modified correctly.
Signed-off-by: Christos Sfakianakis csfakian@redhat.com
lnst/Devices/Device.py | 47 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py index 20cddcf..6a04d1b 100644 --- a/lnst/Devices/Device.py +++ b/lnst/Devices/Device.py @@ -17,10 +17,12 @@ import pyroute2 import logging import pprint from abc import ABCMeta +from itertools import product 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.LnstError import LnstError from lnst.Common.DeviceError import DeviceError, DeviceDeleted, DeviceDisabled from lnst.Common.DeviceError import DeviceConfigError, DeviceConfigValueError from lnst.Common.IpAddress import ipaddress @@ -58,6 +60,7 @@ class Device(object): self._deleted = False
self._ip_addrs = []
self._coalesce_settings = ['', ''] self._nl_update = {} self._bulk_enabled = False@@ -603,6 +606,50 @@ class Device(object): """disable automatic negotiation of speed for this device""" exec_cmd("ethtool -s %s autoneg off" % self.name)
- def _read_coalesce(self):
try:res = exec_cmd("ethtool -c %s" % self.name)except:raise DeviceError("Could not fetch coalesce settings of ""%s." % self.name)res = re.findall('\wX: (o[n|f]*)', res[0])^^^^^^^ i think this can just be (on|off)
if not tuple(res) in product(['off', 'on'], repeat=2):raise LnstError("Invalid values for coalesce settings of ""{} : {}".format(self.name, res))^^^ use DeviceError for exceptions from the Device classSo if I understand this correctly, the condition checks if all the strings found by the re.findall are "on" or "off", and that there's exactly two of them. But the regex already ensures that it's only "on" or "off" (with my modification at least). And you can ensure the "exactly two values" by modifying the regex a bit more:
"Adaptive RX: (on|off) TX: (on|off)"
then I'd just use re.search and return the result.groups().
In case the regex search fails, then throw the exception.
Agree, will apply these changes.
return res- def _store_coalesce(self):
coal = self._read_coalesce()self._coalesce_settings = coal- def _write_coalesce(self, rx_val, tx_val):
if self._coalesce_settings == [rx_val, tx_val]:Depending on if you store a tuple or a list into the _coalesce_settings, this comparison might always fail, tuple never equals a list... just a note in case you rewrite the read method with match.groups() which returns a tuple...
returntry:exec_cmd("ethtool -C %s adaptive-rx %s adaptive-tx %s" %(self.name, rx_val, tx_val))except:raise DeviceConfigError("Not allowed to modify coalesce ""settings for %s." % self.name)if self._read_coalesce != [rx_val, tx_val]:^^^ this is just a reference to a method, you're missing () to actually call it so this condition is always True
raise LnstError("Could not apply coalesce settings for ""%s effectively." % self.name)DeviceError instead of LnstError, DeviceConfigError is also appropriate here I think
- def _disable_coalesce(self):
self._store_coalesce()self._write_coalesce('off', 'off')- def _restore_coalesce(self):
rx_val = self._coalesce_settings[0]tx_val = self._coalesce_settings[1]self._write_coalesce(rx_val, tx_val)- def disable_coalesce(self):
self._disable_coalesce()Does it make sense to also have an enable_coalesce method for completeness?
Adding an enable() method. I will assume the initial values need to be stored only once prior to the first change (and then restored in the end of the test), so modifying the disable method slightly as well. -Christos
- def restore_coalesce(self):
self._restore_coalesce()- #TODO implement proper Route objects #consider the same as with tc? # def route_add(self, dest):
-- 2.17.1 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahos...
LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahos...
On Wed, Feb 27, 2019 at 10:40:10AM -0500, Christos Sfakianakis wrote:
----- Original Message -----
From: "Ondrej Lichtner" olichtne@redhat.com To: csfakian@redhat.com Cc: lnst-developers@lists.fedorahosted.org Sent: Wednesday, February 27, 2019 12:39:08 PM Subject: Re: [PATCH-next 1/2] lnst.Devices.Device: handle coalesce
On Tue, Feb 26, 2019 at 04:34:43PM +0100, csfakian@redhat.com wrote:
From: Christos Sfakianakis csfakian@redhat.com
Add methods for handling coalesce settings. Raise errors when these settings cannot be read or modified correctly.
Signed-off-by: Christos Sfakianakis csfakian@redhat.com
lnst/Devices/Device.py | 47 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py index 20cddcf..6a04d1b 100644 --- a/lnst/Devices/Device.py +++ b/lnst/Devices/Device.py @@ -17,10 +17,12 @@ import pyroute2 import logging import pprint from abc import ABCMeta +from itertools import product 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.LnstError import LnstError from lnst.Common.DeviceError import DeviceError, DeviceDeleted, DeviceDisabled from lnst.Common.DeviceError import DeviceConfigError, DeviceConfigValueError from lnst.Common.IpAddress import ipaddress @@ -58,6 +60,7 @@ class Device(object): self._deleted = False
self._ip_addrs = []
self._coalesce_settings = ['', ''] self._nl_update = {} self._bulk_enabled = False@@ -603,6 +606,50 @@ class Device(object): """disable automatic negotiation of speed for this device""" exec_cmd("ethtool -s %s autoneg off" % self.name)
- def _read_coalesce(self):
try:res = exec_cmd("ethtool -c %s" % self.name)except:raise DeviceError("Could not fetch coalesce settings of ""%s." % self.name)res = re.findall('\wX: (o[n|f]*)', res[0])^^^^^^^ i think this can just be (on|off)
if not tuple(res) in product(['off', 'on'], repeat=2):raise LnstError("Invalid values for coalesce settings of ""{} : {}".format(self.name, res))^^^ use DeviceError for exceptions from the Device classSo if I understand this correctly, the condition checks if all the strings found by the re.findall are "on" or "off", and that there's exactly two of them. But the regex already ensures that it's only "on" or "off" (with my modification at least). And you can ensure the "exactly two values" by modifying the regex a bit more:
"Adaptive RX: (on|off) TX: (on|off)"
then I'd just use re.search and return the result.groups().
In case the regex search fails, then throw the exception.
Agree, will apply these changes.
return res- def _store_coalesce(self):
coal = self._read_coalesce()self._coalesce_settings = coal- def _write_coalesce(self, rx_val, tx_val):
if self._coalesce_settings == [rx_val, tx_val]:Depending on if you store a tuple or a list into the _coalesce_settings, this comparison might always fail, tuple never equals a list... just a note in case you rewrite the read method with match.groups() which returns a tuple...
returntry:exec_cmd("ethtool -C %s adaptive-rx %s adaptive-tx %s" %(self.name, rx_val, tx_val))except:raise DeviceConfigError("Not allowed to modify coalesce ""settings for %s." % self.name)if self._read_coalesce != [rx_val, tx_val]:^^^ this is just a reference to a method, you're missing () to actually call it so this condition is always True
raise LnstError("Could not apply coalesce settings for ""%s effectively." % self.name)DeviceError instead of LnstError, DeviceConfigError is also appropriate here I think
- def _disable_coalesce(self):
self._store_coalesce()self._write_coalesce('off', 'off')- def _restore_coalesce(self):
rx_val = self._coalesce_settings[0]tx_val = self._coalesce_settings[1]self._write_coalesce(rx_val, tx_val)- def disable_coalesce(self):
self._disable_coalesce()Does it make sense to also have an enable_coalesce method for completeness?
Adding an enable() method. I will assume the initial values need to be stored only once prior to the first change (and then restored in the end of the test), so modifying the disable method slightly as well. -Christos
Yeah I guess I didn't properly notice... I'd expect store and restore methods to be independent of enable/disable. I'm not even sure if I'd store the state inside the Device class, in case someone wants to change this multiple times to different values and calling store/restore during a single recipe... might be better to directly expose read/write_coalesce methods as public (but at that point I think I'd want to rename them to get/set instead of read/write) so that the tester can store multiple "states" and set whatever, whenever during a recipe. And then the enable/disable would just be shortcuts for on/off.
What do you think? This might mean that the recipes need to use the API differently but that's fine, the API should come first...
-Ondrej
- def restore_coalesce(self):
self._restore_coalesce()- #TODO implement proper Route objects #consider the same as with tc? # def route_add(self, dest):
-- 2.17.1 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahos...
LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahos...
----- Original Message -----
From: "Ondrej Lichtner" olichtne@redhat.com To: "Christos Sfakianakis" csfakian@redhat.com Cc: lnst-developers@lists.fedorahosted.org Sent: Wednesday, February 27, 2019 4:51:52 PM Subject: Re: [PATCH-next 1/2] lnst.Devices.Device: handle coalesce
On Wed, Feb 27, 2019 at 10:40:10AM -0500, Christos Sfakianakis wrote:
----- Original Message -----
From: "Ondrej Lichtner" olichtne@redhat.com To: csfakian@redhat.com Cc: lnst-developers@lists.fedorahosted.org Sent: Wednesday, February 27, 2019 12:39:08 PM Subject: Re: [PATCH-next 1/2] lnst.Devices.Device: handle coalesce
On Tue, Feb 26, 2019 at 04:34:43PM +0100, csfakian@redhat.com wrote:
From: Christos Sfakianakis csfakian@redhat.com
Add methods for handling coalesce settings. Raise errors when these settings cannot be read or modified correctly.
Signed-off-by: Christos Sfakianakis csfakian@redhat.com
lnst/Devices/Device.py | 47 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py index 20cddcf..6a04d1b 100644 --- a/lnst/Devices/Device.py +++ b/lnst/Devices/Device.py @@ -17,10 +17,12 @@ import pyroute2 import logging import pprint from abc import ABCMeta +from itertools import product 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.LnstError import LnstError from lnst.Common.DeviceError import DeviceError, DeviceDeleted, DeviceDisabled from lnst.Common.DeviceError import DeviceConfigError, DeviceConfigValueError from lnst.Common.IpAddress import ipaddress @@ -58,6 +60,7 @@ class Device(object): self._deleted = False
self._ip_addrs = []
self._coalesce_settings = ['', ''] self._nl_update = {} self._bulk_enabled = False@@ -603,6 +606,50 @@ class Device(object): """disable automatic negotiation of speed for this device""" exec_cmd("ethtool -s %s autoneg off" % self.name)
- def _read_coalesce(self):
try:res = exec_cmd("ethtool -c %s" % self.name)except:raise DeviceError("Could not fetch coalesce settings of ""%s." % self.name)res = re.findall('\wX: (o[n|f]*)', res[0])^^^^^^^ i think this can just be (on|off)
if not tuple(res) in product(['off', 'on'], repeat=2):raise LnstError("Invalid values for coalesce settings of ""{} : {}".format(self.name, res))^^^ use DeviceError for exceptions from the Device classSo if I understand this correctly, the condition checks if all the strings found by the re.findall are "on" or "off", and that there's exactly two of them. But the regex already ensures that it's only "on" or "off" (with my modification at least). And you can ensure the "exactly two values" by modifying the regex a bit more:
"Adaptive RX: (on|off) TX: (on|off)"
then I'd just use re.search and return the result.groups().
In case the regex search fails, then throw the exception.
Agree, will apply these changes.
return res- def _store_coalesce(self):
coal = self._read_coalesce()self._coalesce_settings = coal- def _write_coalesce(self, rx_val, tx_val):
if self._coalesce_settings == [rx_val, tx_val]:Depending on if you store a tuple or a list into the _coalesce_settings, this comparison might always fail, tuple never equals a list... just a note in case you rewrite the read method with match.groups() which returns a tuple...
returntry:exec_cmd("ethtool -C %s adaptive-rx %s adaptive-tx %s" %(self.name, rx_val, tx_val))except:raise DeviceConfigError("Not allowed to modify coalesce ""settings for %s." % self.name)if self._read_coalesce != [rx_val, tx_val]:^^^ this is just a reference to a method, you're missing () to actually call it so this condition is always True
raise LnstError("Could not apply coalesce settings for ""%s effectively." % self.name)DeviceError instead of LnstError, DeviceConfigError is also appropriate here I think
- def _disable_coalesce(self):
self._store_coalesce()self._write_coalesce('off', 'off')- def _restore_coalesce(self):
rx_val = self._coalesce_settings[0]tx_val = self._coalesce_settings[1]self._write_coalesce(rx_val, tx_val)- def disable_coalesce(self):
self._disable_coalesce()Does it make sense to also have an enable_coalesce method for completeness?
Adding an enable() method. I will assume the initial values need to be stored only once prior to the first change (and then restored in the end of the test), so modifying the disable method slightly as well. -Christos
Yeah I guess I didn't properly notice... I'd expect store and restore methods to be independent of enable/disable. I'm not even sure if I'd store the state inside the Device class, in case someone wants to change this multiple times to different values and calling store/restore during a single recipe... might be better to directly expose read/write_coalesce methods as public (but at that point I think I'd want to rename them to get/set instead of read/write) so that the tester can store multiple "states" and set whatever, whenever during a recipe. And then the enable/disable would just be shortcuts for on/off.
Ok, but for the config phase, it would be good to have the initial values stored in the Device class in order to restore the original settings in the deconfig phase. So my proposal is to keep _store method there and call it only once, irrespective of what the user/tester tests.
So in a generic scenario, config phase: - Store coal settings if you need to do any kind of coal tuning - Proceed to set/get/enable/dsiable coal as you wish
deconfig phase: - Always call restore function, which does nothing in the worst case.
We can then expose the functions you mentioned without issues.
What do you think? This might mean that the recipes need to use the API differently but that's fine, the API should come first...
+1
-Ondrej
- def restore_coalesce(self):
self._restore_coalesce()- #TODO implement proper Route objects #consider the same as with tc? # def route_add(self, dest):
-- 2.17.1 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahos...
LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahos...
From: Christos Sfakianakis csfakian@redhat.com
Use existing 'adaptive_coalescing' parameter of BaseEnrtRecipe and updated Device API to disable adaptive coalesce when opted.
Signed-off-by: Christos Sfakianakis csfakian@redhat.com --- lnst/Recipes/ENRT/BondRecipe.py | 8 ++++++++ lnst/Recipes/ENRT/DoubleBondRecipe.py | 10 ++++++++++ lnst/Recipes/ENRT/DoubleTeamRecipe.py | 10 ++++++++++ lnst/Recipes/ENRT/SimplePerfRecipe.py | 8 ++++++++ lnst/Recipes/ENRT/TeamRecipe.py | 8 ++++++++ lnst/Recipes/ENRT/TeamVsBondRecipe.py | 10 ++++++++++ lnst/Recipes/ENRT/VlansOverBondRecipe.py | 8 ++++++++ lnst/Recipes/ENRT/VlansOverTeamRecipe.py | 8 ++++++++ lnst/Recipes/ENRT/VlansRecipe.py | 8 ++++++++ 9 files changed, 78 insertions(+)
diff --git a/lnst/Recipes/ENRT/BondRecipe.py b/lnst/Recipes/ENRT/BondRecipe.py index d2268fb..ca3b3b3 100644 --- a/lnst/Recipes/ENRT/BondRecipe.py +++ b/lnst/Recipes/ENRT/BondRecipe.py @@ -54,6 +54,10 @@ class BondRecipe(BaseEnrtRecipe): m2.eth0.ip_add(ipaddress(net_addr6 + "::2/64")) m2.eth0.up()
+ if not self.params.adaptive_coalescing: + for dev in [m1.eth0, m1.eth1, m2.eth0]: + dev.disable_coalesce() + #TODO better service handling through HostAPI if self.params.dev_intr_cpu: for m in [m1, m2]: @@ -70,3 +74,7 @@ class BondRecipe(BaseEnrtRecipe): if self.params.dev_intr_cpu: for m in [m1, m2]: m.run("service irqbalance start") + + if not self.params.adaptive_coalescing: + for dev in [m1.eth0, m1.eth1, m2.eth0]: + dev.restore_coalesce() diff --git a/lnst/Recipes/ENRT/DoubleBondRecipe.py b/lnst/Recipes/ENRT/DoubleBondRecipe.py index 2ddbec4..5f91a7c 100644 --- a/lnst/Recipes/ENRT/DoubleBondRecipe.py +++ b/lnst/Recipes/ENRT/DoubleBondRecipe.py @@ -53,6 +53,11 @@ class DoubleBondRecipe(BaseEnrtRecipe): m.eth1.up() m.bond.up()
+ if not self.params.adaptive_coalescing: + for m in [m1, m2]: + for dev in [m.eth0, m.eth1]: + dev.disable_coalesce() + #TODO better service handling through HostAPI if self.params.dev_intr_cpu: for m in [m1, m2]: @@ -69,3 +74,8 @@ class DoubleBondRecipe(BaseEnrtRecipe): if self.params.dev_intr_cpu: for m in [m1, m2]: m.run("service irqbalance start") + + if not self.params.adaptive_coalescing: + for m in [m1, m2]: + for dev in [m.eth0, m.eth1]: + dev.restore_coalesce() diff --git a/lnst/Recipes/ENRT/DoubleTeamRecipe.py b/lnst/Recipes/ENRT/DoubleTeamRecipe.py index 1dd5899..0df162e 100644 --- a/lnst/Recipes/ENRT/DoubleTeamRecipe.py +++ b/lnst/Recipes/ENRT/DoubleTeamRecipe.py @@ -67,6 +67,11 @@ class DoubleTeamRecipe(BaseEnrtRecipe): m2.eth2.up() m2.team.up()
+ if not self.params.adaptive_coalescing: + for m in [m1, m2]: + for dev in [m.eth1, m.eth2]: + dev.disable_coalesce() + #TODO better service handling through HostAPI if self.params.dev_intr_cpu: for m in [m1, m2]: @@ -83,3 +88,8 @@ class DoubleTeamRecipe(BaseEnrtRecipe): if self.params.dev_intr_cpu: for m in [m1, m2]: m.run("service irqbalance start") + + if not self.params.adaptive_coalescing: + for m in [m1, m2]: + for dev in [m.eth1, m.eth2]: + dev.restore_coalesce() diff --git a/lnst/Recipes/ENRT/SimplePerfRecipe.py b/lnst/Recipes/ENRT/SimplePerfRecipe.py index 92e1e1b..4d35eeb 100644 --- a/lnst/Recipes/ENRT/SimplePerfRecipe.py +++ b/lnst/Recipes/ENRT/SimplePerfRecipe.py @@ -51,6 +51,10 @@ class SimplePerfRecipe(BaseEnrtRecipe): m2.eth0.ip_add(ipaddress("fc00::2/64")) m2.eth0.up()
+ if not self.params.adaptive_coalescing: + for m in [m1, m2]: + m.eth0.disable_coalesce() + #TODO better service handling through HostAPI if self.params.dev_intr_cpu: for m in [m1, m2]: @@ -67,6 +71,10 @@ class SimplePerfRecipe(BaseEnrtRecipe): for m in [m1, m2]: m.run("service irqbalance start")
+ if not self.params.adaptive_coalescing: + for m in [m1, m2]: + m.eth0.restore_coalesce() + # redo # m1.eth0.adaptive_tx_coalescing = self.saved_coalescing_state["m1_if"]["tx"] # m1.eth0.adaptive_rx_coalescing = self.saved_coalescing_state["m1_if"]["rx"] diff --git a/lnst/Recipes/ENRT/TeamRecipe.py b/lnst/Recipes/ENRT/TeamRecipe.py index 35bd988..b2b65c2 100644 --- a/lnst/Recipes/ENRT/TeamRecipe.py +++ b/lnst/Recipes/ENRT/TeamRecipe.py @@ -58,6 +58,10 @@ class TeamRecipe(BaseEnrtRecipe): m1.team.up() m2.eth1.up()
+ if not self.params.adaptive_coalescing: + for dev in [m1.eth1, m1.eth2, m2.eth1]: + dev.disable_coalesce() + #TODO better service handling through HostAPI if self.params.dev_intr_cpu: for m in [m1, m2]: @@ -74,3 +78,7 @@ class TeamRecipe(BaseEnrtRecipe): if self.params.dev_intr_cpu: for m in [m1, m2]: m.run("service irqbalance start") + + if not self.params.adaptive_coalescing: + for dev in [m1.eth1, m1.eth2, m2.eth1]: + dev.restore_coalesce() diff --git a/lnst/Recipes/ENRT/TeamVsBondRecipe.py b/lnst/Recipes/ENRT/TeamVsBondRecipe.py index ed555e0..6bf90fb 100644 --- a/lnst/Recipes/ENRT/TeamVsBondRecipe.py +++ b/lnst/Recipes/ENRT/TeamVsBondRecipe.py @@ -71,6 +71,11 @@ class TeamVsBondRecipe(BaseEnrtRecipe): m2.eth2.up() m2.bond.up()
+ if not self.params.adaptive_coalescing: + for m in [m1, m2]: + for dev in [m.eth1, m.eth2]: + dev.disable_coalesce() + #TODO better service handling through HostAPI if self.params.dev_intr_cpu: for m in [m1, m2]: @@ -87,3 +92,8 @@ class TeamVsBondRecipe(BaseEnrtRecipe): if self.params.dev_intr_cpu: for m in [m1, m2]: m.run("service irqbalance start") + + if not self.params.adaptive_coalescing: + for m in [m1, m2]: + for dev in [m.eth1, m.eth2]: + dev.restore_coalesce() diff --git a/lnst/Recipes/ENRT/VlansOverBondRecipe.py b/lnst/Recipes/ENRT/VlansOverBondRecipe.py index d332268..b409bdf 100644 --- a/lnst/Recipes/ENRT/VlansOverBondRecipe.py +++ b/lnst/Recipes/ENRT/VlansOverBondRecipe.py @@ -71,6 +71,10 @@ class VlansOverBondRecipe(BaseEnrtRecipe): m2.vlan1.up() m2.vlan2.up()
+ if not self.params.adaptive_coalescing: + for dev in [m1.eth0, m1.eth1, m2.eth0]: + dev.disable_coalesce() + #TODO better service handling through HostAPI if self.params.dev_intr_cpu: for m in [m1, m2]: @@ -87,3 +91,7 @@ class VlansOverBondRecipe(BaseEnrtRecipe): if self.params.dev_intr_cpu: for m in [m1, m2]: m1.run("service irqbalance start") + + if not self.params.adaptive_coalescing: + for dev in [m1.eth0, m1.eth1, m2.eth0]: + dev.restore_coalesce() diff --git a/lnst/Recipes/ENRT/VlansOverTeamRecipe.py b/lnst/Recipes/ENRT/VlansOverTeamRecipe.py index 5f5ad0b..303c05d 100644 --- a/lnst/Recipes/ENRT/VlansOverTeamRecipe.py +++ b/lnst/Recipes/ENRT/VlansOverTeamRecipe.py @@ -74,6 +74,10 @@ class VlansOverTeamRecipe(BaseEnrtRecipe): m2.vlan1.up() m2.vlan2.up()
+ if not self.params.adaptive_coalescing: + for dev in [m1.eth1, m1.eth2, m2.eth1]: + dev.disable_coalesce() + #TODO better service handling through HostAPI if self.params.dev_intr_cpu: for m in [m1, m2]: @@ -90,3 +94,7 @@ class VlansOverTeamRecipe(BaseEnrtRecipe): if self.params.dev_intr_cpu: for m in [m1, m2]: m.run("service irqbalance start") + + if not self.params.adaptive_coalescing: + for dev in [m1.eth1, m1.eth2, m2.eth1]: + dev.restore_coalesce() diff --git a/lnst/Recipes/ENRT/VlansRecipe.py b/lnst/Recipes/ENRT/VlansRecipe.py index 44497b9..8caf5d3 100644 --- a/lnst/Recipes/ENRT/VlansRecipe.py +++ b/lnst/Recipes/ENRT/VlansRecipe.py @@ -61,6 +61,10 @@ class VlansRecipe(BaseEnrtRecipe): m2.vlan1.up() m2.vlan2.up()
+ if not self.params.adaptive_coalescing: + for m in [m1, m2]: + m.eth0.disable_coalesce() + #TODO better service handling through HostAPI if self.params.dev_intr_cpu: for m in [m1, m2]: @@ -76,3 +80,7 @@ class VlansRecipe(BaseEnrtRecipe): if self.params.dev_intr_cpu: for m in [m1, m2]: m.run("service irqbalance start") + + if not self.params.adaptive_coalescing: + for m in [m1, m2]: + m.eth0.restore_coalesce()
On Tue, Feb 26, 2019 at 04:34:42PM +0100, csfakian@redhat.com wrote:
From: Christos Sfakianakis csfakian@redhat.com
This patch set inlcudes modifications in the Device module and in some
^
ENRT recipes that hanlde adaptive coalescence.
^
In the Device module, methods are added to read, modify and verify the coalescence settings of a target device.
In the ENRT recipes, the necessary calls are added into both configuration and deconfiguration phase in case coalescence needs to be disabled. This is in agreement with old phase1/2 recipes.
Due to the fact that the target devices are ethernet and we currenlty
^ For some reason you've put the 'l' character one character earlier than it should be 3 times in the same email :). Not an issue just a funny coincidence that stood out to me :).
cannot filter them out of 2 arbitrary perf configuration endpoints, it was not possible to apply the calls in BaseEnrtRecipe.
Christos Sfakianakis (2): lnst.Devices.Device: handle coalesce lnst.Recipes.ENRT: use coalesce tuning
lnst/Devices/Device.py | 47 ++++++++++++++++++++++++ lnst/Recipes/ENRT/BondRecipe.py | 8 ++++ lnst/Recipes/ENRT/DoubleBondRecipe.py | 10 +++++ lnst/Recipes/ENRT/DoubleTeamRecipe.py | 10 +++++ lnst/Recipes/ENRT/SimplePerfRecipe.py | 8 ++++ lnst/Recipes/ENRT/TeamRecipe.py | 8 ++++ lnst/Recipes/ENRT/TeamVsBondRecipe.py | 10 +++++ lnst/Recipes/ENRT/VlansOverBondRecipe.py | 8 ++++ lnst/Recipes/ENRT/VlansOverTeamRecipe.py | 8 ++++ lnst/Recipes/ENRT/VlansRecipe.py | 8 ++++ 10 files changed, 125 insertions(+)
-- 2.17.1 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahos...
----- Original Message -----
From: "Ondrej Lichtner" olichtne@redhat.com To: csfakian@redhat.com Cc: lnst-developers@lists.fedorahosted.org Sent: Wednesday, February 27, 2019 12:12:55 PM Subject: Re: [PATCH-next 0/2] Handle coalescence tuning
On Tue, Feb 26, 2019 at 04:34:42PM +0100, csfakian@redhat.com wrote:
From: Christos Sfakianakis csfakian@redhat.com
This patch set inlcudes modifications in the Device module and in some
^ENRT recipes that hanlde adaptive coalescence.
^In the Device module, methods are added to read, modify and verify the coalescence settings of a target device.
In the ENRT recipes, the necessary calls are added into both configuration and deconfiguration phase in case coalescence needs to be disabled. This is in agreement with old phase1/2 recipes.
Due to the fact that the target devices are ethernet and we currenlty
^For some reason you've put the 'l' character one character earlier than it should be 3 times in the same email :). Not an issue just a funny coincidence that stood out to me :).
Indeed, thanks for noting:)
cannot filter them out of 2 arbitrary perf configuration endpoints, it was not possible to apply the calls in BaseEnrtRecipe.
Christos Sfakianakis (2): lnst.Devices.Device: handle coalesce lnst.Recipes.ENRT: use coalesce tuning
lnst/Devices/Device.py | 47 ++++++++++++++++++++++++ lnst/Recipes/ENRT/BondRecipe.py | 8 ++++ lnst/Recipes/ENRT/DoubleBondRecipe.py | 10 +++++ lnst/Recipes/ENRT/DoubleTeamRecipe.py | 10 +++++ lnst/Recipes/ENRT/SimplePerfRecipe.py | 8 ++++ lnst/Recipes/ENRT/TeamRecipe.py | 8 ++++ lnst/Recipes/ENRT/TeamVsBondRecipe.py | 10 +++++ lnst/Recipes/ENRT/VlansOverBondRecipe.py | 8 ++++ lnst/Recipes/ENRT/VlansOverTeamRecipe.py | 8 ++++ lnst/Recipes/ENRT/VlansRecipe.py | 8 ++++ 10 files changed, 125 insertions(+)
-- 2.17.1 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahos...
LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahos...
lnst-developers@lists.fedorahosted.org
-
Christos Sfakianakis -
csfakian＠redhat.com
-
Ondrej Lichtner