From 08b333eac3c274a9111e68823e5c9de585add5e4 Mon Sep 17 00:00:00 2001 From: Viacheslav Hletenko Date: Fri, 16 Jun 2023 13:29:41 +0000 Subject: T5295: Fix QoS shaper rate limit Do not handle rate via 'tc filter' directly but rather set the 'tc filter' to direct traffic to the correct tc class flow. As it in 1.3. It fixes random unexpected shapes, when you set for example 300mbit but get 3-11mbit Current implementation seems not correct as it uses rate limits two times (in class and in filter): tc class replace dev eth0 parent 1:1 classid 1:17 htb rate 250000000 \ burst 15k quantum 1514 tc filter replace dev eth0 parent 1: protocol all u32 match \ ip dst 192.168.122.11 action police rate 250000000 burst 15k flowid 1:17 The correct way after fix: tc class replace dev eth0 parent 1:1 classid 1:17 htb rate 250000000 \ burst 15k quantum 1514 tc filter replace dev eth0 parent 1: protocol all u32 match \ ip dst 192.168.122.11 flowid 1:17 --- python/vyos/qos/base.py | 114 ++++++++++++++++++++++++++---------------------- 1 file changed, 62 insertions(+), 52 deletions(-) (limited to 'python') diff --git a/python/vyos/qos/base.py b/python/vyos/qos/base.py index 33bb8ae28..bd1676107 100644 --- a/python/vyos/qos/base.py +++ b/python/vyos/qos/base.py @@ -1,4 +1,4 @@ -# Copyright 2022 VyOS maintainers and contributors +# Copyright 2022-2023 VyOS maintainers and contributors # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -243,60 +243,70 @@ class QoSBase: # The police block allows limiting of the byte or packet rate of # traffic matched by the filter it is attached to. # https://man7.org/linux/man-pages/man8/tc-police.8.html - if any(tmp in ['exceed', 'bandwidth', 'burst'] for tmp in cls_config): - filter_cmd += f' action police' - if 'exceed' in cls_config: - action = cls_config['exceed'] - filter_cmd += f' conform-exceed {action}' - if 'not_exceed' in cls_config: - action = cls_config['not_exceed'] - filter_cmd += f'/{action}' - - if 'bandwidth' in cls_config: - rate = self._rate_convert(cls_config['bandwidth']) - filter_cmd += f' rate {rate}' - - if 'burst' in cls_config: - burst = cls_config['burst'] - filter_cmd += f' burst {burst}' + # T5295: We do not handle rate via tc filter directly, + # but rather set the tc filter to direct traffic to the correct tc class flow. + # + # if any(tmp in ['exceed', 'bandwidth', 'burst'] for tmp in cls_config): + # filter_cmd += f' action police' + # + # if 'exceed' in cls_config: + # action = cls_config['exceed'] + # filter_cmd += f' conform-exceed {action}' + # if 'not_exceed' in cls_config: + # action = cls_config['not_exceed'] + # filter_cmd += f'/{action}' + # + # if 'bandwidth' in cls_config: + # rate = self._rate_convert(cls_config['bandwidth']) + # filter_cmd += f' rate {rate}' + # + # if 'burst' in cls_config: + # burst = cls_config['burst'] + # filter_cmd += f' burst {burst}' cls = int(cls) filter_cmd += f' flowid {self._parent:x}:{cls:x}' self._cmd(filter_cmd) - if 'default' in config: - if 'class' in config: - class_id_max = self._get_class_max_id(config) - default_cls_id = int(class_id_max) +1 - self._build_base_qdisc(config['default'], default_cls_id) - - filter_cmd = f'tc filter replace dev {self._interface} parent {self._parent:x}: ' - filter_cmd += 'prio 255 protocol all basic' - - # The police block allows limiting of the byte or packet rate of - # traffic matched by the filter it is attached to. - # https://man7.org/linux/man-pages/man8/tc-police.8.html - if any(tmp in ['exceed', 'bandwidth', 'burst'] for tmp in config['default']): - filter_cmd += f' action police' - - if 'exceed' in config['default']: - action = config['default']['exceed'] - filter_cmd += f' conform-exceed {action}' - if 'not_exceed' in config['default']: - action = config['default']['not_exceed'] - filter_cmd += f'/{action}' - - if 'bandwidth' in config['default']: - rate = self._rate_convert(config['default']['bandwidth']) - filter_cmd += f' rate {rate}' - - if 'burst' in config['default']: - burst = config['default']['burst'] - filter_cmd += f' burst {burst}' - - if 'class' in config: - filter_cmd += f' flowid {self._parent:x}:{default_cls_id:x}' - - self._cmd(filter_cmd) - + # T5295: Do not do any tc filter action for 'default' + # In VyOS 1.4, we have the following configuration: + # tc filter replace dev eth0 parent 1: prio 255 protocol all basic action police rate 300000000 burst 15k + # However, this caused unexpected random speeds. + # In VyOS 1.3, we do not use any 'tc filter' for rate limits, + # It gets rate from tc class classid 1:1 + # + # if 'default' in config: + # if 'class' in config: + # class_id_max = self._get_class_max_id(config) + # default_cls_id = int(class_id_max) +1 + # self._build_base_qdisc(config['default'], default_cls_id) + # + # filter_cmd = f'tc filter replace dev {self._interface} parent {self._parent:x}: ' + # filter_cmd += 'prio 255 protocol all basic' + # + # # The police block allows limiting of the byte or packet rate of + # # traffic matched by the filter it is attached to. + # # https://man7.org/linux/man-pages/man8/tc-police.8.html + # if any(tmp in ['exceed', 'bandwidth', 'burst'] for tmp in config['default']): + # filter_cmd += f' action police' + # + # if 'exceed' in config['default']: + # action = config['default']['exceed'] + # filter_cmd += f' conform-exceed {action}' + # if 'not_exceed' in config['default']: + # action = config['default']['not_exceed'] + # filter_cmd += f'/{action}' + # + # if 'bandwidth' in config['default']: + # rate = self._rate_convert(config['default']['bandwidth']) + # filter_cmd += f' rate {rate}' + # + # if 'burst' in config['default']: + # burst = config['default']['burst'] + # filter_cmd += f' burst {burst}' + # + # if 'class' in config: + # filter_cmd += f' flowid {self._parent:x}:{default_cls_id:x}' + # + # self._cmd(filter_cmd) -- cgit v1.2.3