From 88ba9a1a98b5cb63c7549c31a01d8689c7cc67e1 Mon Sep 17 00:00:00 2001 From: khramshinr Date: Tue, 29 Oct 2024 22:54:57 +0600 Subject: [PATCH] T6806: Rework QoS Policy for HFSC Shaper - Removed default `m1` and `m2` values from interface definitions - Adjusted filter priorities for shapers - Fixed SFQ qdisc and HFSC class creation to fully support `m1`, `d`, and `m2` parameters - Added validation logic similar to VyOS 1.3 to improve error handling and user experience --- .../include/qos/hfsc-m1.xml.i | 1 - .../include/qos/hfsc-m2.xml.i | 1 - python/vyos/qos/base.py | 2 +- python/vyos/qos/trafficshaper.py | 116 +++++++--------- smoketest/scripts/cli/test_qos.py | 131 ++++++++++++++++++ src/conf_mode/qos.py | 47 ++++++- 6 files changed, 225 insertions(+), 73 deletions(-) diff --git a/interface-definitions/include/qos/hfsc-m1.xml.i b/interface-definitions/include/qos/hfsc-m1.xml.i index 21b9c4f324..ca37f6ecfc 100644 --- a/interface-definitions/include/qos/hfsc-m1.xml.i +++ b/interface-definitions/include/qos/hfsc-m1.xml.i @@ -27,6 +27,5 @@ bps(8),kbps(8*10^3),mbps(8*10^6), gbps, tbps - Byte/sec - 0bit diff --git a/interface-definitions/include/qos/hfsc-m2.xml.i b/interface-definitions/include/qos/hfsc-m2.xml.i index 24e8f5d632..816546657b 100644 --- a/interface-definitions/include/qos/hfsc-m2.xml.i +++ b/interface-definitions/include/qos/hfsc-m2.xml.i @@ -27,6 +27,5 @@ bps(8),kbps(8*10^3),mbps(8*10^6), gbps, tbps - Byte/sec - 100% diff --git a/python/vyos/qos/base.py b/python/vyos/qos/base.py index 322cdca449..dee624453f 100644 --- a/python/vyos/qos/base.py +++ b/python/vyos/qos/base.py @@ -258,7 +258,7 @@ def update(self, config, direction, priority=None): has_filter = True break - if self.qostype == 'shaper' and 'prio ' not in filter_cmd: + if self.qostype in ['shaper', 'shaper_hfsc'] and 'prio ' not in filter_cmd: filter_cmd += f' prio {index}' if 'mark' in match_config: mark = match_config['mark'] diff --git a/python/vyos/qos/trafficshaper.py b/python/vyos/qos/trafficshaper.py index 8b0333c217..9f92ccd8b1 100644 --- a/python/vyos/qos/trafficshaper.py +++ b/python/vyos/qos/trafficshaper.py @@ -126,91 +126,71 @@ def update(self, config, direction): # call base class super().update(config, direction) + class TrafficShaperHFSC(QoSBase): + """ + Traffic shaper using Hierarchical Fair Service Curve (HFSC). + Documentation: https://man7.org/linux/man-pages/man8/tc-hfsc.8.html + """ + _parent = 1 qostype = 'shaper_hfsc' - # https://man7.org/linux/man-pages/man8/tc-hfsc.8.html - def update(self, config, direction): - class_id_max = 0 - if 'class' in config: - tmp = list(config['class']) - tmp.sort() - class_id_max = tmp[-1] + criteria = ['linkshare', 'realtime', 'upperlimit'] + short_criterion = { + 'linkshare': 'ls', + 'realtime': 'rt', + 'upperlimit': 'ul', + } + + def _gen_class(self, cls: int, cls_config: dict): + """ + Generate HFSC class and add Stochastic Fair Queueing (SFQ) qdisc. + + Args: + cls (int): Class ID + cls_config (dict): Configuration for the class + """ + tmp = f'tc class replace dev {self._interface} parent {self._parent:x}:1 classid {self._parent:x}:{cls:x} hfsc' + + for crit in self.criteria: + param = cls_config.get(crit) + if param: + tmp += ( + f' {self.short_criterion[crit]}' + f' m1 {self._rate_convert(param["m1"]) if param.get("m1") else 0}' + f' d {param.get("d", 0)}ms' + f' m2 {self._rate_convert(param["m2"])}' + ) - r2q = 10 - # bandwidth is a mandatory CLI node - speed = self._rate_convert(config['bandwidth']) - speed_bps = int(speed) // 8 + self._cmd(tmp) - # need a bigger r2q if going fast than 16 mbits/sec - if (speed_bps // r2q) >= MAXQUANTUM: # integer division - r2q = ceil(speed_bps // MAXQUANTUM) - else: - # if there is a slow class then may need smaller value - if 'class' in config: - min_speed = speed_bps - for cls, cls_options in config['class'].items(): - # find class with the lowest bandwidth used - if 'bandwidth' in cls_options: - bw_bps = int(self._rate_convert(cls_options['bandwidth'])) // 8 # bandwidth in bytes per second - if bw_bps < min_speed: - min_speed = bw_bps + tmp = f'tc qdisc replace dev {self._interface} parent {self._parent:x}:{cls:x} sfq perturb 10' + self._cmd(tmp) - while (r2q > 1) and (min_speed // r2q) < MINQUANTUM: - tmp = r2q -1 - if (speed_bps // tmp) >= MAXQUANTUM: - break - r2q = tmp + def update(self, config, direction): + class_id_max = self._get_class_max_id(config) + default_cls_id = int(class_id_max) + 1 if class_id_max else 2 - default_minor_id = int(class_id_max) +1 - tmp = f'tc qdisc replace dev {self._interface} root handle {self._parent:x}: hfsc default {default_minor_id:x}' # default is in hex + speed = self._rate_convert(config['bandwidth']) + + tmp = f'tc qdisc replace dev {self._interface} root handle {self._parent:x}: hfsc default {default_cls_id:x}' # default is in hex self._cmd(tmp) tmp = f'tc class replace dev {self._interface} parent {self._parent:x}: classid {self._parent:x}:1 hfsc sc rate {speed} ul rate {speed}' self._cmd(tmp) + # tmp = f'tc qdisc add dev {self._interface} parent {self._parent:x}:1 handle f1: sfq perturb 10' + # self._cmd(tmp) + if 'class' in config: for cls, cls_config in config['class'].items(): - # class id is used later on and passed as hex, thus this needs to be an int - cls = int(cls) - # ls m1 - if cls_config.get('linkshare', {}).get('m1').endswith('%'): - percent = cls_config['linkshare']['m1'].rstrip('%') - m_one_rate = self._rate_convert(config['bandwidth']) * int(percent) // 100 - else: - m_one_rate = cls_config['linkshare']['m1'] - # ls m2 - if cls_config.get('linkshare', {}).get('m2').endswith('%'): - percent = cls_config['linkshare']['m2'].rstrip('%') - m_two_rate = self._rate_convert(config['bandwidth']) * int(percent) // 100 - else: - m_two_rate = self._rate_convert(cls_config['linkshare']['m2']) - - tmp = f'tc class replace dev {self._interface} parent {self._parent:x}:1 classid {self._parent:x}:{cls:x} hfsc ls m1 {m_one_rate} m2 {m_two_rate} ' - self._cmd(tmp) - - tmp = f'tc qdisc replace dev {self._interface} parent {self._parent:x}:{cls:x} sfq perturb 10' - self._cmd(tmp) + self._gen_class(cls=int(cls), cls_config=cls_config) if 'default' in config: - # ls m1 - if config.get('default', {}).get('linkshare', {}).get('m1').endswith('%'): - percent = config['default']['linkshare']['m1'].rstrip('%') - m_one_rate = self._rate_convert(config['default']['linkshare']['m1']) * int(percent) // 100 - else: - m_one_rate = config['default']['linkshare']['m1'] - # ls m2 - if config.get('default', {}).get('linkshare', {}).get('m2').endswith('%'): - percent = config['default']['linkshare']['m2'].rstrip('%') - m_two_rate = self._rate_convert(config['default']['linkshare']['m2']) * int(percent) // 100 - else: - m_two_rate = self._rate_convert(config['default']['linkshare']['m2']) - tmp = f'tc class replace dev {self._interface} parent {self._parent:x}:1 classid {self._parent:x}:{default_minor_id:x} hfsc ls m1 {m_one_rate} m2 {m_two_rate} ' - self._cmd(tmp) - - tmp = f'tc qdisc replace dev {self._interface} parent {self._parent:x}:{default_minor_id:x} sfq perturb 10' - self._cmd(tmp) + self._gen_class( + cls=int(default_cls_id), cls_config=config.get('default', {}) + ) # call base class super().update(config, direction) diff --git a/smoketest/scripts/cli/test_qos.py b/smoketest/scripts/cli/test_qos.py index 9c3e848cdb..be49863998 100755 --- a/smoketest/scripts/cli/test_qos.py +++ b/smoketest/scripts/cli/test_qos.py @@ -985,6 +985,137 @@ def test_20_round_robin_policy_default(self): tmp[2]['options'], ) + def test_21_shaper_hfsc(self): + interface = self._interfaces[0] + policy_name = f'qos-policy-{interface}' + ul = { + 'm1': '100kbit', + 'm2': '150kbit', + 'd': '100', + } + ls = {'m2': '120kbit'} + rt = { + 'm1': '110kbit', + 'm2': '130kbit', + 'd': '75', + } + self.cli_set(base_path + ['interface', interface, 'egress', policy_name]) + self.cli_set(base_path + ['policy', 'shaper-hfsc', policy_name]) + + # Policy {policy_name} misses "default" class! + with self.assertRaises(ConfigSessionError): + self.cli_commit() + + self.cli_set( + base_path + ['policy', 'shaper-hfsc', policy_name, 'default', 'upperlimit'] + ) + + # At least one m2 value needs to be set for class: {class_name} + with self.assertRaises(ConfigSessionError): + self.cli_commit() + + self.cli_set( + base_path + ['policy', 'shaper-hfsc', policy_name, 'default', 'upperlimit', 'm1', ul['m1']] + ) + # {class_name} upperlimit m1 value is set, but no m2 was found! + with self.assertRaises(ConfigSessionError): + self.cli_commit() + + self.cli_set( + base_path + ['policy', 'shaper-hfsc', policy_name, 'default', 'upperlimit', 'm2', ul['m2']] + ) + # {class_name} upperlimit m1 value is set, but no d was found! + with self.assertRaises(ConfigSessionError): + self.cli_commit() + + self.cli_set( + base_path + ['policy', 'shaper-hfsc', policy_name, 'default', 'upperlimit', 'd', ul['d']] + ) + # Linkshare m2 needs to be defined to use upperlimit m2 for class: {class_name} + with self.assertRaises(ConfigSessionError): + self.cli_commit() + + self.cli_set( + base_path + ['policy', 'shaper-hfsc', policy_name, 'default', 'linkshare', 'm2', ls['m2']] + ) + self.cli_commit() + + # use raw because tc json is incorrect here + tmp = cmd(f'tc -details qdisc show dev {interface}') + for rec in tmp.split('\n'): + rec = rec.strip() + if 'root' in rec: + self.assertEqual(rec, 'qdisc hfsc 1: root refcnt 2 default 2') + else: + self.assertRegex( + rec, + r'qdisc sfq \S+: parent 1:2 limit 127p quantum 1514b depth 127 flows 128 divisor 1024 perturb 10sec', + ) + # use raw because tc json is incorrect here + tmp = cmd(f'tc -details class show dev {interface}') + for rec in tmp.split('\n'): + rec = rec.strip().lower() + if 'root' in rec: + self.assertEqual(rec, 'class hfsc 1: root') + elif 'hfsc 1:1' in rec: + # m2 \S+bit is auto bandwidth + self.assertRegex( + rec, + r'class hfsc 1:1 parent 1: sc m1 0bit d 0us m2 \S+bit ul m1 0bit d 0us m2 \S+bit', + ) + else: + self.assertRegex( + rec, + rf'class hfsc 1:2 parent 1:1 leaf \S+: ls m1 0bit d 0us m2 {ls["m2"]} ul m1 {ul["m1"]} d {ul["d"]}ms m2 {ul["m2"]}', + ) + + for key, val in rt.items(): + self.cli_set( + base_path + ['policy', 'shaper-hfsc', policy_name, 'default', 'realtime', key, val] + ) + self.cli_commit() + + tmp = cmd(f'tc -details class show dev {interface}') + for rec in tmp.split('\n'): + rec = rec.strip().lower() + if 'hfsc 1:2' in rec: + self.assertTrue( + f'rt m1 {rt["m1"]} d {rt["d"]}ms m2 {rt["m2"]} ls m1 0bit d 0us m2 {ls["m2"]} ul m1 {ul["m1"]} d {ul["d"]}ms m2 {ul["m2"]}' + in rec + ) + + # add some class + self.cli_set( + base_path + ['policy', 'shaper-hfsc', policy_name, 'class', '10', 'linkshare', 'm2', '300kbit'] + ) + self.cli_set( + base_path + ['policy', 'shaper-hfsc', policy_name, 'class', '10', 'match', 'tst', 'ip', 'dscp', 'internet'] + ) + + self.cli_set( + base_path + ['policy', 'shaper-hfsc', policy_name, 'class', '30', 'realtime', 'm2', '250kbit'] + ) + self.cli_set( + base_path + ['policy', 'shaper-hfsc', policy_name, 'class', '30', 'realtime', 'd', '77'] + ) + self.cli_set( + base_path + ['policy', 'shaper-hfsc', policy_name, 'class', '30', 'match', 'tst30', 'ip', 'dscp', 'critical'] + ) + self.cli_commit() + + tmp = cmd(f'tc -details qdisc show dev {interface}') + self.assertEqual(4, len(tmp.split('\n'))) + + tmp = cmd(f'tc -details class show dev {interface}') + tmp = tmp.lower() + + self.assertTrue( + f'rt m1 {rt["m1"]} d {rt["d"]}ms m2 {rt["m2"]} ls m1 0bit d 0us m2 {ls["m2"]} ul m1 {ul["m1"]} d {ul["d"]}ms m2 {ul["m2"]}' + in tmp + ) + self.assertTrue(': ls m1 0bit d 0us m2 300kbit' in tmp) + self.assertTrue(': rt m1 0bit d 77ms m2 250kbit' in tmp) + def test_22_rate_control_default(self): interface = self._interfaces[0] policy_name = f'qos-policy-{interface}' diff --git a/src/conf_mode/qos.py b/src/conf_mode/qos.py index a4d5f44e7d..f8f32e3274 100755 --- a/src/conf_mode/qos.py +++ b/src/conf_mode/qos.py @@ -210,6 +210,46 @@ def _verify_match_group_exist(cls_config, qos): Warning(f'Match group "{group}" does not exist!') +def _verify_default_policy_exist(policy, policy_config): + if 'default' not in policy_config: + raise ConfigError(f'Policy {policy} misses "default" class!') + + +def _check_shaper_hfsc_rate(cls, cls_conf): + is_m2_exist = False + for crit in TrafficShaperHFSC.criteria: + if cls_conf.get(crit, {}).get('m2') is not None: + is_m2_exist = True + + if cls_conf.get(crit, {}).get('m1') is not None: + for crit_val in ['m2', 'd']: + if cls_conf.get(crit, {}).get(crit_val) is None: + raise ConfigError( + f'{cls} {crit} m1 value is set, but no {crit_val} was found!' + ) + + if not is_m2_exist: + raise ConfigError(f'At least one m2 value needs to be set for class: {cls}') + + if ( + cls_conf.get('upperlimit', {}).get('m2') is not None + and cls_conf.get('linkshare', {}).get('m2') is None + ): + raise ConfigError( + f'Linkshare m2 needs to be defined to use upperlimit m2 for class: {cls}' + ) + + +def _verify_shaper_hfsc(policy, policy_config): + _verify_default_policy_exist(policy, policy_config) + + _check_shaper_hfsc_rate('default', policy_config.get('default')) + + if 'class' in policy_config: + for cls, cls_conf in policy_config['class'].items(): + _check_shaper_hfsc_rate(cls, cls_conf) + + def verify(qos): if not qos or 'interface' not in qos: return None @@ -253,11 +293,13 @@ def verify(qos): if queue_lim < max_tr: raise ConfigError(f'Policy "{policy}" uses queue-limit "{queue_lim}" < max-threshold "{max_tr}"!') if policy_type in ['priority_queue']: - if 'default' not in policy_config: - raise ConfigError(f'Policy {policy} misses "default" class!') + _verify_default_policy_exist(policy, policy_config) if policy_type in ['rate_control']: if 'bandwidth' not in policy_config: raise ConfigError('Bandwidth not defined') + if policy_type in ['shaper_hfsc']: + _verify_shaper_hfsc(policy, policy_config) + if 'default' in policy_config: if 'bandwidth' not in policy_config['default'] and policy_type not in ['priority_queue', 'round_robin', 'shaper_hfsc']: raise ConfigError('Bandwidth not defined for default traffic!') @@ -293,6 +335,7 @@ def generate(qos): return None + def apply(qos): # Always delete "old" shapers first for interface in interfaces():