Re: [ovs-dev] [PATCH ovn v6 2/4] northd, utils: support for RouteTables in LRs
On Tue, Oct 5, 2021 at 1:26 PM Vladislav Odintsov wrote:
>
> This patch extends Logical Router's routing functionality.
> Now user may create multiple routing tables within a Logical Router
> and assign them to Logical Router Ports.
>
> Traffic coming from Logical Router Port with assigned route_table
> is checked against global routes if any (Logical_Router_Static_Routes
> whith empty route_table field), next against directly connected routes
This is not accurate. The "directly connected routes" is NOT after the
global routes. Their priority only depends on the prefix length.
> and then Logical_Router_Static_Routes with same route_table value as
> in Logical_Router_Port options:route_table field.
>
> A new Logical Router ingress table #10 is added - IN_IP_ROUTING_PRE.
> In this table packets which come from LRPs with configured
> options:route_table field are checked against inport and in OVS
> register 7 unique non-zero value identifying route table is written.
>
> Then in 11th table IN_IP_ROUTING routes which have non-empty
> `route_table` field are added with additional match on reg7 value
> associated with appropriate route_table.
>
Hi Vladislav,
First of all, sorry for the delayed review, and thanks for implementing
this new feature.
I have some questions regarding the feature itself. I remember that we had
some discussion earlier for this feature, but it seems I misunderstood the
feature you are implementing here. I thought by multiple routing tables you
were trying to support something like VRF in physical routers, but this
seems to be something different. According to your implementation, instead
of assigning LRPs to different routing tables, a LRP can actually
participate to both the global routing table and the table with a specific
ID. For ingress, global routes are prefered over other routes; for egress
(i.e. forwarding to the next hop), it doesn't even enforce any table-id
check, so packet routed by a entry of table-X can go out of a LRP with
table-id Y. Is my understanding correct about the desired behavior of this
feature?
If this is correct, it doesn't seem to be a common/standard requirement (or
please educate me if I am wrong). Could you explain a little more about the
actual use cases: what kind of real world problems need to be solved by
this feature or how are you going to use this. For example, why would a
port need to participate in both routing tables? It looks like what you
really need is policy routing instead of multiple isolated routing tables.
I understand that you already use policy routing to implement ACLs, so it
is not convenient to combine other policies (e.g. inport based routing)
into the policy routing stage. If that's the case, would it be more generic
to support multiple policy routing stages? My concern to the current
approach is that it is implemented for a very special use case. It makes
the code more complex but when there is a slightly different requirement in
the future it becomes insufficient. I am thinking that policy routing seems
more flexible and has more potential to be made more generic. Maybe I will
have a better understanding when I hear more detailed use cases and
considerations from you.
I haven't finished reviewing the code yet, but I have one comment regarding
adding 100 to the priority of the global routes. For IPv6, the priority
range from 0 to 120x2=240, so adding 100 is not enough. It would create
overlapping priority ranges, and some table-id specific route entries may
override the global routes.
Thanks,
Han
> Signed-off-by: Vladislav Odintsov
> Acked-by: Numan Siddique
> ---
> northd/northd.c | 159 ---
> northd/ovn-northd.8.xml | 63 --
> ovn-nb.ovsschema| 5 +-
> ovn-nb.xml | 30 +++
> tests/ovn-ic.at | 4 +
> tests/ovn-nbctl.at | 196 +-
> tests/ovn-northd.at | 76 ++-
> tests/ovn.at| 441 +++-
> utilities/ovn-nbctl.c | 134 +++-
> 9 files changed, 1041 insertions(+), 67 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 092eca829..6a020cb2e 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -148,15 +148,16 @@ enum ovn_stage {
> PIPELINE_STAGE(ROUTER, IN, ECMP_STATEFUL, 7,
"lr_in_ecmp_stateful") \
> PIPELINE_STAGE(ROUTER, IN, ND_RA_OPTIONS, 8,
"lr_in_nd_ra_options") \
> PIPELINE_STAGE(ROUTER, IN, ND_RA_RESPONSE, 9,
"lr_in_nd_ra_response") \
> -PIPELINE_STAGE(ROUTER, IN, IP_ROUTING, 10, "lr_in_ip_routing")
\
> -PIPELINE_STAGE(ROUTER, IN, IP_ROUTING_ECMP, 11,
"lr_in_ip_routing_ecmp") \
> -PIPELINE_STAGE(ROUTER, IN, POLICY, 12, "lr_in_policy")
\
> -PIPELINE_STAGE(ROUTER, IN, POLICY_ECMP, 13,
"lr_in_policy_ecmp") \
> -PIPELINE_STAGE(ROUTER, IN, ARP_RESOLVE, 14,
"lr_in_arp_resolve") \
> -PIPELINE_STAGE(ROUTER, IN, CHK_PKT_LEN , 15,
"lr_in_chk_pkt_len") \
> -
Re: [ovs-dev] [PATCH ovn v6 1/4] ic: process only local port_bindings
On Tue, Oct 5, 2021 at 1:25 PM Vladislav Odintsov wrote:
>
> This commit adds a small optimization by utilizing ovsdb_index
> to iterate over port_bindings.
> Prior to this change each iteration checked availability_zone
> and continued processing only if port_binding belons to local AZ.
>
> Now we run against port_bindings from local AZ only and don't check
> availability_zone.
>
> Signed-off-by: Vladislav Odintsov
> Acked-by: Numan Siddique
> ---
> ic/ovn-ic.c | 16 ++--
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> index 99356253d..303e93a4f 100644
> --- a/ic/ovn-ic.c
> +++ b/ic/ovn-ic.c
> @@ -68,6 +68,7 @@ struct ic_context {
> struct ovsdb_idl_index *sbrec_port_binding_by_name;
> struct ovsdb_idl_index *icsbrec_port_binding_by_az;
> struct ovsdb_idl_index *icsbrec_port_binding_by_ts;
> +struct ovsdb_idl_index *icsbrec_port_binding_by_ts_az;
> struct ovsdb_idl_index *icsbrec_route_by_ts;
> struct ovsdb_idl_index *icsbrec_route_by_ts_az;
> };
> @@ -1386,17 +1387,14 @@ route_run(struct ic_context *ctx,
> const struct icsbrec_port_binding *isb_pb;
> const struct icsbrec_port_binding *isb_pb_key =
> icsbrec_port_binding_index_init_row(
> -ctx->icsbrec_port_binding_by_ts);
> +ctx->icsbrec_port_binding_by_ts_az);
> icsbrec_port_binding_index_set_transit_switch(isb_pb_key,
ts->name);
> +icsbrec_port_binding_index_set_availability_zone(isb_pb_key, az);
This index row needs to be destroyed by calling
icsbrec_port_binding_index_destroy_row() after the below FOR loop.
Otherwise this patch looks good to me.
Thanks,
Han
>
> /* Each port on TS maps to a logical router, which is stored in
the
> * external_ids:router-id of the IC SB port_binding record. */
> ICSBREC_PORT_BINDING_FOR_EACH_EQUAL (isb_pb, isb_pb_key,
> -
ctx->icsbrec_port_binding_by_ts) {
> -if (isb_pb->availability_zone != az) {
> -continue;
> -}
> -
> +ctx->icsbrec_port_binding_by_ts_az) {
> const char *ts_lrp_name =
> get_lrp_name_by_ts_port_name(ctx, isb_pb->logical_port);
> if (!ts_lrp_name) {
> @@ -1713,6 +1711,11 @@ main(int argc, char *argv[])
> = ovsdb_idl_index_create1(ovnisb_idl_loop.idl,
>
_port_binding_col_transit_switch);
>
> +struct ovsdb_idl_index *icsbrec_port_binding_by_ts_az
> += ovsdb_idl_index_create2(ovnisb_idl_loop.idl,
> +
_port_binding_col_transit_switch,
> +
_port_binding_col_availability_zone);
> +
> struct ovsdb_idl_index *icsbrec_route_by_ts
> = ovsdb_idl_index_create1(ovnisb_idl_loop.idl,
>_route_col_transit_switch);
> @@ -1763,6 +1766,7 @@ main(int argc, char *argv[])
> .sbrec_chassis_by_name = sbrec_chassis_by_name,
> .icsbrec_port_binding_by_az = icsbrec_port_binding_by_az,
> .icsbrec_port_binding_by_ts = icsbrec_port_binding_by_ts,
> +.icsbrec_port_binding_by_ts_az =
icsbrec_port_binding_by_ts_az,
> .icsbrec_route_by_ts = icsbrec_route_by_ts,
> .icsbrec_route_by_ts_az = icsbrec_route_by_ts_az,
> };
> --
> 2.30.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] [python] Allow custom transaction ops to be added
It can be useful to be able to send raw transaction operations
through the Idl's connection. For example, to clean up MAC_Binding
entries for floating IPs without having to monitor the MAC_Binding
table which can be quite large.
Signed-off-by: Terry Wilson
---
python/ovs/db/idl.py | 18 +-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index ecae5e143..b2fd30775 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -1405,6 +1405,8 @@ class Transaction(object):
self._inserted_rows = {} # Map from UUID to _InsertedRow
+self._operations = []
+
def add_comment(self, comment):
"""Appends 'comment' to the comments that will be passed to the OVSDB
server when this transaction is committed. (The comment will be
@@ -1535,7 +1537,7 @@ class Transaction(object):
"rows": [rows]})
# Add updates.
-any_updates = False
+any_updates = bool(self._operations)
for row in self._txn_rows.values():
if row._changes is None:
if row._table.is_root:
@@ -1670,6 +1672,7 @@ class Transaction(object):
if self.dry_run:
operations.append({"op": "abort"})
+operations += self._operations
if not any_updates:
self._status = Transaction.UNCHANGED
else:
@@ -1684,6 +1687,19 @@ class Transaction(object):
self.__disassemble()
return self._status
+def add_op(self, op):
+"""Add a raw OVSDB operation to the transaction
+
+This can be useful for re-using the existing Idl connection to take
+actions that are difficult or expensive to do with the Idl itself. e.g.
+deleting a bunch of rows on the server that you don't want to store
+in memory.
+
+:param op: An "op" for an OVSDB "transact" request (rfc 7047 Sec 5.2)
+:type op: dict
+"""
+self._operations.append(op)
+
def commit_block(self):
"""Attempts to commit this transaction, blocking until the commit
either succeeds or fails. Returns the final commit status, which may
--
2.31.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] python: replace pyOpenSSL with ssl
On Wed, Oct 6, 2021 at 1:50 PM Timothy Redaelli wrote: > > Currently, pyOpenSSL is half-deprecated upstream and so it's removed on > some distributions (for example on CentOS Stream 9, > https://issues.redhat.com/browse/CS-336), but since OVS only > supports Python 3 it's possible to replace pyOpenSSL with "import ssl" > included in base Python 3. > > Stream recv and send had to be splitted as _recv and _send, since SSLError > is a subclass of socket.error and so it was not possible to except for > SSLWantReadError and SSLWantWriteError in recv and send of SSLStream. > > Reported-by: Timothy Redaelli > Reported-at: https://bugzilla.redhat.com/1988429 > Signed-off-by: Timothy Redaelli > --- > v1 -> v2: > - asyncronous connect with ssl is different than with pyOpenSSL, so a > different approach is needed or it fails when connect is not ready > when ctx.wrap_socket is executed. > Moreover it's not possible to use connect, but it's necessary to use > connect_ex (probably due to a python bug or limitation). > So to do not behave diffently than TCPSocket, it's necessary to raise > the errno exception like connect do. > --- > .ci/linux-prepare.sh | 2 +- > .cirrus.yml | 2 +- > .travis.yml | 1 - > python/ovs/poller.py | 6 +-- > python/ovs/stream.py | 118 +-- > tests/ovsdb-idl.at | 2 +- > 6 files changed, 86 insertions(+), 45 deletions(-) > > diff --git a/.ci/linux-prepare.sh b/.ci/linux-prepare.sh > index c55125cf7..b9b499bad 100755 > --- a/.ci/linux-prepare.sh > +++ b/.ci/linux-prepare.sh > @@ -21,7 +21,7 @@ make -j4 HAVE_LLVM= HAVE_SQLITE= install > cd .. > > pip3 install --disable-pip-version-check --user \ > -flake8 hacking sphinx pyOpenSSL wheel setuptools > +flake8 hacking sphinx wheel setuptools > pip3 install --user --upgrade docutils > pip3 install --user 'meson==0.47.1' > > diff --git a/.cirrus.yml b/.cirrus.yml > index 358f2ba25..bb206f35f 100644 > --- a/.cirrus.yml > +++ b/.cirrus.yml > @@ -9,7 +9,7 @@ freebsd_build_task: > >env: > DEPENDENCIES: automake libtool gmake gcc wget openssl python3 > -PY_DEPS: sphinx|openssl > +PY_DEPS: sphinx > matrix: >COMPILER: gcc >COMPILER: clang > diff --git a/.travis.yml b/.travis.yml > index 51d051108..c7aeede06 100644 > --- a/.travis.yml > +++ b/.travis.yml > @@ -17,7 +17,6 @@ addons: >- libjemalloc-dev >- libnuma-dev >- libpcap-dev > - - python3-openssl >- python3-pip >- python3-sphinx >- libelf-dev > diff --git a/python/ovs/poller.py b/python/ovs/poller.py > index 3624ec865..157719c3a 100644 > --- a/python/ovs/poller.py > +++ b/python/ovs/poller.py > @@ -26,9 +26,9 @@ if sys.platform == "win32": > import ovs.winutils as winutils > > try: > -from OpenSSL import SSL > +import ssl > except ImportError: > -SSL = None > +ssl = None > > try: > from eventlet import patcher as eventlet_patcher > @@ -73,7 +73,7 @@ class _SelectSelect(object): > def register(self, fd, events): > if isinstance(fd, socket.socket): > fd = fd.fileno() > -if SSL and isinstance(fd, SSL.Connection): > +if ssl and isinstance(fd, ssl.SSLSocket): > fd = fd.fileno() > > if sys.platform != 'win32': > diff --git a/python/ovs/stream.py b/python/ovs/stream.py > index f5a520862..205e74888 100644 > --- a/python/ovs/stream.py > +++ b/python/ovs/stream.py > @@ -22,9 +22,9 @@ import ovs.socket_util > import ovs.vlog > > try: > -from OpenSSL import SSL > +import ssl > except ImportError: > -SSL = None > +ssl = None > > if sys.platform == 'win32': > import ovs.winutils as winutils > @@ -322,6 +322,12 @@ class Stream(object): > The recv function will not block waiting for data to arrive. If no > data have been received, it returns (errno.EAGAIN, "") > immediately.""" > > +try: > +return self._recv(n) > +except socket.error as e: > +return (ovs.socket_util.get_exception_errno(e), "") > + > +def _recv(self, n): > retval = self.connect() > if retval != 0: > return (retval, "") > @@ -331,10 +337,7 @@ class Stream(object): > if sys.platform == 'win32' and self.socket is None: > return self.__recv_windows(n) > > -try: > -return (0, self.socket.recv(n)) > -except socket.error as e: > -return (ovs.socket_util.get_exception_errno(e), "") > +return (0, self.socket.recv(n)) > > def __recv_windows(self, n): > if self._read_pending: > @@ -396,6 +399,12 @@ class Stream(object): > Will not block. If no bytes can be immediately accepted for > transmission, returns -errno.EAGAIN immediately.""" > > +try: > +return self._send(buf) > +except socket.error as e: > +return
[ovs-dev] [PATCH ovn 2/3] CoPP: add self-test for bfd controller action
Introduce CoPP selftest for bfd controller action
Signed-off-by: Lorenzo Bianconi
---
tests/system-ovn.at | 29 +
1 file changed, 29 insertions(+)
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index bd425f54b..12f072b72 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -6751,6 +6751,35 @@ OVS_WAIT_UNTIL([
test "${n_icmp}" = "2"
])
+check ovn-nbctl meter-add bfd-meter drop 1 pktps 0
+check ovn-nbctl --wait=hv lr-copp-add R1 bfd bfd-meter
+AT_CHECK([ovn-nbctl lr-copp-list R1 |grep bfd], [0], [dnl
+bfd: bfd-meter
+])
+
+check ovn-nbctl --bfd lr-route-add R1 240.0.0.0/8 172.16.1.50 rp-public
+disc=$(ovn-sbctl list bfd | awk '/disc/{print $3}')
+printf "%08x" $disc > /tmp/disc
+NS_EXEC([server], [tcpdump -n -i s1 udp port 3784 -Q in > bfd.pcap &])
+ip netns exec server scapy -H <<-EOF
+import binascii
+f = open("/tmp/disc", "r")
+bfd = binascii.unhexlify("20600518a899e77b" + f.readline().strip() +
"000f424f4240")
+p = IP(src="172.16.1.50", dst="172.16.1.1")/ UDP(dport = 3784, sport = 49152)
/ Raw(load = bfd)
+send (p, iface='s1', loop = 0, verbose = 0, count = 100)
+f.close()
+EOF
+rm /tmp/disc
+
+sleep 2
+kill $(pidof tcpdump)
+
+# 1pps + 1 burst size
+OVS_WAIT_UNTIL([
+n_tcp_rst=$(grep Final bfd.pcap | wc -l)
+test "${n_tcp_rst}" = "2"
+])
+
kill $(pidof ovn-controller)
as ovn-sb
--
2.31.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 3/3] CoPP: add self-test for tcp-reset controller action
Introduce CoPP selftest for tcp-reset controller action Signed-off-by: Lorenzo Bianconi --- tests/ovn-northd.at | 11 +++ 1 file changed, 11 insertions(+) diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 3bcbdf4a9..c180ba65e 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -3272,6 +3272,17 @@ check ovn-nbctl --wait=hv lr-copp-del r0 icmp6-error AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl ]) +check ovn-nbctl --wait=hv lr-copp-add r0 tcp-reset meter2 +AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl +tcp-reset: meter2 +]) + +AT_CHECK([ovn-sbctl list logical_flow | grep tcp -A 2 | grep -q meter2]) + +check ovn-nbctl --wait=hv lr-copp-del r0 tcp-reset +AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl +]) + check ovn-nbctl --wait=hv ls-copp-del sw1 event-elb AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl ]) -- 2.31.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 1/3] CoPP: add self-test for icmp{4, 6}_error controller action
Introduce CoPP selftest for icmp{4,6}_error controller action
Signed-off-by: Lorenzo Bianconi
---
tests/ovn-northd.at | 23 +++
tests/system-ovn.at | 21 +
2 files changed, 44 insertions(+)
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 8b9049899..3bcbdf4a9 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -3249,6 +3249,29 @@ AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
AT_CHECK([ovn-sbctl list logical_flow | grep arp -A 2 | grep -q meter1],[1])
+check ovn-nbctl --wait=hv meter-add meter2 drop 400 pktps 10
+check ovn-nbctl --wait=hv lr-copp-add r0 icmp4-error meter2
+AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
+icmp4-error: meter2
+])
+
+AT_CHECK([ovn-sbctl list logical_flow | grep icmp4 -A 2 | grep -q meter2])
+
+check ovn-nbctl --wait=hv lr-copp-del r0 icmp4-error
+AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
+])
+
+check ovn-nbctl --wait=hv lr-copp-add r0 icmp6-error meter2
+AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
+icmp6-error: meter2
+])
+
+AT_CHECK([ovn-sbctl list logical_flow | grep icmp6 -A 2 | grep -q meter2])
+
+check ovn-nbctl --wait=hv lr-copp-del r0 icmp6-error
+AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
+])
+
check ovn-nbctl --wait=hv ls-copp-del sw1 event-elb
AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
])
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 345384223..bd425f54b 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -6730,6 +6730,27 @@ OVS_WAIT_UNTIL([
test "${n_arp}" = "2"
])
+check ovn-nbctl meter-add icmp-meter drop 1 pktps 0
+check ovn-nbctl --wait=hv lr-copp-add R1 icmp4-error icmp-meter
+AT_CHECK([ovn-nbctl lr-copp-list R1 |grep icmp4-error], [0], [dnl
+icmp4-error: icmp-meter
+])
+
+NS_EXEC([sw01], [tcpdump -n -i sw01 icmp > icmp.pcap &])
+ip netns exec sw01 scapy -H <<-EOF
+p = IP(src="192.168.1.2", dst="172.16.1.100", ttl=1)/ TCP(dport = 8080,
flags="S") / Raw(b"X"*64)
+send (p, iface='sw01', loop = 0, verbose = 0, count = 100)
+EOF
+
+sleep 2
+kill $(pidof tcpdump)
+
+# 1pps + 1 burst size
+OVS_WAIT_UNTIL([
+n_icmp=$(grep ICMP icmp.pcap | wc -l)
+test "${n_icmp}" = "2"
+])
+
kill $(pidof ovn-controller)
as ovn-sb
--
2.31.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 0/3] Introduce more CoPP self-tests
Lorenzo Bianconi (3):
CoPP: add self-test for icmp{4,6}_error controller action
CoPP: add self-test for bfd controller action
CoPP: add self-test for tcp-reset controller action
tests/ovn-northd.at | 34 ++
tests/system-ovn.at | 50 +
2 files changed, 84 insertions(+)
--
2.31.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs] lib: export calc_percentile function
Xavier Simonart writes:
> export calc_percentile function (and percentile struct) so that
> it can be used in other libraries such as OVN.
>
> Signed-off-by: Xavier Simonart
> ---
What is the intent to use this in other libraries? It would be nice to
understand why just running the existing stopwatch API doesn't work
(maybe you cannot start samples). AFAIK, OVN uses the stopwatch API
just fine?
Perhaps it could make sense to expose the functionality in a different
fashion like:
void stopwatch_add_sample(const char *, unsigned long long);
This seems a useful API and doesn't expose all of the internal
information, but I don't know if it really is needed. Can you expand
why you need calc_percentile exposed?
> lib/stopwatch.c | 24 +---
> lib/stopwatch.h | 26 ++
> 2 files changed, 27 insertions(+), 23 deletions(-)
>
> diff --git a/lib/stopwatch.c b/lib/stopwatch.c
> index f5602163b..003c3a05f 100644
> --- a/lib/stopwatch.c
> +++ b/lib/stopwatch.c
> @@ -35,25 +35,6 @@ struct average {
> double alpha; /* Weight given to new samples */
> };
>
> -#define MARKERS 5
> -
> -/* Number of samples to collect before reporting P-square calculated
> - * percentile
> - */
> -#define P_SQUARE_MIN 50
> -
> -/* The naming of these fields is based on the naming used in the
> - * P-square algorithm paper.
> - */
> -struct percentile {
> -int n[MARKERS];
> -double n_prime[MARKERS];
> -double q[MARKERS];
> -double dn[MARKERS];
> -unsigned long long samples[P_SQUARE_MIN];
> -double percentile;
> -};
> -
> struct stopwatch {
> enum stopwatch_units units;
> unsigned long long n_samples;
> @@ -107,10 +88,7 @@ comp_samples(const void *left, const void *right)
> return *right_d > *left_d ? -1 : *right_d < *left_d;
> }
>
> -/* Calculate the percentile using the P-square algorithm. For more
> - * information, see https://www1.cse.wustl.edu/~jain/papers/ftp/psqr.pdf
> - */
> -static void
> +void
> calc_percentile(unsigned long long n_samples, struct percentile *pctl,
> unsigned long long new_sample)
> {
> diff --git a/lib/stopwatch.h b/lib/stopwatch.h
> index 91abd64e4..efb9a9e8a 100644
> --- a/lib/stopwatch.h
> +++ b/lib/stopwatch.h
> @@ -36,6 +36,32 @@ struct stopwatch_stats {
> (alpha 0.01). */
> };
>
> +#define MARKERS 5
> +
> +/* Number of samples to collect before reporting P-square calculated
> + * percentile
> + */
> +#define P_SQUARE_MIN 50
> +
> +/* The naming of these fields is based on the naming used in the
> + * P-square algorithm paper.
> + */
> +struct percentile {
> +int n[MARKERS];
> +double n_prime[MARKERS];
> +double q[MARKERS];
> +double dn[MARKERS];
> +unsigned long long samples[P_SQUARE_MIN];
> +double percentile;
> +};
> +
> +/* Calculate the percentile using the P-square algorithm. For more
> + * information, see https://www1.cse.wustl.edu/~jain/papers/ftp/psqr.pdf
> + */
> +void
> +calc_percentile(unsigned long long n_samples, struct percentile *pctl,
> +unsigned long long new_sample);
> +
> /* Create a new stopwatch.
> * The "units" are not used for any calculations but are printed when
> * statistics are requested.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH branch-2.14] python: idl: Avoid sending transactions when the DB is not synced up.
On Wed, Oct 13, 2021 at 11:13 AM Terry Wilson wrote:
>
> This ports the C IDL change f50714b to the Python IDL:
>
> Until now the code here would happily try to send transactions to the
> database server even if the database connection was not in the correct
> state. In some cases this could lead to strange behavior, such as sending
> a database transaction for a database that the IDL had just learned did not
> exist on the server.
>
> Signed-off-by: Terry Wilson
> (cherry picked from commit 34fbdc41084c16f855efa9c56086b03644408b7d)
> ---
> python/ovs/db/idl.py | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> index 4cf79cf94..5a17cbd00 100644
> --- a/python/ovs/db/idl.py
> +++ b/python/ovs/db/idl.py
> @@ -101,6 +101,7 @@ class Idl(object):
> IDL_S_SERVER_MONITOR_REQUESTED = 2
> IDL_S_DATA_MONITOR_REQUESTED = 3
> IDL_S_DATA_MONITOR_COND_REQUESTED = 4
> +IDL_S_MONITORING = 5
>
> def __init__(self, remote, schema_helper, probe_interval=None,
> leader_only=True):
> @@ -294,6 +295,7 @@ class Idl(object):
> else:
> assert self.state ==
> self.IDL_S_DATA_MONITOR_REQUESTED
> self.__parse_update(msg.result, OVSDB_UPDATE)
> +self.state = self.IDL_S_MONITORING
>
> except error.Error as e:
> vlog.err("%s: parse error in received schema: %s"
> @@ -1468,6 +1470,11 @@ class Transaction(object):
> if self != self.idl.txn:
> return self._status
>
> +if self.idl.state != Idl.IDL_S_MONITORING:
> +self._status = Transaction.TRY_AGAIN
> +self.__disassemble()
> +return self._status
> +
> # If we need a lock but don't have it, give up quickly.
> if self.idl.lock_name and not self.idl.has_lock:
> self._status = Transaction.NOT_LOCKED
> --
> 2.31.1
>
This should also apply cleanly/pass tests for 2.13. Thanks!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH branch-2.14] python: idl: Avoid sending transactions when the DB is not synced up.
This ports the C IDL change f50714b to the Python IDL:
Until now the code here would happily try to send transactions to the
database server even if the database connection was not in the correct
state. In some cases this could lead to strange behavior, such as sending
a database transaction for a database that the IDL had just learned did not
exist on the server.
Signed-off-by: Terry Wilson
(cherry picked from commit 34fbdc41084c16f855efa9c56086b03644408b7d)
---
python/ovs/db/idl.py | 7 +++
1 file changed, 7 insertions(+)
diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index 4cf79cf94..5a17cbd00 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -101,6 +101,7 @@ class Idl(object):
IDL_S_SERVER_MONITOR_REQUESTED = 2
IDL_S_DATA_MONITOR_REQUESTED = 3
IDL_S_DATA_MONITOR_COND_REQUESTED = 4
+IDL_S_MONITORING = 5
def __init__(self, remote, schema_helper, probe_interval=None,
leader_only=True):
@@ -294,6 +295,7 @@ class Idl(object):
else:
assert self.state == self.IDL_S_DATA_MONITOR_REQUESTED
self.__parse_update(msg.result, OVSDB_UPDATE)
+self.state = self.IDL_S_MONITORING
except error.Error as e:
vlog.err("%s: parse error in received schema: %s"
@@ -1468,6 +1470,11 @@ class Transaction(object):
if self != self.idl.txn:
return self._status
+if self.idl.state != Idl.IDL_S_MONITORING:
+self._status = Transaction.TRY_AGAIN
+self.__disassemble()
+return self._status
+
# If we need a lock but don't have it, give up quickly.
if self.idl.lock_name and not self.idl.has_lock:
self._status = Transaction.NOT_LOCKED
--
2.31.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 3/4] netdev-offload-dpdk: Support tnl_pop for gre tunnel
On 10/7/21 13:05, Nir Anteby via dev wrote:
Add support for tnl_pop action for gre vport.
Signed-off-by: Nir Anteby
---
lib/netdev-offload-dpdk.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index 10924de..5c721fe 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -908,6 +908,12 @@ vport_to_rte_tunnel(struct netdev *vport,
ds_put_format(s_tnl, "flow tunnel create %d type vxlan; ",
netdev_dpdk_get_port_id(netdev));
}
+} else if (!strcmp(netdev_get_type(vport), "gre")) {
+tunnel->type = RTE_FLOW_ITEM_TYPE_GRE;
+if (!VLOG_DROP_DBG()) {
+ds_put_format(s_tnl, "flow tunnel create %d type gre; ",
+ netdev_dpdk_get_port_id(netdev));
+}
} else {
VLOG_DBG_RL(, "vport type '%s' is not supported",
netdev_get_type(vport));
@@ -2236,6 +2242,9 @@ get_vport_netdev_cb(struct netdev *netdev,
if (!aux->type || strcmp(netdev_get_type(netdev), aux->type)) {
return false;
}
+if (!strcmp(netdev_get_type(netdev), "gre")) {
Shouldn't you also set aux->vport here?
If you don't, get_vport_netdev() will always return NULL.
+return true;
+}
tnl_cfg = netdev_get_tunnel_config(netdev);
if (!tnl_cfg) {
@@ -2268,6 +2277,8 @@ get_vport_netdev(const char *dpif_type,
if (tunnel->type == RTE_FLOW_ITEM_TYPE_VXLAN) {
aux.type = "vxlan";
+} else if (tunnel->type == RTE_FLOW_ITEM_TYPE_GRE) {
+aux.type = "gre";
}
netdev_ports_traverse(dpif_type, get_vport_netdev_cb, );
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tunnel-push-pop.at: Mask source port in tunnel header.
On 10/13/21 17:08, Timothy Redaelli wrote:
> On Tue, 12 Oct 2021 15:33:07 +0200
> Ilya Maximets wrote:
>
>> Source port is based on a packet hash and hash depends on a chosen
>> implementation. Masking it to avoid test failures with '-msse4.2'.
>>
>> Fixes: 7e6b41ac8d9d ("dpif-netdev: Fix crash when PACKET_OUT is metered.")
>> Reported-by: Kumar Amber
>> Signed-off-by: Ilya Maximets
>> ---
>> tests/tunnel-push-pop.at | 10 ++
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
>> index 12fc1ef91..636465397 100644
>> --- a/tests/tunnel-push-pop.at
>> +++ b/tests/tunnel-push-pop.at
>> @@ -628,20 +628,22 @@ AT_CHECK([
>> AT_CHECK([ovs-vsctl -- set Interface p0 options:tx_pcap=p0.pcap])
>>
>> packet=5054000a50540009123
>> -encap=f8bc124434b6aa55aa5508004532400040113406010102580101025c83a917c1001e65587b00
>> +dnl Source port is based on a packet hash, so it may differ depending on the
>> +dnl compiler flags and CPU type. Masked with ''.
>> +encap=f8bc124434b6aa55aa5508004532400040113406010102580101025c17c1001e65587b00
>>
>> dnl Output to tunnel from a int-br internal port.
>> dnl Checking that the packet arrived and it was correctly encapsulated.
>> AT_CHECK([ovs-ofctl add-flow int-br
>> "in_port=LOCAL,actions=debug_slow,output:2"])
>> AT_CHECK([ovs-appctl netdev-dummy/receive int-br "${packet}4"])
>> -OVS_WAIT_UNTIL([test `ovs-pcap p0.pcap | grep "${encap}${packet}4" | wc -l`
>> -ge 1])
>> +OVS_WAIT_UNTIL([test `ovs-pcap p0.pcap | egrep "${encap}${packet}4" | wc
>> -l` -ge 1])
>> dnl Sending again to exercise the non-miss upcall path.
>> AT_CHECK([ovs-appctl netdev-dummy/receive int-br "${packet}4"])
>> -OVS_WAIT_UNTIL([test `ovs-pcap p0.pcap | grep "${encap}${packet}4" | wc -l`
>> -ge 2])
>> +OVS_WAIT_UNTIL([test `ovs-pcap p0.pcap | egrep "${encap}${packet}4" | wc
>> -l` -ge 2])
>>
>> dnl Output to tunnel from the controller.
>> AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out int-br CONTROLLER
>> "debug_slow,output:2" "${packet}5"])
>> -OVS_WAIT_UNTIL([test `ovs-pcap p0.pcap | grep "${encap}${packet}5" | wc -l`
>> -ge 1])
>> +OVS_WAIT_UNTIL([test `ovs-pcap p0.pcap | egrep "${encap}${packet}5" | wc
>> -l` -ge 1])
>>
>> dnl Datapath actions should not have tunnel push action.
>> AT_CHECK([ovs-appctl dpctl/dump-flows | grep -q tnl_push], [1])
>
> Hi,
> egrep should not be needed in this case, since . is part of basic
> regexp (see man 1 grep and [1]) and so plain grep is enough.
>
> [1]
> https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap09.html#tag_09_03_04
>
Oh, cool! Thanks for pointing that out.
This patch is already applied, but I'll keep that in mind for
the future changes.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 0/6] MFEX Optimizations IPv6 + Hashing
On 10/5/21 16:45, Stokes, Ian wrote: > 9/21/21 12:23, Kumar Amber wrote: >>> --- >>> v3: >>> - rebase to master. >>> v2: >>> - fix the CI build. >>> - fix check-patch for co-author. >>> --- >>> >>> The patch-set introduces AVX512 optimizations of IPv6 >>> traffic profiles and hashing improvements for all AVX512 >>> supported traffic profiles for IPv4 and IPv6. >>> >>> Kumar Amber (6): >>> dpif-netdev/mfex: Add AVX512 basic ipv6 traffic profiles >>> dpif-netdev/mfex: Add AVX512 vlan ipv6 traffic profiles >>> dpif-netdev/mfex: Add packet hash check to autovalidator >>> dpif-netdev/mfex: Add ipv4 profile based hashing >>> dpif-netdev/mfex: Add ipv6 profile based hashing >>> dpif-netdev/mfex: Avoid hashing when opt mfex called >>> >>> NEWS | 7 + >>> lib/automake.mk | 1 + >>> lib/dpif-netdev-avx512.c | 6 +- >>> lib/dpif-netdev-extract-avx512.c | 348 +- >>> lib/dpif-netdev-private-extract.c | 63 +- >>> lib/dpif-netdev-private-extract.h | 12 ++ >>> tests/pcap/mfex_test.pcap | Bin 416 -> 632 bytes >>> 7 files changed, 432 insertions(+), 5 deletions(-) >>> >> >> Hi. A few months ago I was told that it's easy for Intel to set up CI >> to test upstream patches with AVX512 features enabled. Is there any >> progress on that front? >> >> My point is that we should refrain from adding new features in this >> area until we have a proper CI. Especially considering the unit test >> failure you reported yesterday, which is supposedly related to AVX512 >> optimizations. >> > > Hi Ilya, > > Apologies for the delay in response, I've been on PTO the past 2 weeks so > only catching up now. No problem. I just got from my PTO too. > > There is an ongoing effort to deploy an AVX512 CI. Aaron, Michael and myself > have started to look at this. I think the initial target was to have > something up and running by EOY. > >> // Marking this patch-set as deferred for now. > > Agree with marking this deferred for now. > > However I don’t think this should be dependent on delivery of the CI as that > could be some time away yet. I think if we are to run the tests manually on > an AVX512 system (We could even run it on the system that has been setup and > reserved for the CI in the Intel Lab) and post results in response/part of > the review of the series that should help progress the series in the meantime? Yes, sure. As long as there is some progress with a CI and tests could be run manually for patches in meantime during reviews, there should be no problem with having development going. Best regards, Ilya Maximets. > > Thanks > Ian > >> >> Best regards, Ilya Maximets ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tunnel-push-pop.at: Mask source port in tunnel header.
On Tue, 12 Oct 2021 15:33:07 +0200
Ilya Maximets wrote:
> Source port is based on a packet hash and hash depends on a chosen
> implementation. Masking it to avoid test failures with '-msse4.2'.
>
> Fixes: 7e6b41ac8d9d ("dpif-netdev: Fix crash when PACKET_OUT is metered.")
> Reported-by: Kumar Amber
> Signed-off-by: Ilya Maximets
> ---
> tests/tunnel-push-pop.at | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
> index 12fc1ef91..636465397 100644
> --- a/tests/tunnel-push-pop.at
> +++ b/tests/tunnel-push-pop.at
> @@ -628,20 +628,22 @@ AT_CHECK([
> AT_CHECK([ovs-vsctl -- set Interface p0 options:tx_pcap=p0.pcap])
>
> packet=5054000a50540009123
> -encap=f8bc124434b6aa55aa5508004532400040113406010102580101025c83a917c1001e65587b00
> +dnl Source port is based on a packet hash, so it may differ depending on the
> +dnl compiler flags and CPU type. Masked with ''.
> +encap=f8bc124434b6aa55aa5508004532400040113406010102580101025c17c1001e65587b00
>
> dnl Output to tunnel from a int-br internal port.
> dnl Checking that the packet arrived and it was correctly encapsulated.
> AT_CHECK([ovs-ofctl add-flow int-br
> "in_port=LOCAL,actions=debug_slow,output:2"])
> AT_CHECK([ovs-appctl netdev-dummy/receive int-br "${packet}4"])
> -OVS_WAIT_UNTIL([test `ovs-pcap p0.pcap | grep "${encap}${packet}4" | wc -l`
> -ge 1])
> +OVS_WAIT_UNTIL([test `ovs-pcap p0.pcap | egrep "${encap}${packet}4" | wc -l`
> -ge 1])
> dnl Sending again to exercise the non-miss upcall path.
> AT_CHECK([ovs-appctl netdev-dummy/receive int-br "${packet}4"])
> -OVS_WAIT_UNTIL([test `ovs-pcap p0.pcap | grep "${encap}${packet}4" | wc -l`
> -ge 2])
> +OVS_WAIT_UNTIL([test `ovs-pcap p0.pcap | egrep "${encap}${packet}4" | wc -l`
> -ge 2])
>
> dnl Output to tunnel from the controller.
> AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out int-br CONTROLLER
> "debug_slow,output:2" "${packet}5"])
> -OVS_WAIT_UNTIL([test `ovs-pcap p0.pcap | grep "${encap}${packet}5" | wc -l`
> -ge 1])
> +OVS_WAIT_UNTIL([test `ovs-pcap p0.pcap | egrep "${encap}${packet}5" | wc -l`
> -ge 1])
>
> dnl Datapath actions should not have tunnel push action.
> AT_CHECK([ovs-appctl dpctl/dump-flows | grep -q tnl_push], [1])
Hi,
egrep should not be needed in this case, since . is part of basic
regexp (see man 1 grep and [1]) and so plain grep is enough.
[1]
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap09.html#tag_09_03_04
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 0/6] MFEX Optimizations IPv6 + Hashing
On 9/28/21 19:13, Van Haaren, Harry wrote: >> -Original Message- >> From: Ilya Maximets >> Sent: Tuesday, September 21, 2021 1:45 PM >> To: Amber, Kumar ; ovs-dev@openvswitch.org >> Cc: ktray...@redhat.com; i.maxim...@ovn.org; Stokes, Ian >> ; f...@sysclose.org; echau...@redhat.com; Van Haaren, >> Harry >> Subject: Re: [PATCH v3 0/6] MFEX Optimizations IPv6 + Hashing >> >> On 9/21/21 12:23, Kumar Amber wrote: >>> --- >>> v3: >>> - rebase to master. >>> v2: >>> - fix the CI build. >>> - fix check-patch for co-author. >>> --- >>> >>> The patch-set introduces AVX512 optimizations of IPv6 >>> traffic profiles and hashing improvements for all AVX512 >>> supported traffic profiles for IPv4 and IPv6. >>> >>> Kumar Amber (6): >>> dpif-netdev/mfex: Add AVX512 basic ipv6 traffic profiles >>> dpif-netdev/mfex: Add AVX512 vlan ipv6 traffic profiles >>> dpif-netdev/mfex: Add packet hash check to autovalidator >>> dpif-netdev/mfex: Add ipv4 profile based hashing >>> dpif-netdev/mfex: Add ipv6 profile based hashing >>> dpif-netdev/mfex: Avoid hashing when opt mfex called >>> >>> NEWS | 7 + >>> lib/automake.mk | 1 + >>> lib/dpif-netdev-avx512.c | 6 +- >>> lib/dpif-netdev-extract-avx512.c | 348 +- >>> lib/dpif-netdev-private-extract.c | 63 +- >>> lib/dpif-netdev-private-extract.h | 12 ++ >>> tests/pcap/mfex_test.pcap | Bin 416 -> 632 bytes >>> 7 files changed, 432 insertions(+), 5 deletions(-) >>> >> >> Hi. A few months ago I was told that it's easy for Intel to set up CI >> to test upstream patches with AVX512 features enabled. Is there any >> progress on that front? > > Yes there is progress on that front, as you know Ian and Aaron are working on > that, I didn't know that as this information is not public. > and status updates available if you're particularly interested in its > progress. I was interested, so I asked in this email. > >> My point is that we should refrain from adding new features in this >> area until we have a proper CI. > > There is already various CI efforts for OVS, and as you know there is ongoing > efforts > to add AVX512 specifically, and report back on patchwork. > >> Especially considering the unit test failure you reported yesterday, which is >> supposedly related to AVX512 optimizations. > > I don't know why you say this is related to AVX512 - it is not. See the > detailed reply > Amber sent with details of how the test-case assumed SW based murmur hash > output. Sure, now I know that problem was unrelated. But the more or less detailed reply was sent a week after my previous email, so I had no clue what was the actual problem with a test at the moment of writing and the week after. > >> // Marking this patch-set as deferred for now. > > This is not acceptable, deferring patchsets just because the AVX512 CI isn't > in place was never agreed on. > I do not like that AVX512 was "blamed" for the above unit-test failure, and > now other AVX512 patchsets > are being deferred and ignored as a result of that mistaken blame. > > @Amber, Kumar, please mark this v3 patchset as "new". > >> Best regards, Ilya Maximets. > > Regards, -Harry > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 2/4] netdev-dpdk: Add flow_api support for netdev gre vports
On 10/7/21 13:05, Nir Anteby via dev wrote:
Add the acceptance of GRE devices to netdev_dpdk_flow_api_supported() API,
to allow offloading of DPDK GRE devices.
Signed-off-by: Nir Anteby
---
lib/netdev-dpdk.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index ca92c94..a0359ed 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -5215,10 +5215,11 @@ netdev_dpdk_flow_api_supported(struct netdev *netdev)
struct netdev_dpdk *dev;
bool ret = false;
-if (!strcmp(netdev_get_type(netdev), "vxlan") &&
-!strcmp(netdev_get_dpif_type(netdev), "netdev")) {
-ret = true;
-goto out;
+if ((!strcmp(netdev_get_type(netdev), "vxlan") ||
+ !strcmp(netdev_get_type(netdev), "gre")) &&
+ !strcmp(netdev_get_dpif_type(netdev), "netdev")) {
+ ret = true;
+ goto out;
}
if (!is_dpdk_class(netdev->netdev_class)) {
Reviewed-by: Maxime Coquelin
Thanks,
Maxime
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif:Fix typo in function pointer name bond-add
Suggested title:
dpif: Fix function pointer check for bond_add.
On 12/10/2021 09:11, Somnath Chatterjee via dev wrote:
There was wrong typo in function pointer check in
dpif_bond_add() before calling bond_add()
Fixes: 9df65060cf4c ("userspace: Avoid dp_hash recirculation for
balance-tcp bond mode.")
Signed-off-by: Somnath Chatterjee
Patch looks good, maybe you can respin the commit title/msg to save
maintainer doing it.
Acked-by: Kevin Traynor
---
lib/dpif.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/dpif.c b/lib/dpif.c
index 8c4aed4..294b1c4 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -2012,7 +2012,7 @@ dpif_meter_del(struct dpif *dpif, ofproto_meter_id
meter_id,
int
dpif_bond_add(struct dpif *dpif, uint32_t bond_id, odp_port_t *member_map)
{
-return dpif->dpif_class->bond_del
+return dpif->dpif_class->bond_add
? dpif->dpif_class->bond_add(dpif, bond_id, member_map)
: EOPNOTSUPP;
}
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 1/4] netdev-offload-dpdk: Refactor get_vport_netdev()
On 10/7/21 13:05, Nir Anteby via dev wrote: Refactor the function as a pre-step towards supporting more tunnel types. Signed-off-by: Nir Anteby --- lib/netdev-offload-dpdk.c | 22 -- 1 file changed, 8 insertions(+), 14 deletions(-) Reviewed-by: Maxime Coquelin Thanks, Maxime ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] [python] Avoid alloc of an attr dict/row+column
On Tue, 12 Oct 2021 14:13:31 -0500 Terry Wilson wrote: > Python objects normally have a dictionary named __dict__ allocated > for handling dynamically assigned attributes. Depending on > architecture and Python version, that empty dict may be between > 64 and 280 bytes. > > Seeing as Atom and Datum objects do not need dynamic attribute > support and there can be millions of rows in a database, avoiding > this allocation with __slots__ can save 100s of MBs of memory per > Idl process. > > Signed-off-by: Terry Wilson It reduce the memory usage and it's also faster: print(min(timeit.repeat(lambda: data.Atom.default(ovs.db.types.ATOMIC_TYPES[1] 0.7460950060049072 (w/o patch) vs 0.6909653190232348 (with patch) [data.Atom.default(ovs.db.types.ATOMIC_TYPES[1]) for x in range(100)] print(guppy.hpy().heap()) 322605613 bytes (w/o patch) vs 114605277 bytes (with patch) Acked-by: Timothy Redaelli Tested-by: Timothy Redaelli ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] github: Remove workaround fixing up /etc/hosts.
On Fri, Oct 8, 2021 at 1:58 PM Dumitru Ceara wrote: > > The issue that was worked around has been fixed in the meantime: > https://github.com/actions/virtual-environments/issues/3353 > > Signed-off-by: Dumitru Ceara Fairly trivial and ovsrobot already confirmed it was ok. I had a look at a build of mine in GHA yesterday: Run cat /etc/hosts 127.0.0.1 localhost # The following lines are desirable for IPv6 capable hosts ::1 localhost ip6-localhost ip6-loopback fe00::0 ip6-localnet ff00::0 ip6-mcastprefix ff02::1 ip6-allnodes ff02::2 ip6-allrouters ff02::3 ip6-allhosts 127.0.0.1 cpu-pool.com 127.0.0.1 MiningMadness.com 127.0.0.1 stratum-na.rplant.xyz 127.0.0.1 do-dear.com 127.0.0.1 web.do-dear.com 127.0.0.1 git.workflows.live 10.1.0.4 fv-az212-793.lkxvw04jkbfendtxnazko3bsjf.jx.internal.cloudapp.net fv-az212-793 Which looks good. Reviewed-by: David Marchand -- David Marchand ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: Fix use-after-free on PACKET_OUT of IP fragments.
On 10/12/21 23:12, Aaron Conole wrote:
> Ilya Maximets writes:
>
>> IP fragmentation engine may not only steal the packet but also add
>> more. For example, after receiving the last fragment, it will
>> add all previous fragments to a batch. Unfortunately, it will also
>> free the original last fragment and replace it with a copy.
>> This invalidates the 'packet_clone' pointer in the dpif_netdev_execute()
>> leading to the use-after-free:
>>
>> ==3525086==ERROR: AddressSanitizer: heap-use-after-free on
>> address 0x61600020439c at pc 0x00688a6d
>> READ of size 1 at 0x61600020439c thread T0
>> #0 0x688a6c in dp_packet_swap ./lib/dp-packet.h:265:5
>> #1 0x68781d in dpif_netdev_execute lib/dpif-netdev.c:4103:9
>> #2 0x6675db in dpif_netdev_operate lib/dpif-netdev.c:4129:25
>> #3 0x691e5e in dpif_operate lib/dpif.c:1367:13
>> #4 0x692909 in dpif_execute lib/dpif.c:1321:9
>> #5 0x5b19c6 in packet_execute ofproto/ofproto-dpif.c:4991:5
>> #6 0x5a2861 in ofproto_packet_out_finish ofproto/ofproto.c:3662:5
>> #7 0x5a65c6 in do_bundle_commit ofproto/ofproto.c:8270:13
>> #8 0x5a0cae in handle_bundle_control ofproto/ofproto.c:8309:17
>> #9 0x59a476 in handle_single_part_openflow ofproto/ofproto.c:8593:16
>> #10 0x5877ac in handle_openflow ofproto/ofproto.c:8674:21
>> #11 0x6296f1 in ofconn_run ofproto/connmgr.c:1329:13
>> #12 0x62925d in connmgr_run ofproto/connmgr.c:356:9
>> #13 0x586904 in ofproto_run ofproto/ofproto.c:1879:5
>> #14 0x55c830 in bridge_run__ vswitchd/bridge.c:3251:9
>> #15 0x55c015 in bridge_run vswitchd/bridge.c:3310:5
>> #16 0x575f31 in main vswitchd/ovs-vswitchd.c:127:9
>> #17 0x7f01099d3492 in __libc_start_main (/lib64/libc.so.6+0x23492)
>> #18 0x47d96d in _start (vswitchd/ovs-vswitchd+0x47d96d)
>>
>> 0x61600020439c is located 28 bytes inside of 560-byte region
>> freed by thread T0 here:
>> #0 0x5177a8 in free (vswitchd/ovs-vswitchd+0x5177a8)
>> #1 0x6b17b6 in dp_packet_delete ./lib/dp-packet.h:256:9
>> #2 0x6afeee in ipf_extract_frags_from_batch lib/ipf.c:947:17
>> #3 0x6afd63 in ipf_preprocess_conntrack lib/ipf.c:1232:9
>> #4 0x946b2c in conntrack_execute lib/conntrack.c:1446:5
>> #5 0x67e3ed in dp_execute_cb lib/dpif-netdev.c:8277:9
>> #6 0x7097d7 in odp_execute_actions lib/odp-execute.c:865:17
>> #7 0x66409e in dp_netdev_execute_actions lib/dpif-netdev.c:8322:5
>> #8 0x6877ad in dpif_netdev_execute lib/dpif-netdev.c:4090:5
>> #9 0x6675db in dpif_netdev_operate lib/dpif-netdev.c:4129:25
>> #10 0x691e5e in dpif_operate lib/dpif.c:1367:13
>> #11 0x692909 in dpif_execute lib/dpif.c:1321:9
>> #12 0x5b19c6 in packet_execute ofproto/ofproto-dpif.c:4991:5
>> #13 0x5a2861 in ofproto_packet_out_finish ofproto/ofproto.c:3662:5
>> #14 0x5a65c6 in do_bundle_commit ofproto/ofproto.c:8270:13
>> #15 0x5a0cae in handle_bundle_control ofproto/ofproto.c:8309:17
>> #16 0x59a476 in handle_single_part_openflow ofproto/ofproto.c:8593:16
>> #17 0x5877ac in handle_openflow ofproto/ofproto.c:8674:21
>> #18 0x6296f1 in ofconn_run ofproto/connmgr.c:1329:13
>> #19 0x62925d in connmgr_run ofproto/connmgr.c:356:9
>> #20 0x586904 in ofproto_run ofproto/ofproto.c:1879:5
>> #21 0x55c830 in bridge_run__ vswitchd/bridge.c:3251:9
>> #22 0x55c015 in bridge_run vswitchd/bridge.c:3310:5
>> #23 0x575f31 in main vswitchd/ovs-vswitchd.c:127:9
>> #24 0x7f01099d3492 in __libc_start_main (/lib64/libc.so.6+0x23492)
>>
>> The issue can be reproduced with system-userspace testsuite on the
>> 'conntrack - IPv4 fragmentation with fragments specified' test.
>> Previously, there was a leak inside the IP fragmentation module that
>> kept the original segment, so 'packet_clone' remained a valid pointer.
>> But commit 803ed12e31b0 ("ipf: release unhandled packets from the batch")
>> fixed the leak leading to use-after-free.
>>
>> Using the packet from a batch instead of 'packet_clone' to swap packet
>> content to avoid the issue.
>
> This is useful in case some future mechanism does the same.
>
>> While investigating this problem, more issues uncovered. One of them
>> is that IP fragmentation engine can add more packets to the batch, but
>> there is no way to get them to a caller. Adding an extra branch for
>> this case with a 'FIXME' comment in order to highlight the issue.
>
> Thanks.
>
>> Another one is that IP fragmentation engine will keep only 32 fragments
>> dropping all other fragments while refilling a batch, but that should
>> be fixed separately.
>
> :(
>
> There's a lot of work still needed for this feature. Separately, I've
> noticed that the purge timeout isn't settable, and is much longer than
> the 15s or so that is noted in the commit (on my system, it tested at
> almost two minutes)
>
>> Fixes: 7e6b41ac8d9d ("dpif-netdev: Fix crash when PACKET_OUT is metered.")
>> Signed-off-by: Ilya Maximets
>> ---
>
> Thanks for the
Re: [ovs-dev] [PATCH] tunnel-push-pop.at: Mask source port in tunnel header.
On 10/12/21 17:18, Alin-Gabriel Serdean wrote: > Acked-by: Alin-Gabriel Serdean Thanks! I applied this patch and backported down to 2.12. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] AUTHORS: Update email for Alin Serdean
On 10/7/21 05:18, Alin-Gabriel Serdean wrote: > Signed-off-by: Alin-Gabriel Serdean > --- > .mailmap| 4 ++-- > AUTHORS.rst | 2 +- > MAINTAINERS.rst | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/.mailmap b/.mailmap > index 824af56b2..6cefe3cd4 100644 > --- a/.mailmap > +++ b/.mailmap > @@ -14,8 +14,8 @@ Aaron Conole > Aaron Rosen > Alex Wang > Alexey I. Froloff > -Alin Serdean > -Alin Serdean cloudbasesolutions.com> > +Alin Serdean > +Alin Serdean I hope that everything is fine. For the patch: I noticed that you're using different ways to spell your name in different commits (e.g. with or without dash), so you may want to add a 'Name ' line, so git will use the same spelling everywhere. You may add the line on commit, if you want that behavior. Just highlighting a possibility. In any case: Acked-by: Ilya Maximets > Andy Zhou > Andy Zhou > Andy Zhou > diff --git a/AUTHORS.rst b/AUTHORS.rst > index 4bd1aeb17..42eda83dd 100644 > --- a/AUTHORS.rst > +++ b/AUTHORS.rst > @@ -46,7 +46,7 @@ Alexey Roytman royt...@il.ibm.com > Alex Wang ee07b...@gmail.com > Alfredo Finellia...@computationes.de > Alin Balutoiu abalut...@cloudbasesolutions.com > -Alin Serdean aserd...@cloudbasesolutions.com > +Alin Serdean aserd...@ovn.org > Amber Kumarkumar.am...@intel.com > Ambika Arora ambika.ar...@tcs.com > Amit Bose b...@noironetworks.com > diff --git a/MAINTAINERS.rst b/MAINTAINERS.rst > index f4f6188c8..9dcbf3f8c 100644 > --- a/MAINTAINERS.rst > +++ b/MAINTAINERS.rst > @@ -44,7 +44,7 @@ This is the current list of active Open vSwitch committers: > * - Alex Wang > - ee07b...@gmail.com > * - Alin Serdean > - - aserd...@cloudbasesolutions.com > + - aserd...@ovn.org > * - Andy Zhou > - az...@ovn.org > * - Ansis Atteka > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v1 1/1] datapath-windows: add layers when adding the deferred actions
Currently the layers info propogated to ProcessDeferredActions may be
incorrect. Because of this, any subsequent usage of layers might result
in undesired behavior. Accordingly in this patch it will add the related
layers in the deferred action to make sure the layers consistent with
the related NBL.
In the specified case 229, we have encountered one issue when doing
the decap Geneve Packet and doing the twice NAT(via two flow tables)
and found the HTTP packet will be changed the TCP sequence.
After debugging, we found the issue is caused by the not-updated
layers value isTcp and isUdp for Geneve decapping case.
The related function call chains are listed below,
OvsExecuteDpIoctl—>OvsActionsExecute—>OvsDoExecuteActions->OvsTunnelPortRx
——>OvsDoExecuteActions——〉nat ct action and recircle action
->OvsActionsExecute->defered_actions processing for nat and recircle action
For the Geneve packet decaping, it will firstly set the layers for Udp packet.
Then it will go on doing OVS flow extract to get the inner packet layers and
Processing the first nat action and first recircle action. After that datapath
Will do defered_actions processing on OvsActionsExecute. And it does inherit
The incorrect geneve packet layers value( isTCP 0 and isUdp 1).So in the second
Nat action processing it will get the wrong TCP Headers in
OvsUpdateAddressAndPort
And it will update related TCP check field value but in this case it will
change
The packet Tcp seq value.
Reported-at:https://github.com/openvswitch/ovs-issues/issues/229
Signed-off-by: Wilson Peng
---
datapath-windows/ovsext/Actions.c | 10 ++
datapath-windows/ovsext/Recirc.c | 18 --
datapath-windows/ovsext/Recirc.h | 3 +++
3 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/datapath-windows/ovsext/Actions.c
b/datapath-windows/ovsext/Actions.c
index 90ecb59f0..592cb7467 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1803,9 +1803,11 @@ OvsExecuteRecirc(OvsForwardingContext *ovsFwdCtx,
}
if (newNbl) {
-deferredAction = OvsAddDeferredActions(newNbl, key, NULL);
+deferredAction = OvsAddDeferredActions(newNbl, key,
&(ovsFwdCtx->layers),
+ NULL);
} else {
-deferredAction = OvsAddDeferredActions(ovsFwdCtx->curNbl, key, NULL);
+deferredAction = OvsAddDeferredActions(ovsFwdCtx->curNbl, key,
+ &(ovsFwdCtx->layers), NULL);
}
if (deferredAction) {
@@ -1975,7 +1977,7 @@ OvsExecuteSampleAction(OvsForwardingContext *ovsFwdCtx,
return STATUS_SUCCESS;
}
-if (!OvsAddDeferredActions(newNbl, key, a)) {
+if (!OvsAddDeferredActions(newNbl, key, &(ovsFwdCtx->layers), a)) {
OVS_LOG_INFO(
"Deferred actions limit reached, dropping sample action.");
OvsCompleteNBL(ovsFwdCtx->switchContext, newNbl, TRUE);
@@ -2361,7 +2363,7 @@ OvsActionsExecute(POVS_SWITCH_CONTEXT switchContext,
if (status == STATUS_SUCCESS) {
status = OvsProcessDeferredActions(switchContext, completionList,
- portNo, sendFlags, layers);
+ portNo, sendFlags, NULL);
}
return status;
diff --git a/datapath-windows/ovsext/Recirc.c b/datapath-windows/ovsext/Recirc.c
index 2febf060d..a32b75352 100644
--- a/datapath-windows/ovsext/Recirc.c
+++ b/datapath-windows/ovsext/Recirc.c
@@ -277,16 +277,23 @@ OvsDeferredActionsQueuePush(POVS_DEFERRED_ACTION_QUEUE
queue)
POVS_DEFERRED_ACTION
OvsAddDeferredActions(PNET_BUFFER_LIST nbl,
OvsFlowKey *key,
+ POVS_PACKET_HDR_INFO layers,
const PNL_ATTR actions)
{
POVS_DEFERRED_ACTION_QUEUE queue = OvsDeferredActionsQueueGet();
POVS_DEFERRED_ACTION deferredAction = NULL;
+OVS_PACKET_HDR_INFO layersInit = { 0 };
deferredAction = OvsDeferredActionsQueuePush(queue);
if (deferredAction) {
deferredAction->nbl = nbl;
deferredAction->actions = actions;
deferredAction->key = *key;
+if (layers) {
+deferredAction->layers = *layers;
+} else {
+deferredAction->layers = layersInit;
+}
}
return deferredAction;
@@ -309,9 +316,16 @@ OvsProcessDeferredActions(POVS_SWITCH_CONTEXT
switchContext,
NDIS_STATUS status = NDIS_STATUS_SUCCESS;
POVS_DEFERRED_ACTION_QUEUE queue = OvsDeferredActionsQueueGet();
POVS_DEFERRED_ACTION deferredAction = NULL;
+POVS_PACKET_HDR_INFO layersDeferred = NULL;
/* Process all deferred actions. */
while ((deferredAction = OvsDeferredActionsQueuePop(queue)) != NULL) {
+if (layers) {
+layersDeferred = layers;
+ } else {
+layersDeferred = &(deferredAction->layers);
+ }
+
if (deferredAction->actions) {
status =
Re: [ovs-dev] [PATCH v3] [python] Avoid sending transactions when the DB is not synced up
On 10/12/21 9:24 PM, Ilya Maximets wrote: > Dumitru, sorry, I messed up and didn't include the suggested comment. > Feel free to submit a separate patch for that if you think it's needed. It was a nit, I'm not sure it's worth a separate patch. It's OK, thanks! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
