Ido Barkan has uploaded a new change for review.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
net: Support VLAN hardware offloading in hostQOS
Apparently, the tc filter that VDSM uses to classify 802.1q traffic is not working when the kernel offloads VLAN tagging to the underlying physical nic. This is because the VLAN field in the link layer header is only added by the hardware and is not present by the time the filter inspects it. Since the filter uses a straight forward matcher which only looks at the headers it fails to match and the packets aren't classified to the desired queuing discipline (qdisc). Using a kernel packet meta data matcher should work regardless of VLAN tagging offloading is enabled.
Change-Id: I667a9f38f4314da309685f6ba247f705a2e9c23e Signed-off-by: Ido Barkan ibarkan@redhat.com Bug-Url: https://bugzilla.redhat.com/1271205 --- M vdsm/network/configurators/qos.py M vdsm/network/tc/filter.py 2 files changed, 47 insertions(+), 6 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/43/47443/1
diff --git a/vdsm/network/configurators/qos.py b/vdsm/network/configurators/qos.py index 25d049f..d1c94a7 100644 --- a/vdsm/network/configurators/qos.py +++ b/vdsm/network/configurators/qos.py @@ -130,9 +130,14 @@
def _qdisc_conf_out(dev, root_qdisc_handle, vlan_tag, class_id, qos): """Adds the traffic class and filtering to the current hfsc qdisc""" - flow_id = ':' + class_id + flow_id = _ROOT_QDISC_HANDLE + class_id + + def filt_flow_id(filt, kind): + return filt.get(kind, {}).get('flowid') + filters = [filt for filt in tc._filters(dev, parent=root_qdisc_handle) if - 'u32' in filt and filt['u32'].get('flowid') == flow_id] + flow_id in + (filt_flow_id(filt, 'basic'), filt_flow_id(filt, 'u32'))]
# Clear up any previous filters to the class for filt in filters: @@ -158,10 +163,11 @@
def _add_vlan_filter(dev, vlan_tag, root_qdisc_handle, class_id): - tc.filter.replace(dev, parent=root_qdisc_handle, protocol='802.1q', + tc.filter.replace(dev, parent=root_qdisc_handle, protocol='all', pref=vlan_tag, - u32=['match', 'u16', '0x%x' % vlan_tag, '0xFFF', 'at', - '-4', 'flowid', '0x' + class_id]) + basic=['match', 'meta(vlan eq %s)' % vlan_tag, + 'flowid', root_qdisc_handle + class_id] + )
def _add_non_vlanned_filter(dev, root_qdisc_handle): diff --git a/vdsm/network/tc/filter.py b/vdsm/network/tc/filter.py index 9b3913c..778e7ed 100644 --- a/vdsm/network/tc/filter.py +++ b/vdsm/network/tc/filter.py @@ -130,6 +130,27 @@ return data
+def _parse_ematch(tokens): + """Parses tokens describing a raw ematch, (see man tc-ematch) e.g., + 'meta(vlan mask 0x00000000 eq 16)' into a data dictionary. + currently only a single 'meta' module predicate is supported""" + data = {} + for token in tokens: + if token == _parser.LINE_DELIMITER: # line break + continue + elif token == 'handle': + data[token] = _parser.parse_str(tokens) + elif token == 'flowid': + data['flowid'] = _parser.parse_str(tokens) + elif '(' in token: + module, first_arg = token.split('(') + if module != 'meta': + _parser.parse_skip_line(tokens) + data['module'] = module + data.update(_parse_ematch_match(first_arg, tokens)) + return data + + _parse_match_ip = _parser.parse_skip_line # Unimplemented, skip line
@@ -142,6 +163,20 @@ _parser.consume(tokens, 'at') offset = _parser.parse_int(tokens) return {'value': value, 'mask': mask, 'offset': offset} + + +def _parse_ematch_match(first_arg, tokens): + data = {} + if first_arg in ('random', 'loadavg_1', 'nf_mark', 'vlan', 'sk_rcvbuf', + 'sk_snd_queue'): + data['object'] = first_arg + for token in tokens: + if token in ('eq', 'lt', 'gt'): + data['relation'] = token + data['value'] = next(tokens).strip(')') + elif token == 'mask': + data['mask'] = _parser.parse_str(tokens) + return data
def _parse_action(tokens): @@ -199,5 +234,5 @@ 'skbedit': None, 'xt': None}
-_CLASSES = {'basic': None, 'cgroup': None, 'flow': None, 'fw': None, +_CLASSES = {'basic': _parse_ematch, 'cgroup': None, 'flow': None, 'fw': None, 'route': None, 'rsvp': None, 'tcindex': None, 'u32': _parse_u32}
automation@ovirt.org has posted comments on this change.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
Patch Set 1:
* Update tracker::#1271205::OK * Check Bug-Url::OK * Check Public Bug::#1271205::OK, public bug * Check Product::#1271205::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Dan Kenigsberg has posted comments on this change.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
Patch Set 1: Code-Review-1
(5 comments)
https://gerrit.ovirt.org/#/c/47443/1/vdsm/network/configurators/qos.py File vdsm/network/configurators/qos.py:
Line 129: Line 130: Line 131: def _qdisc_conf_out(dev, root_qdisc_handle, vlan_tag, class_id, qos): Line 132: """Adds the traffic class and filtering to the current hfsc qdisc""" Line 133: flow_id = _ROOT_QDISC_HANDLE + class_id update test_qdiscs() ? Line 134: Line 135: def filt_flow_id(filt, kind): Line 136: return filt.get(kind, {}).get('flowid') Line 137:
https://gerrit.ovirt.org/#/c/47443/1/vdsm/network/tc/filter.py File vdsm/network/tc/filter.py:
Line 131: Line 132: Line 133: def _parse_ematch(tokens): Line 134: """Parses tokens describing a raw ematch, (see man tc-ematch) e.g., Line 135: 'meta(vlan mask 0x00000000 eq 16)' into a data dictionary. please add a filter like this one to
TestFilters.test_filters Line 136: currently only a single 'meta' module predicate is supported""" Line 137: data = {} Line 138: for token in tokens: Line 139: if token == _parser.LINE_DELIMITER: # line break
Line 142: data[token] = _parser.parse_str(tokens) Line 143: elif token == 'flowid': Line 144: data['flowid'] = _parser.parse_str(tokens) Line 145: elif '(' in token: Line 146: module, first_arg = token.split('(') Your Compilation 101 teacher now hates you for not tokenizing '('. I can live with this.
But I think that using split('(', 1) is a bit safer. Line 147: if module != 'meta': Line 148: _parser.parse_skip_line(tokens) Line 149: data['module'] = module Line 150: data.update(_parse_ematch_match(first_arg, tokens))
Line 146: module, first_arg = token.split('(') Line 147: if module != 'meta': Line 148: _parser.parse_skip_line(tokens) Line 149: data['module'] = module Line 150: data.update(_parse_ematch_match(first_arg, tokens)) please add a final "else" clause. With either a log or an apologetic comment. Line 151: return data Line 152: Line 153: Line 154: _parse_match_ip = _parser.parse_skip_line # Unimplemented, skip line
Line 174: if token in ('eq', 'lt', 'gt'): Line 175: data['relation'] = token Line 176: data['value'] = next(tokens).strip(')') Line 177: elif token == 'mask': Line 178: data['mask'] = _parser.parse_str(tokens) again, please add a protective final "else". Line 179: return data Line 180: Line 181: Line 182: def _parse_action(tokens):
Ido Barkan has posted comments on this change.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
Patch Set 1: Verified+1
Ido Barkan has posted comments on this change.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
Patch Set 1:
(4 comments)
https://gerrit.ovirt.org/#/c/47443/1/vdsm/network/configurators/qos.py File vdsm/network/configurators/qos.py:
Line 129: Line 130: Line 131: def _qdisc_conf_out(dev, root_qdisc_handle, vlan_tag, class_id, qos): Line 132: """Adds the traffic class and filtering to the current hfsc qdisc""" Line 133: flow_id = _ROOT_QDISC_HANDLE + class_id
update test_qdiscs() ?
it is so lame, that it does not justify the amount of work it would take to modify it to properly cover my changes. most of them are in qos.py in this logic is not covered here. however, TestFilters might be worth a change. Line 134: Line 135: def filt_flow_id(filt, kind): Line 136: return filt.get(kind, {}).get('flowid') Line 137:
https://gerrit.ovirt.org/#/c/47443/1/vdsm/network/tc/filter.py File vdsm/network/tc/filter.py:
Line 131: Line 132: Line 133: def _parse_ematch(tokens): Line 134: """Parses tokens describing a raw ematch, (see man tc-ematch) e.g., Line 135: 'meta(vlan mask 0x00000000 eq 16)' into a data dictionary.
please add a filter like this one to
Done Line 136: currently only a single 'meta' module predicate is supported""" Line 137: data = {} Line 138: for token in tokens: Line 139: if token == _parser.LINE_DELIMITER: # line break
Line 142: data[token] = _parser.parse_str(tokens) Line 143: elif token == 'flowid': Line 144: data['flowid'] = _parser.parse_str(tokens) Line 145: elif '(' in token: Line 146: module, first_arg = token.split('(')
Your Compilation 101 teacher now hates you for not tokenizing '('. I can li
Done Line 147: if module != 'meta': Line 148: _parser.parse_skip_line(tokens) Line 149: data['module'] = module Line 150: data.update(_parse_ematch_match(first_arg, tokens))
Line 146: module, first_arg = token.split('(') Line 147: if module != 'meta': Line 148: _parser.parse_skip_line(tokens) Line 149: data['module'] = module Line 150: data.update(_parse_ematch_match(first_arg, tokens))
please add a final "else" clause. With either a log or an apologetic commen
well, I can add a per-unknown-token log but it will not say much. I prefer fail here silently as we would get a partially parsed line rather then failing. Line 151: return data Line 152: Line 153: Line 154: _parse_match_ip = _parser.parse_skip_line # Unimplemented, skip line
automation@ovirt.org has posted comments on this change.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
Patch Set 2:
* Update tracker::#1271205::OK * Check Bug-Url::OK * Check Public Bug::#1271205::OK, public bug * Check Product::#1271205::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Ido Barkan has posted comments on this change.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/47443/1/vdsm/network/tc/filter.py File vdsm/network/tc/filter.py:
Line 174: if token in ('eq', 'lt', 'gt'): Line 175: data['relation'] = token Line 176: data['value'] = next(tokens).strip(')') Line 177: elif token == 'mask': Line 178: data['mask'] = _parser.parse_str(tokens)
again, please add a protective final "else".
done. see comment up. Line 179: return data Line 180: Line 181: Line 182: def _parse_action(tokens):
automation@ovirt.org has posted comments on this change.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
Patch Set 3:
* Update tracker::#1271205::OK * Check Bug-Url::OK * Check Public Bug::#1271205::OK, public bug * Check Product::#1271205::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Dan Kenigsberg has posted comments on this change.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
Patch Set 3: Code-Review-1
(2 comments)
minor nits regarding parsing (which we never use, but let's make it less misleading)
https://gerrit.ovirt.org/#/c/47443/3/vdsm/network/tc/filter.py File vdsm/network/tc/filter.py:
Line 171: def _parse_ematch_match(first_arg, tokens): Line 172: data = {} Line 173: if first_arg in ('random', 'loadavg_1', 'nf_mark', 'vlan', 'sk_rcvbuf', Line 174: 'sk_snd_queue'): Line 175: data['object'] = first_arg there are plenty more objects. We don't have to parse them all (or any other than 'vlan'), but we'd better acknowledge their existence with an "else" clause and an early exit from the function.
-------------------------------------------------------- ID Type Description -------------------------------------------------------- Generic: random INT Random value (32 bit) loadavg_1 INT Load average in last minute loadavg_5 INT Load average in last 5 minutes loadavg_15 INT Load average in last 15 minutes
Interfaces: dev INT,VAR Device the packet is on
Packet attributes: priority INT Priority of packet protocol INT Link layer protocol pkt_type INT Packet type (uni|multi|broad|...)cast pkt_len INT Length of packet data_len INT Length of data in packet mac_len INT Length of link layer header
Netfilter: nf_mark INT Netfilter mark fwmark INT Alias for nf_mark
Traffic Control: tc_index INT TC Index
Routing: rt_classid INT Routing ClassID (cls_route) rt_iif INT Incoming interface index vlan INT Vlan tag
Sockets: sk_family INT Address family sk_state INT State sk_reuse INT Reuse Flag sk_bind_if INT,VAR Bound interface sk_refcnt INT Reference counter sk_shutdown INT Shutdown mask sk_proto INT Protocol sk_type INT Type sk_rcvbuf INT Receive buffer size sk_rmem INT RMEM sk_wmem INT WMEM sk_omem INT OMEM sk_wmem_queue INT WMEM queue sk_snd_queue INT Send queue length sk_rcv_queue INT Receive queue length sk_err_queue INT Error queue length sk_fwd_alloc INT Forward allocations sk_sndbuf INT Send buffer size -------------------------------------------------------- Illegal "ematch" Line 176: for token in tokens: Line 177: if token in ('eq', 'lt', 'gt'): Line 178: data['relation'] = token Line 179: data['value'] = next(tokens).strip(')')
Line 175: data['object'] = first_arg Line 176: for token in tokens: Line 177: if token in ('eq', 'lt', 'gt'): Line 178: data['relation'] = token Line 179: data['value'] = next(tokens).strip(')') It's not important for our use case, but the value for all our supported "objects" is an int. so we'd better call parse_int (and parse_hex() for mask. Line 180: elif token == 'mask': Line 181: data['mask'] = _parser.parse_str(tokens) Line 182: else: Line 183: logging.info('could not parse ematch filter. token=%s', token)
Ido Barkan has posted comments on this change.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
Patch Set 3:
(2 comments)
https://gerrit.ovirt.org/#/c/47443/3/vdsm/network/tc/filter.py File vdsm/network/tc/filter.py:
Line 171: def _parse_ematch_match(first_arg, tokens): Line 172: data = {} Line 173: if first_arg in ('random', 'loadavg_1', 'nf_mark', 'vlan', 'sk_rcvbuf', Line 174: 'sk_snd_queue'): Line 175: data['object'] = first_arg
there are plenty more objects. We don't have to parse them all (or any othe
Ill be stricter in what I try to parse Line 176: for token in tokens: Line 177: if token in ('eq', 'lt', 'gt'): Line 178: data['relation'] = token Line 179: data['value'] = next(tokens).strip(')')
Line 175: data['object'] = first_arg Line 176: for token in tokens: Line 177: if token in ('eq', 'lt', 'gt'): Line 178: data['relation'] = token Line 179: data['value'] = next(tokens).strip(')')
It's not important for our use case, but the value for all our supported "o
parse int is cool, but I am stuck with this horrible ')', so I'll have to settle with a mere int(). added a parse_hex. Line 180: elif token == 'mask': Line 181: data['mask'] = _parser.parse_str(tokens) Line 182: else: Line 183: logging.info('could not parse ematch filter. token=%s', token)
automation@ovirt.org has posted comments on this change.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
Patch Set 4:
* Update tracker::#1271205::OK * Check Bug-Url::OK * Check Public Bug::#1271205::OK, public bug * Check Product::#1271205::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Dan Kenigsberg has posted comments on this change.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
Patch Set 4: Continuous-Integration+1
09:11:37 ERROR: Tests mkimage.mkIsoFs creating an image and checking its content 09:11:37 ---------------------------------------------------------------------- 09:11:37 Traceback (most recent call last): 09:11:37 File "/home/jenkins/workspace/vdsm_3.6_check-patch-fc22-x86_64/vdsm/tests/testlib.py", line 67, in wrapper 09:11:37 return f(self, *args) 09:11:37 File "/home/jenkins/workspace/vdsm_3.6_check-patch-fc22-x86_64/vdsm/tests/mkimageTests.py", line 189, in test_mkIsoFs 09:11:37 m.mount(mntOpts='loop') 09:11:37 File "/home/jenkins/workspace/vdsm_3.6_check-patch-fc22-x86_64/vdsm/vdsm/storage/mount.py", line 225, in mount 09:11:37 return self._runcmd(cmd, timeout) 09:11:37 File "/home/jenkins/workspace/vdsm_3.6_check-patch-fc22-x86_64/vdsm/vdsm/storage/mount.py", line 241, in _runcmd 09:11:37 raise MountError(rc, ";".join((out, err))) 09:11:37 MountError: (32, ';mount: /tmp/tmppdMN8E/vmId_iso.187bef6f9db1bb9e4d443e5a78fd3d59.img: failed to setup loop device: No such file or directory\n') 09:11:37 -----------------
Dan Kenigsberg has posted comments on this change.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
Patch Set 4:
(1 comment)
https://gerrit.ovirt.org/#/c/47443/4/vdsm/network/tc/filter.py File vdsm/network/tc/filter.py:
Line 170: Line 171: def _parse_ematch_match(first_arg, tokens): Line 172: if first_arg != 'vlan': Line 173: _parser.parse_skip_line(tokens) Line 174: # empty data. currently we do not support other ematches than vlan but don't you want to eat all token up to the end of the line? Line 175: return {} Line 176: Line 177: data = {'object': first_arg} Line 178: for token in tokens:
Ido Barkan has posted comments on this change.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
Patch Set 4:
(1 comment)
https://gerrit.ovirt.org/#/c/47443/4/vdsm/network/tc/filter.py File vdsm/network/tc/filter.py:
Line 170: Line 171: def _parse_ematch_match(first_arg, tokens): Line 172: if first_arg != 'vlan': Line 173: _parser.parse_skip_line(tokens) Line 174: # empty data. currently we do not support other ematches than vlan
but don't you want to eat all token up to the end of the line?
this is what skip_line does Line 175: return {} Line 176: Line 177: data = {'object': first_arg} Line 178: for token in tokens:
Dan Kenigsberg has posted comments on this change.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://gerrit.ovirt.org/#/c/47443/4/vdsm/network/tc/filter.py File vdsm/network/tc/filter.py:
Line 170: Line 171: def _parse_ematch_match(first_arg, tokens): Line 172: if first_arg != 'vlan': Line 173: _parser.parse_skip_line(tokens) Line 174: # empty data. currently we do not support other ematches than vlan
this is what skip_line does
argh, of course. Line 175: return {} Line 176: Line 177: data = {'object': first_arg} Line 178: for token in tokens:
Ido Barkan has posted comments on this change.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
Patch Set 4: Verified+1
automation@ovirt.org has posted comments on this change.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
Patch Set 5:
* Update tracker::#1271205::OK * Check Bug-Url::OK * Check Public Bug::#1271205::OK, public bug * Check Product::#1271205::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Dan Kenigsberg has submitted this change and it was merged.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
net: Support VLAN hardware offloading in hostQOS
Apparently, the tc filter that VDSM uses to classify 802.1q traffic is not working when the kernel offloads VLAN tagging to the underlying physical nic. This is because the VLAN field in the link layer header is only added by the hardware and is not present by the time the filter inspects it. Since the filter uses a straight forward matcher which only looks at the headers it fails to match and the packets aren't classified to the desired queuing discipline (qdisc). Using a kernel packet meta data matcher should work regardless of VLAN tagging offloading is enabled.
Change-Id: I667a9f38f4314da309685f6ba247f705a2e9c23e Signed-off-by: Ido Barkan ibarkan@redhat.com Bug-Url: https://bugzilla.redhat.com/1271205 Reviewed-on: https://gerrit.ovirt.org/47443 Reviewed-by: Dan Kenigsberg danken@redhat.com Continuous-Integration: Jenkins CI --- M tests/tcTests.py M tests/tc_filter_show.out M vdsm/network/configurators/qos.py M vdsm/network/tc/_parser.py M vdsm/network/tc/filter.py 5 files changed, 85 insertions(+), 6 deletions(-)
Approvals: Ido Barkan: Verified Jenkins CI: Passed CI tests Dan Kenigsberg: Looks good to me, approved
automation@ovirt.org has posted comments on this change.
Change subject: net: Support VLAN hardware offloading in hostQOS ......................................................................
Patch Set 6:
* Update tracker::#1271205::OK * Set MODIFIED::bug 1271205::::#1271205::::IGNORE, not oVirt prod but vdsm
vdsm-patches@lists.fedorahosted.org
-
automation＠ovirt.org -
Dan Kenigsberg -
ibarkan＠redhat.com