Re: [ovs-dev] [BUG] [ofpacts_equal_stringwise] ovs-ofctl replace xxx will cause flow_mod even if openflow is same
Hi, all, I know this, it's NOT bug. The design of this command is right. The option carried is use for read, the flow FILE is used for write. User need to ensure the version of option and flow FILE matches, like, If your flow FILE is written by openflow13, then you should use -O OpenFlow13. Thank you~ Simon Jones Simon Jones 于2023年10月27日周五 17:51写道： > Hi all, > > I'm using ovs-dpdk version 2.17.1. > > Now I found ovs-ofctl replace xxx will cause flow_mod even if openflow is > same. > > Detail: > ``` > [root@localhost ~]# cat flows > in_port="p0",dl_vlan=11,ip,nw_proto=17,actions=output:vf2 > [root@localhost ~]# cat flows2 > in_port="p0",dl_vlan=11,ip,nw_proto=17,actions=mod_vlan_pcp(2),output:vf2 > [root@localhost ~]# cat flows3 > > in_port="p0",dl_vlan=11,ip,nw_proto=17,actions=set_field:2->vlan_pcp,output:vf2 > > [root@localhost ~]# ovs-ofctl del-flows br-phy > [root@localhost ~]# ovs-ofctl -O OpenFlow13 replace-flows br-phy flows > # 1st add this openflow > [root@localhost ~]# ovs-ofctl -O OpenFlow13 replace-flows br-phy flows > # then add this openflow again > # then this command will NOT cause flow_mods > > [root@localhost ~]# ovs-ofctl del-flows br-phy > [root@localhost ~]# ovs-ofctl -O OpenFlow13 replace-flows br-phy flows2 > # 1st add this openflow > [root@localhost ~]# ovs-ofctl -O OpenFlow13 replace-flows br-phy flows2 > # then add this openflow again > # This will cause flow_mods > # log: xxx|connmgr|INFO|br-phy<->unix#48: 1 flow_mods in the last 0 s > (1 adds) > > [root@localhost ~]# ovs-ofctl del-flows br-phy > [root@localhost ~]# ovs-ofctl -O OpenFlow13 replace-flows br-phy flows3 > # 1st add this openflow > [root@localhost ~]# ovs-ofctl -O OpenFlow13 replace-flows br-phy flows3 > # then add this openflow again > # this will NOT cause flow_mods > [root@localhost ~]# ovs-ofctl replace-flows br-phy flows3 > # This will cause flow_mods > # log: xxx|connmgr|INFO|br-phy<->unix#48: 1 flow_mods in the last 0 s > (1 adds) > ``` > > The reason cause flow_mods is because these function, fte_version_equals > - ofpacts_equal_stringwise . > Which is different openflow version process between mod and set_field > actions. > > As the flow_mods will cause ovs-vswitchd to delete megaflow and then add > same megaflow again. > So the rte_flow rule which is offload by this megaflow will delete and add. > Then the network flow will interrupt and miss upcall. > > Is this a bug? > > As the openflow controller is NOT controlled by us. > Which means the command is NOT controlled by us. > But the ovs-ofctl and ovs-vswitchd is code by us. > > So how to deal with this? > Is a good idea to change the behavior of fte_version_equals > - ofpacts_equal_stringwise, which could process all version? > > Thank you~ > > > > Simon Jones > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 6/6] build-aux: Enable flake8 checks for python extraction scripts.
These were recently updated to pass the checks, so should be added to the list in order to avoid regressions in the future. While at it, fixing the indentation. Signed-off-by: Ilya Maximets --- build-aux/automake.mk | 14 +- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/build-aux/automake.mk b/build-aux/automake.mk index 8d7e8ae19..d65b6da6c 100644 --- a/build-aux/automake.mk +++ b/build-aux/automake.mk @@ -21,8 +21,12 @@ EXTRA_DIST += \ build-aux/xml2nroff FLAKE8_PYFILES += \ -build-aux/dpdkstrip.py \ -build-aux/gen_ofp_field_decoders \ -build-aux/sodepends.py \ -build-aux/soexpand.py \ -build-aux/xml2nroff + build-aux/dpdkstrip.py \ + build-aux/extract-ofp-actions \ + build-aux/extract-ofp-errors \ + build-aux/extract-ofp-fields \ + build-aux/extract-ofp-msgs \ + build-aux/gen_ofp_field_decoders \ + build-aux/sodepends.py \ + build-aux/soexpand.py \ + build-aux/xml2nroff -- 2.41.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 5/6] build-aux/extract-ofp-msgs: Fix flake8 and syntax errors.
A few general style issues like extra spacing and line length,
semicolons at the end of the line and unused variable 'raw_types'.
And a few invalid escape sequences, which are not actual escape
sequences, but cause actual syntax warnings starting python 3.12
and will eventually become syntax errors [1]:
extract-ofp-msgs:118: SyntaxWarning: invalid escape sequence '\s'
m = re.match('\s+(?:OFPRAW_%s)(\d*)_([A-Z0-9_]+),?$' % type_,
These are fixed by converting to raw strings.
[1] https://docs.python.org/3/reference/lexical_analysis.html#escape-sequences
Signed-off-by: Ilya Maximets
---
build-aux/extract-ofp-msgs | 26 --
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/build-aux/extract-ofp-msgs b/build-aux/extract-ofp-msgs
index 6b3295cf6..c26ea1d35 100755
--- a/build-aux/extract-ofp-msgs
+++ b/build-aux/extract-ofp-msgs
@@ -24,6 +24,9 @@ OFPT11_STATS_REQUEST = 18
OFPT11_STATS_REPLY = 19
OFPST_VENDOR = 0x
+n_errors = 0
+
+
def decode_version_range(range):
if range in VERSION:
return (VERSION[range], VERSION[range])
@@ -35,6 +38,7 @@ def decode_version_range(range):
a, b = re.match(r'^([^-]+)-([^-]+)$', range).groups()
return (VERSION[a], VERSION[b])
+
def get_line():
global line
global line_number
@@ -43,16 +47,18 @@ def get_line():
if line == "":
fatal("unexpected end of input")
-n_errors = 0
+
def error(msg):
global n_errors
sys.stderr.write("%s:%d: %s\n" % (file_name, line_number, msg))
n_errors += 1
+
def fatal(msg):
error(msg)
sys.exit(1)
+
def usage():
argv0 = os.path.basename(sys.argv[0])
print('''\
@@ -65,6 +71,7 @@ only controls #line directives in the output.\
''' % {"argv0": argv0})
sys.exit(0)
+
def make_sizeof(s):
m = re.match(r'(.*) up to (.*)', s)
if m:
@@ -73,9 +80,8 @@ def make_sizeof(s):
else:
return "sizeof(%s)" % s
-def extract_ofp_msgs(output_file_name):
-raw_types = []
+def extract_ofp_msgs(output_file_name):
all_hdrs = {}
all_raws = {}
all_raws_order = []
@@ -108,15 +114,16 @@ def extract_ofp_msgs(output_file_name):
comment += ' %s' % line.lstrip('* \t').rstrip(' \t\r\n')
comment = comment[:-2].rstrip()
-m = re.match(r'([A-Z]+) ([-.+\d]+|) \((\d+)\): ([^.]+)\.$',
comment)
+m = re.match(
+r'([A-Z]+) ([-.+\d]+|) \((\d+)\): ([^.]+)\.$', comment
+)
if not m:
fatal("unexpected syntax between messages")
type_, versions, number, contents = m.groups()
number = int(number)
get_line()
-m = re.match('\s+(?:OFPRAW_%s)(\d*)_([A-Z0-9_]+),?$' % type_,
- line)
+m = re.match(r'\s+(?:OFPRAW_%s)(\d*)_([A-Z0-9_]+),?$' % type_, line)
if not m:
fatal("syntax error expecting OFPRAW_ enum")
vinfix, name = m.groups()
@@ -300,7 +307,7 @@ def extract_ofp_msgs(output_file_name):
for hdrs in r['hdrs']:
output.append("{ {0, NULL}, {%d, %d, %d, 0x%x, %d}, %s, 0 },"
% (hdrs + (raw,)))
-
+
output.append("};")
output.append("")
@@ -349,8 +356,8 @@ def extract_ofp_msgs(output_file_name):
% r["human_name"])
output.append("};")
-output.append("");
-output.append("static const char *type_names[] = {");
+output.append("")
+output.append("static const char *type_names[] = {")
for t in all_types:
output.append("\"%s\"," % t)
output.append("};")
@@ -378,4 +385,3 @@ if __name__ == '__main__':
for line in extract_ofp_msgs(sys.argv[2]):
print(line)
-
--
2.41.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 3/6] build-aux/extract-ofp-errors: Fix flake8 and syntax errors.
A few general style issues like extra spacing and lines being too long,
unused variable 'error_types', passing more arguments than a format
string has. And a few invalid escape sequences, which are not actual
escape sequences, but cause actual syntax warnings starting python 3.12
and will eventually become syntax errors [1]:
extract-ofp-errors:244: SyntaxWarning: invalid escape sequence '\.'
m = re.match('Expected: (.*)\.$', comment)
extract-ofp-errors:249: SyntaxWarning: invalid escape sequence '\.'
m = re.match('((?:.(?!\. ))+.)\.\s+(.*)$', comment)
extract-ofp-errors:256: SyntaxWarning: invalid escape sequence '\s'
m = re.match('\s+(?:OFPERR_([A-Z0-9_]+))(\s*=\s*OFPERR_OFS)?,',
extract-ofp-errors:265: SyntaxWarning: invalid escape sequence '\['
comments.append(re.sub('\[[^]]*\]', '', comment))
These are fixed by converting to raw strings.
[1] https://docs.python.org/3/reference/lexical_analysis.html#escape-sequences
Signed-off-by: Ilya Maximets
---
build-aux/extract-ofp-errors | 101 +--
1 file changed, 62 insertions(+), 39 deletions(-)
diff --git a/build-aux/extract-ofp-errors b/build-aux/extract-ofp-errors
index 2c3fbfc88..eeefccbee 100755
--- a/build-aux/extract-ofp-errors
+++ b/build-aux/extract-ofp-errors
@@ -22,6 +22,9 @@ tokenRe = "#?" + idRe + "|[0-9]+|."
inComment = False
inDirective = False
+n_errors = 0
+
+
def open_file(fn):
global fileName
global inputFile
@@ -30,6 +33,7 @@ def open_file(fn):
inputFile = open(fileName)
lineNumber = 0
+
def tryGetLine():
global inputFile
global line
@@ -38,10 +42,12 @@ def tryGetLine():
lineNumber += 1
return line != ""
+
def getLine():
if not tryGetLine():
fatal("unexpected end of input")
+
def getToken():
global token
global line
@@ -82,37 +88,43 @@ def getToken():
line = line[:-2] + inputFile.readline()
lineNumber += 1
if line == "":
-if token == None:
+if token is None:
fatal("unexpected end of input")
token = None
return False
-n_errors = 0
+
def error(msg):
global n_errors
sys.stderr.write("%s:%d: %s\n" % (fileName, lineNumber, msg))
n_errors += 1
+
def fatal(msg):
error(msg)
sys.exit(1)
+
def skipDirective():
getToken()
while token != '$':
getToken()
+
def isId(s):
-return re.match(idRe + "$", s) != None
+return re.match(idRe + "$", s) is not None
+
def forceId():
if not isId(token):
fatal("identifier expected")
+
def forceInteger():
-if not re.match('[0-9]+$', token):
+if not re.match(r'[0-9]+$', token):
fatal("integer expected")
+
def match(t):
if token == t:
getToken()
@@ -120,10 +132,12 @@ def match(t):
else:
return False
+
def forceMatch(t):
if not match(t):
fatal("%s expected" % t)
+
def parseTaggedName():
assert token in ('struct', 'union')
name = token
@@ -133,26 +147,26 @@ def parseTaggedName():
getToken()
return name
+
def print_enum(tag, constants, storage_class):
-print ("""
+print("""
%(storage_class)sconst char *
%(tag)s_to_string(uint16_t value)
{
switch (value) {\
""" % {"tag": tag,
- "bufferlen": len(tag) + 32,
"storage_class": storage_class})
for constant in constants:
-print ("case %s: return \"%s\";" % (constant, constant))
-print ("""\
+print("case %s: return \"%s\";" % (constant, constant))
+print("""\
}
return NULL;
-}\
-""" % {"tag": tag})
+}""")
+
def usage():
argv0 = os.path.basename(sys.argv[0])
-print ('''\
+print('''\
%(argv0)s, for extracting OpenFlow error codes from header files
usage: %(argv0)s ERROR_HEADER VENDOR_HEADER
@@ -167,6 +181,7 @@ The output is suitable for use as lib/ofp-errors.inc.\
''' % {"argv0": argv0})
sys.exit(0)
+
def extract_vendor_ids(fn):
global vendor_map
vendor_map = {}
@@ -174,7 +189,10 @@ def extract_vendor_ids(fn):
open_file(fn)
while tryGetLine():
-m =
re.match(r'#define\s+([A-Z0-9_]+)_VENDOR_ID\s+(0x[0-9a-fA-F]+|[0-9]+)', line)
+m = re.match(
+r'#define\s+([A-Z0-9_]+)_VENDOR_ID\s+(0x[0-9a-fA-F]+|[0-9]+)',
+line
+)
if not m:
continue
@@ -202,9 +220,8 @@ def extract_vendor_ids(fn):
% (id_, vendor_reverse_map[id_], name))
vendor_reverse_map[id_] = name
-def extract_ofp_errors(fn):
-error_types = {}
+def extract_ofp_errors(fn):
comments = []
names = []
domain = {}
@@ -220,14 +237,14 @@ def extract_ofp_errors(fn):
while True:
getLine()
-if re.match('enum ofperr', line):
+if re.match(r'enum ofperr', line):
break
while True:
getLine()
if
[ovs-dev] [PATCH 4/6] build-aux/extract-ofp-fields: Fix flake8 and syntax errors.
A few general style issues like extra spacing and block comment format.
And a few invalid escape sequences, which are not actual escape
sequences, but cause actual syntax warnings starting python 3.12 and
will eventually become syntax errors [1]:
extract-ofp-fields:323: SyntaxWarning: invalid escape sequence '\_'
"\_;\_;\_;\_;\_;\_\n",
extract-ofp-fields:332: SyntaxWarning: invalid escape sequence '\_'
s = """tab(;);
extract-ofp-fields:374: SyntaxWarning: invalid escape sequence '\-'
"""\
These are fixed by converting to raw strings. While doing that we
also have to remove all the now unnecessary escaping from actual
escape sequences like '\\'.
[1] https://docs.python.org/3/reference/lexical_analysis.html#escape-sequences
Signed-off-by: Ilya Maximets
---
build-aux/extract-ofp-fields | 50 ++--
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/build-aux/extract-ofp-fields b/build-aux/extract-ofp-fields
index 05d3e1df3..89d80c208 100755
--- a/build-aux/extract-ofp-fields
+++ b/build-aux/extract-ofp-fields
@@ -167,9 +167,9 @@ def make_nx_match(meta_flow_h):
print(oline)
-## ##
-## Documentation Generation ##
-## ##
+# #
+# Documentation Generation #
+# #
def field_to_xml(field_node, f, body, summary):
@@ -189,9 +189,9 @@ def field_to_xml(field_node, f, body, summary):
ovs_version = [int(x) for x in ovs_version_s.split(".")]
if min_ovs_version is None or ovs_version < min_ovs_version:
min_ovs_version = ovs_version
-summary += ["\\fB%s\\fR" % f["name"]]
+summary += [r"\fB%s\fR" % f["name"]]
if f["extra_name"]:
-summary += [" aka \\fB%s\\fR" % f["extra_name"]]
+summary += [r" aka \fB%s\fR" % f["extra_name"]]
summary += [";%d" % f["n_bytes"]]
if f["n_bits"] != 8 * f["n_bytes"]:
summary += [" (low %d bits)" % f["n_bits"]]
@@ -213,8 +213,8 @@ def field_to_xml(field_node, f, body, summary):
title = field_node.attributes["title"].nodeValue
body += [
-""".PP
-\\fB%s Field\\fR
+r""".PP
+\fB%s Field\fR
.TS
tab(;),nowarn;
l lx.
@@ -222,9 +222,9 @@ l lx.
% title
]
-body += ["Name:;\\fB%s\\fR" % f["name"]]
+body += [r"Name:;\fB%s\fR" % f["name"]]
if f["extra_name"]:
-body += [" (aka \\fB%s\\fR)" % f["extra_name"]]
+body += [r" (aka \fB%s\fR)" % f["extra_name"]]
body += ["\n"]
body += ["Width:;"]
@@ -320,7 +320,8 @@ def group_xml_to_nroff(group_node, fields):
"tab(;),nowarn;\n",
"l l l l l l l.\n",
"Name;Bytes;Mask;RW?;Prereqs;NXM/OXM Support\n",
-"\_;\_;\_;\_;\_;\_\n",
+r"\_;\_;\_;\_;\_;\_",
+"\n",
]
content += summary
content += [".TE\n"]
@@ -329,7 +330,7 @@ def group_xml_to_nroff(group_node, fields):
def make_oxm_classes_xml(document):
-s = """tab(;),nowarn;
+s = r"""tab(;),nowarn;
l l l.
Prefix;Vendor;Class
\_;\_;\_
@@ -367,42 +368,41 @@ def make_ovs_fields(meta_flow_h, meta_flow_xml):
doc = document.documentElement
global version
-if version == None:
+if version is None:
version = "UNKNOWN"
print(
-"""\
-'\\" tp
-.\\" -*- mode: troff; coding: utf-8 -*-
+r"""'\" tp
+.\" -*- mode: troff; coding: utf-8 -*-
.TH "ovs\-fields" 7 "%s" "Open vSwitch" "Open vSwitch Manual"
-.fp 5 L CR \\" Make fixed-width font available as \\fL.
+.fp 5 L CR \" Make fixed-width font available as \fL.
.de ST
. PP
. RS -0.15in
-. I "$1"
+. I "\\$1"
. RE
..
.de SU
. PP
-. I "$1"
+. I "\\$1"
..
.de IQ
. br
. ns
-. IP "$1"
+. IP "\\$1"
..
.de TQ
. br
. ns
-. TP "$1"
+. TP "\\$1"
..
.de URL
-$2 \\(laURL: $1 \\(ra$3
+\\$2 \(laURL: \\$1 \(ra\\$3
..
-.if \\n[.g] .mso www.tmac
+.if \n[.g] .mso www.tmac
.SH NAME
ovs\-fields \- protocol header fields in OpenFlow and Open vSwitch
.
@@ -460,9 +460,9 @@ ovs\-fields \- protocol header fields in OpenFlow and Open
vSwitch
print(output[i])
-## ##
-## Main Program ##
-## ##
+# #
+# Main Program #
+# #
if __name__ == "__main__":
argv0 = sys.argv[0]
--
2.41.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 2/6] build-aux/extract-ofp-actions: Fix flake8 and syntax errors.
A few general style issues like extra spacing and lines being too long.
Also, unused variables 'error_types' and 'comments'. And a few invalid
escape sequences, which are not actual escape sequences, but cause
actual syntax warnings starting python 3.12 and will eventually become
syntax errors [1]:
extract-ofp-actions:122: SyntaxWarning: invalid escape sequence '\['
comment = re.sub('\[[^]]*\]', '', comment)
extract-ofp-actions:125: SyntaxWarning: invalid escape sequence '\s'
m = re.match('([^:]+):\s+(.*)$', comment)
These are fixed by converting to raw strings.
[1] https://docs.python.org/3/reference/lexical_analysis.html#escape-sequences
Signed-off-by: Ilya Maximets
---
build-aux/extract-ofp-actions | 108 --
1 file changed, 64 insertions(+), 44 deletions(-)
diff --git a/build-aux/extract-ofp-actions b/build-aux/extract-ofp-actions
index 0aa6c65f3..cc5c1dbb0 100755
--- a/build-aux/extract-ofp-actions
+++ b/build-aux/extract-ofp-actions
@@ -17,27 +17,30 @@ version_map = {"1.0": 0x01,
version_reverse_map = dict((v, k) for (k, v) in version_map.items())
# Map from vendor name to the length of the action header.
-vendor_map = {"OF": (0x, 4),
+vendor_map = {"OF": (0x, 4),
"ONF": (0x4f4e4600, 10),
"NX": (0x2320, 10)}
# Basic types used in action arguments.
-types = {}
-types['uint8_t'] = {"size": 1, "align": 1, "ntoh": None, "hton": None}
-types['ovs_be16'] = {"size": 2, "align": 2, "ntoh": "ntohs", "hton": "htons"}
-types['ovs_be32'] = {"size": 4, "align": 4, "ntoh": "ntohl", "hton": "htonl"}
-types['ovs_be64'] = {"size": 8, "align": 8, "ntoh": "ntohll", "hton": "htonll"}
-types['uint16_t'] = {"size": 2, "align": 2, "ntoh": None, "hton": None}
-types['uint32_t'] = {"size": 4, "align": 4, "ntoh": None, "hton": None}
-types['uint64_t'] = {"size": 8, "align": 8, "ntoh": None, "hton": None}
+types = {
+"uint8_t" : {"size": 1, "align": 1, "ntoh": None, "hton": None},
+"ovs_be16": {"size": 2, "align": 2, "ntoh": "ntohs", "hton": "htons"},
+"ovs_be32": {"size": 4, "align": 4, "ntoh": "ntohl", "hton": "htonl"},
+"ovs_be64": {"size": 8, "align": 8, "ntoh": "ntohll", "hton": "htonll"},
+"uint16_t": {"size": 2, "align": 2, "ntoh": None, "hton": None},
+"uint32_t": {"size": 4, "align": 4, "ntoh": None, "hton": None},
+"uint64_t": {"size": 8, "align": 8, "ntoh": None, "hton": None},
+}
line = ""
-
+n_errors = 0
arg_structs = set()
+
def round_up(x, y):
return int((x + (y - 1)) / y) * y
+
def open_file(fn):
global file_name
global input_file
@@ -46,6 +49,7 @@ def open_file(fn):
input_file = open(file_name)
line_number = 0
+
def get_line():
global input_file
global line
@@ -56,16 +60,18 @@ def get_line():
fatal("unexpected end of input")
return line
-n_errors = 0
+
def error(msg):
global n_errors
sys.stderr.write("%s:%d: %s\n" % (file_name, line_number, msg))
n_errors += 1
+
def fatal(msg):
error(msg)
sys.exit(1)
+
def usage():
argv0 = os.path.basename(sys.argv[0])
print('''\
@@ -84,10 +90,8 @@ Commands:
''' % {"argv0": argv0})
sys.exit(0)
-def extract_ofp_actions(fn, definitions):
-error_types = {}
-comments = []
+def extract_ofp_actions(fn, definitions):
names = []
domain = {}
for code, size in vendor_map.values():
@@ -100,14 +104,14 @@ def extract_ofp_actions(fn, definitions):
while True:
get_line()
-if re.match('enum ofp_raw_action_type {', line):
+if re.match(r'enum ofp_raw_action_type {', line):
break
while True:
get_line()
if line.startswith('/*') or not line or line.isspace():
continue
-elif re.match('}', line):
+elif re.match(r'}', line):
break
if not line.lstrip().startswith('/*'):
@@ -119,10 +123,10 @@ def extract_ofp_actions(fn, definitions):
if line.startswith('/*') or not line or line.isspace():
fatal("unexpected syntax within action")
comment += ' %s' % line.lstrip('* \t').rstrip(' \t\r\n')
-comment = re.sub('\[[^]]*\]', '', comment)
+comment = re.sub(r'\[[^]]*\]', '', comment)
comment = comment[:-2].rstrip()
-m = re.match('([^:]+):\s+(.*)$', comment)
+m = re.match(r'([^:]+):\s+(.*)$', comment)
if not m:
fatal("unexpected syntax between actions")
@@ -147,7 +151,9 @@ def extract_ofp_actions(fn, definitions):
names.append(enum)
for dst in dsts.split(', '):
-m = re.match(r'([A-Z]+)([0-9.]+)(\+|-[0-9.]+)?(?:\((\d+)\))(?: is
deprecated \(([^)]+)\))?$', dst)
+m = re.match(
+r'([A-Z]+)([0-9.]+)(\+|-[0-9.]+)?(?:\((\d+)\))(?:'
+r' is deprecated \(([^)]+)\))?$', dst)
if not m:
fatal("%r: syntax error
[ovs-dev] [PATCH 0/6] build-aux: Fix flake8 and syntax issues with python 3.12.
The main goal of this patch set is to enable builds without warnings on systems with Python 3.12. Python 3.12 turned having invalid escape sequences into a SyntaxWarning. In the future it will become an actual syntax error: https://docs.python.org/3/reference/lexical_analysis.html#escape-sequences Fix all the instances to avoid these warnings. Adding all the scripts to a flake8 check to avoid re-introducing the issue. Fixing all the current flake8 warnings along the way. Note: before this patch set the build is not failing even with --enable-Werror, but the warnings are just printed out during the build. After this patch set, new warnings will fail the flake8 check and fail the build. Also, I tried to use black to fix these files more or less automatically, but it makes way too many unnecessary changes, so I decided to just fix files manually. Ilya Maximets (6): automake: Move build-aux EXTRA_DIST updates to their own file. build-aux/extract-ofp-actions: Fix flake8 and syntax errors. build-aux/extract-ofp-errors: Fix flake8 and syntax errors. build-aux/extract-ofp-fields: Fix flake8 and syntax errors. build-aux/extract-ofp-msgs: Fix flake8 and syntax errors. build-aux: Enable flake8 checks for python extraction scripts. build-aux/automake.mk| 24 -- build-aux/extract-ofp-actions| 108 --- build-aux/extract-ofp-errors | 101 +++-- build-aux/extract-ofp-fields | 50 ++--- build-aux/extract-ofp-msgs | 26 --- datapath-windows/include/automake.mk | 2 - include/automake.mk | 1 - include/openflow/automake.mk | 3 - lib/automake.mk | 4 - 9 files changed, 185 insertions(+), 134 deletions(-) -- 2.41.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 1/6] automake: Move build-aux EXTRA_DIST updates to their own file.
Otherwise it's hard to keep track of all the scripts we have. Signed-off-by: Ilya Maximets --- build-aux/automake.mk| 10 +- datapath-windows/include/automake.mk | 2 -- include/automake.mk | 1 - include/openflow/automake.mk | 3 --- lib/automake.mk | 4 5 files changed, 9 insertions(+), 11 deletions(-) diff --git a/build-aux/automake.mk b/build-aux/automake.mk index b9a77a51c..8d7e8ae19 100644 --- a/build-aux/automake.mk +++ b/build-aux/automake.mk @@ -1,11 +1,19 @@ EXTRA_DIST += \ build-aux/calculate-schema-cksum \ build-aux/cccl \ + build-aux/check-structs \ build-aux/cksum-schema-check \ build-aux/dist-docs \ build-aux/dpdkstrip.py \ - build-aux/generate-dhparams-c \ + build-aux/extract-odp-netlink-h \ + build-aux/extract-odp-netlink-macros-h \ + build-aux/extract-odp-netlink-windows-dp-h \ + build-aux/extract-ofp-actions \ + build-aux/extract-ofp-errors \ + build-aux/extract-ofp-fields \ + build-aux/extract-ofp-msgs \ build-aux/gen_ofp_field_decoders \ + build-aux/generate-dhparams-c \ build-aux/initial-tab-allowed-files \ build-aux/sodepends.py \ build-aux/soexpand.py \ diff --git a/datapath-windows/include/automake.mk b/datapath-windows/include/automake.mk index a354f007f..185a06b03 100644 --- a/datapath-windows/include/automake.mk +++ b/datapath-windows/include/automake.mk @@ -7,6 +7,4 @@ $(srcdir)/datapath-windows/include/OvsDpInterface.h: \ build-aux/extract-odp-netlink-windows-dp-h $(AM_V_GEN)sed -f $(srcdir)/build-aux/extract-odp-netlink-windows-dp-h < $< > $@ -EXTRA_DIST += $(srcdir)/build-aux/extract-odp-netlink-windows-dp-h - CLEANFILES += $(srcdir)/datapath-windows/include/OvsDpInterface.h diff --git a/include/automake.mk b/include/automake.mk index 1e3390ae0..a276c680b 100644 --- a/include/automake.mk +++ b/include/automake.mk @@ -8,7 +8,6 @@ include/odp-netlink-macros.h: include/odp-netlink.h \ build-aux/extract-odp-netlink-macros-h $(AM_V_GEN)sh -f $(srcdir)/build-aux/extract-odp-netlink-macros-h $< > $@ -EXTRA_DIST += build-aux/extract-odp-netlink-h build-aux/extract-odp-netlink-macros-h CLEANFILES += include/odp-netlink.h include/odp-netlink-macros.h include include/openflow/automake.mk diff --git a/include/openflow/automake.mk b/include/openflow/automake.mk index a1d75756c..820c09f84 100644 --- a/include/openflow/automake.mk +++ b/include/openflow/automake.mk @@ -22,6 +22,3 @@ HSTAMP_FILES = $(openflowinclude_HEADERS:.h=.hstamp) CLEANFILES += $(HSTAMP_FILES) ALL_LOCAL += $(HSTAMP_FILES) $(HSTAMP_FILES): build-aux/check-structs $(openflowinclude_HEADERS) - -EXTRA_DIST += build-aux/check-structs - diff --git a/lib/automake.mk b/lib/automake.mk index 24b0ffefe..1be13a420 100644 --- a/lib/automake.mk +++ b/lib/automake.mk @@ -642,7 +642,6 @@ lib/nx-match.inc: $(srcdir)/build-aux/extract-ofp-fields include/openvswitch/met $(AM_V_at)mv $@.tmp $@ lib/nx-match.lo: lib/nx-match.inc CLEANFILES += lib/meta-flow.inc lib/nx-match.inc -EXTRA_DIST += build-aux/extract-ofp-fields lib/ofp-actions.inc1: $(srcdir)/build-aux/extract-ofp-actions lib/ofp-actions.c $(AM_V_GEN)$(run_python) $< prototypes $(srcdir)/lib/ofp-actions.c > $@.tmp && mv $@.tmp $@ @@ -650,7 +649,6 @@ lib/ofp-actions.inc2: $(srcdir)/build-aux/extract-ofp-actions lib/ofp-actions.c $(AM_V_GEN)$(run_python) $< definitions $(srcdir)/lib/ofp-actions.c > $@.tmp && mv $@.tmp $@ lib/ofp-actions.lo: lib/ofp-actions.inc1 lib/ofp-actions.inc2 CLEANFILES += lib/ofp-actions.inc1 lib/ofp-actions.inc2 -EXTRA_DIST += build-aux/extract-ofp-actions lib/ofp-errors.inc: include/openvswitch/ofp-errors.h include/openflow/openflow-common.h \ $(srcdir)/build-aux/extract-ofp-errors @@ -660,14 +658,12 @@ lib/ofp-errors.inc: include/openvswitch/ofp-errors.h include/openflow/openflow-c mv $@.tmp $@ lib/ofp-errors.lo: lib/ofp-errors.inc CLEANFILES += lib/ofp-errors.inc -EXTRA_DIST += build-aux/extract-ofp-errors lib/ofp-msgs.inc: include/openvswitch/ofp-msgs.h $(srcdir)/build-aux/extract-ofp-msgs $(AM_V_GEN)$(run_python) $(srcdir)/build-aux/extract-ofp-msgs \ $(srcdir)/include/openvswitch/ofp-msgs.h $@ > $@.tmp && mv $@.tmp $@ lib/ofp-msgs.lo: lib/ofp-msgs.inc CLEANFILES += lib/ofp-msgs.inc -EXTRA_DIST += build-aux/extract-ofp-msgs # _server IDL OVSIDL_BUILT += lib/ovsdb-server-idl.c lib/ovsdb-server-idl.h lib/ovsdb-server-idl.ovsidl -- 2.41.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v3 1/4] Add global option for JSON output to ovs-appctl/dpctl.
Hi Ilya, thanks for sharing your thoughts, always appreciated! ☺️ Please find comments below. On 28.10.23 00:05, Ilya Maximets wrote: > On 10/27/23 23:51, Ilya Maximets wrote: >> On 10/26/23 13:44, Jakob Meng wrote: >>> On 25.10.23 11:37, jm...@redhat.com wrote: From: Jakob Meng For monitoring systems such as Prometheus it would be beneficial if OVS and OVS-DPDK would expose statistics in a machine-readable format. > BTW, there is no such separate thing as OVS-DPDK, it's just OVS. With OVS-DPDK I wanted to highlight one of the potential use cases of this change. It came up in discussions and the OVS codebase. Will remove it, if it is frowned upon, no worry. > This patch introduces support for different output formats to ovs-xxx tools. They gain a global option '-f,--format' which allows users to request JSON instead of plain-text for humans. An example call implemented in a later patch is 'ovs-appctl --format json dpif/show'. For that it is necessary to change the JSON-RPC API lib/unixctl.* which ovs-xxx tools use to communicate with ovs-vswitchd across unix sockets. It now allows to transport the requested output format besides the command and its args. This change has been implemented in a backward compatible way. One can use an updated client (ovs-appctl/dpctl) with an old server (ovs-vswitchd) and vice versa. Of course, JSON output only works when both sides have been updated. Previously, the command was copied to the 'method' parameter in JSON-RPC while the args were copied to the 'params' parameter. Without any other means to transport parameters via JSON-RPC left, the meaning of 'method' and 'params' had to be changed: 'method' will now be 'execute/v1' when an output format other than 'text' is requested. In that case, the first parameter of the JSON array 'params' will now be the designated command, the second one the output format and the rest will be command args. >>> Ilya brought up the question why I changed the meaning of 'method' and >>> 'params' instead of adding the output format as an addition argument to the >>> command arguments in 'params'. The server side would then interpret and >>> filter out this argument before passing the remaining arguments to the >>> command callbacks. >>> >>> I decided against this approach because the code would get more involved, >>> in particular we would have to implement option/argument parsing inside >>> process_command() in lib/unixctl.c. (Besides, using getopt*() functions in >>> a safe way would be difficult in general because their global state.) >>> The current implementation is based purely on JSON objects/arrays which are >>> nicely supported by OVS with functions from lib/json.*. >> I'm not sure I got this point, but see below. >> >>> Ilya also voiced concerns about the limited extensibility of the proposed >>> API. To fix this, this patch series could be tweaked in the follow way: >>> >>> (1.) Instead of passing the output format as second entry in the JSON array >>> 'params', turn the second entry into a JSON object (shash). > It has to be an array, not an object. Parameters are positional. > Unless you want to change API for every existing command and give > each argument a unique name. > > And that will not help with supporting older servers, because they > expect an array. JSON-RPC 1.0 defines 'params' as a "Array of objects to pass as arguments to the method." [0]. Putting JSON objects into that array is valid. Please have a look at the source code changes in unixctl.c. Hopefully it helps with understanding my approach: Basically, I changed the meaning of the JSON-RPC API. Previously, there was a 1-1 mapping between command+args and JSON-RPC's method+params. Now, command+args are part of 'params'. [0] https://www.jsonrpc.org/specification_v1 (After reading though your mail completely, I am thinking about reevaluating this decision as explained below.) > >>> The output format would be one entry in this JSON object, e.g. called >>> 'format'. >>> >>> The server (process_command() from lib/unixctl.c) will parse the content of >>> this JSON object into known options. Clients would only add non-default >>> options to this JSON object to keep compatibility with older servers. >>> Options which are supported by the server but not transferred via this JSON >>> object would be initialized with default values to keep compatibility with >>> older clients. Unknown entries would cause the server to return an error. >> This kind of implements what I had in mind, but in a slightly different >> manner. How about something like this: >> >> * ovs-appctl has a global parameter --format passed to it, not part of >>the requested command paramaters. (I think this is implemented in the >>code, below, but I didn't read very carefully.) Yes, --format is a global parameter. >> >> * ovs-appctl
[ovs-dev] [PATCH] netdev-offload-tc: Fix offload of tunnel key tp_src.
There is no TCA_TUNNEL_KEY_ENC_SRC_PORT in the kernel, so the offload
should not be attempted if OVS_TUNNEL_KEY_ATTR_TP_SRC is requested
by OVS. Current code just ignores the attribute in the tunnel(set())
action leading to a flow mismatch and potential incorrect datapath
behavior:
|tc(handler21)|DBG|tc flower compare failed action compare
...
Action 0 mismatch:
- Expected Action:
0010 01 00 00 00 00 00 00 00-00 00 00 00 00 ff 00 11
0020 c0 5b 17 c1 00 40 00 00-0a 01 00 6d 0a 01 01 12
0050 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0060 01 02 80 01 00 18 00 0b-00 00 00 00 00 00 00 00
...
- Received Action:
0010 01 00 00 00 00 00 00 00-00 00 00 00 00 ff 00 11
0020 00 00 17 c1 00 40 00 00-0a 01 00 6d 0a 01 01 12
0050 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0060 01 02 80 01 00 18 00 0b-00 00 00 00 00 00 00 00
...
In the tc_action dump above we can see the difference on the second
line. The action dumped from a kernel is missing 'c0 5b' - source
port for a tunnel(set()) action on the second line.
Removing the field from the tc_action_encap structure entirely to
avoid any potential confusion.
Note: In general, the source port number in the tunnel(set()) action
is not particularly useful for most tunnels, because they may just
ignore the value. Specs for Geneve and VXLAN suggest using a value
based on the headers of the inner packet as a source port.
And I'm not really sure how to make OVS to generate the action with
a source port in it, so the commit doesn't include the test case.
In vast majority of scenarios the source port doesn't actually end
up in the action itself.
Having a mismatch between the userspace and TC leads to constant
revalidation of the flow and warnings in the log, and might
potentially cause mishandling of the traffic, even though the impact
is unclear at the moment.
Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using tc
interface")
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2023-October/052744.html
Reported-by: Vladislav Odintsov
Signed-off-by: Ilya Maximets
---
lib/netdev-offload-tc.c | 4 +++-
lib/tc.h| 3 ++-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index b846a63c2..164c7eef6 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1627,7 +1627,9 @@ parse_put_flow_set_action(struct tc_flower *flower,
struct tc_action *action,
}
break;
case OVS_TUNNEL_KEY_ATTR_TP_SRC: {
-action->encap.tp_src = nl_attr_get_be16(tun_attr);
+/* There is no corresponding attribute in TC. */
+VLOG_DBG_RL(, "unsupported tunnel key attribute TP_SRC");
+return EOPNOTSUPP;
}
break;
case OVS_TUNNEL_KEY_ATTR_TP_DST: {
diff --git a/lib/tc.h b/lib/tc.h
index 06707ffa4..fdbcf4b7c 100644
--- a/lib/tc.h
+++ b/lib/tc.h
@@ -213,7 +213,8 @@ enum nat_type {
struct tc_action_encap {
bool id_present;
ovs_be64 id;
-ovs_be16 tp_src;
+/* ovs_be16 tp_src; Could have been here, but there is no
+ * TCA_TUNNEL_KEY_ENC_ attribute for it in the kernel. */
ovs_be16 tp_dst;
uint8_t tos;
uint8_t ttl;
--
2.41.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6 0/3] netdev: Sync and clean {get, set}_config() callbacks.
On 30.10.23 14:07, Ilya Maximets wrote: > On 10/30/23 10:54, Jakob Meng wrote: >> On 27.10.23 17:25, Kevin Traynor wrote: >>> On 27/10/2023 14:38, Ilya Maximets wrote: On 10/26/23 11:29, Jakob Meng wrote: > On 25.10.23 19:10, Ilya Maximets wrote: >> ... >> Maybe something along these lines: >> >> - ovs-appctl: >> * Output of 'dpctl/show' command no longer shows interface >> configuration >> status, only values of the actual configuration options, a.k.a. >> 'requested' configuration. The interface configuration status, >> a.k.a. 'configured' values, can be found in the 'status' column >> of >> the Interface table, i.e. with 'ovs-vsctl get interface <..> >> status'. >> Reported names adjusted accordingly. >> >> What do you think? > Simple and concise I will use that. However, we could add an example, > it could make it easier to grasp the meaning. I guess, the reference to 'configured' and 'requested' should be enough of an example. I think, if we would add an example, it should be very short, i.e. no longer than one extra line. This entry is already too long. > Should I put the NEWS change in one of the patches or in a separate patch > 4/4? I'd say since the netdev-dpdk changes are the most noticeable, add the NEWS change into netdev-dpdk patch, but move the patch itself to the end of the set, so the NEWS entry is correct. Having a NEWS entry as a separate patch is not a good practice as if impairs ability to git blame the NEWS file. >>> The plan above looks good to me too, >>> >>> Kevin. >> Thank you both, Ilya and Kevin! I have incorporated your suggestions into v7: >> >> https://patchwork.ozlabs.org/project/openvswitch/list/?series=379901 >> >> (cover letter includes list of what was changed, but it's not listed in >> patchwork) > It actually is in the patchwork, you just need to 'expand' the 'Series' after > navigating to one of the patches: > > https://patchwork.ozlabs.org/project/openvswitch/cover/20231030095000.720854-1-jm...@redhat.com/ > > Best regards, Ilya Maximets. Ah, nice! Thank you ☺️ ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6 0/3] netdev: Sync and clean {get, set}_config() callbacks.
On 10/30/23 10:54, Jakob Meng wrote: > On 27.10.23 17:25, Kevin Traynor wrote: >> On 27/10/2023 14:38, Ilya Maximets wrote: >>> On 10/26/23 11:29, Jakob Meng wrote: On 25.10.23 19:10, Ilya Maximets wrote: > ... > Maybe something along these lines: > > - ovs-appctl: > * Output of 'dpctl/show' command no longer shows interface > configuration > status, only values of the actual configuration options, a.k.a. > 'requested' configuration. The interface configuration status, > a.k.a. 'configured' values, can be found in the 'status' column of > the Interface table, i.e. with 'ovs-vsctl get interface <..> > status'. > Reported names adjusted accordingly. > > What do you think? Simple and concise I will use that. However, we could add an example, it could make it easier to grasp the meaning. >>> >>> I guess, the reference to 'configured' and 'requested' should be enough >>> of an example. I think, if we would add an example, it should be very >>> short, i.e. no longer than one extra line. This entry is already too long. >>> Should I put the NEWS change in one of the patches or in a separate patch 4/4? >>> >>> I'd say since the netdev-dpdk changes are the most noticeable, add the >>> NEWS change into netdev-dpdk patch, but move the patch itself to the >>> end of the set, so the NEWS entry is correct. >>> >>> Having a NEWS entry as a separate patch is not a good practice as if >>> impairs ability to git blame the NEWS file. >>> >> >> The plan above looks good to me too, >> >> Kevin. > > Thank you both, Ilya and Kevin! I have incorporated your suggestions into v7: > > https://patchwork.ozlabs.org/project/openvswitch/list/?series=379901 > > (cover letter includes list of what was changed, but it's not listed in > patchwork) It actually is in the patchwork, you just need to 'expand' the 'Series' after navigating to one of the patches: https://patchwork.ozlabs.org/project/openvswitch/cover/20231030095000.720854-1-jm...@redhat.com/ Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v3 2/4] python: Add global option for JSON output to Python tools.
On 30 Oct 2023, at 11:50, Jakob Meng wrote: > On 27.10.23 15:52, Eelco Chaudron wrote: >> On 25 Oct 2023, at 11:37, jm...@redhat.com wrote: >>> From: Jakob Meng >>> >>> This patch introduces support for different output formats to the >>> Python code, as did the previous commit for ovs-xxx tools like >>> 'ovs-appctl --format json dpif/show'. >>> In particular, tests/appctl.py gains a global option '-f,--format' >>> which allows users to request JSON instead of plain-text for humans. >>> >>> This patch does not yet pass the choosen output format to commands. >>> Doing so requires changes to all command_register() calls and all >>> command callbacks. To improve readibility those changes have been >>> split out into a follow up patch. Respectively, whenever an output >>> format other than 'text' is choosen for tests/appctl.py, the script >>> will fail. >>> >>> Reported-at: https://bugzilla.redhat.com/1824861 >>> Signed-off-by: Jakob Meng >> Note reviewed anything yet, but I get some flake8 errors when compiling your >> series: >> >> pends.py build-aux/soexpand.py build-aux/xml2nroff' && \ >> flake8 $src --select=H231,H232,H233,H238 && \ >> flake8 $src >> --ignore=E121,E123,E125,E126,E127,E128,E129,E131,E203,E722,W503,W504,F811,D,H,I >> && \ >> touch flake8-check >> python/ovs/unixctl/__init__.py:25:50: E261 at least two spaces before inline >> comment >> python/ovs/unixctl/__init__.py:30:9: E265 block comment should start with '# >> ' >> python/ovs/unixctl/__init__.py:73:68: E261 at least two spaces before inline >> comment >> python/ovs/unixctl/server.py:114:27: F821 undefined name 'params' >> python/ovs/unixctl/server.py:118:19: E111 indentation is not a multiple of 4 >> python/ovs/unixctl/server.py:148:51: E261 at least two spaces before inline >> comment >> python/ovs/unixctl/server.py:239:1: E302 expected 2 blank lines, found 1 >> make[1]: *** [Makefile:6795: flake8-check] Error 1 >> >> >> FYI if you install flake8 on your system, re-run ./configure it will pick >> these up as part of your compile process (and make sure you add >> --enable-Werror). >> > > Thank you, Eelco, for pointing this out. I was not aware that flake8 is > actually incorporated into the build if available. This reminds me of > something which bothers me about the OVS docs: > > It documents requirements and steps for building and installing OVS in great > detail (e.g. [0]). It lists all kinds of configure flags one could use and > software packages one could install and often gives rationale for it. But for > starters this is overwhelming! > > Please gimme a brief list of commands to get started on common OSes such as > Debian/Ubuntu and/or Fedora/RHEL. For example, put it at the beginning of > [0]. A list which reflects what you and other experienced OVS devs typically > use in their day to day work. Without experience in OVS, how else are > starters supposed to know which configure flags to use and which software to > install? I believe all the information you're looking for is actually included in the document you mentioned. I understand that it might appear a bit scattered, but the main issue here is that not everyone takes the time to thoroughly read through it. It's worth noting that all the essential development tools are listed right here: "If you're planning to make significant modifications to Open vSwitch, it's a good idea to install the following tools for better warnings." However, if you have time to submit a patch making the documentation more new user-friendly, that would be fantastic! Unfortunately, it seems that many people tend to overlook the finer details. For instance, very few actually follow through with running utilities/checkpatch.py, as suggested in the submitting-patches section. The following gives an idea of what the CI installs for Ubuntu; https://github.com/chaudron/ovs/tree/master/.ci or if you want my personal Fedora set (does not include sparse): https://github.com/chaudron/ovs_dp_test/blob/main/Vagrantfile Cheers, Eelco > [0] https://docs.openvswitch.org/en/latest/intro/install/general/ > > The benefit for you will hopefully be better patches, without basic > shortcomings such as flake8 failures. > > Wdyt? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v3 1/4] Add global option for JSON output to ovs-appctl/dpctl.
On 10/30/23 10:52, Eelco Chaudron wrote:
>
>
> On 27 Oct 2023, at 23:51, Ilya Maximets wrote:
>
>> On 10/26/23 13:44, Jakob Meng wrote:
>>> On 25.10.23 11:37, jm...@redhat.com wrote:
From: Jakob Meng
For monitoring systems such as Prometheus it would be beneficial if
OVS and OVS-DPDK would expose statistics in a machine-readable format.
This patch introduces support for different output formats to ovs-xxx
tools. They gain a global option '-f,--format' which allows users to
request JSON instead of plain-text for humans. An example call
implemented in a later patch is 'ovs-appctl --format json dpif/show'.
>
> I started to look at some of the code, but I guess some of the comments from
> Ilya could change the implementation, so I’ll stop for now.
>
> Please take a look at my commands from the v1 review, and incorporate them if
> still valid after potential changes (and flake8 warnings).
>
> Also, instead of TODO use use 'XXX' or 'FIXME' as mentioned in the coding
> style.
>
> On the ‘pretty print or not discussion’, I added the following:
>
> I think we should use the same option as ovs-vsctl has for the dbase.
>
> -f, --format=FORMAT set output formatting to FORMAT
> ("table", "html", "csv", or "json")
> --prettypretty-print JSON in output
>
> This looks like the following:
>
>ovs-vsctl --format=json list Open_vSwitch
> {"data":[[["uuid","8acf0aa8-7b3c-4b2a-9a15-6a4143ef63f2"],["uuid","630c4d69-c905-4645-ba59-66fd0bfa66c7"],32,["set",["netdev","system"]],["map",[]],"8.3.1",true,"DPDK
>
> 22.11.1",["map",[["hostname","wsfd-advnetlab224.anl.lab.eng.bos.redhat.com"],["rundir","/var/run/openvswitch"],["system-id","5ee96c43-4823-4456-9bc3-9616c1acdce3"]]],["set",["bareudp","dpdk","dpdkvhostuser","dpdkvhostuserclient","erspan","geneve","gre","gtpu","internal","ip6erspan","ip6gre","lisp","patch","stt","system","tap","vxlan"]],["set",[]],32,["map",[["dpdk-init","true"],["dpdk-socket-mem","2048"],["pmd-cpu-mask","0x40004000"]]],"3.1.4",["set",[]],["map",[]],"rhel","9.2"]],"headings":["_uuid","bridges","cur_cfg","datapath_types","datapaths","db_version","dpdk_initialized","dpdk_version","external_ids","iface_types","manager_options","next_cfg","other_config","ovs_version","ssl","statistics","system_type","system_version"]}
> [wsfd-advnetlab224:~]$ ovs-vsctl --format=json list Open_vSwitch
>
> [wsfd-advnetlab224:~]$ ovs-vsctl --format=json --pretty list Open_vSwitch
> {
> "data": [
> ...
> }
>
For that it is necessary to change the JSON-RPC API lib/unixctl.*
which ovs-xxx tools use to communicate with ovs-vswitchd across unix
sockets. It now allows to transport the requested output format
besides the command and its args. This change has been implemented in
a backward compatible way. One can use an updated client
(ovs-appctl/dpctl) with an old server (ovs-vswitchd) and vice versa.
Of course, JSON output only works when both sides have been updated.
Previously, the command was copied to the 'method' parameter in
JSON-RPC while the args were copied to the 'params' parameter. Without
any other means to transport parameters via JSON-RPC left, the meaning
of 'method' and 'params' had to be changed: 'method' will now be
'execute/v1' when an output format other than 'text' is requested. In
that case, the first parameter of the JSON array 'params' will now be
the designated command, the second one the output format and the rest
will be command args.
>>>
>>> Ilya brought up the question why I changed the meaning of 'method' and
>>> 'params' instead of adding the output format as an addition argument to the
>>> command arguments in 'params'. The server side would then interpret and
>>> filter out this argument before passing the remaining arguments to the
>>> command callbacks.
>>>
>>> I decided against this approach because the code would get more involved,
>>> in particular we would have to implement option/argument parsing inside
>>> process_command() in lib/unixctl.c. (Besides, using getopt*() functions in
>>> a safe way would be difficult in general because their global state.)
>>> The current implementation is based purely on JSON objects/arrays which are
>>> nicely supported by OVS with functions from lib/json.*.
>>
>> I'm not sure I got this point, but see below.
>>
>>>
>>> Ilya also voiced concerns about the limited extensibility of the proposed
>>> API. To fix this, this patch series could be tweaked in the follow way:
>>>
>>> (1.) Instead of passing the output format as second entry in the JSON array
>>> 'params', turn the second entry into a JSON object (shash). The output
>>> format would be one entry in this JSON object, e.g. called 'format'.
>>>
>>> The server (process_command() from lib/unixctl.c) will parse the content of
>>> this JSON object into known options. Clients would only
Re: [ovs-dev] [PATCH v3] editorconfig: Remove [*] section and trim_trailing_whitespace.
On Mon, Oct 30, 2023 at 09:54:58AM +0100, Jakob Meng wrote:
> Hi Aaron,
>
> first and foremost: I am fully committed to make our OVS community more
> inclusive. Replacing terminology that promotes racial and cultural bias is
> one of many (!) steps in achieving that. For the objective there is a broad
> consensus, on the practical implementation not so much, it seems.
>
> IBM Academy of Technology conducted a evaluation of IT terminology and
> published a guideline on which terms to replace including rationale [0]:
>
> [0] https://github.com/IBM/IBMInclusiveITLanguage
>
> For example, it replaces {black,white}list with {block,allow}list because:
>
> "As a pair, "blacklist" and "whitelist" promote racial bias by implying that
> black is bad and white is good. When the terms 'white' or 'black' are used in
> a context where white is represented as good or black is represented as bad,
> this usage reinforces a model that promotes racial bias." [0]
>
> However, for "white space" there is "No change recommended" because:
>
> "This term is not based on a good/bad binary and so does not fall under our
> guiding principle for black and white terms (see "blacklist")." [0]
>
>
> Precision of expression is part of inclusive language. In my commit message I
> am using the term "white space" because that is exactly the technical term
> used in Unicode [1], editors and our ecosystem. Using other words such as
> empty-space or blank-space could be confusing for readers.
>
> [1] https://en.wikipedia.org/wiki/Whitespace_character
>
> Although disturbed at first, I appreciate that you brought this up. We should
> discuss this with Simon and others to get a common understanding of what
> inclusive language is in practice. I assume you do not advocate for banning
> the words white and black completely from the English language, do you?
FWIIW, based on the above I using the term whitespace seems ok to me.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] flow: Consistent VXLAN UDP src ports for fragmented packets
On 10/30/23 11:03, Hemanth Aramadaka wrote:
> Hi Ilya,
>
> The changes are not required for flow_hash_5tuple() function. I have added
> debug statements, and we are not hitting this function as part of this code
> flow.
It doesn't matter if you hit this particular function in your scenario
or not. All the hash functions must produce the same result.
Best regards, Ilya Maximets.
>
> Thanks,
> Hemanth.
>
> -Original Message-
> From: Ilya Maximets
> Sent: Monday, June 26, 2023 7:59 PM
> To: Hemanth Aramadaka ; 'Simon Horman'
> ; ovs-dev@openvswitch.org
> Cc: i.maxim...@ovn.org
> Subject: Re: [ovs-dev] [PATCH] flow: Consistent VXLAN UDP src ports for
> fragmented packets
>
> On 6/26/23 08:59, Hemanth Aramadaka via dev wrote:
>> Hi Simon,
>>
>> Sorry for the late response. Yes, the changes are specific to IPV6
>> protocol only.
>
> Please, fix the flow_hash_5tuple() function as well. All variants of the
> same hash function should give the same result. For some reason you just
> removed the fix for this function from the patch instead of addressing a
> review comment.
>
> And add the unit test for the issue.
>
> Thanks.
> Best regards, Ilya Maximets.
>
>>
>> Thanks,
>> Hemanth.
>>
>> -Original Message-
>> From: Simon Horman
>> Sent: Friday, January 20, 2023 8:52 PM
>> To: ovs-dev@openvswitch.org
>> Cc: Hemanth Aramadaka
>> Subject: Re: [ovs-dev] [PATCH] flow: Consistent VXLAN UDP src ports
>> for fragmented packets
>>
>> On Thu, Jan 05, 2023 at 06:55:03PM +0530, Hemanth Aramadaka via dev wrote:
>>> Issue:
>>>
>>> The src-port for UDP is based on RSS hash in the packet metadata.
>>> In case of packets coming from VM it will be 5-tuple, if available,
>>> otherwise just IP addresses.If the VM fragments a large IP packet and
>>> sends the fragments to ovs, only the first fragment will contain the
>>> L4 header. Therefore, the first fragment and subsequent fragments get
>>> different UDP src ports in the outgoing VXLAN header.This can lead to
>>> fragment re-ordering in the fabric as packet will take different
>>> paths.
>>>
>>> Fix:
>>>
>>> Intention of this is to avoid fragment packets taking different paths.
>>> For example, due to presence of firewalls, fragment packets will take
>>> different paths and will get dropped.To avoid this we ignore the L4
>>> header during hash calculation only in the case of fragmented packets.
>>>
>>> Signed-off-by: Hemanth Aramadaka
>>> ---
>>> lib/flow.c | 18 +++---
>>> 1 file changed, 15 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/lib/flow.c b/lib/flow.c
>>> index c3a3aa3ce..4f4197b1c 100644
>>> --- a/lib/flow.c
>>> +++ b/lib/flow.c
>>> @@ -1018,7 +1018,9 @@ miniflow_extract(struct dp_packet *packet,
>>> struct
>> miniflow *dst)
>>> miniflow_push_be16(mf, ct_tp_src, ct_tp_src);
>>> miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst);
>>> if (dl_type == htons(ETH_TYPE_IP)) {
>>> -dp_packet_update_rss_hash_ipv4_tcp_udp(packet);
>>> +if (!(nw_frag & FLOW_NW_FRAG_MASK)) {
>>> +
>> dp_packet_update_rss_hash_ipv4_tcp_udp(packet);
>>> +}
>>> } else if (dl_type == htons(ETH_TYPE_IPV6)) {
>>> dp_packet_update_rss_hash_ipv6_tcp_udp(packet);
>>> }
>>> @@ -1033,7 +1035,9 @@ miniflow_extract(struct dp_packet *packet,
>>> struct
>> miniflow *dst)
>>> miniflow_push_be16(mf, ct_tp_src, ct_tp_src);
>>> miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst);
>>> if (dl_type == htons(ETH_TYPE_IP)) {
>>> -dp_packet_update_rss_hash_ipv4_tcp_udp(packet);
>>> +if (!(nw_frag & FLOW_NW_FRAG_MASK)) {
>>> +dp_packet_update_rss_hash_ipv4_tcp_udp(packet);
>>> +}
>>> } else if (dl_type == htons(ETH_TYPE_IPV6)) {
>>> dp_packet_update_rss_hash_ipv6_tcp_udp(packet);
>>> }
>>> @@ -2248,7 +2252,7 @@ miniflow_hash_5tuple(const struct miniflow
>>> *flow, uint32_t basis)
>>>
>>> if (flow) {
>>> ovs_be16 dl_type = MINIFLOW_GET_BE16(flow, dl_type);
>>> -uint8_t nw_proto;
>>> +uint8_t nw_proto, nw_frag;
>>>
>>> if (dl_type == htons(ETH_TYPE_IPV6)) {
>>> struct flowmap map = FLOWMAP_EMPTY_INITIALIZER; @@
>>> -2270,6 +2274,14 @@ miniflow_hash_5tuple(const struct miniflow *flow,
>>> uint32_t basis)
>>>
>>> nw_proto = MINIFLOW_GET_U8(flow, nw_proto);
>>> hash = hash_add(hash, nw_proto);
>>> +
>>> +/* Skip l4 header fields if IP packet is fragmented since
>>> + * only first fragment will carry l4 header.
>>> + */
>>> +nw_frag = MINIFLOW_GET_U8(flow, nw_frag);
>>> +if (nw_frag & FLOW_NW_FRAG_MASK) {
>>> +goto out;
>>> +}
>>
>> Maybe I am reading
Re: [ovs-dev] [PATCH v5 1/3] netdev-linux: Use ethtool to detect offload support.
On Mon, Oct 30, 2023 at 02:58:36AM -0400, Mike Pattrick wrote:
> Currently when userspace-tso is enabled, netdev-linux interfaces will
> indicate support for all offload flags regardless of interface
> configuration. This patch checks for which offload features are enabled
> during netdev construction.
>
> Signed-off-by: Mike Pattrick
...
> @@ -2381,6 +2376,153 @@ netdev_internal_get_stats(const struct netdev
> *netdev_,
> return error;
> }
>
> +static int
> +netdev_linux_read_stringset_info(struct netdev_linux *netdev, uint32_t *len)
> +{
> +/*
> +struct {
> +union {
> +struct ethtool_sset_info hdr;
> +struct {
> +uint64_t pad[2];
> +uint32_t sset_len[1];
> +};
> +};
> +} sset_info;
> +*/
Hi Mike,
maybe the comment above is an artifact that should be removed?
> +union {
> +struct ethtool_sset_info hdr;
> +struct {
> +uint64_t pad[2];
> +uint32_t sset_len[1];
> +};
> +} sset_info;
> +
> +sset_info.hdr.cmd = ETHTOOL_GSSET_INFO;
> +sset_info.hdr.reserved = 0;
> +sset_info.hdr.sset_mask = 1ULL << ETH_SS_FEATURES;
> +
> +int error = netdev_linux_do_ethtool(netdev->up.name,
> +(struct ethtool_cmd *)_info,
> +ETHTOOL_GSSET_INFO, "ETHTOOL_GSSET_INFO");
> +if (error) {
> +return error;
> +}
> +*len = sset_info.sset_len[0];
> +return 0;
> +}
> +
> +
> +static int
> +netdev_linux_read_definitions(struct netdev_linux *netdev,
> + struct ethtool_gstrings **pstrings)
> +{
> +int error = 0;
> +struct ethtool_gstrings *strings = NULL;
> +uint32_t len = 0;
Reverse xmas tree please.
> +
> +error = netdev_linux_read_stringset_info(netdev, );
> +if (error || !len) {
> +return error;
> +}
> +strings = xcalloc(1, sizeof(*strings) + len * ETH_GSTRING_LEN);
> +if (!strings) {
> +return ENOMEM;
> +}
> +
> +strings->cmd = ETHTOOL_GSTRINGS;
> +strings->string_set = ETH_SS_FEATURES;
> +strings->len = len;
> +error = netdev_linux_do_ethtool(netdev->up.name,
> +(struct ethtool_cmd *) strings,
> +ETHTOOL_GSTRINGS, "ETHTOOL_GSTRINGS");
> +if (error) {
> +goto out;
> +}
> +
> +for (int i = 0; i < len; i++) {
> +strings->data[(i + 1) * ETH_GSTRING_LEN - 1] = 0;
> +}
> +
> +*pstrings = strings;
> +
> +return 0;
> +out:
> +*pstrings = NULL;
> +free(strings);
> +return error;
> +}
> +
> +static void
> +netdev_linux_set_ol(struct netdev *netdev_)
> +{
> +struct netdev_linux *netdev = netdev_linux_cast(netdev_);
> +struct ethtool_gstrings *names = NULL;
> +struct ethtool_gfeatures *features = NULL;
> +int error;
Ditto.
...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 1/1] ofproto-dpif-trace: Improve conjunctive match tracing.
On Mon, Oct 30, 2023 at 02:00:04PM +0900, Nobuhiro MIKI wrote: > A conjunctive flow consists of two or more multiple flows with > conjunction actions. When input to the ofproto/trace command > matches a conjunctive flow, it outputs flows of all dimensions. > > Signed-off-by: Nobuhiro MIKI Thanks Miki-san, I see that Ilya's review of v3 has been addressed. And we agreed that addressing excessive number of function parameters can be deferred. This one looks good to me. Acked-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v3 0/4] Add global option to output JSON from ovs-appctl cmds.
On 30.10.23 11:19, Eelco Chaudron wrote:
> On 30 Oct 2023, at 11:07, Jakob Meng wrote:
>
>> On 27.10.23 16:27, Eelco Chaudron wrote:
>>> On 25 Oct 2023, at 11:37, jm...@redhat.com wrote:
From: Jakob Meng
Add global option to output JSON from ovs-appctl cmds.
This patch is an update of [0] with the following major changes:
* The JSON-RPC API change is now backward compatible. One can use an
updated client (ovs-appctl/dpctl) with an old server (ovs-vswitchd)
and vice versa. Of course, JSON output only works when both are
updated.
* tests/pmd.at from forth patch now features an example of how the
output looks like when a command does not support JSON output.
* The patch has been split into a series of four. The first patch
introduces the '-f,--format' option for ovs-appctl/ovs-dpctl and
necessary changes to the JSON-RPC API. It does not yet pass the
output format to individual commands because that requires a lot
of changes. Those changes have been split out into the third patch
to increase readability of the series.
* The second patch introduces equivalent changes to the Python files.
* The third patch moves all commands to the updated functions in
lib/unixctl.*, in particular unixctl_command_register() and the
unixctl_cb_func type, as well as their Python counterparts. The
output is still text-only (no json) for all commands.
* The forth patch shows how JSON output could be implemented using
'dpif/show' as an example.
The following paragraphs are taken from the previous patch revision
and have been updated to changes mentioned above.
For monitoring systems such as Prometheus it would be beneficial if OVS
and OVS-DPDK would expose statistics in a machine-readable format.
Several approaches like UNIX socket, OVSDB queries and JSON output from
ovs-xxx tools have been proposed [2],[3]. This proof of concept
describes one way how ovs-xxx tools could output JSON in addition to
plain-text for humans.
This patch follows an alternative approach to RFC [1] which
implemented JSON output as a separate option for each command like
'dpif/show'. The option was called '-o|--output' in the latter. It
has been renamed to '-f,--format' because ovs-appctl already has a
short option '-o' which prints the available ovs-appctl options
('--option'). The new option name '-f,--format' is in line with
ovsdb-client where it controls output formatting, too.
An example call would be 'ovs-appctl --format json dpif/show' as
shown in tests/pmd.at of the forth patch. By default, the output
format is plain-text as before.
With this patch, all commands announce their support for output
formats when being registered with unixctl_command_register() from
lib/unixctl.*, e.g. OVS_OUTPUT_FMT_TEXT and/or OVS_OUTPUT_FMT_JSON.
When a requested output format is not supported by a command, then
process_command() in lib/unixctl.c will return an error. This is an
advantage over the previous approach [1] where each command would have
to parse the output format option and handle requests for unsupported
output formats on its own.
The question whether JSON output should be pretty printed and sorted
remains. In most cases it would be unnecessary because machines
consume the output or humans could use jq for pretty printing. However,
it would make tests more readable (for humans) without having to use jq
(which would require us to introduce a dependency on jq).
>>> Hi Jakob,
>>>
>>> I had some code-related comments on V1, which I do not see addressed or
>>> replied to. Did you miss them? Anyway, I’ll go over my old review notes,
>>> and add them to the split-up patch.
>> Which one did I miss?
>>
>> The note on JSON pretty printing I left here as a remainder for the
>> discussion on last thursday. With the consensus on adding an extra flag for
>> pretty printing I will remove it in the next patch version.
>>
>> You asked about freeing args->target which is done a couple of lines above
>> in "cmdl_args_destroy()".
>>
>> The rest of your comments has been incorporated in v3 in one way or another
> Maybe I did not go over it in to much detail (it was Friday afternoon), but I
> missed the first comment:
>
>
>>> +const char *
>>> +ovs_output_fmt_to_string(enum ovs_output_fmt fmt)
>>> +{
>>> +switch (fmt) {
>>> +case OVS_OUTPUT_FMT_TEXT:
>>> +return "text";
>>> +
>>> +case OVS_OUTPUT_FMT_JSON:
>>> +return "json";
>>> +
>>> +default:
>>> +return NULL;
>> Returning NULL might cause a problem in the json_string_create() below, how
>> about returning “text” as this is the default value?
You discovered an bug here. I addressed it by fixing the
ovs_output_fmt_to_json() function instead: It will
Re: [ovs-dev] [RFC v3 2/4] python: Add global option for JSON output to Python tools.
On 27.10.23 15:52, Eelco Chaudron wrote: > On 25 Oct 2023, at 11:37, jm...@redhat.com wrote: >> From: Jakob Meng >> >> This patch introduces support for different output formats to the >> Python code, as did the previous commit for ovs-xxx tools like >> 'ovs-appctl --format json dpif/show'. >> In particular, tests/appctl.py gains a global option '-f,--format' >> which allows users to request JSON instead of plain-text for humans. >> >> This patch does not yet pass the choosen output format to commands. >> Doing so requires changes to all command_register() calls and all >> command callbacks. To improve readibility those changes have been >> split out into a follow up patch. Respectively, whenever an output >> format other than 'text' is choosen for tests/appctl.py, the script >> will fail. >> >> Reported-at: https://bugzilla.redhat.com/1824861 >> Signed-off-by: Jakob Meng > Note reviewed anything yet, but I get some flake8 errors when compiling your > series: > > pends.py build-aux/soexpand.py build-aux/xml2nroff' && \ > flake8 $src --select=H231,H232,H233,H238 && \ > flake8 $src > --ignore=E121,E123,E125,E126,E127,E128,E129,E131,E203,E722,W503,W504,F811,D,H,I > && \ > touch flake8-check > python/ovs/unixctl/__init__.py:25:50: E261 at least two spaces before inline > comment > python/ovs/unixctl/__init__.py:30:9: E265 block comment should start with '# ' > python/ovs/unixctl/__init__.py:73:68: E261 at least two spaces before inline > comment > python/ovs/unixctl/server.py:114:27: F821 undefined name 'params' > python/ovs/unixctl/server.py:118:19: E111 indentation is not a multiple of 4 > python/ovs/unixctl/server.py:148:51: E261 at least two spaces before inline > comment > python/ovs/unixctl/server.py:239:1: E302 expected 2 blank lines, found 1 > make[1]: *** [Makefile:6795: flake8-check] Error 1 > > > FYI if you install flake8 on your system, re-run ./configure it will pick > these up as part of your compile process (and make sure you add > --enable-Werror). > Thank you, Eelco, for pointing this out. I was not aware that flake8 is actually incorporated into the build if available. This reminds me of something which bothers me about the OVS docs: It documents requirements and steps for building and installing OVS in great detail (e.g. [0]). It lists all kinds of configure flags one could use and software packages one could install and often gives rationale for it. But for starters this is overwhelming! Please gimme a brief list of commands to get started on common OSes such as Debian/Ubuntu and/or Fedora/RHEL. For example, put it at the beginning of [0]. A list which reflects what you and other experienced OVS devs typically use in their day to day work. Without experience in OVS, how else are starters supposed to know which configure flags to use and which software to install? [0] https://docs.openvswitch.org/en/latest/intro/install/general/ The benefit for you will hopefully be better patches, without basic shortcomings such as flake8 failures. Wdyt? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3] ci: Remove '--recheck' in CI.
On 10/27/23 18:05, Numan Siddique wrote:
> On Fri, Jul 14, 2023 at 2:12 AM Ales Musil wrote:
>>
>> On Thu, Jul 13, 2023 at 12:52 PM Dumitru Ceara wrote:
>>
>>> If we want to catch new failures faster we have a better chance if CI
>>> doesn't auto-retry (once).
>>>
>>> There are some tests that are still "unstable" and fail every now and
>>> then. In order to reduce the number of false negatives keep the
>>> --recheck for them. To achieve that we use a new macro, TAG_UNSTABLE,
>>> to tag all these tests. The list of "unstable" tests is compiled based
>>> on the following discussion:
>>> https://mail.openvswitch.org/pipermail/ovs-dev/2023-June/405465.html
>>>
>>> In order to avoid new GitHub actions jobs, we re-purpose the last job of
>>> each target type to also run the unstable tests. These jobs were
>>> already running less tests than others so the additional run time should
>>> not be an issue.
>>>
>>> Signed-off-by: Dumitru Ceara
>>> ---
>>> V3:
>>> - Addressed Ales' comment:
>>> - pass RECHECK to the container running the tests in ci.sh
>>> V2:
>>> - Addressed Ales' comments:
>>> - always run stable and unstable tests before declaring pass/fail
>>> Changes in v1 (since RFC):
>>> - kept recheck for unstable tests
>>> - introduced TAG_UNSTABLE
>>> - changed test.yml to run unstable tests in the last batch of every
>>> test target type.
>>> ---
>>> .ci/ci.sh | 2 +-
>>> .ci/linux-build.sh | 76 ++
>>> .github/workflows/test.yml | 15
>>> tests/ovn-ic.at| 1 +
>>> tests/ovn-ipsec.at | 1 +
>>> tests/ovn-macros.at| 5 +++
>>> tests/ovn-northd.at| 1 +
>>> tests/ovn-performance.at | 1 +
>>> tests/ovn.at | 13 +++
>>> 9 files changed, 92 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/.ci/ci.sh b/.ci/ci.sh
>>> index 10f11939c5..3a7ee97696 100755
>>> --- a/.ci/ci.sh
>>> +++ b/.ci/ci.sh
>>> @@ -101,7 +101,7 @@ function run_tests() {
>>> && \
>>> ARCH=$ARCH CC=$CC LIBS=$LIBS OPTS=$OPTS TESTSUITE=$TESTSUITE \
>>> TEST_RANGE=$TEST_RANGE SANITIZERS=$SANITIZERS DPDK=$DPDK \
>>> -./.ci/linux-build.sh
>>> +RECHECK=$RECHECK UNSTABLE=$UNSTABLE ./.ci/linux-build.sh
>>> "
>>> }
>>>
>>> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
>>> index 5a79a52daf..4c5361f3ca 100755
>>> --- a/.ci/linux-build.sh
>>> +++ b/.ci/linux-build.sh
>>> @@ -9,6 +9,7 @@ COMMON_CFLAGS=""
>>> OVN_CFLAGS=""
>>> OPTS="$OPTS --enable-Werror"
>>> JOBS=${JOBS:-"-j4"}
>>> +RECHECK=${RECHECK:-"no"}
>>>
>>> function install_dpdk()
>>> {
>>> @@ -99,6 +100,17 @@ function configure_clang()
>>> COMMON_CFLAGS="${COMMON_CFLAGS}
>>> -Wno-error=unused-command-line-argument"
>>> }
>>>
>>> +function run_tests()
>>> +{
>>> +if ! make distcheck CFLAGS="${COMMON_CFLAGS} ${OVN_CFLAGS}" $JOBS \
>>> +TESTSUITEFLAGS="$JOBS $TEST_RANGE" RECHECK=$RECHECK
>>> +then
>>> +# testsuite.log is necessary for debugging.
>>> +cat */_build/sub/tests/testsuite.log
>>> +return 1
>>> +fi
>>> +}
>>> +
>>> function execute_tests()
>>> {
>>> # 'distcheck' will reconfigure with required options.
>>> @@ -106,27 +118,61 @@ function execute_tests()
>>> configure_ovn
>>>
>>> export DISTCHECK_CONFIGURE_FLAGS="$OPTS"
>>> -if ! make distcheck CFLAGS="${COMMON_CFLAGS} ${OVN_CFLAGS}" $JOBS \
>>> -TESTSUITEFLAGS="$JOBS $TEST_RANGE" RECHECK=yes
>>> -then
>>> -# testsuite.log is necessary for debugging.
>>> -cat */_build/sub/tests/testsuite.log
>>> +
>>> +local stable_rc=0
>>> +local unstable_rc=0
>>> +
>>> +if ! SKIP_UNSTABLE=yes run_tests; then
>>> +stable_rc=1
>>> +fi
>>> +
>>> +if [ "$UNSTABLE" ]; then
>>> +if ! SKIP_UNSTABLE=no TEST_RANGE="-k unstable" RECHECK=yes \
>>> +run_tests; then
>>> +unstable_rc=1
>>> +fi
>>> +fi
>>> +
>>> +if [[ $stable_rc -ne 0 ]] || [[ $unstable_rc -ne 0 ]]; then
>>> exit 1
>>> fi
>>> }
>>>
>>> +function run_system_tests()
>>> +{
>>> +local type=$1
>>> +local log_file=$2
>>> +
>>> +if ! sudo make $JOBS $type TESTSUITEFLAGS="$TEST_RANGE" \
>>> +RECHECK=$RECHECK; then
>>> +# $log_file is necessary for debugging.
>>> +cat tests/$log_file
>>> +return 1
>>> +fi
>>> +}
>>> +
>>> function execute_system_tests()
>>> {
>>> - type=$1
>>> - log_file=$2
>>> -
>>> - configure_ovn $OPTS
>>> - make $JOBS || { cat config.log; exit 1; }
>>> - if ! sudo make $JOBS $type TESTSUITEFLAGS="$TEST_RANGE"
>>> RECHECK=yes; then
>>> - # $log_file is necessary for debugging.
>>> - cat tests/$log_file
>>> - exit 1
>>> - fi
>>> +configure_ovn $OPTS
>>> +make $JOBS || { cat config.log; exit 1; }
>>> +
>>> +local stable_rc=0
>>> +local unstable_rc=0
>>> +
>>> +if !
Re: [ovs-dev] [PATCH ovn v3] controller: Don't artificially limit group and meter IDs to 16bit.
On 10/27/23 17:57, Numan Siddique wrote:
> On Thu, Oct 26, 2023 at 7:37 AM Dumitru Ceara wrote:
>>
>> OVS actually supports way more. Detect the real number of groups and
>> meters instead. To avoid preallocating huge bitmaps for the IDs,
>> switch to id-pool instead (as suggested by Ilya).
>>
>> Reported-at: https://issues.redhat.com/browse/FDP-70
>> Suggested-by: Ilya Maximets
>> Signed-off-by: Dumitru Ceara
>> ---
>> V3:
>> - Addressed Ilya's comments:
>> - removed references to 'bitmap' in comments
>> - fixed indentation & typos
>> - moved "id-pool.h" include to C file
>> - extended the OVS feature detection component to also check for the
>> max number of groups.
>> V2:
>> - Addressed Ilya's comment: fixed the way id_pool_create() is called.
>> - renamed max_size to max_id, it's more accurate.
>> ---
>> controller/ofctrl.c | 2 +
>> controller/ovn-controller.c | 6 +--
>> include/ovn/features.h | 3 ++
>> lib/extend-table.c | 81 --
>> lib/extend-table.h | 13 --
>> lib/features.c | 88 ++---
>> tests/test-ovn.c| 4 +-
>> 7 files changed, 140 insertions(+), 57 deletions(-)
>
> Acked-by: Numan Siddique
>
> Please see one comment/suggestion below.
>
> Thanks
> Numan
>
>
>>
>> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
>> index 63b0aa975b..06f8b33232 100644
>> --- a/controller/ofctrl.c
>> +++ b/controller/ofctrl.c
>> @@ -677,11 +677,13 @@ run_S_CLEAR_FLOWS(void)
>> /* Clear existing groups, to match the state of the switch. */
>> if (groups) {
>> ovn_extend_table_clear(groups, true);
>> +ovn_extend_table_reinit(groups,
>> ovs_feature_max_select_groups_get());
>> }
>>
>> /* Clear existing meters, to match the state of the switch. */
>> if (meters) {
>> ovn_extend_table_clear(meters, true);
>> +ovn_extend_table_reinit(meters, ovs_feature_max_meters_get());
>> ofctrl_meter_bands_clear();
>> }
>>
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index da7d145ed3..7a8ca38286 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -3948,8 +3948,8 @@ en_lflow_output_init(struct engine_node *node
>> OVS_UNUSED,
>> {
>> struct ed_type_lflow_output *data = xzalloc(sizeof *data);
>> ovn_desired_flow_table_init(>flow_table);
>> -ovn_extend_table_init(>group_table);
>> -ovn_extend_table_init(>meter_table);
>> +ovn_extend_table_init(>group_table, "group-table", 0);
>> +ovn_extend_table_init(>meter_table, "meter-table", 0);
>> objdep_mgr_init(>lflow_deps_mgr);
>> lflow_conj_ids_init(>conj_ids);
>> uuidset_init(>objs_processed);
>> @@ -5656,7 +5656,7 @@ main(int argc, char *argv[])
>> engine_set_force_recompute(true);
>> }
>>
>> -if (br_int) {
>> +if (br_int && ovs_feature_set_discovered()) {
>> ct_zones_data = engine_get_data(_ct_zones);
>> if (ct_zones_data && ofctrl_run(br_int, ovs_table,
>> _zones_data->pending)) {
>> diff --git a/include/ovn/features.h b/include/ovn/features.h
>> index 7c3004b271..2c47ab766e 100644
>> --- a/include/ovn/features.h
>> +++ b/include/ovn/features.h
>> @@ -49,5 +49,8 @@ void ovs_feature_support_destroy(void);
>> bool ovs_feature_is_supported(enum ovs_feature_value feature);
>> bool ovs_feature_support_run(const struct smap *ovs_capabilities,
>> const char *br_name);
>> +bool ovs_feature_set_discovered(void);
>> +uint32_t ovs_feature_max_meters_get(void);
>> +uint32_t ovs_feature_max_select_groups_get(void);
>>
>> #endif
>> diff --git a/lib/extend-table.c b/lib/extend-table.c
>> index ebb1a054cb..ed975a511e 100644
>> --- a/lib/extend-table.c
>> +++ b/lib/extend-table.c
>> @@ -17,9 +17,9 @@
>> #include
>> #include
>>
>> -#include "bitmap.h"
>> #include "extend-table.h"
>> #include "hash.h"
>> +#include "id-pool.h"
>> #include "lib/uuid.h"
>> #include "openvswitch/vlog.h"
>>
>> @@ -30,13 +30,29 @@ ovn_extend_table_delete_desired(struct ovn_extend_table
>> *table,
>> struct ovn_extend_table_lflow_to_desired
>> *l);
>>
>> void
>> -ovn_extend_table_init(struct ovn_extend_table *table)
>> +ovn_extend_table_init(struct ovn_extend_table *table, const char
>> *table_name,
>> + uint32_t max_id)
>> {
>> -table->table_ids = bitmap_allocate(MAX_EXT_TABLE_ID);
>> -bitmap_set1(table->table_ids, 0); /* table id 0 is invalid. */
>> -hmap_init(>desired);
>> -hmap_init(>lflow_to_desired);
>> -hmap_init(>existing);
>> +*table = (struct ovn_extend_table) {
>> +.name = table_name,
>> +.max_id = max_id,
>> +/* Table id 0 is invalid, set id-pool base to 1. */
>> +.table_ids =
Re: [ovs-dev] [PATCH v4] Implement compose-packet --hexified [--bad-csum].
On Tue, Oct 24, 2023 at 05:04:05PM +, Ihar Hrachyshka wrote: > With --hexified, it will produce a bare hexified payload with no spaces > or offset indicators inserted, which is useful in tests to produce > frames to pass to e.g. `receive`. > > With --bad-csum, it will produce a frame that has an invalid IP checksum > (applicable to IPv4 only because IPv6 doesn't have checksums.) > > The command is now more useful in tests, where we may need to produce > hex frame payloads to compare observed frames against. > > As an example of the tool use, a single test case is converted to it. > The test uses both normal --hexified and --bad-csum behaviors of the > command, confirming they work as advertised. > > Signed-off-by: Ihar Hrachyshka Acked-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v3 0/4] Add global option to output JSON from ovs-appctl cmds.
On 30 Oct 2023, at 11:07, Jakob Meng wrote:
> On 27.10.23 16:27, Eelco Chaudron wrote:
>> On 25 Oct 2023, at 11:37, jm...@redhat.com wrote:
>>> From: Jakob Meng
>>>
>>> Add global option to output JSON from ovs-appctl cmds.
>>>
>>> This patch is an update of [0] with the following major changes:
>>> * The JSON-RPC API change is now backward compatible. One can use an
>>> updated client (ovs-appctl/dpctl) with an old server (ovs-vswitchd)
>>> and vice versa. Of course, JSON output only works when both are
>>> updated.
>>> * tests/pmd.at from forth patch now features an example of how the
>>> output looks like when a command does not support JSON output.
>>> * The patch has been split into a series of four. The first patch
>>> introduces the '-f,--format' option for ovs-appctl/ovs-dpctl and
>>> necessary changes to the JSON-RPC API. It does not yet pass the
>>> output format to individual commands because that requires a lot
>>> of changes. Those changes have been split out into the third patch
>>> to increase readability of the series.
>>> * The second patch introduces equivalent changes to the Python files.
>>> * The third patch moves all commands to the updated functions in
>>> lib/unixctl.*, in particular unixctl_command_register() and the
>>> unixctl_cb_func type, as well as their Python counterparts. The
>>> output is still text-only (no json) for all commands.
>>> * The forth patch shows how JSON output could be implemented using
>>> 'dpif/show' as an example.
>>>
>>> The following paragraphs are taken from the previous patch revision
>>> and have been updated to changes mentioned above.
>>>
>>> For monitoring systems such as Prometheus it would be beneficial if OVS
>>> and OVS-DPDK would expose statistics in a machine-readable format.
>>> Several approaches like UNIX socket, OVSDB queries and JSON output from
>>> ovs-xxx tools have been proposed [2],[3]. This proof of concept
>>> describes one way how ovs-xxx tools could output JSON in addition to
>>> plain-text for humans.
>>>
>>> This patch follows an alternative approach to RFC [1] which
>>> implemented JSON output as a separate option for each command like
>>> 'dpif/show'. The option was called '-o|--output' in the latter. It
>>> has been renamed to '-f,--format' because ovs-appctl already has a
>>> short option '-o' which prints the available ovs-appctl options
>>> ('--option'). The new option name '-f,--format' is in line with
>>> ovsdb-client where it controls output formatting, too.
>>>
>>> An example call would be 'ovs-appctl --format json dpif/show' as
>>> shown in tests/pmd.at of the forth patch. By default, the output
>>> format is plain-text as before.
>>>
>>> With this patch, all commands announce their support for output
>>> formats when being registered with unixctl_command_register() from
>>> lib/unixctl.*, e.g. OVS_OUTPUT_FMT_TEXT and/or OVS_OUTPUT_FMT_JSON.
>>> When a requested output format is not supported by a command, then
>>> process_command() in lib/unixctl.c will return an error. This is an
>>> advantage over the previous approach [1] where each command would have
>>> to parse the output format option and handle requests for unsupported
>>> output formats on its own.
>>>
>>> The question whether JSON output should be pretty printed and sorted
>>> remains. In most cases it would be unnecessary because machines
>>> consume the output or humans could use jq for pretty printing. However,
>>> it would make tests more readable (for humans) without having to use jq
>>> (which would require us to introduce a dependency on jq).
>> Hi Jakob,
>>
>> I had some code-related comments on V1, which I do not see addressed or
>> replied to. Did you miss them? Anyway, I’ll go over my old review notes, and
>> add them to the split-up patch.
>
> Which one did I miss?
>
> The note on JSON pretty printing I left here as a remainder for the
> discussion on last thursday. With the consensus on adding an extra flag for
> pretty printing I will remove it in the next patch version.
>
> You asked about freeing args->target which is done a couple of lines above in
> "cmdl_args_destroy()".
>
> The rest of your comments has been incorporated in v3 in one way or another
Maybe I did not go over it in to much detail (it was Friday afternoon), but I
missed the first comment:
>> +const char *
>> +ovs_output_fmt_to_string(enum ovs_output_fmt fmt)
>> +{
>> +switch (fmt) {
>> +case OVS_OUTPUT_FMT_TEXT:
>> +return "text";
>> +
>> +case OVS_OUTPUT_FMT_JSON:
>> +return "json";
>> +
>> +default:
>> +return NULL;
>
> Returning NULL might cause a problem in the json_string_create() below, how
> about returning “text” as this is the default value?
>
>> +}
>> +}
Anyway, will do a full review of the next version after the discussion with
Ilya finishes.
//Eelco
___
dev mailing list
d...@openvswitch.org
Re: [ovs-dev] [External] Re: [PATCH] ofproto-dpif: remove checking setting nd_ext field
On Mon, Oct 23, 2023 at 07:22:08PM -0700, Kaixi Fan wrote: > Hi Simon Horman, > > Thanks. If it's ready, I will repost again. Thanks, much appreciated. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] flow: Consistent VXLAN UDP src ports for fragmented packets
Hi Ilya,
The changes are not required for flow_hash_5tuple() function. I have added
debug statements, and we are not hitting this function as part of this code
flow.
Thanks,
Hemanth.
-Original Message-
From: Ilya Maximets
Sent: Monday, June 26, 2023 7:59 PM
To: Hemanth Aramadaka ; 'Simon Horman'
; ovs-dev@openvswitch.org
Cc: i.maxim...@ovn.org
Subject: Re: [ovs-dev] [PATCH] flow: Consistent VXLAN UDP src ports for
fragmented packets
On 6/26/23 08:59, Hemanth Aramadaka via dev wrote:
> Hi Simon,
>
> Sorry for the late response. Yes, the changes are specific to IPV6
> protocol only.
Please, fix the flow_hash_5tuple() function as well. All variants of the same
hash function should give the same result. For some reason you just removed
the fix for this function from the patch instead of addressing a review comment.
And add the unit test for the issue.
Thanks.
Best regards, Ilya Maximets.
>
> Thanks,
> Hemanth.
>
> -Original Message-
> From: Simon Horman
> Sent: Friday, January 20, 2023 8:52 PM
> To: ovs-dev@openvswitch.org
> Cc: Hemanth Aramadaka
> Subject: Re: [ovs-dev] [PATCH] flow: Consistent VXLAN UDP src ports
> for fragmented packets
>
> On Thu, Jan 05, 2023 at 06:55:03PM +0530, Hemanth Aramadaka via dev wrote:
>> Issue:
>>
>> The src-port for UDP is based on RSS hash in the packet metadata.
>> In case of packets coming from VM it will be 5-tuple, if available,
>> otherwise just IP addresses.If the VM fragments a large IP packet and
>> sends the fragments to ovs, only the first fragment will contain the
>> L4 header. Therefore, the first fragment and subsequent fragments get
>> different UDP src ports in the outgoing VXLAN header.This can lead to
>> fragment re-ordering in the fabric as packet will take different
>> paths.
>>
>> Fix:
>>
>> Intention of this is to avoid fragment packets taking different paths.
>> For example, due to presence of firewalls, fragment packets will take
>> different paths and will get dropped.To avoid this we ignore the L4
>> header during hash calculation only in the case of fragmented packets.
>>
>> Signed-off-by: Hemanth Aramadaka
>> ---
>> lib/flow.c | 18 +++---
>> 1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/flow.c b/lib/flow.c
>> index c3a3aa3ce..4f4197b1c 100644
>> --- a/lib/flow.c
>> +++ b/lib/flow.c
>> @@ -1018,7 +1018,9 @@ miniflow_extract(struct dp_packet *packet,
>> struct
> miniflow *dst)
>> miniflow_push_be16(mf, ct_tp_src, ct_tp_src);
>> miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst);
>> if (dl_type == htons(ETH_TYPE_IP)) {
>> -dp_packet_update_rss_hash_ipv4_tcp_udp(packet);
>> +if (!(nw_frag & FLOW_NW_FRAG_MASK)) {
>> +
> dp_packet_update_rss_hash_ipv4_tcp_udp(packet);
>> +}
>> } else if (dl_type == htons(ETH_TYPE_IPV6)) {
>> dp_packet_update_rss_hash_ipv6_tcp_udp(packet);
>> }
>> @@ -1033,7 +1035,9 @@ miniflow_extract(struct dp_packet *packet,
>> struct
> miniflow *dst)
>> miniflow_push_be16(mf, ct_tp_src, ct_tp_src);
>> miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst);
>> if (dl_type == htons(ETH_TYPE_IP)) {
>> -dp_packet_update_rss_hash_ipv4_tcp_udp(packet);
>> +if (!(nw_frag & FLOW_NW_FRAG_MASK)) {
>> +dp_packet_update_rss_hash_ipv4_tcp_udp(packet);
>> +}
>> } else if (dl_type == htons(ETH_TYPE_IPV6)) {
>> dp_packet_update_rss_hash_ipv6_tcp_udp(packet);
>> }
>> @@ -2248,7 +2252,7 @@ miniflow_hash_5tuple(const struct miniflow
>> *flow, uint32_t basis)
>>
>> if (flow) {
>> ovs_be16 dl_type = MINIFLOW_GET_BE16(flow, dl_type);
>> -uint8_t nw_proto;
>> +uint8_t nw_proto, nw_frag;
>>
>> if (dl_type == htons(ETH_TYPE_IPV6)) {
>> struct flowmap map = FLOWMAP_EMPTY_INITIALIZER; @@
>> -2270,6 +2274,14 @@ miniflow_hash_5tuple(const struct miniflow *flow,
>> uint32_t basis)
>>
>> nw_proto = MINIFLOW_GET_U8(flow, nw_proto);
>> hash = hash_add(hash, nw_proto);
>> +
>> +/* Skip l4 header fields if IP packet is fragmented since
>> + * only first fragment will carry l4 header.
>> + */
>> +nw_frag = MINIFLOW_GET_U8(flow, nw_frag);
>> +if (nw_frag & FLOW_NW_FRAG_MASK) {
>> +goto out;
>> +}
>
> Maybe I am reading things wrong, but it seems to me that this change
> seems to effect all protocols, and in particular both
> IPv4 and IPv6. Whereas the changes above to miniflow_extract() only
> affect IPv6. Is this intentional?
>
>> if (nw_proto != IPPROTO_TCP && nw_proto != IPPROTO_UDP
>> && nw_proto != IPPROTO_SCTP && nw_proto != IPPROTO_ICMP
>>
Re: [ovs-dev] [PATCH v7 3/3] netdev-dpdk: Sync and clean {get, set}_config() callbacks.
, Oct 30, 2023 at 10:50:
From: Jakob Meng
For better usability, the function pairs get_config() and
set_config() for netdevs should be symmetric: Options which are
accepted by set_config() should be returned by get_config() and the
latter should output valid options for set_config() only.
This patch moves key-value pairs which are no valid options from
get_config() to the get_status() callback. For example, get_config()
in lib/netdev-dpdk.c returned {configured,requested}_{rx,tx}_queues
previously. For requested rx queues the proper option name is n_rxq,
so requested_rx_queues has been renamed respectively. Tx queues
cannot be changed by the user, hence requested_tx_queues has been
dropped. Both configured_{rx,tx}_queues will be returned as
n_{r,t}xq in the get_status() callback.
The netdev dpdk classes no longer share a common get_config() callback,
instead both the dpdk_class and the dpdk_vhost_client_class define
their own callbacks. The get_config() callback for dpdk_vhost_class has
been dropped because it does not have a set_config() callback.
The documentation in vswitchd/vswitch.xml for status columns as well
as tests have been updated accordingly.
Reported-at: https://bugzilla.redhat.com/1949855
Signed-off-by: Jakob Meng
Hi Jakob, this looks good. Thanks!
Reviewed-by: Robin Jarry
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v3 0/4] Add global option to output JSON from ovs-appctl cmds.
On 27.10.23 16:27, Eelco Chaudron wrote:
> On 25 Oct 2023, at 11:37, jm...@redhat.com wrote:
>> From: Jakob Meng
>>
>> Add global option to output JSON from ovs-appctl cmds.
>>
>> This patch is an update of [0] with the following major changes:
>> * The JSON-RPC API change is now backward compatible. One can use an
>> updated client (ovs-appctl/dpctl) with an old server (ovs-vswitchd)
>> and vice versa. Of course, JSON output only works when both are
>> updated.
>> * tests/pmd.at from forth patch now features an example of how the
>> output looks like when a command does not support JSON output.
>> * The patch has been split into a series of four. The first patch
>> introduces the '-f,--format' option for ovs-appctl/ovs-dpctl and
>> necessary changes to the JSON-RPC API. It does not yet pass the
>> output format to individual commands because that requires a lot
>> of changes. Those changes have been split out into the third patch
>> to increase readability of the series.
>> * The second patch introduces equivalent changes to the Python files.
>> * The third patch moves all commands to the updated functions in
>> lib/unixctl.*, in particular unixctl_command_register() and the
>> unixctl_cb_func type, as well as their Python counterparts. The
>> output is still text-only (no json) for all commands.
>> * The forth patch shows how JSON output could be implemented using
>> 'dpif/show' as an example.
>>
>> The following paragraphs are taken from the previous patch revision
>> and have been updated to changes mentioned above.
>>
>> For monitoring systems such as Prometheus it would be beneficial if OVS
>> and OVS-DPDK would expose statistics in a machine-readable format.
>> Several approaches like UNIX socket, OVSDB queries and JSON output from
>> ovs-xxx tools have been proposed [2],[3]. This proof of concept
>> describes one way how ovs-xxx tools could output JSON in addition to
>> plain-text for humans.
>>
>> This patch follows an alternative approach to RFC [1] which
>> implemented JSON output as a separate option for each command like
>> 'dpif/show'. The option was called '-o|--output' in the latter. It
>> has been renamed to '-f,--format' because ovs-appctl already has a
>> short option '-o' which prints the available ovs-appctl options
>> ('--option'). The new option name '-f,--format' is in line with
>> ovsdb-client where it controls output formatting, too.
>>
>> An example call would be 'ovs-appctl --format json dpif/show' as
>> shown in tests/pmd.at of the forth patch. By default, the output
>> format is plain-text as before.
>>
>> With this patch, all commands announce their support for output
>> formats when being registered with unixctl_command_register() from
>> lib/unixctl.*, e.g. OVS_OUTPUT_FMT_TEXT and/or OVS_OUTPUT_FMT_JSON.
>> When a requested output format is not supported by a command, then
>> process_command() in lib/unixctl.c will return an error. This is an
>> advantage over the previous approach [1] where each command would have
>> to parse the output format option and handle requests for unsupported
>> output formats on its own.
>>
>> The question whether JSON output should be pretty printed and sorted
>> remains. In most cases it would be unnecessary because machines
>> consume the output or humans could use jq for pretty printing. However,
>> it would make tests more readable (for humans) without having to use jq
>> (which would require us to introduce a dependency on jq).
> Hi Jakob,
>
> I had some code-related comments on V1, which I do not see addressed or
> replied to. Did you miss them? Anyway, I’ll go over my old review notes, and
> add them to the split-up patch.
Which one did I miss?
The note on JSON pretty printing I left here as a remainder for the discussion
on last thursday. With the consensus on adding an extra flag for pretty
printing I will remove it in the next patch version.
You asked about freeing args->target which is done a couple of lines above in
"cmdl_args_destroy()".
The rest of your comments has been incorporated in v3 in one way or another
Best,
Jakob
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6 0/3] netdev: Sync and clean {get, set}_config() callbacks.
On 27.10.23 17:25, Kevin Traynor wrote: > On 27/10/2023 14:38, Ilya Maximets wrote: >> On 10/26/23 11:29, Jakob Meng wrote: >>> On 25.10.23 19:10, Ilya Maximets wrote: ... Maybe something along these lines: - ovs-appctl: * Output of 'dpctl/show' command no longer shows interface configuration status, only values of the actual configuration options, a.k.a. 'requested' configuration. The interface configuration status, a.k.a. 'configured' values, can be found in the 'status' column of the Interface table, i.e. with 'ovs-vsctl get interface <..> status'. Reported names adjusted accordingly. What do you think? >>> >>> Simple and concise I will use that. However, we could add an example, >>> it could make it easier to grasp the meaning. >> >> I guess, the reference to 'configured' and 'requested' should be enough >> of an example. I think, if we would add an example, it should be very >> short, i.e. no longer than one extra line. This entry is already too long. >> >>> >>> Should I put the NEWS change in one of the patches or in a separate patch >>> 4/4? >> >> I'd say since the netdev-dpdk changes are the most noticeable, add the >> NEWS change into netdev-dpdk patch, but move the patch itself to the >> end of the set, so the NEWS entry is correct. >> >> Having a NEWS entry as a separate patch is not a good practice as if >> impairs ability to git blame the NEWS file. >> > > The plan above looks good to me too, > > Kevin. Thank you both, Ilya and Kevin! I have incorporated your suggestions into v7: https://patchwork.ozlabs.org/project/openvswitch/list/?series=379901 (cover letter includes list of what was changed, but it's not listed in patchwork) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v3 1/4] Add global option for JSON output to ovs-appctl/dpctl.
On 27 Oct 2023, at 23:51, Ilya Maximets wrote:
> On 10/26/23 13:44, Jakob Meng wrote:
>> On 25.10.23 11:37, jm...@redhat.com wrote:
>>> From: Jakob Meng
>>>
>>> For monitoring systems such as Prometheus it would be beneficial if
>>> OVS and OVS-DPDK would expose statistics in a machine-readable format.
>>>
>>> This patch introduces support for different output formats to ovs-xxx
>>> tools. They gain a global option '-f,--format' which allows users to
>>> request JSON instead of plain-text for humans. An example call
>>> implemented in a later patch is 'ovs-appctl --format json dpif/show'.
I started to look at some of the code, but I guess some of the comments from
Ilya could change the implementation, so I’ll stop for now.
Please take a look at my commands from the v1 review, and incorporate them if
still valid after potential changes (and flake8 warnings).
Also, instead of TODO use use 'XXX' or 'FIXME' as mentioned in the coding style.
On the ‘pretty print or not discussion’, I added the following:
I think we should use the same option as ovs-vsctl has for the dbase.
-f, --format=FORMAT set output formatting to FORMAT
("table", "html", "csv", or "json")
--prettypretty-print JSON in output
This looks like the following:
ovs-vsctl --format=json list Open_vSwitch
{"data":[[["uuid","8acf0aa8-7b3c-4b2a-9a15-6a4143ef63f2"],["uuid","630c4d69-c905-4645-ba59-66fd0bfa66c7"],32,["set",["netdev","system"]],["map",[]],"8.3.1",true,"DPDK
22.11.1",["map",[["hostname","wsfd-advnetlab224.anl.lab.eng.bos.redhat.com"],["rundir","/var/run/openvswitch"],["system-id","5ee96c43-4823-4456-9bc3-9616c1acdce3"]]],["set",["bareudp","dpdk","dpdkvhostuser","dpdkvhostuserclient","erspan","geneve","gre","gtpu","internal","ip6erspan","ip6gre","lisp","patch","stt","system","tap","vxlan"]],["set",[]],32,["map",[["dpdk-init","true"],["dpdk-socket-mem","2048"],["pmd-cpu-mask","0x40004000"]]],"3.1.4",["set",[]],["map",[]],"rhel","9.2"]],"headings":["_uuid","bridges","cur_cfg","datapath_types","datapaths","db_version","dpdk_initialized","dpdk_version","external_ids","iface_types","manager_options","next_cfg","other_config","ovs_version","ssl","statistics","system_type","system_version"]}
[wsfd-advnetlab224:~]$ ovs-vsctl --format=json list Open_vSwitch
[wsfd-advnetlab224:~]$ ovs-vsctl --format=json --pretty list Open_vSwitch
{
"data": [
...
}
>>> For that it is necessary to change the JSON-RPC API lib/unixctl.*
>>> which ovs-xxx tools use to communicate with ovs-vswitchd across unix
>>> sockets. It now allows to transport the requested output format
>>> besides the command and its args. This change has been implemented in
>>> a backward compatible way. One can use an updated client
>>> (ovs-appctl/dpctl) with an old server (ovs-vswitchd) and vice versa.
>>> Of course, JSON output only works when both sides have been updated.
>>>
>>> Previously, the command was copied to the 'method' parameter in
>>> JSON-RPC while the args were copied to the 'params' parameter. Without
>>> any other means to transport parameters via JSON-RPC left, the meaning
>>> of 'method' and 'params' had to be changed: 'method' will now be
>>> 'execute/v1' when an output format other than 'text' is requested. In
>>> that case, the first parameter of the JSON array 'params' will now be
>>> the designated command, the second one the output format and the rest
>>> will be command args.
>>
>> Ilya brought up the question why I changed the meaning of 'method' and
>> 'params' instead of adding the output format as an addition argument to the
>> command arguments in 'params'. The server side would then interpret and
>> filter out this argument before passing the remaining arguments to the
>> command callbacks.
>>
>> I decided against this approach because the code would get more involved, in
>> particular we would have to implement option/argument parsing inside
>> process_command() in lib/unixctl.c. (Besides, using getopt*() functions in a
>> safe way would be difficult in general because their global state.)
>> The current implementation is based purely on JSON objects/arrays which are
>> nicely supported by OVS with functions from lib/json.*.
>
> I'm not sure I got this point, but see below.
>
>>
>> Ilya also voiced concerns about the limited extensibility of the proposed
>> API. To fix this, this patch series could be tweaked in the follow way:
>>
>> (1.) Instead of passing the output format as second entry in the JSON array
>> 'params', turn the second entry into a JSON object (shash). The output
>> format would be one entry in this JSON object, e.g. called 'format'.
>>
>> The server (process_command() from lib/unixctl.c) will parse the content of
>> this JSON object into known options. Clients would only add non-default
>> options to this JSON object to keep compatibility with older servers.
>> Options which are supported by the server but not transferred via this
[ovs-dev] [PATCH v7 3/3] netdev-dpdk: Sync and clean {get, set}_config() callbacks.
From: Jakob Meng
For better usability, the function pairs get_config() and
set_config() for netdevs should be symmetric: Options which are
accepted by set_config() should be returned by get_config() and the
latter should output valid options for set_config() only.
This patch moves key-value pairs which are no valid options from
get_config() to the get_status() callback. For example, get_config()
in lib/netdev-dpdk.c returned {configured,requested}_{rx,tx}_queues
previously. For requested rx queues the proper option name is n_rxq,
so requested_rx_queues has been renamed respectively. Tx queues
cannot be changed by the user, hence requested_tx_queues has been
dropped. Both configured_{rx,tx}_queues will be returned as
n_{r,t}xq in the get_status() callback.
The netdev dpdk classes no longer share a common get_config() callback,
instead both the dpdk_class and the dpdk_vhost_client_class define
their own callbacks. The get_config() callback for dpdk_vhost_class has
been dropped because it does not have a set_config() callback.
The documentation in vswitchd/vswitch.xml for status columns as well
as tests have been updated accordingly.
Reported-at: https://bugzilla.redhat.com/1949855
Signed-off-by: Jakob Meng
---
Documentation/topics/dpdk/phy.rst | 4 +-
NEWS | 7 ++
lib/netdev-dpdk.c | 113 +-
tests/system-dpdk.at | 64 ++---
vswitchd/vswitch.xml | 14 +++-
5 files changed, 143 insertions(+), 59 deletions(-)
diff --git a/Documentation/topics/dpdk/phy.rst
b/Documentation/topics/dpdk/phy.rst
index f66b106c4..41cc3588a 100644
--- a/Documentation/topics/dpdk/phy.rst
+++ b/Documentation/topics/dpdk/phy.rst
@@ -198,7 +198,7 @@ Example::
a dedicated queue, it will be explicit::
$ ovs-vsctl get interface dpdk-p0 status
- {..., rx_steering=unsupported}
+ {..., rx-steering=unsupported}
More details can often be found in ``ovs-vswitchd.log``::
@@ -499,7 +499,7 @@ its options::
$ ovs-appctl dpctl/show
[...]
- port 3: dpdk-rep0 (dpdk: configured_rx_queues=1, ...,
dpdk-vf-mac=00:11:22:33:44:55, ...)
+ port 3: dpdk-rep0 (dpdk: ..., dpdk-vf-mac=00:11:22:33:44:55, ...)
$ ovs-vsctl show
[...]
diff --git a/NEWS b/NEWS
index 6b45492f1..43aea97b5 100644
--- a/NEWS
+++ b/NEWS
@@ -6,6 +6,13 @@ Post-v3.2.0
from older version is supported but it may trigger more leader elections
during the process, and error logs complaining unrecognized fields may
be observed on old nodes.
+ - ovs-appctl:
+ * Output of 'dpctl/show' command no longer shows interface configuration
+ status, only values of the actual configuration options, a.k.a.
+ 'requested' configuration. The interface configuration status,
+ a.k.a. 'configured' values, can be found in the 'status' column of
+ the Interface table, i.e. with 'ovs-vsctl get interface <..> status'.
+ Reported names adjusted accordingly.
v3.2.0 - 17 Aug 2023
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 55700250d..29f2b280d 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1905,31 +1905,41 @@ netdev_dpdk_get_config(const struct netdev *netdev,
struct smap *args)
ovs_mutex_lock(>mutex);
-smap_add_format(args, "requested_rx_queues", "%d", dev->user_n_rxq);
-smap_add_format(args, "configured_rx_queues", "%d", netdev->n_rxq);
-smap_add_format(args, "requested_tx_queues", "%d", dev->requested_n_txq);
-smap_add_format(args, "configured_tx_queues", "%d", netdev->n_txq);
-smap_add_format(args, "mtu", "%d", dev->mtu);
+if (dev->devargs && dev->devargs[0]) {
+smap_add_format(args, "dpdk-devargs", "%s", dev->devargs);
+}
-if (dev->type == DPDK_DEV_ETH) {
-smap_add_format(args, "n_rxq_desc", "%d", dev->rxq_size);
-smap_add_format(args, "n_txq_desc", "%d", dev->txq_size);
-if (dev->hw_ol_features & NETDEV_RX_CHECKSUM_OFFLOAD) {
-smap_add(args, "rx_csum_offload", "true");
-} else {
-smap_add(args, "rx_csum_offload", "false");
-}
-if (dev->rx_steer_flags == DPDK_RX_STEER_LACP) {
-smap_add(args, "rx-steering", "rss+lacp");
-}
-smap_add(args, "lsc_interrupt_mode",
- dev->lsc_interrupt_mode ? "true" : "false");
+smap_add_format(args, "n_rxq", "%d", dev->user_n_rxq);
-if (dpdk_port_is_representor(dev)) {
-smap_add_format(args, "dpdk-vf-mac", ETH_ADDR_FMT,
-ETH_ADDR_ARGS(dev->requested_hwaddr));
-}
+if (dev->fc_conf.mode == RTE_ETH_FC_TX_PAUSE ||
+dev->fc_conf.mode == RTE_ETH_FC_FULL) {
+smap_add(args, "rx-flow-ctrl", "true");
}
+
+if (dev->fc_conf.mode == RTE_ETH_FC_RX_PAUSE ||
+dev->fc_conf.mode == RTE_ETH_FC_FULL) {
+smap_add(args, "tx-flow-ctrl", "true");
+}
+
+if
[ovs-dev] [PATCH v7 2/3] netdev-afxdp: Sync and clean {get, set}_config() callbacks.
From: Jakob Meng
For better usability, the function pairs get_config() and
set_config() for netdevs should be symmetric: Options which are
accepted by set_config() should be returned by get_config() and the
latter should output valid options for set_config() only. This patch
also moves key-value pairs which are no valid options from get_config()
to the get_status() callback.
The documentation in vswitchd/vswitch.xml for status columns has been
updated accordingly.
Reported-at: https://bugzilla.redhat.com/1949855
Signed-off-by: Jakob Meng
---
Documentation/intro/install/afxdp.rst | 12
lib/netdev-afxdp.c| 21 +++--
lib/netdev-afxdp.h| 1 +
lib/netdev-linux-private.h| 1 +
lib/netdev-linux.c| 4 ++--
vswitchd/vswitch.xml | 11 +++
6 files changed, 38 insertions(+), 12 deletions(-)
diff --git a/Documentation/intro/install/afxdp.rst
b/Documentation/intro/install/afxdp.rst
index 51c24bf5b..5776614c8 100644
--- a/Documentation/intro/install/afxdp.rst
+++ b/Documentation/intro/install/afxdp.rst
@@ -219,14 +219,10 @@ Otherwise, enable debugging by::
ovs-appctl vlog/set netdev_afxdp::dbg
To check which XDP mode was chosen by ``best-effort``, you can look for
-``xdp-mode-in-use`` in the output of ``ovs-appctl dpctl/show``::
-
- # ovs-appctl dpctl/show
- netdev@ovs-netdev:
-<...>
-port 2: ens802f0 (afxdp: n_rxq=1, use-need-wakeup=true,
- xdp-mode=best-effort,
- xdp-mode-in-use=native-with-zerocopy)
+``xdp-mode`` in the output of ``ovs-vsctl get interface INT status:xdp-mode``::
+
+ # ovs-vsctl get interface ens802f0 status:xdp-mode
+ "native-with-zerocopy"
References
--
diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
index 16f26bc30..b680a1479 100644
--- a/lib/netdev-afxdp.c
+++ b/lib/netdev-afxdp.c
@@ -672,8 +672,6 @@ netdev_afxdp_get_config(const struct netdev *netdev, struct
smap *args)
ovs_mutex_lock(>mutex);
smap_add_format(args, "n_rxq", "%d", netdev->n_rxq);
smap_add_format(args, "xdp-mode", "%s", xdp_modes[dev->xdp_mode].name);
-smap_add_format(args, "xdp-mode-in-use", "%s",
-xdp_modes[dev->xdp_mode_in_use].name);
smap_add_format(args, "use-need-wakeup", "%s",
dev->use_need_wakeup ? "true" : "false");
ovs_mutex_unlock(>mutex);
@@ -1367,3 +1365,22 @@ netdev_afxdp_get_stats(const struct netdev *netdev,
return error;
}
+
+int
+netdev_afxdp_get_status(const struct netdev *netdev, struct smap *args)
+{
+int error = netdev_linux_get_status(netdev, args);
+
+if (error) {
+return error;
+}
+
+struct netdev_linux *dev = netdev_linux_cast(netdev);
+
+ovs_mutex_lock(>mutex);
+smap_add_format(args, "xdp-mode", "%s",
+xdp_modes[dev->xdp_mode_in_use].name);
+ovs_mutex_unlock(>mutex);
+
+return error;
+}
diff --git a/lib/netdev-afxdp.h b/lib/netdev-afxdp.h
index e91cd102d..bd3b9dfbe 100644
--- a/lib/netdev-afxdp.h
+++ b/lib/netdev-afxdp.h
@@ -63,6 +63,7 @@ int netdev_afxdp_set_config(struct netdev *netdev, const
struct smap *args,
int netdev_afxdp_get_config(const struct netdev *netdev, struct smap *args);
int netdev_afxdp_get_stats(const struct netdev *netdev_,
struct netdev_stats *stats);
+int netdev_afxdp_get_status(const struct netdev *netdev, struct smap *args);
int netdev_afxdp_get_custom_stats(const struct netdev *netdev,
struct netdev_custom_stats *custom_stats);
diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h
index 0ecf0f748..188e8438a 100644
--- a/lib/netdev-linux-private.h
+++ b/lib/netdev-linux-private.h
@@ -50,6 +50,7 @@ struct netdev_rxq_linux {
};
int netdev_linux_construct(struct netdev *);
+int netdev_linux_get_status(const struct netdev *, struct smap *);
void netdev_linux_run(const struct netdev_class *);
int get_stats_via_netlink(const struct netdev *netdev_,
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index cca340879..70521e3c7 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -3493,7 +3493,7 @@ netdev_linux_get_next_hop(const struct in_addr *host,
struct in_addr *next_hop,
return ENXIO;
}
-static int
+int
netdev_linux_get_status(const struct netdev *netdev_, struct smap *smap)
{
struct netdev_linux *netdev = netdev_linux_cast(netdev_);
@@ -3759,7 +3759,7 @@ const struct netdev_class netdev_internal_class = {
.destruct = netdev_afxdp_destruct, \
.get_stats = netdev_afxdp_get_stats,\
.get_custom_stats = netdev_afxdp_get_custom_stats, \
-.get_status = netdev_linux_get_status, \
+.get_status = netdev_afxdp_get_status, \
.set_config = netdev_afxdp_set_config, \
.get_config =
[ovs-dev] [PATCH v7 0/3] netdev: Sync and clean {get, set}_config() callbacks.
From: Jakob Meng
This patch series incorporates Ilya's and Kevin's change requests for v6:
* added NEWS suggestion to netdev-dpdk patch
* netdev-dpdk patch with NEWS entry has been put at the end of the series
* get_config shuffle in dpdk_class, dpdk_vhost_class and
dpdk_vhost_client_class has been documented in commit msg of netdev-dpdk patch
* dpdk-devargs config option for netdev-dpdk is only returned if it is set
* flow ctrl options for netdev-dpdk are only returned if their value is true
* no longer clear numa_id in tests/pmd.at because it is always numa_id=1
* formatting changes for netdev-dpdk and netdev-afxdp
* reworded rx_csum_offload in vswitchd/vswitch.xml
Jakob Meng (3):
netdev-dummy: Sync and clean {get,set}_config() callbacks.
netdev-afxdp: Sync and clean {get,set}_config() callbacks.
netdev-dpdk: Sync and clean {get,set}_config() callbacks.
Documentation/intro/install/afxdp.rst | 12 +--
Documentation/topics/dpdk/phy.rst | 4 +-
NEWS | 7 ++
lib/netdev-afxdp.c| 21 -
lib/netdev-afxdp.h| 1 +
lib/netdev-dpdk.c | 113 ++
lib/netdev-dummy.c| 19 -
lib/netdev-linux-private.h| 1 +
lib/netdev-linux.c| 4 +-
tests/pmd.at | 26 +++---
tests/system-dpdk.at | 64 +--
vswitchd/vswitch.xml | 25 +-
12 files changed, 209 insertions(+), 88 deletions(-)
--
2.39.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v7 1/3] netdev-dummy: Sync and clean {get, set}_config() callbacks.
From: Jakob Meng
For better usability, the function pairs get_config() and
set_config() for netdevs should be symmetric: Options which are
accepted by set_config() should be returned by get_config() and the
latter should output valid options for set_config() only. This patch
also moves key-value pairs which are no valid options from get_config()
to the get_status() callback. The tests have been updated accordingly.
Reported-at: https://bugzilla.redhat.com/1949855
Signed-off-by: Jakob Meng
---
lib/netdev-dummy.c | 19 +++
tests/pmd.at | 26 +-
2 files changed, 28 insertions(+), 17 deletions(-)
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 1a54add87..fe82317d7 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -795,14 +795,25 @@ netdev_dummy_get_config(const struct netdev *dev, struct
smap *args)
dummy_packet_conn_get_config(>conn, args);
+/* pcap, rxq_pcap and tx_pcap cannot be recovered because filenames have
+ * been discarded after opening file descriptors */
+
+if (netdev->ol_ip_csum) {
+smap_add_format(args, "ol_ip_csum", "%s", "true");
+}
+
+if (netdev->ol_ip_csum_set_good) {
+smap_add_format(args, "ol_ip_csum_set_good", "%s", "true");
+}
+
/* 'dummy-pmd' specific config. */
if (!netdev_is_pmd(dev)) {
goto exit;
}
-smap_add_format(args, "requested_rx_queues", "%d",
netdev->requested_n_rxq);
-smap_add_format(args, "configured_rx_queues", "%d", dev->n_rxq);
-smap_add_format(args, "requested_tx_queues", "%d",
netdev->requested_n_txq);
-smap_add_format(args, "configured_tx_queues", "%d", dev->n_txq);
+
+smap_add_format(args, "n_rxq", "%d", netdev->requested_n_rxq);
+smap_add_format(args, "n_txq", "%d", netdev->requested_n_txq);
+smap_add_format(args, "numa_id", "%d", netdev->requested_numa_id);
exit:
ovs_mutex_unlock(>mutex);
diff --git a/tests/pmd.at b/tests/pmd.at
index 7bdaca9e7..06cc90477 100644
--- a/tests/pmd.at
+++ b/tests/pmd.at
@@ -93,11 +93,11 @@ pmd thread numa_id core_id :
overhead: NOT AVAIL
])
-AT_CHECK([ovs-appctl dpif/show | sed
's/\(tx_queues=\)[[0-9]]*/\1/g'], [0], [dnl
+AT_CHECK([ovs-appctl dpif/show], [0], [dnl
dummy@ovs-dummy: hit:0 missed:0
br0:
br0 65534/100: (dummy-internal)
-p0 1/1: (dummy-pmd: configured_rx_queues=1,
configured_tx_queues=, requested_rx_queues=1,
requested_tx_queues=)
+p0 1/1: (dummy-pmd: n_rxq=1, n_txq=1, numa_id=0)
])
OVS_VSWITCHD_STOP
@@ -111,11 +111,11 @@ CHECK_PMD_THREADS_CREATED()
AT_CHECK([ovs-vsctl set interface p0 options:n_rxq=8])
-AT_CHECK([ovs-appctl dpif/show | sed
's/\(tx_queues=\)[[0-9]]*/\1/g'], [0], [dnl
+AT_CHECK([ovs-appctl dpif/show], [0], [dnl
dummy@ovs-dummy: hit:0 missed:0
br0:
br0 65534/100: (dummy-internal)
-p0 1/1: (dummy-pmd: configured_rx_queues=8,
configured_tx_queues=, requested_rx_queues=8,
requested_tx_queues=)
+p0 1/1: (dummy-pmd: n_rxq=8, n_txq=1, numa_id=0)
])
AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed SED_NUMA_CORE_PATTERN],
[0], [dnl
@@ -144,11 +144,11 @@ OVS_VSWITCHD_START([add-port br0 p0 -- set Interface p0
type=dummy-pmd options:n
CHECK_CPU_DISCOVERED(2)
CHECK_PMD_THREADS_CREATED()
-AT_CHECK([ovs-appctl dpif/show | sed
's/\(tx_queues=\)[[0-9]]*/\1/g'], [0], [dnl
+AT_CHECK([ovs-appctl dpif/show], [0], [dnl
dummy@ovs-dummy: hit:0 missed:0
br0:
br0 65534/100: (dummy-internal)
-p0 1/1: (dummy-pmd: configured_rx_queues=8,
configured_tx_queues=, requested_rx_queues=8,
requested_tx_queues=)
+p0 1/1: (dummy-pmd: n_rxq=8, n_txq=1, numa_id=0)
])
AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed SED_NUMA_CORE_PATTERN],
[0], [dnl
@@ -227,11 +227,11 @@ TMP=$(($(cat ovs-vswitchd.log | wc -l | tr -d
[[:blank:]])+1))
CHECK_CPU_DISCOVERED(4)
CHECK_PMD_THREADS_CREATED()
-AT_CHECK([ovs-appctl dpif/show | sed
's/\(tx_queues=\)[[0-9]]*/\1/g'], [0], [dnl
+AT_CHECK([ovs-appctl dpif/show], [0], [dnl
dummy@ovs-dummy: hit:0 missed:0
br0:
br0 65534/100: (dummy-internal)
-p0 1/1: (dummy-pmd: configured_rx_queues=8,
configured_tx_queues=, requested_rx_queues=8,
requested_tx_queues=)
+p0 1/1: (dummy-pmd: n_rxq=8, n_txq=1, numa_id=1)
])
AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed SED_NUMA_CORE_PATTERN],
[0], [dnl
@@ -436,11 +436,11 @@ AT_CHECK([ovs-vsctl set Open_vSwitch .
other_config:smc-enable=true])
sleep 1
-AT_CHECK([ovs-appctl dpif/show | sed
's/\(tx_queues=\)[[0-9]]*/\1/g'], [0], [dnl
+AT_CHECK([ovs-appctl dpif/show], [0], [dnl
dummy@ovs-dummy: hit:0 missed:0
br0:
br0 65534/100: (dummy-internal)
-p0 7/1: (dummy-pmd: configured_rx_queues=4,
configured_tx_queues=, requested_rx_queues=4,
requested_tx_queues=)
+p0 7/1: (dummy-pmd: n_rxq=4, n_txq=1, numa_id=0)
])
AT_CHECK([ovs-appctl dpif-netdev/pmd-stats-show | sed SED_NUMA_CORE_PATTERN |
sed '/cycles/d' | grep pmd -A 12], [0], [dnl
@@ -604,8 +604,8
[ovs-dev] [PATCH ovn v3 2/3] binding: handle pb->chassis and pb->up from if-status module
Before this patch, if-status module handled pb->chassis and pb->up
for vif ports, but not for non-VIF ports.
This caused a few potential issues:
- It was difficult to check that a no-vif port has been previously claimed
as there was no state in if-status. Hence it was difficult to always release
such a port when pb->chassis was set to a different chassis.
This issue will be fixed in a future patch.
- Releasing such ports caused ovn-controller to log warnings such as
"Trying to delete unknown ports".
- If sb is readonly at the time of the claim, it forced the module to recompute.
This patch fixed issues two and three by moving the work of setting pb->chassis
and pb->up to the if-status module.
Signed-off-by: Xavier Simonart
---
v2: Avoid clearing iface if already deleted
v3: - handled Mark's feedback i.e.
- clear comment in claimed_lport_set_up
- changed notify_up to is_vif and clarify comments
- change claimed_lport_set_up to void as was always called with !sb_readonly
---
controller/binding.c| 143
controller/binding.h| 17 +
controller/if-status.c | 137 ++
controller/if-status.h | 9 ++-
controller/ovn-controller.c | 6 +-
tests/ovn.at| 94
6 files changed, 325 insertions(+), 81 deletions(-)
diff --git a/controller/binding.c b/controller/binding.c
index 9a09b2074..bd48db621 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -1193,33 +1193,22 @@ local_binding_set_pb(struct shash *local_bindings,
const char *pb_name,
* - set the 'pb.up' field to true if 'parent_pb.up' is 'true' (e.g., for
* container and virtual ports).
*
- * Returns false if lport is not claimed due to 'sb_readonly'.
- * Returns true otherwise.
- *
* Note:
* Updates the 'pb->up' field only if it's explicitly set to 'false'.
* This is to ensure compatibility with older versions of ovn-northd.
*/
-static bool
+void
claimed_lport_set_up(const struct sbrec_port_binding *pb,
- const struct sbrec_port_binding *parent_pb,
- bool sb_readonly)
+ const struct sbrec_port_binding *parent_pb)
{
-/* When notify_up is false in claim_port(), no state is created
- * by if_status_mgr. In such cases, return false (i.e. trigger recompute)
- * if we can't update sb (because it is readonly).
- */
bool up = true;
if (!parent_pb || (parent_pb->n_up && parent_pb->up[0])) {
-if (!sb_readonly) {
-if (pb->n_up) {
-sbrec_port_binding_set_up(pb, , 1);
-}
-} else if (pb->n_up && !pb->up[0]) {
-return false;
+if (pb->n_up) {
+VLOG_INFO("Setting lport %s up in Southbound",
+ pb->logical_port);
+sbrec_port_binding_set_up(pb, , 1);
}
}
-return true;
}
typedef void (*set_func)(const struct sbrec_port_binding *pb,
@@ -1322,7 +1311,7 @@ claim_lport(const struct sbrec_port_binding *pb,
const struct sbrec_port_binding *parent_pb,
const struct sbrec_chassis *chassis_rec,
const struct ovsrec_interface *iface_rec,
-bool sb_readonly, bool notify_up,
+bool sb_readonly, bool is_vif,
struct hmap *tracked_datapaths,
struct if_status_mgr *if_mgr,
struct sset *postponed_ports)
@@ -1347,40 +1336,27 @@ claim_lport(const struct sbrec_port_binding *pb,
}
update_tracked = true;
-if (!notify_up) {
-if (!claimed_lport_set_up(pb, parent_pb, sb_readonly)) {
-return false;
-}
-if (sb_readonly) {
-return false;
-}
-set_pb_chassis_in_sbrec(pb, chassis_rec, true);
-} else {
-if_status_mgr_claim_iface(if_mgr, pb, chassis_rec, iface_rec,
- sb_readonly, can_bind);
-}
+if_status_mgr_claim_iface(if_mgr, pb, chassis_rec, iface_rec,
+ sb_readonly, can_bind, is_vif,
+ parent_pb);
register_claim_timestamp(pb->logical_port, now);
sset_find_and_delete(postponed_ports, pb->logical_port);
} else {
update_tracked = true;
-if (!notify_up) {
-if (!claimed_lport_set_up(pb, parent_pb, sb_readonly)) {
-return false;
-}
-} else {
-if ((pb->n_up && !pb->up[0]) ||
-!smap_get_bool(_rec->external_ids,
- OVN_INSTALLED_EXT_ID, false)) {
-if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
- iface_rec, sb_readonly,
-
[ovs-dev] [PATCH ovn v3 0/3] handle pb->chassis and pb->up from if-status module
Xavier Simonart (3): binding: slight refactor if no local binding in consider_iface_release binding: handle pb->chassis and pb->up from if-status module Fix flows not removed in ha migration controller/binding.c| 221 ++-- controller/binding.h| 17 +++ controller/if-status.c | 137 +- controller/if-status.h | 9 +- controller/ovn-controller.c | 6 +- tests/ovn.at| 94 +++ 6 files changed, 364 insertions(+), 120 deletions(-) -- 2.31.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v3 3/3] Fix flows not removed in ha migration
Signed-off-by: Xavier Simonart
---
controller/binding.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/controller/binding.c b/controller/binding.c
index bd48db621..cc316f390 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -1841,7 +1841,9 @@ consider_nonvif_lport_(const struct sbrec_port_binding
*pb,
}
if (pb->chassis == b_ctx_in->chassis_rec ||
-is_additional_chassis(pb, b_ctx_in->chassis_rec)) {
+is_additional_chassis(pb, b_ctx_in->chassis_rec)
+|| if_status_is_port_claimed(b_ctx_out->if_mgr,
+ pb->logical_port)) {
return release_lport(pb, b_ctx_in->chassis_rec,
!b_ctx_in->ovnsb_idl_txn,
b_ctx_out->tracked_dp_bindings,
--
2.31.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v3 1/3] binding: slight refactor if no local binding in consider_iface_release
Signed-off-by: Xavier Simonart
---
controller/binding.c | 74 +---
1 file changed, 36 insertions(+), 38 deletions(-)
diff --git a/controller/binding.c b/controller/binding.c
index 8020d052f..9a09b2074 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -2392,7 +2392,7 @@ consider_iface_release(const struct ovsrec_interface
*iface_rec,
lbinding = local_binding_find(local_bindings, iface_id);
- if (lbinding) {
+if (lbinding) {
if (lbinding->multiple_bindings) {
VLOG_INFO("Multiple bindings for %s: force recompute to clean up",
iface_id);
@@ -2412,52 +2412,50 @@ consider_iface_release(const struct ovsrec_interface
*iface_rec,
return true;
}
}
-}
-struct binding_lport *b_lport =
-local_binding_get_primary_or_localport_lport(lbinding);
-if (is_binding_lport_this_chassis(b_lport, b_ctx_in->chassis_rec)) {
-struct local_datapath *ld =
-get_local_datapath(b_ctx_out->local_datapaths,
- b_lport->pb->datapath->tunnel_key);
-if (ld) {
-remove_pb_from_local_datapath(b_lport->pb,
- b_ctx_out, ld);
-}
+struct binding_lport *b_lport =
+local_binding_get_primary_or_localport_lport(lbinding);
-add_or_del_qos_port(b_lport->pb->logical_port, false);
+if (is_binding_lport_this_chassis(b_lport, b_ctx_in->chassis_rec)) {
+struct local_datapath *ld =
+get_local_datapath(b_ctx_out->local_datapaths,
+ b_lport->pb->datapath->tunnel_key);
+if (ld) {
+remove_pb_from_local_datapath(b_lport->pb,
+ b_ctx_out, ld);
+}
+add_or_del_qos_port(b_lport->pb->logical_port, false);
-/* Release the primary binding lport and other children lports if
- * any. */
-LIST_FOR_EACH (b_lport, list_node, >binding_lports) {
-if (!release_binding_lport(b_ctx_in->chassis_rec, b_lport,
- !b_ctx_in->ovnsb_idl_txn,
- b_ctx_out)) {
-return false;
+/* Release the primary binding lport and other children lports if
+ * any. */
+LIST_FOR_EACH (b_lport, list_node, >binding_lports) {
+if (!release_binding_lport(b_ctx_in->chassis_rec, b_lport,
+ !b_ctx_in->ovnsb_idl_txn,
+ b_ctx_out)) {
+return false;
+}
+}
+if (lbinding->iface && lbinding->iface->name) {
+if_status_mgr_remove_ovn_installed(b_ctx_out->if_mgr,
+ lbinding->iface->name,
+ >iface->header_.uuid);
}
-}
-if (lbinding->iface && lbinding->iface->name) {
-if_status_mgr_remove_ovn_installed(b_ctx_out->if_mgr,
- lbinding->iface->name,
- >iface->header_.uuid);
-}
-} else if (lbinding && b_lport && b_lport->type == LP_LOCALPORT) {
-/* lbinding is associated with a localport. Remove it from the
- * related lports. */
-remove_related_lport(b_lport->pb, b_ctx_out);
-}
+} else if (b_lport && b_lport->type == LP_LOCALPORT) {
+/* lbinding is associated with a localport. Remove it from the
+ * related lports. */
+remove_related_lport(b_lport->pb, b_ctx_out);
+}
-if (lbinding) {
/* Clear the iface of the local binding. */
lbinding->iface = NULL;
-}
-/* Check if the lbinding has children of type PB_CONTAINER.
- * If so, don't delete the local_binding. */
-if (lbinding && !is_lbinding_container_parent(lbinding)) {
-local_binding_delete(lbinding, local_bindings, binding_lports,
- b_ctx_out->if_mgr);
+/* Check if the lbinding has children of type PB_CONTAINER.
+ * If so, don't delete the local_binding. */
+if (!is_lbinding_container_parent(lbinding)) {
+local_binding_delete(lbinding, local_bindings, binding_lports,
+ b_ctx_out->if_mgr);
+}
}
remove_local_lports(iface_id, b_ctx_out);
--
2.31.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] python: Remove duplicate UnixctlClient implementation.
On 28.10.23 00:08, Ilya Maximets wrote:
> On 10/26/23 14:10, jm...@redhat.com wrote:
>> From: Jakob Meng
>>
>> The unixctl implementation in Python has been split into three parts in
>> the past. During this process the UnixctlClient was duplicated, in
>> python/ovs/unixctl/client.py and python/ovs/unixctl/server.py. This
>> patch removes the duplicate from the latter.
>>
>> Fixes: 53cf9963ccc6 ("python: Break unixctl implementation into re...")
> Please, don't abbreviate tags, i.e. the Fixes tag should contain the full
> commit name regardless of the length.
>
> Otherwise, the change seem fine.
Missed that part in "Submitting patches" guide. Thank you for pointing out!
Patch v2 is out:
https://patchwork.ozlabs.org/project/openvswitch/patch/20231030090259.15251-2-jm...@redhat.com/
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2 1/1] python: Remove duplicate UnixctlClient implementation.
From: Jakob Meng
The unixctl implementation in Python has been split into three parts in
the past. During this process the UnixctlClient was duplicated, in
python/ovs/unixctl/client.py and python/ovs/unixctl/server.py. This
patch removes the duplicate from the latter.
Fixes: 53cf9963ccc6 ("python: Break unixctl implementation into registry,
client, and server.")
Signed-off-by: Jakob Meng
Acked-by: Simon Horman
Acked-by: Eelco Chaudron
---
python/ovs/unixctl/server.py | 44
1 file changed, 44 deletions(-)
diff --git a/python/ovs/unixctl/server.py b/python/ovs/unixctl/server.py
index 5f9b3e739..b9cb52fad 100644
--- a/python/ovs/unixctl/server.py
+++ b/python/ovs/unixctl/server.py
@@ -211,47 +211,3 @@ class UnixctlServer(object):
version)
return 0, UnixctlServer(listener)
-
-
-class UnixctlClient(object):
-def __init__(self, conn):
-assert isinstance(conn, ovs.jsonrpc.Connection)
-self._conn = conn
-
-def transact(self, command, argv):
-assert isinstance(command, str)
-assert isinstance(argv, list)
-for arg in argv:
-assert isinstance(arg, str)
-
-request = Message.create_request(command, argv)
-error, reply = self._conn.transact_block(request)
-
-if error:
-vlog.warn("error communicating with %s: %s"
- % (self._conn.name, os.strerror(error)))
-return error, None, None
-
-if reply.error is not None:
-return 0, str(reply.error), None
-else:
-assert reply.result is not None
-return 0, None, str(reply.result)
-
-def close(self):
-self._conn.close()
-self.conn = None
-
-@staticmethod
-def create(path):
-assert isinstance(path, str)
-
-unix = "unix:%s" % ovs.util.abs_file_name(ovs.dirs.RUNDIR, path)
-error, stream = ovs.stream.Stream.open_block(
-ovs.stream.Stream.open(unix))
-
-if error:
-vlog.warn("failed to connect to %s" % path)
-return error, None
-
-return 0, UnixctlClient(ovs.jsonrpc.Connection(stream))
--
2.39.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2 0/1] python: Remove duplicate UnixctlClient implementation.
From: Jakob Meng Only change from v1 is that the "Fixes" tag in the commit message is not abbreviated anymore. Jakob Meng (1): python: Remove duplicate UnixctlClient implementation. python/ovs/unixctl/server.py | 44 1 file changed, 44 deletions(-) -- 2.39.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] editorconfig: Remove [*] section and trim_trailing_whitespace.
Hi Aaron,
first and foremost: I am fully committed to make our OVS community more
inclusive. Replacing terminology that promotes racial and cultural bias is one
of many (!) steps in achieving that. For the objective there is a broad
consensus, on the practical implementation not so much, it seems.
IBM Academy of Technology conducted a evaluation of IT terminology and
published a guideline on which terms to replace including rationale [0]:
[0] https://github.com/IBM/IBMInclusiveITLanguage
For example, it replaces {black,white}list with {block,allow}list because:
"As a pair, "blacklist" and "whitelist" promote racial bias by implying that
black is bad and white is good. When the terms 'white' or 'black' are used in a
context where white is represented as good or black is represented as bad, this
usage reinforces a model that promotes racial bias." [0]
However, for "white space" there is "No change recommended" because:
"This term is not based on a good/bad binary and so does not fall under our
guiding principle for black and white terms (see "blacklist")." [0]
Precision of expression is part of inclusive language. In my commit message I
am using the term "white space" because that is exactly the technical term used
in Unicode [1], editors and our ecosystem. Using other words such as
empty-space or blank-space could be confusing for readers.
[1] https://en.wikipedia.org/wiki/Whitespace_character
Although disturbed at first, I appreciate that you brought this up. We should
discuss this with Simon and others to get a common understanding of what
inclusive language is in practice. I assume you do not advocate for banning the
words white and black completely from the English language, do you?
Best,
Jakob
On 27.10.23 23:53, Aaron Conole wrote:
> Hi Jakob,
>
> Just a comment about the use of 'whitespace' in the commit message.
> There is an effort to try and use more inclusive language, so it might
> be good to use empty-space or blank-space in the commit message. I know
> that it is an editor config property, so not much to do about
> trim_trailing_whitespaces in the message.
>
> Additionally, for things like "contain trailing whitespaces" maybe we
> can use "contain trailing blank characters?"
>
> Thanks!
>
> jm...@redhat.com writes:
>
>> From: Jakob Meng
>>
>> Wildcard sections [*] and [**] are unsafe because properties cannot be
>> applied safely to any filetype in general. For example, IDEs like
>> Visual Studio Code and KDevelop store configuration files in subfolders
>> like .vscode or .kdev4. Properties from wildcard sections also apply to
>> those files which is not safe in general.
>> Another example are patches created with 'git format-patch' which can
>> contain trailing whitespaces. When editing a patch, e.g. to fix a typo
>> in the title, trailing whitespaces should not be removed.
>>
>> Property trim_trailing_whitespace should not be defined at all because
>> it is interpreted differently by editors. Some wipe whitespaces from
>> the whole file, others remove them from edited lines only and a few
>> change their behavior between releases [0]. Limiting the property to a
>> subset of files like *.c/*.h will not mitigate the issue:
>>
>> Multiple definitions of a whitespace exist. Unicode considers a form
>> feed (0x0C) to be a whitespace [1]. QChar::isSpace() [2] from Qt
>> follows this definition, causing the Kate editor identify a form feed
>> as a trailing whitespace and removing it from sources [3]. This breaks
>> patches when editors remove form feeds and thus causing broken patches
>> which cannot be applied cleanly.
>>
>> Removing trim_trailing_whitespace will be a minor inconvienence, in
>> particular because utilities/checkpatch.py and thus 0-day Robot will
>> prevent trailing whitespaces for our definition of a whitespace.
> Luckily, developers can install a hook that will run checkpatch on a
> commit. Something like the below installed in .git/hooks/pre-commit
> should run when trying to commit and catch trailing spaces errors.
>
> --8<--
> #!/bin/sh
> if git rev-parse --verify HEAD 2>/dev/null
> then
> git diff-index -p --cached HEAD
> else
> :
> fi | utilities/checkpatch.py -s -S
> -->8--
>
>> [0]
>> https://github.com/KDE/ktexteditor/commit/94b328fc64e543d91930845d2a96ce08d3043295
>> [1] https://en.wikipedia.org/wiki/Whitespace_character
>> [2]
>> https://github.com/qt/qtbase/blob/5628600a07295db6ed6683e97fafb0c45ddea505/src/corelib/text/qchar.h#L554
>> [3]
>> https://github.com/KDE/ktexteditor/blob/10210ec1dd06afa1e7b19a4fff722a8a23719161/src/document/katedocument.cpp#L5643
>>
>> Fixes: 07f6d6a0cb51 ("Add editorconfig file.")
>>
>> Signed-off-by: Jakob Meng
>> ---
>> .editorconfig | 13 -
>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/.editorconfig b/.editorconfig
>> index 685c72750..41ba51bf3 100644
>> --- a/.editorconfig
>> +++ b/.editorconfig
>> @@ -2,15 +2,18 @@
>>
>> root = true
>>
>>
[ovs-dev] [PATCH ovn v2] controller: fixed potential segfault when changing tunnel_key and deleting ls
When a tunnel_key for a datapath was changed, the local_datapaths hmap was not
properly updated
as the old/initial entry was not removed.
- If the datapath was not deleted at the same time, a new entry (for the same
dp) was created
in the local_datapaths as the previous entry was not found (wrong hash).
- If the datapath was deleted at the same time, the former entry also remained
(as, again, the hash
was wrong). So, we kept a deleted (dp) entry in the hmap, and might crash
when we used it later.
When tunnel_key is updated for an existing datapath, we now clean the
local_datapaths,
removing bad entries (i.e entries for which the hash is not the tunnel_key).
This issue was identified by flaky failures of test "ovn-ic -- route deletion
upon TS deletion".
Backtrace:
0 0x00504a9a in hmap_first_with_hash (hmap=0x20f4d90,
hmap@entry=0x5d5634, hmap=0x20f4d90, hmap@entry=0x5d5634, hash=1956414673) at
./include/openvswitch/hmap.h:360
1 smap_find__ (smap=smap@entry=0x20f4d90, key=key@entry=0x5d5634
"snat-ct-zone", key_len=key_len@entry=12, hash=1956414673) at lib/smap.c:421
2 0x00505073 in smap_get_node (key=0x5d5634 "snat-ct-zone",
smap=0x20f4d90) at lib/smap.c:217
3 smap_get_def (def=0x0, key=0x5d5634 "snat-ct-zone", smap=0x20f4d90) at
lib/smap.c:208
4 smap_get (smap=0x20f4d90, key=key@entry=0x5d5634 "snat-ct-zone") at
lib/smap.c:200
5 0x00419898 in get_common_nat_zone (ldp=0x20a8c40, ldp=0x20a8c40) at
controller/lflow.c:831
6 add_matches_to_flow_table (lflow=lflow@entry=0x21ddf90,
ldp=ldp@entry=0x20a8c40, matches=matches@entry=0x211bb50,
ptable=ptable@entry=10 '\n', output_ptable=output_ptable@entry=37 '%',
ovnacts=ovnacts@entry=0x7ffd19b99de0,
ingress=true, l_ctx_in=0x7ffd19b9a300, l_ctx_out=0x7ffd19b9a2c0) at
controller/lflow.c:892
7 0x0041a29b in consider_logical_flow__ (lflow=lflow@entry=0x21ddf90,
dp=0x20eb720, l_ctx_in=l_ctx_in@entry=0x7ffd19b9a300,
l_ctx_out=l_ctx_out@entry=0x7ffd19b9a2c0) at controller/lflow.c:1207
8 0x0041a587 in consider_logical_flow (lflow=lflow@entry=0x21ddf90,
is_recompute=is_recompute@entry=false, l_ctx_in=l_ctx_in@entry=0x7ffd19b9a300,
l_ctx_out=l_ctx_out@entry=0x7ffd19b9a2c0) at controller/lflow.c:1276
9 0x0041e30f in lflow_handle_changed_flows
(l_ctx_in=l_ctx_in@entry=0x7ffd19b9a300,
l_ctx_out=l_ctx_out@entry=0x7ffd19b9a2c0) at controller/lflow.c:271
10 0x00439fc7 in lflow_output_sb_logical_flow_handler
(node=0x7ffd19ba5b70, data=) at controller/ovn-controller.c:4045
11 0x004682fe in engine_compute (recompute_allowed=,
node=) at lib/inc-proc-eng.c:441
12 engine_run_node (recompute_allowed=true, node=0x7ffd19ba5b70) at
lib/inc-proc-eng.c:503
13 engine_run (recompute_allowed=recompute_allowed@entry=true) at
lib/inc-proc-eng.c:528
14 0x0040ade2 in main (argc=, argv=) at
controller/ovn-controller.c:5709
Signed-off-by: Xavier Simonart
---
v2: Fixed potential memory leak.
---
controller/local_data.c | 14 +
controller/local_data.h | 4
controller/ovn-controller.c | 22 +++
tests/ovn.at| 42 +
4 files changed, 82 insertions(+)
diff --git a/controller/local_data.c b/controller/local_data.c
index 3a7d3afeb..8f0890a14 100644
--- a/controller/local_data.c
+++ b/controller/local_data.c
@@ -56,6 +56,20 @@ static bool datapath_is_transit_switch(const struct
sbrec_datapath_binding *);
static uint64_t local_datapath_usage;
+/* To be used when hmap_node.hash might be wrong e.g. tunnel_key got updated */
+struct local_datapath *
+get_local_datapath_no_hash(const struct hmap *local_datapaths,
+uint32_t tunnel_key)
+{
+struct local_datapath *ld;
+HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
+if (ld->datapath->tunnel_key == tunnel_key) {
+return ld;
+}
+}
+return NULL;
+}
+
struct local_datapath *
get_local_datapath(const struct hmap *local_datapaths, uint32_t tunnel_key)
{
diff --git a/controller/local_data.h b/controller/local_data.h
index f6d8f725f..a1bf0790b 100644
--- a/controller/local_data.h
+++ b/controller/local_data.h
@@ -69,6 +69,10 @@ struct local_datapath *local_datapath_alloc(
const struct sbrec_datapath_binding *);
struct local_datapath *get_local_datapath(const struct hmap *,
uint32_t tunnel_key);
+struct local_datapath
+*get_local_datapath_no_hash(const struct hmap *local_datapaths,
+ uint32_t tunnel_key);
+
bool
need_add_peer_to_local(
struct ovsdb_idl_index *sbrec_port_binding_by_name,
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index da7d145ed..ca8e383d8 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -1902,6 +1902,7 @@ runtime_data_sb_datapath_binding_handler(struct
engine_node *node OVS_UNUSED,
[ovs-dev] [PATCH ovn v2] controller: have I+P assigning ct_zones for l3gateway ports
When l3gateway ports get added, ct_zones were assigned during
(ct_zones) recomputes, but not by I+P.
Before this patch, test "Migration of CT zone from UUID to name"
was randomly failing, as ct_zone was not assigned by I+P but
a ct_zone recompute happened most of the time (and hence test succeeded).
Test case has been adapted so that ct_zone recompute usually happens
before adding the sw0-lr0 port.
Signed-off-by: Xavier Simonart
---
v2: - updated comment as suggested by Dumitru
- rebased on origin/main
---
controller/ovn-controller.c | 5 +++--
tests/ovn.at| 6 +++---
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index da7d145ed..d19e6ac05 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -2642,9 +2642,10 @@ ct_zones_runtime_data_handler(struct engine_node *node,
void *data)
struct tracked_lport *t_lport = shash_node->data;
if (strcmp(t_lport->pb->type, "")
&& strcmp(t_lport->pb->type, "localport")
+&& strcmp(t_lport->pb->type, "l3gateway")
&& strcmp(t_lport->pb->type, "localnet")) {
-/* We allocate zone-id's only to VIF, localport, and localnet
- * lports. */
+/* We allocate zone-id's only to VIF, localport, l3gateway,
+ * and localnet lports. */
continue;
}
diff --git a/tests/ovn.at b/tests/ovn.at
index 637d92bed..91ed8f73b 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -36813,14 +36813,14 @@ check ovn-nbctl ls-add sw0
check ovn-nbctl lsp-add sw0 sw0-port1
check ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:01 192.168.0.2"
+ovs-vsctl add-port br-int p1 -- \
+set Interface p1 external_ids:iface-id=sw0-port1
+
check ovn-nbctl lsp-add sw0 sw0-lr0
check ovn-nbctl lsp-set-type sw0-lr0 router
check ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01
check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0
-ovs-vsctl add-port br-int p1 -- \
-set Interface p1 external_ids:iface-id=sw0-port1
-
check ovn-appctl -t ovn-controller vlog/set dbg:main
wait_for_ports_up
--
2.31.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v5 3/3] userspace: Add Generic Segmentation Offloading.
From: Flavio Leitner
This provides a software implementation in the case
the egress netdev doesn't support segmentation in hardware.
The challenge here is to guarantee packet ordering in the
original batch that may be full of TSO packets. Each TSO
packet can go up to ~64kB, so with segment size of 1440
that means about 44 packets for each TSO. Each batch has
32 packets, so the total batch amounts to 1408 normal
packets.
The segmentation estimates the total number of packets
and then the total number of batches. Then allocate
enough memory and finally do the work.
Finally each batch is sent in order to the netdev.
Signed-off-by: Flavio Leitner
Co-authored-by: Mike Pattrick
Signed-off-by: Mike Pattrick
---
lib/automake.mk | 2 +
lib/dp-packet-gso.c | 168
lib/dp-packet-gso.h | 23 ++
lib/dp-packet.h | 7 ++
lib/netdev-dpdk.c | 44 ---
lib/netdev.c| 139 +
lib/packets.c | 4 +-
tests/system-traffic.at | 45 +++
8 files changed, 367 insertions(+), 65 deletions(-)
create mode 100644 lib/dp-packet-gso.c
create mode 100644 lib/dp-packet-gso.h
diff --git a/lib/automake.mk b/lib/automake.mk
index 24b0ffefe..82dd85c5f 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -118,6 +118,8 @@ lib_libopenvswitch_la_SOURCES = \
lib/dpctl.h \
lib/dp-packet.h \
lib/dp-packet.c \
+ lib/dp-packet-gso.c \
+ lib/dp-packet-gso.h \
lib/dpdk.h \
lib/dpif-netdev-extract-study.c \
lib/dpif-netdev-lookup.h \
diff --git a/lib/dp-packet-gso.c b/lib/dp-packet-gso.c
new file mode 100644
index 0..44dc8061e
--- /dev/null
+++ b/lib/dp-packet-gso.c
@@ -0,0 +1,168 @@
+/*
+ * Copyright (c) 2023 Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include
+#include
+#include
+
+#include "dp-packet.h"
+#include "dp-packet-gso.h"
+#include "netdev-provider.h"
+#include "openvswitch/vlog.h"
+
+VLOG_DEFINE_THIS_MODULE(dp_packet_gso);
+
+/* Retuns a new packet that is a segment of packet 'p'.
+ *
+ * The new packet is initialized with 'hdr_len' bytes from the
+ * start of packet 'p' and then appended with 'data_len' bytes
+ * from the 'data' buffer.
+ *
+ * Note: The packet headers are not updated. */
+static struct dp_packet *
+dp_packet_gso_seg_new(const struct dp_packet *p, size_t hdr_len,
+ const char *data, size_t data_len)
+{
+struct dp_packet *seg = dp_packet_new_with_headroom(hdr_len + data_len,
+dp_packet_headroom(p));
+
+/* Append the original packet headers and then the payload. */
+dp_packet_put(seg, dp_packet_data(p), hdr_len);
+dp_packet_put(seg, data, data_len);
+
+/* The new segment should have the same offsets. */
+seg->l2_5_ofs = p->l2_5_ofs;
+seg->l3_ofs = p->l3_ofs;
+seg->l4_ofs = p->l4_ofs;
+
+/* The protocol headers remain the same, so preserve hash and mark. */
+*dp_packet_rss_ptr(seg) = *dp_packet_rss_ptr(p);
+*dp_packet_flow_mark_ptr(seg) = *dp_packet_flow_mark_ptr(p);
+
+/* The segment should inherit all the offloading flags from the
+ * original packet, except for the TCP segmentation, external
+ * buffer and indirect buffer flags. */
+*dp_packet_ol_flags_ptr(seg) = *dp_packet_ol_flags_ptr(p)
+& DP_PACKET_OL_SUPPORTED_MASK;
+
+dp_packet_hwol_reset_tcp_seg(seg);
+
+return seg;
+}
+
+/* Returns the calculated number of TCP segments in packet 'p'. */
+int
+dp_packet_gso_nr_segs(struct dp_packet *p)
+{
+uint16_t segsz = dp_packet_get_tso_segsz(p);
+const char *data_tail;
+const char *data_pos;
+
+data_pos = dp_packet_get_tcp_payload(p);
+data_tail = (char *) dp_packet_tail(p) - dp_packet_l2_pad_size(p);
+
+return DIV_ROUND_UP((data_tail - data_pos), segsz);
+}
+
+/* Perform software segmentation on packet 'p'.
+ *
+ * Segments packet 'p' into the array of preallocated batches in 'batches',
+ * updating the 'batches' pointer as needed and returns true.
+ *
+ * Returns false if the packet cannot be segmented. */
+bool
+dp_packet_gso(struct dp_packet *p, struct dp_packet_batch **batches)
+{
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+struct dp_packet_batch *curr_batch = *batches;
+struct tcp_header *tcp_hdr;
+struct ip_header *ip_hdr;
+struct dp_packet
[ovs-dev] [PATCH v5 1/3] netdev-linux: Use ethtool to detect offload support.
Currently when userspace-tso is enabled, netdev-linux interfaces will
indicate support for all offload flags regardless of interface
configuration. This patch checks for which offload features are enabled
during netdev construction.
Signed-off-by: Mike Pattrick
---
lib/netdev-linux.c | 156 ++--
tests/ofproto-macros.at | 1 +
2 files changed, 150 insertions(+), 7 deletions(-)
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index cca340879..d1482178b 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -558,6 +558,7 @@ static bool netdev_linux_miimon_enabled(void);
static void netdev_linux_miimon_run(void);
static void netdev_linux_miimon_wait(void);
static int netdev_linux_get_mtu__(struct netdev_linux *netdev, int *mtup);
+static void netdev_linux_set_ol(struct netdev *netdev);
static bool
is_tap_netdev(const struct netdev *netdev)
@@ -959,14 +960,8 @@ netdev_linux_construct(struct netdev *netdev_)
return error;
}
-/* The socket interface doesn't offer the option to enable only
- * csum offloading without TSO. */
if (userspace_tso_enabled()) {
-netdev_->ol_flags |= NETDEV_TX_OFFLOAD_TCP_TSO;
-netdev_->ol_flags |= NETDEV_TX_OFFLOAD_TCP_CKSUM;
-netdev_->ol_flags |= NETDEV_TX_OFFLOAD_UDP_CKSUM;
-netdev_->ol_flags |= NETDEV_TX_OFFLOAD_SCTP_CKSUM;
-netdev_->ol_flags |= NETDEV_TX_OFFLOAD_IPV4_CKSUM;
+netdev_linux_set_ol(netdev_);
}
error = get_flags(>up, >ifi_flags);
@@ -2381,6 +2376,153 @@ netdev_internal_get_stats(const struct netdev *netdev_,
return error;
}
+static int
+netdev_linux_read_stringset_info(struct netdev_linux *netdev, uint32_t *len)
+{
+/*
+struct {
+union {
+struct ethtool_sset_info hdr;
+struct {
+uint64_t pad[2];
+uint32_t sset_len[1];
+};
+};
+} sset_info;
+*/
+union {
+struct ethtool_sset_info hdr;
+struct {
+uint64_t pad[2];
+uint32_t sset_len[1];
+};
+} sset_info;
+
+sset_info.hdr.cmd = ETHTOOL_GSSET_INFO;
+sset_info.hdr.reserved = 0;
+sset_info.hdr.sset_mask = 1ULL << ETH_SS_FEATURES;
+
+int error = netdev_linux_do_ethtool(netdev->up.name,
+(struct ethtool_cmd *)_info,
+ETHTOOL_GSSET_INFO, "ETHTOOL_GSSET_INFO");
+if (error) {
+return error;
+}
+*len = sset_info.sset_len[0];
+return 0;
+}
+
+
+static int
+netdev_linux_read_definitions(struct netdev_linux *netdev,
+ struct ethtool_gstrings **pstrings)
+{
+int error = 0;
+struct ethtool_gstrings *strings = NULL;
+uint32_t len = 0;
+
+error = netdev_linux_read_stringset_info(netdev, );
+if (error || !len) {
+return error;
+}
+strings = xcalloc(1, sizeof(*strings) + len * ETH_GSTRING_LEN);
+if (!strings) {
+return ENOMEM;
+}
+
+strings->cmd = ETHTOOL_GSTRINGS;
+strings->string_set = ETH_SS_FEATURES;
+strings->len = len;
+error = netdev_linux_do_ethtool(netdev->up.name,
+(struct ethtool_cmd *) strings,
+ETHTOOL_GSTRINGS, "ETHTOOL_GSTRINGS");
+if (error) {
+goto out;
+}
+
+for (int i = 0; i < len; i++) {
+strings->data[(i + 1) * ETH_GSTRING_LEN - 1] = 0;
+}
+
+*pstrings = strings;
+
+return 0;
+out:
+*pstrings = NULL;
+free(strings);
+return error;
+}
+
+static void
+netdev_linux_set_ol(struct netdev *netdev_)
+{
+struct netdev_linux *netdev = netdev_linux_cast(netdev_);
+struct ethtool_gstrings *names = NULL;
+struct ethtool_gfeatures *features = NULL;
+int error;
+
+COVERAGE_INC(netdev_get_ethtool);
+
+error = netdev_linux_read_definitions(netdev, );
+if (error) {
+return;
+}
+
+features = xmalloc(sizeof *features +
+ DIV_ROUND_UP(names->len, 32) *
+ sizeof features->features[0]);
+if (!features) {
+goto out;
+}
+
+features->cmd = ETHTOOL_GFEATURES;
+features->size = DIV_ROUND_UP(names->len, 32);
+error = netdev_linux_do_ethtool(netdev_get_name(netdev_),
+(struct ethtool_cmd *) features,
+ETHTOOL_GFEATURES, "ETHTOOL_GFEATURES");
+
+if (error) {
+goto out;
+}
+
+#define FEATURE_WORD(blocks, index, field) ((blocks)[(index) / 32U].field)
+#define FEATURE_FIELD_FLAG(index) (1U << (index) % 32U)
+#define FEATURE_BIT_IS_SET(blocks, index, field)\
+(FEATURE_WORD(blocks, index, field) & FEATURE_FIELD_FLAG(index))
+
+netdev->up.ol_flags = 0;
+static const struct {
+char * string;
+int value;
+} t_list[] = {
+{"tx-checksum-ipv4", NETDEV_TX_OFFLOAD_IPV4_CKSUM |
+ NETDEV_TX_OFFLOAD_TCP_CKSUM |
+ NETDEV_TX_OFFLOAD_UDP_CKSUM},
+
[ovs-dev] [PATCH v5 2/3] userspace: Respect tso/gso segment size.
From: Flavio Leitner
Currently OVS will calculate the segment size based on the
MTU of the egress port. That usually happens to be correct
when the ports share the same MTU, but that is not always true.
Therefore, if the segment size is provided, then use that and
make sure the over sized packets are dropped.
Signed-off-by: Flavio Leitner
Co-authored-by: Mike Pattrick
Signed-off-by: Mike Pattrick
---
lib/dp-packet.c| 3 ++
lib/dp-packet.h| 26
lib/netdev-dpdk.c | 12 +++-
lib/netdev-linux.c | 76 --
4 files changed, 94 insertions(+), 23 deletions(-)
diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index ed004c3b9..920402369 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -34,6 +34,7 @@ dp_packet_init__(struct dp_packet *b, size_t allocated, enum
dp_packet_source so
pkt_metadata_init(>md, 0);
dp_packet_reset_cutlen(b);
dp_packet_reset_offload(b);
+dp_packet_set_tso_segsz(b, 0);
/* Initialize implementation-specific fields of dp_packet. */
dp_packet_init_specific(b);
/* By default assume the packet type to be Ethernet. */
@@ -203,6 +204,8 @@ dp_packet_clone_with_headroom(const struct dp_packet
*buffer, size_t headroom)
*dp_packet_ol_flags_ptr(new_buffer) = *dp_packet_ol_flags_ptr(buffer);
*dp_packet_ol_flags_ptr(new_buffer) &= DP_PACKET_OL_SUPPORTED_MASK;
+dp_packet_set_tso_segsz(new_buffer, dp_packet_get_tso_segsz(buffer));
+
if (dp_packet_rss_valid(buffer)) {
dp_packet_set_rss_hash(new_buffer, dp_packet_get_rss_hash(buffer));
}
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 70ddf8aa4..6a924f3ff 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -126,6 +126,7 @@ struct dp_packet {
uint32_t ol_flags; /* Offloading flags. */
uint32_t rss_hash; /* Packet hash. */
uint32_t flow_mark; /* Packet flow mark. */
+uint16_t tso_segsz; /* TCP TSO segment size. */
#endif
enum dp_packet_source source; /* Source of memory allocated as 'base'. */
@@ -166,6 +167,9 @@ static inline void dp_packet_set_size(struct dp_packet *,
uint32_t);
static inline uint16_t dp_packet_get_allocated(const struct dp_packet *);
static inline void dp_packet_set_allocated(struct dp_packet *, uint16_t);
+static inline uint16_t dp_packet_get_tso_segsz(const struct dp_packet *);
+static inline void dp_packet_set_tso_segsz(struct dp_packet *, uint16_t);
+
void *dp_packet_resize_l2(struct dp_packet *, int increment);
void *dp_packet_resize_l2_5(struct dp_packet *, int increment);
static inline void *dp_packet_eth(const struct dp_packet *);
@@ -644,6 +648,17 @@ dp_packet_set_allocated(struct dp_packet *b, uint16_t s)
b->mbuf.buf_len = s;
}
+static inline uint16_t
+dp_packet_get_tso_segsz(const struct dp_packet *p)
+{
+return p->mbuf.tso_segsz;
+}
+
+static inline void
+dp_packet_set_tso_segsz(struct dp_packet *p, uint16_t s)
+{
+p->mbuf.tso_segsz = s;
+}
#else /* DPDK_NETDEV */
static inline void
@@ -700,6 +715,17 @@ dp_packet_set_allocated(struct dp_packet *b, uint16_t s)
b->allocated_ = s;
}
+static inline uint16_t
+dp_packet_get_tso_segsz(const struct dp_packet *p)
+{
+return p->tso_segsz;
+}
+
+static inline void
+dp_packet_set_tso_segsz(struct dp_packet *p, uint16_t s)
+{
+p->tso_segsz = s;
+}
#endif /* DPDK_NETDEV */
static inline void
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 55700250d..9f20cc689 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -2453,6 +2453,7 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk *dev,
struct rte_mbuf *mbuf)
if (mbuf->ol_flags & RTE_MBUF_F_TX_TCP_SEG) {
struct tcp_header *th = dp_packet_l4(pkt);
+int hdr_len;
if (!th) {
VLOG_WARN_RL(, "%s: TCP Segmentation without L4 header"
@@ -2462,7 +2463,15 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk *dev,
struct rte_mbuf *mbuf)
mbuf->l4_len = TCP_OFFSET(th->tcp_ctl) * 4;
mbuf->ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM;
+hdr_len = mbuf->l2_len + mbuf->l3_len + mbuf->l4_len;
mbuf->tso_segsz = dev->mtu - mbuf->l3_len - mbuf->l4_len;
+if (OVS_UNLIKELY((hdr_len + mbuf->tso_segsz) > dev->max_packet_len)) {
+VLOG_WARN_RL(, "%s: Oversized TSO packet. "
+ "hdr: %"PRIu32", gso: %"PRIu32", max len: %"PRIu32"",
+ dev->up.name, hdr_len, mbuf->tso_segsz,
+ dev->max_packet_len);
+return false;
+}
if (mbuf->ol_flags & RTE_MBUF_F_TX_IPV4) {
mbuf->ol_flags |= RTE_MBUF_F_TX_IP_CKSUM;
@@ -2737,7 +2746,8 @@ netdev_dpdk_filter_packet_len(struct netdev_dpdk *dev,
struct rte_mbuf **pkts,
int cnt = 0;
struct rte_mbuf *pkt;
-/* Filter oversized packets, unless are marked for TSO. */
+/* Filter oversized packets. The TSO packets are filtered out
+ * during the
