----- Original Message -----
From: "Ondrej Lichtner" <olichtne(a)redhat.com>
To: csfakian(a)redhat.com
Cc: lnst-developers(a)lists.fedorahosted.org
Sent: Friday, March 8, 2019 7:34:35 PM
Subject: Re: [PATCH-next 4/7] lnst.Devices.Device: handle adaptive coalescing
On Tue, Mar 05, 2019 at 05:09:08PM +0100, csfakian(a)redhat.com wrote:
> From: Christos Sfakianakis <csfakian(a)redhat.com>
>
> Add methods for handling coalescence settings. Raise error when
> the settings can not be obtained or written to the device. Need not
> verify the settings after a write command (to be done for all
> parameters during validation stage). Raise no errors during original
> configuration stage (_get_if_data, _store_cleanup_data).
>
> Signed-off-by: Christos Sfakianakis <csfakian(a)redhat.com>
> ---
> lnst/Devices/Device.py | 77 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 77 insertions(+)
>
> diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py
> index 20cddcf..ed3ed28 100644
> --- a/lnst/Devices/Device.py
> +++ b/lnst/Devices/Device.py
> @@ -228,6 +228,13 @@ class Device(object):
> "mtu": self.mtu,
> "driver": self.driver,
> "devlink": self._devlink}
> + try:
> + ad_rx_coal, ad_tx_coal = self._read_adaptive_coalescing()
> + except DeviceError:
> + ad_rx_coal, ad_tx_coal = None, None
> + if_data["adaptive_rx_coalescing"] = ad_rx_coal
> + if_data["adaptive_tx_coalescing"] = ad_tx_coal
> +
> return if_data
>
> def _vars(self):
> @@ -268,6 +275,12 @@ class Device(object):
> "mtu": self.mtu,
> "name": self.name,
> "hwaddr": self.hwaddr}
> + try:
> + ad_rx_coal, ad_tx_coal = self._read_adaptive_coalescing()
> + except DeviceError:
> + ad_rx_coal, ad_tx_coal = None, None
> + self._cleanup_data["adaptive_rx_coalescing"] = ad_rx_coal
> + self._cleanup_data["adaptive_tx_coalescing"] = ad_tx_coal
>
> def restore_original_data(self):
> """Restores initial configuration from stored
values"""
> @@ -284,6 +297,14 @@ class Device(object):
> if self.hwaddr != self._cleanup_data["hwaddr"]:
> self.hwaddr = self._cleanup_data["hwaddr"]
>
> + try:
> + def_ad_rx_coal =
self._cleanup_data["adaptive_rx_coalescing"]
> + def_ad_tx_coal =
self._cleanup_data["adaptive_tx_coalescing"]
> + if self._read_adaptive_coalescing() != [def_ad_rx_coal,
> def_ad_tx_coal]:
> + self._write_adaptive_coalescing(def_ad_rx_coal,
> def_ad_tx_coal)
> + except DeviceError:
> + pass
> +
Why not just use the restore_coalescing method?
Good point, will use that method in the try clause instead.
> self._cleanup_data = None
>
> def _create(self):
> @@ -365,6 +386,30 @@ class Device(object):
> self._update_attr(str(addr), "IFLA_ADDRESS")
> self._nl_sync("set")
>
> + @property
> + def adaptive_rx_coalescing(self):
> + return self._read_adaptive_coalescing()[0] == 'on'
> +
> + @adaptive_rx_coalescing.setter
> + def adaptive_rx_coalescing(self, value):
> + rx_val = 'off'
> + if value:
> + rx_val = 'on'
> + tx_val = self._read_adaptive_coalescing()[1]
> + self._write_adaptive_coalescing(rx_val, tx_val)
> +
> + @property
> + def adaptive_tx_coalescing(self):
> + return self._read_adaptive_coalescing()[1] == 'on'
> +
> + @adaptive_tx_coalescing.setter
> + def adaptive_tx_coalescing(self, value):
> + tx_val = 'off'
> + if value:
> + tx_val = 'on'
> + rx_val = self._read_adaptive_coalescing()[0]
> + self._write_adaptive_coalescing(rx_val, tx_val)
> +
> @property
> def state(self):
> """state attribute
> @@ -603,6 +648,38 @@ class Device(object):
> """disable automatic negotiation of speed for this
device"""
> exec_cmd("ethtool -s %s autoneg off" % self.name)
>
> + def _read_adaptive_coalescing(self):
> + try:
> + res = exec_cmd("ethtool -c %s" % self.name)
> + except:
> + raise DeviceError("Could not read coalescence values of "
> + "%s." % self.name)
> + regex = "Adaptive RX: (on|off) TX: (on|off)"
> + try:
> + res = re.search(regex, res[0]).groups()
> + except AttributeError:
> + raise DeviceError("No values for coalescence of %s." %
> + self.name)
> + return list(res)
> +
> + def _write_adaptive_coalescing(self, rx_val, tx_val):
> + if self._read_adaptive_coalescing() == [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 coalescence "
> + "settings for %s." % self.name)
> +
> + def _restore_coalescing(self):
> + rx_val = self._cleanup_data["adaptive_rx_coalescing"]
> + tx_val = self._cleanup_data["adaptive_tx_coalescing"]
> + self._write_adaptive_coalescing(rx_val, tx_val)
> +
> + def restore_coalescing(self):
> + self._restore_coalescing()
> +
Why is this 2 methods? Why not just have the public one with the code of
the private one?
I will make the private method public, thanks.
> > #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(a)lists.fedorahosted.org
> > To unsubscribe send an email to
> > lnst-developers-leave(a)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.fedora...
>