[ovs-dev] [PATCH v3 1/1] acinclude: Also use LIBS from dpkg pkg-config
DPDK 18.11 builds using the more modern meson build system no more provide the -ldpdk linker script. Instead it is expected to use pkgconfig for linker options as well. This change will set DPDK_LIB from pkg-config (if pkg-config was available) and since that already carries the whole-archive flags around the PMDs skips the further wrapping in more whole-archive if that is already part of DPDK_LIB. To work reliable in all environments this needs pkg-config 0.29.1. We want to be able to use PKG_CHECK_MODULES_STATIC which is not yet available in 0.24. Therefore update pkg.m4 to pkg-config 0.29.1. This should be backport-safe as these macro files are all versioned. autoconf is smart enough to check the version if you have it locally, and if the system's is higher, it will use that one instead. Finally make configure.ac use the locally provided pkg.m4 before calling the PKG_PROG_PKG_CONFIG macro. Signed-off-by: Christian Ehrhardt --- acinclude.m4 | 17 ++-- configure.ac | 2 + m4/pkg.m4| 217 +-- 3 files changed, 153 insertions(+), 83 deletions(-) diff --git a/acinclude.m4 b/acinclude.m4 index 95241b142..f82b989e6 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -223,9 +223,9 @@ AC_DEFUN([OVS_CHECK_DPDK], [ case "$with_dpdk" in yes) DPDK_AUTO_DISCOVER="true" -PKG_CHECK_MODULES([DPDK], [libdpdk], - [DPDK_INCLUDE="$DPDK_CFLAGS"], - [DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk"]) +PKG_CHECK_MODULES_STATIC([DPDK], [libdpdk], + [DPDK_INCLUDE="$DPDK_CFLAGS", DPDK_LIB="$DPDK_LIBS"], + [DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk", DPDK_LIB="-ldpdk"]) ;; *) DPDK_AUTO_DISCOVER="false" @@ -238,10 +238,10 @@ AC_DEFUN([OVS_CHECK_DPDK], [ DPDK_INCLUDE="-I$DPDK_INCLUDE_PATH/dpdk" fi DPDK_LIB_DIR="$with_dpdk/lib" +DPDK_LIB="-ldpdk" ;; esac -DPDK_LIB="-ldpdk" DPDK_EXTRA_LIB="" ovs_save_CFLAGS="$CFLAGS" @@ -346,7 +346,14 @@ AC_DEFUN([OVS_CHECK_DPDK], [ # # These options are specified inside a single -Wl directive to prevent # autotools from reordering them. -DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-archive +# +# OTOH newer versions of dpdk pkg-config (generated with Meson) +# will already have flagged just the right set of libs with +# --whole-archive - in those cases do not wrap it once more. +case "$DPDK_LIB" in + *whole-archive*) DPDK_vswitchd_LDFLAGS=$DPDK_LIB;; + *) DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-archive +esac AC_SUBST([DPDK_vswitchd_LDFLAGS]) AC_DEFINE([DPDK_NETDEV], [1], [System uses the DPDK module.]) fi diff --git a/configure.ac b/configure.ac index 505e3d041..dc6eebbf5 100644 --- a/configure.ac +++ b/configure.ac @@ -28,6 +28,8 @@ AC_PROG_CPP AC_PROG_MKDIR_P AC_PROG_FGREP AC_PROG_EGREP + +m4_include([m4/pkg.m4]) PKG_PROG_PKG_CONFIG AM_MISSING_PROG([AUTOM4TE], [autom4te]) diff --git a/m4/pkg.m4 b/m4/pkg.m4 index c5b26b52e..82bea96ee 100644 --- a/m4/pkg.m4 +++ b/m4/pkg.m4 @@ -1,29 +1,60 @@ -# pkg.m4 - Macros to locate and utilise pkg-config.-*- Autoconf -*- -# serial 1 (pkg-config-0.24) -# -# Copyright © 2004 Scott James Remnant . -# -# This program is free software; you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation; either version 2 of the License, or -# (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, but -# WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU -# General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. -# -# As a special exception to the GNU General Public License, if you -# distribute this file as part of a program that contains a -# configuration script generated by Autoconf, you may include it under -# the same distribution terms that you use for the rest of that program. - -# PKG_PROG_PKG_CONFIG([MIN-VERSION]) -# -- +dnl pkg.m4 - Macros to locate and utilise pkg-config. -*- Autoconf -*- +dnl serial 11 (pkg-config-0.29.1) +dnl +dnl Copyright © 2004 Scott James Remnant . +dnl Copyright © 2012-2015 Dan Nicholson +dnl +dnl This program is free software; you can redistribute it and/or modify +dnl it under the terms of the GNU General Public License as published by +dnl the Free Software Foundation; either version 2 of the License, or +dnl (at your option) any later version. +dnl
[ovs-dev] [PATCH v3 0/1] fix build with DPDK 18.11 without -ldpdk
DPDK 18.11 can be built using the more modern meson build system. In that case it no more provides the -ldpdk linker script. Instead it is expected to use pkgconfig for linker options as well. FYI here a build log on Ubuntu 19.04 with the three patches applied: https://launchpadlibrarian.net/409021278/buildlog_ubuntu-disco-amd64.openvswitch_2.11.0~git20190121.4e4f80ec2-0ubuntu1~ppa5_BUILDING.txt.gz Updates from v2: - patches were formerly structured for reviewability, but to guarantee travis requirements to pass the build individually and not as a series they are now squashed to be acceptable Updates from v1: - add updated pkg.m4 to support PKG_CHECK_MODULES_STATIC - include local pkg.m4 in configure.ac Christian Ehrhardt (1): acinclude: Also use LIBS from dpkg pkg-config acinclude.m4 | 17 ++-- configure.ac | 2 + m4/pkg.m4| 217 +-- 3 files changed, 153 insertions(+), 83 deletions(-) -- 2.17.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] travis: Speed up linux kernel downloads.
CDN links are much faster in average. https://www.kernel.org/
links shows usually less than 10 MB/s, while https://cdn.kernel.org/
could give up to 200 MB/s and usually shows speeds much higher than
10 MB/s. Also, 'xz' archives are 30-50 MB smaller than gzip ones.
It takes a bit more time to unpack them, but it's negligible in
compare with download time.
For exmaple,
linux-3.16.54.tar.gz - 122064395 (116M)
linux-3.16.54.tar.xz - 81057528 (77M)
'xz' archive download via CDN link is the default way for kernel
downloading that provided by the kernel.org.
Some exmaples from Travis builds:
Before:
100%[==>] 122,064,395 3.11MB/s in 36s
(3.23 MB/s) - 'linux-3.16.54.tar.gz' saved [122064395/122064395]
100%[==>] 157,764,715 7.16MB/s in 24s
(6.28 MB/s) - 'linux-4.17.14.tar.gz' saved [157764715/157764715]
After:
100%[==>] 81,057,528 95.0MB/s in 0.8s
(95.0 MB/s) - 'linux-3.16.54.tar.xz' saved [81057528/81057528]
100%[==>] 102,195,552 218MB/s in 0.4s
(218 MB/s) - 'linux-4.17.14.tar.xz' saved [102195552/102195552]
Signed-off-by: Ilya Maximets
---
.travis/linux-build.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh
index 0a2091061..9d177aa1b 100755
--- a/.travis/linux-build.sh
+++ b/.travis/linux-build.sh
@@ -18,8 +18,8 @@ function install_kernel()
PREFIX="v2.6/longterm/v2.6.32"
fi
-wget https://www.kernel.org/pub/linux/kernel/${PREFIX}/linux-${1}.tar.gz
-tar xzvf linux-${1}.tar.gz > /dev/null
+wget https://cdn.kernel.org/pub/linux/kernel/${PREFIX}/linux-${1}.tar.xz
+tar xvf linux-${1}.tar.xz > /dev/null
cd linux-${1}
make allmodconfig
--
2.17.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4] Support for match & set ICMPv6 reserved and options type fields
> > Currently OVS supports all ARP protocol fields as OXM match fields to > > implement the relevant ARP procedures for IPv4. This includes support > > for matching copying and setting ARP fields. In IPv6 ARP has been > > replaced by ICMPv6 neighbor discovery (ND) procedures, neighbor > > advertisement and neighbor solicitation. > > > > The support for ICMPv6 fields in OVS is not complete for the use cases > > equivalent to ARP in IPv4. OVS lacks support for matching, copying and > > setting the “ND option type” and “ND reserved” fields. Without these > > user cannot implement all ICMPv6 ND procedures for IPv6 support. > > > > This commit adds additional OXM fields to OVS for ICMPv6 “ND option > > type“ and ICMPv6 “ND reserved” using the OXM extension mechanism. This > > allows support for parsing these fields from an ICMPv6 packet header > > and extending the OpenFlow protocol with specifications for these new > > OXM fields for matching, copying and setting. > > > > Signed-off-by: Vishal Deep Ajmera > > Co-authored-by: Ashvin Lakshmikantha > > > > Signed-off-by: Ashvin Lakshmikantha > > > > Applied to master, thanks! Thanks Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v3 2/2] conntrack: Exclude l2 padding in 'conn_key_extract()'.
Thanks Vishal Yes, it needs to go back to 2.6; I will be doing some backport patches soon. Darrell On 2/1/19, 8:57 PM, "ovs-dev-boun...@openvswitch.org on behalf of Vishal Deep Ajmera" wrote: > > 'conn_key_extract()' in userspace conntrack is including L2 > (Ethernet) pad bytes for both L3 and L4 sizes. One problem is any packet with > non-zero L2 padding can incorrectly fail L4 checksum validation. > > This patch fixes conn_key_extract() by ignoring L2 pad bytes. > Thanks Darrell for the patch. It looks fine. Can we get this in for upcoming OVS release ? Also need a backport till OVS branch v2.6 if possible. Warm Regards, Vishal Ajmera ___ dev mailing list d...@openvswitch.org https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-devdata=02%7C01%7Cdball%40vmware.com%7Ce4be58be855d46ff030408d688caf431%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636846802608503836sdata=vXiygqAX5RhzX6y5GuaOET7TkqSX%2BfKRzTTmWowWqLA%3Dreserved=0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 4/6] connmgr: Make treatment of active and passive connections more uniform.
> On Oct 29, 2018, at 3:57 PM, Ben Pfaff wrote: > > Until now, connmgr has handled active and passive OpenFlow connections in > quite different ways. Any active connection, whether it was currently > connected or not, was always maintained as an ofconn. Whenever such a > connection (re)connected, its settings were cleared. On the other hand, > passive connections had a separate listener which created an ofconn when > a new connection came in, and these ofconns would be deleted when such a > connection was closed. This approach is inelegant and has occasionally > led to bugs when reconnection didn't clear all of the state that it > should have. > > There's another motivation here. Currently, active connections are > always primary controllers and passive connections are always service > controllers (as documented in ovs-vswitchd.conf.db(5)). Sometimes it would > be useful to have passive primary controllers (maybe active service > controllers too but I haven't personally run into that use case). As is, > this is difficult to implement because there is so much different code in > use between active and passive connections. This commit will make it > easier. > > Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --Justin ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Remove support for OpenFlow 1.6 (draft).
> On Jan 17, 2019, at 4:20 PM, Ben Pfaff wrote: > > ONF abandoned the OpenFlow specification, so that OpenFlow 1.6 will never > be completed. It did not contain much in the way of useful features, so > remove what support Open vSwitch already had. > > Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --Justin ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] conntrack: Fix possibly uninitialized memory.
Bleep bloop. Greetings Darrell Ball, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: fatal: sha1 information is lacking or useless (lib/conntrack.c). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 conntrack: Fix possibly uninitialized memory. The copy of the patch that failed is found in: /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch When you have resolved this problem, run "git am --resolved". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Please check this out. If you feel there has been an error, please email acon...@bytheb.org Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] skiplist: Drop data comparison in skiplist_delete.
On Tue, Jan 29, 2019 at 04:09:55PM +0300, Ilya Maximets wrote: > Current version of 'skiplist_delete' uses data comparator to check > if the node that we're removing exists on current level. i.e. our > node 'x' is the next of update[i] on the level i. > But it's enough to just check pointers for that purpose. Thanks! Applied to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [patch v4 2/2] conntrack: Exclude l2 padding in 'conn_key_extract()'.
'conn_key_extract()' in userspace conntrack is including L2
(Ethernet) pad bytes for both L3 and L4 sizes. One problem is
any packet with non-zero L2 padding can incorrectly fail L4
checksum validation.
This patch fixes conn_key_extract() by ignoring L2 pad bytes.
Fixes: a489b16854b5 ("conntrack: New userspace connection tracker.")
CC: Daniele Di Proietto
Co-authored-by: Vishal Deep Ajmera
Co-authored-by: Venkatesan Pradeep
Co-authored-by: Nitin Katiyar
Signed-off-by: Vishal Deep Ajmera
Signed-off-by: Venkatesan Pradeep
Signed-off-by: Nitin Katiyar
Signed-off-by: Darrell Ball
---
v3: Removed line inadvertently merged from another patch.
lib/conntrack.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 0fe..e1f4041 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -1875,6 +1875,9 @@ extract_l4_icmp6(struct conn_key *key, const void *data,
size_t size,
* processed, the function will extract the key from the packet nested
* in the ICMP payload and set '*related' to true.
*
+ * 'size' here is the layer 4 size, which can be a nested size if parsing
+ * an ICMP or ICMP6 header.
+ *
* If 'related' is NULL, it means that we're already parsing a header nested
* in an ICMP error. In this case, we skip checksum and length validation. */
static inline bool
@@ -1949,7 +1952,6 @@ conn_key_extract(struct conntrack *ct, struct dp_packet
*pkt, ovs_be16 dl_type,
* we use a sparse representation (miniflow).
*
*/
-const char *tail = dp_packet_tail(pkt);
bool ok;
ctx->key.dl_type = dl_type;
@@ -1960,11 +1962,11 @@ conn_key_extract(struct conntrack *ct, struct dp_packet
*pkt, ovs_be16 dl_type,
} else {
bool hwol_good_l3_csum = dp_packet_ip_checksum_valid(pkt);
/* Validate the checksum only when hwol is not supported. */
-ok = extract_l3_ipv4(>key, l3, tail - (char *) l3, NULL,
+ok = extract_l3_ipv4(>key, l3, dp_packet_l3_size(pkt), NULL,
!hwol_good_l3_csum);
}
} else if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
-ok = extract_l3_ipv6(>key, l3, tail - (char *) l3, NULL);
+ok = extract_l3_ipv6(>key, l3, dp_packet_l3_size(pkt), NULL);
} else {
ok = false;
}
@@ -1974,8 +1976,8 @@ conn_key_extract(struct conntrack *ct, struct dp_packet
*pkt, ovs_be16 dl_type,
if (!hwol_bad_l4_csum) {
bool hwol_good_l4_csum = dp_packet_l4_checksum_valid(pkt);
/* Validate the checksum only when hwol is not supported. */
-if (extract_l4(>key, l4, tail - l4, >icmp_related, l3,
- !hwol_good_l4_csum)) {
+if (extract_l4(>key, l4, dp_packet_l4_size(pkt),
+ >icmp_related, l3, !hwol_good_l4_csum)) {
ctx->hash = conn_key_hash(>key, ct->hash_basis);
return true;
}
--
1.9.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [patch v4 1/2] dp-packet: Add 'dp_packet_l3_size()'.
The new api will be used in a subsequent patch.
Signed-off-by: Darrell Ball
---
v4: Added function header comments to the new and existing APIs (Ben).
v2: Added patch to series.
lib/dp-packet.h | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 7b85dd9..c297a8f 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -352,10 +352,23 @@ dp_packet_set_l4(struct dp_packet *b, void *l4)
b->l4_ofs = l4 ? (char *) l4 - (char *) dp_packet_data(b) : UINT16_MAX;
}
+/* Returns the size of the packet from the beginning of the L3 header to the
+ * end of the L3 payload. Hence L2 padding is not included. */
+static inline size_t
+dp_packet_l3_size(const struct dp_packet *b)
+{
+return OVS_LIKELY(b->l3_ofs != UINT16_MAX)
+? (const char *)dp_packet_tail(b) - (const char *)dp_packet_l3(b)
+- dp_packet_l2_pad_size(b)
+: 0;
+}
+
+/* Returns the size of the packet from the beginning of the L4 header to the
+ * end of the L4 payload. Hence L2 padding is not included. */
static inline size_t
dp_packet_l4_size(const struct dp_packet *b)
{
-return b->l4_ofs != UINT16_MAX
+return OVS_LIKELY(b->l4_ofs != UINT16_MAX)
? (const char *)dp_packet_tail(b) - (const char *)dp_packet_l4(b)
- dp_packet_l2_pad_size(b)
: 0;
--
1.9.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] skiplist: Remove 'height' from skiplist_node.
On Tue, Jan 29, 2019 at 02:18:24PM +0300, Ilya Maximets wrote:
> On 28.01.2019 20:48, Ilya Maximets wrote:
> > On 25.01.2019 23:22, Ben Pfaff wrote:
> >> This member was write-only: it was initialized and never used later on.
> >>
> >> Thanks to Esteban Rodriguez Betancourt for the
> >> following additional rationale:
> >>
> >> In this case you are right, the "height" member is not only not
> >> used, it is in fact not required, and can be safely removed,
> >> without causing security issues.
> >>
> >> The code can't read past the end of the 'forward' array because
> >> the skiplist "level" member, that specifies the maximum height of
> >> the whole skiplist.
> >>
> >> The "level" field is updated in insertions and deletions, so that
> >> in insertion the root node will point to the newly created item
> >> (if there isn't a list there yet). At the deletions, if the
> >> deleted item is the last one at that height then the root is
> >> modified to point to NULL at that height, and the whole skiplist
> >> height is decremented.
> >>
> >> For the forward_to case:
> >>
> >> - If a node is found in a list of level/height N, then it has
> >> height N (that's why it was inserted in that list)
> >>
> >> - forward_to travels throught nodes in the same level, so it is
> >> safe, as it doesn't go up.
> >>
> >> - If a node has height N, then it belongs to all the lists
> >> initiated at root->forward[n, n-1 ,n-2, ..., 0]
> >>
> >> - forward_to may go to lower levels, but that is safe, because of
> >> previous point.
> >>
> >> So, the protection is given by the "level" field in skiplist root
> >> node, and it is enough to guarantee that the code won't go off
> >> limits at 'forward' array. But yes, the height field is unused,
> >> unneeded, and can be removed safely.
> >>
> >> CC: Esteban Rodriguez Betancourt
> >> Signed-off-by: Ben Pfaff
> >> ---
> >
> > Patch looks correct.
> > However, maybe we could use this field inside skiplist_delete ?
> >
> > Here is the part of skiplist_delete function:
> >
> > 191 if (x && list->cmp(x->data, value, list->cfg) == 0) {
> > 192 for (i = 0; i <= list->level; i++) {
> > 193 if (!update[i]->forward[i] ||
> > 194 list->cmp(update[i]->forward[i]->data, value,
> > 195 list->cfg) != 0) {
> > 196 break;
> > 197 }
> > 198 update[i]->forward[i] = x->forward[i];
> > 199 }
> >
> > On above lines comparator (list->cmp) used to determine if our "x"
> > exists on current level (i). I think, that we can simply replace this
> > with checking: if (i > x->height || !update[i]->forward[i]).
> > We may probably skip the "update[i]->forward[i]" check because it
> > could not be NULL if we still have "x" on this level.
> >
> > So, It looks like we could re-write above loop dropping all the checks:
> >
> > for (i = 0; i <= x->height; i++) {
> > update[i]->forward[i] = x->forward[i];
> > }
> >
> > Any thoughts ?
>
> There is another possibility to simplify above loop.
> I read the article that mentioned at the file header. It clearly states that
> implementation does not require to store height for each node. But the
> deletion loop looks more sane there. We do not need to check the data using
> the comparator, we just need to check that we still have 'x' on this level,
> i.e. that 'update[i]->forward[i] == x' because that is the node that we're
> removing and we do not need to update links on levels where we have no 'x'.
> The loop will look like in the article:
>
>for (i = 0; i <= list->level; i++) {
>if (update[i]->forward[i] != x) {
>break;
>}
>update[i]->forward[i] = x->forward[i];
>}
>
>
> So, there are 2 ways here:
> 1. Keep the 'height' and use it inside 'skiplist_delete'.
>
> 2. Remove the 'height' and later simplify the loop inside
>'skiplist_delete' by checking only 'update[i]->forward[i] != x'.
>
> IMHO, we could go with the second approach. Apply this patch as is and
> prepare another one for 'skiplist_delete' improvement.
>
> If so,
> Acked-by: Ilya Maximets
Thanks. I applied this patch and yours to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] flow: fix a possible memory leak in parse_ct_state
On Wed, Jan 30, 2019 at 05:10:38PM +0300, Ilya Maximets wrote:
> On 28.01.2019 8:49, Li RongQing wrote:
> > state_s should be freed always before exit parse_ct_state
> >
>
> Fixes: b4293a336d8d ("conntrack: Move ct_state parsing to lib/flow.c")
>
> > Signed-off-by: Li RongQing
>
> LGTM,
> Acked-by: Ilya Maximets
Thanks, applied to master, backported as far as branch-2.8.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v4] conntrack: Fix possibly uninitialized memory.
On Mon, Feb 04, 2019 at 04:02:15PM -0800, Darrell Ball wrote: > There are a few cases where struct 'conn_key' padding may be unspecified > according to the C standard. Practically, it seems implementations don't > have issue, but it is better to be safe. The code paths modified are not > hot ones. Fix this by doing a memcpy in these cases in lieu of a > structure copy. > > Found by inspection. > > Signed-off-by: Darrell Ball Thanks, applied to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1] ofproto-dpif-trace: Fix for the segmentation fault in ofproto_trace().
On Mon, Feb 04, 2019 at 03:34:34PM -0800, Ashish Varma wrote: > Added the check for NULL in "next_ct_states" argument passed to the > "ofproto_trace()" function. Under normal scenario, this is non-NULL. A NULL > "next_ct_states" argument is passed from the "upcall_xlate()" function on > encountering XLATE_RECURSION_TOO_DEEP or XLATE_TOO_MANY_RESUBMITS error. > > VMware-BZ: #2282287 > Signed-off-by: Ashish Varma Thanks, applied to master and backported as far as branch-2.8. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [patch v4] conntrack: Fix possibly uninitialized memory.
There are a few cases where struct 'conn_key' padding may be unspecified
according to the C standard. Practically, it seems implementations don't
have issue, but it is better to be safe. The code paths modified are not
hot ones. Fix this by doing a memcpy in these cases in lieu of a
structure copy.
Found by inspection.
Signed-off-by: Darrell Ball
---
v4: Use memcpy as safest option (Thanks Ben) in lieu of memset zero and
structure copy; reinstate a couple lines carelessly removed
(Thanks Aaron).
v3: Removed an unnecessary change and limited scope of 2 others.
v2: Found another one; will double check for others later.
lib/conntrack.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index e1f4041..7a03009 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -748,7 +748,7 @@ conn_lookup(struct conntrack *ct, const struct conn_key
*key, long long now)
{
struct conn_lookup_ctx ctx;
ctx.conn = NULL;
-ctx.key = *key;
+memcpy(, key, sizeof ctx.key);
ctx.hash = conn_key_hash(key, ct->hash_basis);
unsigned bucket = hash_to_bucket(ctx.hash);
conn_key_lookup(>buckets[bucket], , now);
@@ -905,7 +905,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
nc->rev_key.src.addr = alg_exp->alg_nat_repl_addr;
nc->nat_info->nat_action = NAT_ACTION_DST;
}
-*conn_for_un_nat_copy = *nc;
+memcpy(conn_for_un_nat_copy, nc, sizeof *conn_for_un_nat_copy);
ct_rwlock_wrlock(>resources_lock);
bool new_insert = nat_conn_keys_insert(>nat_conn_keys,
conn_for_un_nat_copy,
@@ -919,7 +919,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
free(log_msg);
}
} else {
-*conn_for_un_nat_copy = *nc;
+memcpy(conn_for_un_nat_copy, nc, sizeof *conn_for_un_nat_copy);
ct_rwlock_wrlock(>resources_lock);
bool nat_res = nat_select_range_tuple(ct, nc,
conn_for_un_nat_copy);
@@ -1262,7 +1262,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
ct->hash_basis,
alg_src_ip_wc(ct_alg_ctl));
if (alg_exp) {
-alg_exp_entry = *alg_exp;
+memcpy(_exp_entry, alg_exp, sizeof alg_exp_entry);
alg_exp = _exp_entry;
}
ct_rwlock_unlock(>resources_lock);
@@ -2614,7 +2614,8 @@ static struct alg_exp_node *
expectation_lookup(struct hmap *alg_expectations, const struct conn_key *key,
uint32_t basis, bool src_ip_wc)
{
-struct conn_key check_key = *key;
+struct conn_key check_key;
+memcpy(_key, key, sizeof check_key);
check_key.src.port = ALG_WC_SRC_PORT;
if (src_ip_wc) {
--
1.9.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ovs-discuss] Question about using struct ofproto‘s port_by_name without lock protected
On Fri, Feb 01, 2019 at 04:23:07AM +, Lilijun (Jerry, Cloud Networking) wrote: > Currently, when insert/delete/lookup the shash list struct ofproto‘s > port_by_name, we have no lock to protect this list. This list was used lots > of other functions. > > Is there something race issues? Can we make sure it's only used in the main > ovs thread? I think it's only used in the main thread. (If you see some evidence that it's not, let's fix it!) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1] ofproto-dpif-trace: Fix for the segmentation fault in ofproto_trace().
Thanks for the fix. I am wondering if we can output some useful information
in 'struct ds' for this case?
On Mon, Feb 4, 2019 at 3:45 PM Ashish Varma
wrote:
> Added the check for NULL in "next_ct_states" argument passed to the
> "ofproto_trace()" function. Under normal scenario, this is non-NULL. A NULL
> "next_ct_states" argument is passed from the "upcall_xlate()" function on
> encountering XLATE_RECURSION_TOO_DEEP or XLATE_TOO_MANY_RESUBMITS error.
>
> VMware-BZ: #2282287
> Signed-off-by: Ashish Varma
> ---
> ofproto/ofproto-dpif-trace.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index eca61ce..4a981e1 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -740,7 +740,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, const
> struct flow *flow,
> ds_put_format(output, "\nrecirc(%#"PRIx32")",
>recirc_node->recirc_id);
>
> -if (recirc_node->type == OFT_RECIRC_CONNTRACK) {
> +if (next_ct_states && recirc_node->type == OFT_RECIRC_CONNTRACK) {
> uint32_t ct_state;
> if (ovs_list_is_empty(next_ct_states)) {
> ct_state = CS_TRACKED | CS_NEW;
> --
> 2.7.4
>
> ___
> 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
Re: [ovs-dev] [PATCH v2] ofp-monitor: Added support for OpenFlow 1.4+ Flow Monitor
On Thu, Jan 31, 2019 at 03:49:39PM -0800, Ashish Varma wrote: > OVS supports Nicira version of Flow Monitor feature which allows an OpenFlow > controller to keep track of any changes in the flow table. (The changes can > done by the controller itself or by any other controller connected to OVS.) > This patch adds support for the OpenFlow 1.4+ OFPMP_FLOW_MONITOR multipart > message. > Also added support in "ovs-ofctl monitor" to send OpenFlow 1.4+ messages to > OVS. > Added unit test cases to test the OpenFlow version of Flow Monitor messages. > > Signed-off-by: Ashish Varma Thanks for the revision! It will be nice to finally have this feature! Since parse_flow_monitor_request() already has a 'usable_protocols' parameter, I would suggest using that to limit the support for out_group, rather than putting a special case into ofctl_monitor(). I would add an example of the new message to tests/ofp-print.at. I think we talked about supporting flow monitors in OF1.1, 1.2, and 1.3. I seem to recall that you plan to add that afterward, in a separate patch. Is that correct? I hope that, if so, you can start work on it soon after this patch is accepted. I would add an item to NEWS. Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v1] ofproto-dpif-trace: Fix for the segmentation fault in ofproto_trace().
Added the check for NULL in "next_ct_states" argument passed to the
"ofproto_trace()" function. Under normal scenario, this is non-NULL. A NULL
"next_ct_states" argument is passed from the "upcall_xlate()" function on
encountering XLATE_RECURSION_TOO_DEEP or XLATE_TOO_MANY_RESUBMITS error.
VMware-BZ: #2282287
Signed-off-by: Ashish Varma
---
ofproto/ofproto-dpif-trace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
index eca61ce..4a981e1 100644
--- a/ofproto/ofproto-dpif-trace.c
+++ b/ofproto/ofproto-dpif-trace.c
@@ -740,7 +740,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct
flow *flow,
ds_put_format(output, "\nrecirc(%#"PRIx32")",
recirc_node->recirc_id);
-if (recirc_node->type == OFT_RECIRC_CONNTRACK) {
+if (next_ct_states && recirc_node->type == OFT_RECIRC_CONNTRACK) {
uint32_t ct_state;
if (ovs_list_is_empty(next_ct_states)) {
ct_state = CS_TRACKED | CS_NEW;
--
2.7.4
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/3] datapath: Avoid OOB read when parsing flow nlattrs
Thanks Ben!! On 2/4/2019 1:49 PM, Ben Pfaff wrote: I applied this series to master and backported as far as branch-2.6. (It did not apply cleanly to branch-2.5.) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofp-packet: Fix NXT_RESUME with geneve tunnel metadata
Doesn't apply cleanly. Would you mind manually backporting and posting the patch? On Fri, Feb 01, 2019 at 11:59:37AM +0100, Daniel Alvarez Sanchez wrote: > We have hit this issue as well on 2.9, would it be possible to backport it? > > On Mon, Oct 8, 2018 at 7:17 PM Ben Pfaff wrote: > > > > On Fri, Oct 05, 2018 at 09:19:54AM -0700, Yi-Hung Wei wrote: > > > The patch address vswitchd crash when it receives NXT_RESUME with geneve > > > tunnel metadata. The crash is due to segmentation fault with the > > > following stack trace, and it is observed only in kernel datapath. > > > A test is added to prevent regression. > > > > > > Thread 1 "ovs-vswitchd" received signal SIGSEGV, Segmentation fault. > > > 0 0x7fcffd0c5412 in tun_metadata_to_geneve__ > > > (flow=flow@entry=0x7ffcb7106680, b=b@entry=0x7ffcb70eb5a8, > > > crit_opt=crit_opt@entry=0x7ffcb70eb287) > > >at lib/tun-metadata.c:676 > > > 1 0x7fcffd0c6858 in tun_metadata_to_geneve_nlattr_flow > > > (b=0x7ffcb70eb5a8, flow=0x7ffcb7106638) at lib/tun-metadata.c:706 > > > 2 tun_metadata_to_geneve_nlattr (tun=tun@entry=0x7ffcb7106638, > > > flow=flow@entry=0x7ffcb7106638, key=key@entry=0x0, > > > b=b@entry=0x7ffcb70eb5a8) > > >at lib/tun-metadata.c:810 > > > 3 0x7fcffd048464 in tun_key_to_attr (a=a@entry=0x7ffcb70eb5a8, > > > tun_key=tun_key@entry=0x7ffcb7106638, > > > tun_flow_key=tun_flow_key@entry=0x7ffcb7106638, > > >key_buf=key_buf@entry=0x0, tnl_type=, > > > tnl_type@entry=0x0) at lib/odp-util.c:2886 > > > 4 0x7fcffd0551cf in odp_key_from_dp_packet > > > (buf=buf@entry=0x7ffcb70eb5a8, packet=0x7ffcb7106590) at > > > lib/odp-util.c:5909 > > > 5 0x7fcffd0d7870 in dpif_netlink_encode_execute (buf=0x7ffcb70eb5a8, > > > d_exec=0x7ffcb7106428, dp_ifindex=) at > > > lib/dpif-netlink.c:1873 > > > 6 dpif_netlink_operate__ (dpif=dpif@entry=0xe65e00, > > > ops=ops@entry=0x7ffcb7106418, n_ops=n_ops@entry=1) at > > > lib/dpif-netlink.c:1959 > > > 7 0x7fcffd0d842e in dpif_netlink_operate_chunks (n_ops=1, > > > ops=0x7ffcb7106418, dpif=) at lib/dpif-netlink.c:2258 > > > 8 dpif_netlink_operate (dpif_=0xe65e00, ops=, > > > n_ops=) at lib/dpif-netlink.c:2294 > > > 9 0x7fcffd014680 in dpif_operate (dpif=, > > > ops=, ops@entry=0x7ffcb7106418, n_ops=n_ops@entry=1) at > > > lib/dpif.c:1359 > > > 10 0x7fcffd014c58 in dpif_execute (dpif=, > > > execute=execute@entry=0x7ffcb71064e0) at lib/dpif.c:1324 > > > 11 0x7fcffd40d3e6 in nxt_resume (ofproto_=0xe6af50, > > > pin=0x7ffcb7107150) at ofproto/ofproto-dpif.c:4885 > > > 12 0x7fcffd3f88c3 in handle_nxt_resume (ofconn=ofconn@entry=0xf8c8f0, > > > oh=oh@entry=0xf7ebd0) at ofproto/ofproto.c:3612 > > > 13 0x7fcffd404a3b in handle_openflow__ (msg=0xeac460, > > > ofconn=0xf8c8f0) at ofproto/ofproto.c:8137 > > > 14 handle_openflow (ofconn=0xf8c8f0, ofp_msg=0xeac460) at > > > ofproto/ofproto.c:8258 > > > 15 0x7fcffd3f4653 in ofconn_run (handle_openflow=0x7fcffd4046f0 > > > , ofconn=0xf8c8f0) at ofproto/connmgr.c:1432 > > > 16 connmgr_run (mgr=0xe422f0, > > > handle_openflow=handle_openflow@entry=0x7fcffd4046f0 ) > > > at ofproto/connmgr.c:363 > > > 17 0x7fcffd3fdc76 in ofproto_run (p=0xe6af50) at > > > ofproto/ofproto.c:1821 > > > 18 0x0040ca94 in bridge_run__ () at vswitchd/bridge.c:2939 > > > 19 0x00411d44 in bridge_run () at vswitchd/bridge.c:2997 > > > 20 0x004094fd in main (argc=12, argv=0x7ffcb71085b8) at > > > vswitchd/ovs-vswitchd.c:119 > > > > > > VMWare-BZ: #2210216 > > > Signed-off-by: Yi-Hung Wei > > > --- > > > Please backport it to 2.10. > > > > Applied to master and branch-2.10, thanks! > > ___ > > 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
Re: [ovs-dev] [PATCH v2] test: Fix failed test "flow resume with geneve tun_metadata"
Done.
On Mon, Feb 04, 2019 at 01:40:33PM -0800, Yifeng Sun wrote:
> Thanks Ben, please backport to 2.11 if possible.
>
> On Mon, Feb 4, 2019 at 1:36 PM Ben Pfaff wrote:
>
> > On Fri, Feb 01, 2019 at 11:11:45AM -0800, Yi-Hung Wei wrote:
> > > On Fri, Feb 1, 2019 at 10:02 AM Yifeng Sun
> > wrote:
> > > >
> > > > Test "flow resume with geneve tun_metadata" failed because there is
> > > > no controller running to handle the continuation message. A previous
> > > > commit deleted the line that starts ovs-ofctl as a controller in
> > > > order to avoid a race condition on monitor log. This patch adds
> > > > back this line but omits the log file because this test doesn't
> > > > depend on the log file.
> > > >
> > > > Fixes: e8833217914f9c071c49 ("system-traffic.at: avoid a race
> > condition on monitor log")
> > > > CC: David Marchand
> > > > Signed-off-by: Yifeng Sun
> > > > ---
> > > Thanks for the fix.
> > >
> > > Acked-by: Yi-Hung Wei
> >
> > I applied this to master. If it should be backported, let me know.
> >
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/3] datapath: Avoid OOB read when parsing flow nlattrs
I applied this series to master and backported as far as branch-2.6. (It did not apply cleanly to branch-2.5.) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] datapath: Add support for kernel 4.18.x
Thanks Yifeng (and Greg). I applied this to master and branch-2.11. On Tue, Jan 29, 2019 at 04:53:37PM -0800, Yifeng Sun wrote: > Thanks Greg for reviewing, I ran the tests locally and there is no issue. > > Ben, could you backport this patch to 2.11 if you can? thanks! > > Yifeng > > On Tue, Jan 29, 2019 at 4:45 PM Gregory Rose wrote: > > > > > On 1/29/2019 2:18 PM, Yifeng Sun wrote: > > > No code changing is necessary to support 4.18.x. > > > > > > Only one kernel test failed and it is in the process of being fixed. > > > > > > Updated .travis.yml to include 4.18.x and also use latest 4.17 version. > > > Updated test files to test 4.18 kernel. > > > > > > Signed-off-by: Yifeng Sun > > > > LGTM and presuming it passes Travis... > > > > Tested-by: Greg Rose > > Reviewed-by: Greg Rose > > > > > --- > > > .travis.yml| 3 ++- > > > Documentation/faq/releases.rst | 1 + > > > NEWS | 3 ++- > > > acinclude.m4 | 4 ++-- > > > tests/system-traffic.at| 12 ++-- > > > 5 files changed, 13 insertions(+), 10 deletions(-) > > > > > > diff --git a/.travis.yml b/.travis.yml > > > index 2f7746d1d3c1..465876a67b31 100644 > > > --- a/.travis.yml > > > +++ b/.travis.yml > > > @@ -38,7 +38,8 @@ env: > > > - KERNEL=3.16.54 TESTSUITE=1 DPDK=1 > > > - KERNEL=3.16.54 DPDK_SHARED=1 > > > - KERNEL=3.16.54 DPDK_SHARED=1 OPTS="--enable-shared" > > > - - KERNEL=4.17.14 > > > + - KERNEL=4.18.20 > > > + - KERNEL=4.17.19 > > > - KERNEL=4.16.18 > > > - KERNEL=4.15.18 > > > - KERNEL=4.14.63 > > > diff --git a/Documentation/faq/releases.rst > > b/Documentation/faq/releases.rst > > > index 96da23c20ce4..86f09e6e2aec 100644 > > > --- a/Documentation/faq/releases.rst > > > +++ b/Documentation/faq/releases.rst > > > @@ -68,6 +68,7 @@ Q: What Linux kernel versions does each Open vSwitch > > release work with? > > > 2.8.x3.10 to 4.12 > > > 2.9.x3.10 to 4.13 > > > 2.10.x 3.10 to 4.17 > > > +2.11.x 3.10 to 4.18 > > > == > > > > > > Open vSwitch userspace should also work with the Linux kernel > > module built > > > diff --git a/NEWS b/NEWS > > > index 4985dbaacfe5..da33c0a9056c 100644 > > > --- a/NEWS > > > +++ b/NEWS > > > @@ -46,7 +46,8 @@ v2.11.0 - xx xxx > > > - RHEL packaging: > > >* OVN packages are split from OVS packages. A new spec > > > file - ovn-fedora.spec.in is added to generate OVN packages. > > > - > > > + - Linux datapath: > > > + * Support for the kernel versions 4.18.x > > > > > > v2.10.0 - 18 Aug 2018 > > > - > > > diff --git a/acinclude.m4 b/acinclude.m4 > > > index f038fd45701a..95241b142999 100644 > > > --- a/acinclude.m4 > > > +++ b/acinclude.m4 > > > @@ -151,10 +151,10 @@ AC_DEFUN([OVS_CHECK_LINUX], [ > > > AC_MSG_RESULT([$kversion]) > > > > > > if test "$version" -ge 4; then > > > - if test "$version" = 4 && test "$patchlevel" -le 17; then > > > + if test "$version" = 4 && test "$patchlevel" -le 18; then > > > : # Linux 4.x > > > else > > > - AC_ERROR([Linux kernel in $KBUILD is version $kversion, but > > version newer than 4.17.x is not supported (please refer to the FAQ for > > advice)]) > > > + AC_ERROR([Linux kernel in $KBUILD is version $kversion, but > > version newer than 4.18.x is not supported (please refer to the FAQ for > > advice)]) > > > fi > > > elif test "$version" = 3 && test "$patchlevel" -ge 10; then > > > : # Linux 3.x > > > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > > > index 3a62e17f43cb..ffe508dd61f7 100644 > > > --- a/tests/system-traffic.at > > > +++ b/tests/system-traffic.at > > > @@ -614,7 +614,7 @@ OVS_TRAFFIC_VSWITCHD_STOP > > > AT_CLEANUP > > > > > > AT_SETUP([datapath - ping over gre tunnel by simulated packets]) > > > -OVS_CHECK_KERNEL(3, 10, 4, 17) > > > +OVS_CHECK_KERNEL(3, 10, 4, 18) > > > > > > OVS_TRAFFIC_VSWITCHD_START() > > > AT_CHECK([ovs-vsctl -- set bridge br0 > > other-config:hwaddr=\"f2:ff:00:00:00:01\"]) > > > @@ -664,7 +664,7 @@ OVS_TRAFFIC_VSWITCHD_STOP > > > AT_CLEANUP > > > > > > AT_SETUP([datapath - ping over erspan v1 tunnel by simulated packets]) > > > -OVS_CHECK_KERNEL(3, 10, 4, 17) > > > +OVS_CHECK_KERNEL(3, 10, 4, 18) > > > > > > OVS_TRAFFIC_VSWITCHD_START() > > > AT_CHECK([ovs-vsctl -- set bridge br0 > > other-config:hwaddr=\"f2:ff:00:00:00:01\"]) > > > @@ -716,7 +716,7 @@ OVS_TRAFFIC_VSWITCHD_STOP > > > AT_CLEANUP > > > > > > AT_SETUP([datapath - ping over erspan v2 tunnel by simulated packets]) > > > -OVS_CHECK_KERNEL(3, 10, 4, 17) > > > +OVS_CHECK_KERNEL(3, 10, 4, 18) > > > > > > OVS_TRAFFIC_VSWITCHD_START() > > > AT_CHECK([ovs-vsctl -- set bridge br0 > > other-config:hwaddr=\"f2:ff:00:00:00:01\"]) > > > @@ -769,7 +769,7 @@ OVS_TRAFFIC_VSWITCHD_STOP > > > AT_CLEANUP
Re: [ovs-dev] [PATCH 1/2] dpif-netlink: Fix a bug that causes duplicate key error in datapath
On Thu, Jan 31, 2019 at 03:10:00PM -0800, Yifeng Sun wrote: > Kmod tests 122 and 123 failed and kernel reports a "Duplicate key of > type 6" error. Further debugging reveals that nl_attr_find__() should > start looking for OVS_KEY_ATTR_ETHERTYPE from offset returned by > a previous called nl_msg_start_nested(). This patch fixes it. > > Tests 122 and 123 were skipped by kernel 4.15 and older versions. > Kernel 4.16 and later kernels start showing this failure. > > Signed-off-by: Yifeng Sun Thanks. I applied this to master and backported as far as it would go. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] test: Fix failed test "flow resume with geneve tun_metadata"
Thanks Ben, please backport to 2.11 if possible.
On Mon, Feb 4, 2019 at 1:36 PM Ben Pfaff wrote:
> On Fri, Feb 01, 2019 at 11:11:45AM -0800, Yi-Hung Wei wrote:
> > On Fri, Feb 1, 2019 at 10:02 AM Yifeng Sun
> wrote:
> > >
> > > Test "flow resume with geneve tun_metadata" failed because there is
> > > no controller running to handle the continuation message. A previous
> > > commit deleted the line that starts ovs-ofctl as a controller in
> > > order to avoid a race condition on monitor log. This patch adds
> > > back this line but omits the log file because this test doesn't
> > > depend on the log file.
> > >
> > > Fixes: e8833217914f9c071c49 ("system-traffic.at: avoid a race
> condition on monitor log")
> > > CC: David Marchand
> > > Signed-off-by: Yifeng Sun
> > > ---
> > Thanks for the fix.
> >
> > Acked-by: Yi-Hung Wei
>
> I applied this to master. If it should be backported, let me know.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] test: Fix failed test "flow resume with geneve tun_metadata"
On Fri, Feb 01, 2019 at 11:11:45AM -0800, Yi-Hung Wei wrote:
> On Fri, Feb 1, 2019 at 10:02 AM Yifeng Sun wrote:
> >
> > Test "flow resume with geneve tun_metadata" failed because there is
> > no controller running to handle the continuation message. A previous
> > commit deleted the line that starts ovs-ofctl as a controller in
> > order to avoid a race condition on monitor log. This patch adds
> > back this line but omits the log file because this test doesn't
> > depend on the log file.
> >
> > Fixes: e8833217914f9c071c49 ("system-traffic.at: avoid a race condition on
> > monitor log")
> > CC: David Marchand
> > Signed-off-by: Yifeng Sun
> > ---
> Thanks for the fix.
>
> Acked-by: Yi-Hung Wei
I applied this to master. If it should be backported, let me know.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4] Support for match & set ICMPv6 reserved and options type fields
On Mon, Jan 28, 2019 at 11:41:06AM +, Vishal Deep Ajmera wrote: > Currently OVS supports all ARP protocol fields as OXM match fields to > implement the relevant ARP procedures for IPv4. This includes support > for matching copying and setting ARP fields. In IPv6 ARP has been > replaced by ICMPv6 neighbor discovery (ND) procedures, neighbor > advertisement and neighbor solicitation. > > The support for ICMPv6 fields in OVS is not complete for the use cases > equivalent to ARP in IPv4. OVS lacks support for matching, copying and > setting the “ND option type” and “ND reserved” fields. Without these user > cannot implement all ICMPv6 ND procedures for IPv6 support. > > This commit adds additional OXM fields to OVS for ICMPv6 “ND option type“ > and ICMPv6 “ND reserved” using the OXM extension mechanism. This allows > support for parsing these fields from an ICMPv6 packet header and extending > the OpenFlow protocol with specifications for these new OXM fields for > matching, copying and setting. > > Signed-off-by: Vishal Deep Ajmera > Co-authored-by: Ashvin Lakshmikantha > Signed-off-by: Ashvin Lakshmikantha Applied to master, thanks! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] odp-util: Stop parse odp actions if nlattr is overflow
On Fri, Feb 01, 2019 at 03:56:04PM -0800, Yifeng Sun wrote: > `encap = nl_msg_start_nested(key, OVS_KEY_ATTR_ENCAP)` ensures that > key->size >= (encap + NLA_HDRLEN), so the `if` statement is safe. > > Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=11306 > Signed-off-by: Yifeng Sun Thank you. I applied this to master and backported it as far as it would go. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofp-actions: Set an action depth limit to prevent stackoverflow by ofpacts_parse
On Fri, Feb 01, 2019 at 04:44:26PM -0800, Yifeng Sun wrote: > Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12557 > Signed-off-by: Yifeng Sun I'm not enthusiastic about the CONST_CASTs here, but it solves the problem and I don't expect them to cause a real problem of their own. Thank you! I applied this to master and backported as far as it would go. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v3] conntrack: Fix possibly uninitialized memory.
On 2/4/19, 12:24 PM, "Ben Pfaff" wrote:
On Mon, Feb 04, 2019 at 07:52:18PM +, Darrell Ball wrote:
>
>
> On 2/4/19, 11:15 AM, "ovs-dev-boun...@openvswitch.org on behalf of Ben
Pfaff" wrote:
>
> On Sun, Feb 03, 2019 at 02:15:27PM -0800, Darrell Ball wrote:
> > There are a few cases where padding may be undefined according to
> > the C standard. Practically, it seems implementations don't have
issue,
> > but it is better to be safe. The code paths modified are not hot
ones.
> >
> > Found by inspection.
> >
> > Signed-off-by: Darrell Ball
> > ---
> >
> > v3: Removed an unnecessary change and limited scope of 2 others.
> > v2: Found another one; will double check for others later.
> >
> > lib/conntrack.c | 9 ++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index e1f4041..4c8d71b 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -748,6 +748,7 @@ conn_lookup(struct conntrack *ct, const struct
conn_key *key, long long now)
> > {
> > struct conn_lookup_ctx ctx;
> > ctx.conn = NULL;
> > +memset(, 0, sizeof ctx.key);
> > ctx.key = *key;
> > ctx.hash = conn_key_hash(key, ct->hash_basis);
> > unsigned bucket = hash_to_bucket(ctx.hash);
>
> We are talking about technical aspects of the C standard here. While
> it's a somewhat petty distinction, padding bytes take unspecified
> values, not undefined ones.
>
> One can certainly assign values to the padding bytes with memset (or
> otherwise using character types), but there's no guarantee (as far as
I
> know) that struct assignment doesn't overwrite the padding bytes with
> unspecified values.
>
> That is a very interesting interpretation of the standard.
> My understanding is that there were only two possibilities
>
> 1/ Structure assignment copies the padding, which are always originally
initialized
> 2/ Structure assignment simply skips copying the padding, in which case
the destination padding bytes
>are left as they were b4.
>
> The third possibility of structure assignment overwriting the padding
with unspecified values is not
> something I considered, because it seemed pointless to do that.
The standard does not say much in this area. However, if the spec
writers meant for there to be a limited number of possibilities, then
the logical way to express that would have been to explain those
possibilities, or to at least term this implementation-defined behavior
instead of leaving it unspecified.
I don't know of any constraints that prevent possibility #3. The
"obvious" way that it would happen is a compiler that generates a
partial-word load and then a full-word write, so that data previously in
the register before the partial load "leaks" into the full-word write.
I don't know that any compiler does that--but I also don't know that
compilers are written to avoid it.
Hard to disprove the possibility in all cases.
It's the sort of thing where you
don't find out until you troubleshoot a really nasty bug that only
appears with GCC x.y.z.
> C11 6.2.6.1#11 essentially says that;
>
> When a value is stored in an object of structure or union type,
> including in a member object, the bytes of the object
representation
> that correspond to any padding bytes take unspecified values.51)
>
> 51) Thus, for example, structure assignment need not copy any
> padding bits.
>
> So I don't think there is value, standards-wise, to putting a memset
> here. I don't know whether there is value, implementation-wise. Do
you
> know of a reason to believe it?
>
> memcpy is certainly the first choice that came to mind
> I chose memset because:
> 1/ It is highly optimized and combined with structure copy (also
optimized) is potentially faster than memcpy.
> 2/ memset zero and structure copy are easier to digest compared to
memcpy, although there are
> two lines instead of one.
>
> I am certainly fine either way though.
If the goal is to avoid poorly defined or specified behavior, then I'd
argue for memcpy.
Since the goal of the patch is to play it safe in all possible cases, memcpy is
the clear choice.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovs-tcpdump: Fix an undefined variable
On Mon, Feb 04, 2019 at 11:50:22AM -0500, Aaron Conole wrote:
> Hyong Youb Kim via dev writes:
>
> > From: Hyong Youb Kim
> >
> > Run ovs-tcpdump without --span, and it throws the following
> > exception. Define mirror_select_all to avoid the error.
> >
> > Traceback (most recent call last):
> > File "/usr/local/bin/ovs-tcpdump", line 488, in
> > main()
> > File "/usr/local/bin/ovs-tcpdump", line 454, in main
> > mirror_select_all)
> > UnboundLocalError: local variable 'mirror_select_all' referenced before
> > assignment
> >
> > Fixes: 0475db71c650 ("ovs-tcpdump: Add --span to mirror all ports on
> > bridge.")
> >
> > Signed-off-by: Hyong Youb Kim
> > Acked-by: Ilya Maximets
> > ---
>
> Acked-by: Aaron Conole
Thanks, Hyong (and Aaron). I applied this to master and backported it
as far as it would go.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v3] conntrack: Fix possibly uninitialized memory.
On Mon, Feb 04, 2019 at 07:52:18PM +, Darrell Ball wrote:
>
>
> On 2/4/19, 11:15 AM, "ovs-dev-boun...@openvswitch.org on behalf of Ben
> Pfaff" wrote:
>
> On Sun, Feb 03, 2019 at 02:15:27PM -0800, Darrell Ball wrote:
> > There are a few cases where padding may be undefined according to
> > the C standard. Practically, it seems implementations don't have issue,
> > but it is better to be safe. The code paths modified are not hot ones.
> >
> > Found by inspection.
> >
> > Signed-off-by: Darrell Ball
> > ---
> >
> > v3: Removed an unnecessary change and limited scope of 2 others.
> > v2: Found another one; will double check for others later.
> >
> > lib/conntrack.c | 9 ++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index e1f4041..4c8d71b 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -748,6 +748,7 @@ conn_lookup(struct conntrack *ct, const struct
> conn_key *key, long long now)
> > {
> > struct conn_lookup_ctx ctx;
> > ctx.conn = NULL;
> > +memset(, 0, sizeof ctx.key);
> > ctx.key = *key;
> > ctx.hash = conn_key_hash(key, ct->hash_basis);
> > unsigned bucket = hash_to_bucket(ctx.hash);
>
> We are talking about technical aspects of the C standard here. While
> it's a somewhat petty distinction, padding bytes take unspecified
> values, not undefined ones.
>
> One can certainly assign values to the padding bytes with memset (or
> otherwise using character types), but there's no guarantee (as far as I
> know) that struct assignment doesn't overwrite the padding bytes with
> unspecified values.
>
> That is a very interesting interpretation of the standard.
> My understanding is that there were only two possibilities
>
> 1/ Structure assignment copies the padding, which are always originally
> initialized
> 2/ Structure assignment simply skips copying the padding, in which case the
> destination padding bytes
>are left as they were b4.
>
> The third possibility of structure assignment overwriting the padding with
> unspecified values is not
> something I considered, because it seemed pointless to do that.
The standard does not say much in this area. However, if the spec
writers meant for there to be a limited number of possibilities, then
the logical way to express that would have been to explain those
possibilities, or to at least term this implementation-defined behavior
instead of leaving it unspecified.
I don't know of any constraints that prevent possibility #3. The
"obvious" way that it would happen is a compiler that generates a
partial-word load and then a full-word write, so that data previously in
the register before the partial load "leaks" into the full-word write.
I don't know that any compiler does that--but I also don't know that
compilers are written to avoid it. It's the sort of thing where you
don't find out until you troubleshoot a really nasty bug that only
appears with GCC x.y.z.
> C11 6.2.6.1#11 essentially says that;
>
> When a value is stored in an object of structure or union type,
> including in a member object, the bytes of the object representation
> that correspond to any padding bytes take unspecified values.51)
>
> 51) Thus, for example, structure assignment need not copy any
> padding bits.
>
> So I don't think there is value, standards-wise, to putting a memset
> here. I don't know whether there is value, implementation-wise. Do you
> know of a reason to believe it?
>
> memcpy is certainly the first choice that came to mind
> I chose memset because:
> 1/ It is highly optimized and combined with structure copy (also optimized)
> is potentially faster than memcpy.
> 2/ memset zero and structure copy are easier to digest compared to memcpy,
> although there are
> two lines instead of one.
>
> I am certainly fine either way though.
If the goal is to avoid poorly defined or specified behavior, then I'd
argue for memcpy.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 3/3] configure.ac: use the locally provided pkg.m4
On Wed, Jan 30, 2019 at 11:08:32AM +0100, Christian Ehrhardt wrote: > Include the locally provided pkg.m4 before calling the > PKG_PROG_PKG_CONFIG macro. > > Signed-off-by: Christian Ehrhardt > --- > configure.ac | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/configure.ac b/configure.ac > index 505e3d041..dc6eebbf5 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -28,6 +28,8 @@ AC_PROG_CPP > AC_PROG_MKDIR_P > AC_PROG_FGREP > AC_PROG_EGREP > + > +m4_include([m4/pkg.m4]) This should not be needed: aclocal should take care of it. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next V2 1/1] openvswitch: Declare ovs key structures using macros
From: Gregory Rose
Date: Mon, 4 Feb 2019 11:41:29 -0800
>
> On 2/3/2019 1:12 AM, Eli Britstein wrote:
>> Declare ovs key structures using macros as a pre-step towards to
>> enable retrieving fields information, as a work done in proposed
>> commit in the OVS tree https://patchwork.ozlabs.org/patch/1023406/
>> ("odp-util: Do not rewrite fields with the same values as matched"),
>> with no functional change.
>>
>> Signed-off-by: Eli Britstein
>> Reviewed-by: Roi Dayan
>
> Obscuring the structures with these macros is awful. I'm opposed but
> I see it has already been
> accepted upstream so I guess that's that.
I am personally in no way obligated to apply this patch to my tree
just because "upstream" did, and I absolutely have no plans to do so
at this point.
This patch is absolutely awful.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next V2 1/1] openvswitch: Declare ovs key structures using macros
From: Yi-Hung Wei Date: Mon, 4 Feb 2019 10:47:18 -0800 > For example, to see how 'struct ovs_key_ipv6' is defined, now we need > to trace how OVS_KEY_IPV6_FIELDS is defined, and how OVS_KEY_FIELD_ARR > and OVS_KEY_FIELD defined. I think it makes the header file to be > more complicated. I completely agree. Unless this is totally unavoidable, I do not want to apply a patch which makes reading and auditing the networking code more difficult. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 1/3] acinclude: Also use LIBS from dpkg pkg-config
> DPDK 18.11 builds using the more modern meson build system no more provide > the -ldpdk linker script. Instead it is expected to use pkgconfig for > linker options as well. > > This change will set DPDK_LIB from pkg-config (if pkg-config was > available) and since that already carries the whole-archive flags around > the PMDs skips the further wrapping in more whole-archive if that is > already part of DPDK_LIB. > > Signed-off-by: Christian Ehrhardt > --- Hi Christian, Thanks for this, this patch will cause the travis build to fail, I'd suggest it be rolled into patch 2 of the series as its expected for all patches to pass the build individually rather than being corrected by a later patch in the series. https://travis-ci.org/istokes/ovs/builds/488462598 Just a query for the RHEL folks, I remember there being some concern as the DPDK userspace conference with regards to dependencies required for the meson build of DPDK not being supported in RHEL distros, is this still the case? Ian ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v3 1/2] dp-packet: Add 'dp_packet_l3_size()'.
On 2/4/19, 11:23 AM, "ovs-dev-boun...@openvswitch.org on behalf of Ben Pfaff" wrote: On Mon, Jan 28, 2019 at 10:49:03AM -0800, Darrell Ball wrote: > The new api will be used in a subsequent patch. > > Signed-off-by: Darrell Ball I think that this could use some comments on the functions, because "L3 size" (and L4 size) is ambiguous: does it refer to the size of the L3 header, or the size of the packet starting at the L3 header? In this case I guess it is the latter, but I had to look. done ___ dev mailing list d...@openvswitch.org https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-devdata=02%7C01%7Cdball%40vmware.com%7Cfa5bc3dd42c845354a4008d68ad62da9%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636849049856155523sdata=9tIjQ%2B%2FNyuwitN1bhJn9NxREk%2BnzymUNQSmgJfCyRrg%3Dreserved=0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v3] conntrack: Fix possibly uninitialized memory.
On 2/4/19, 11:15 AM, "ovs-dev-boun...@openvswitch.org on behalf of Ben Pfaff"
wrote:
On Sun, Feb 03, 2019 at 02:15:27PM -0800, Darrell Ball wrote:
> There are a few cases where padding may be undefined according to
> the C standard. Practically, it seems implementations don't have issue,
> but it is better to be safe. The code paths modified are not hot ones.
>
> Found by inspection.
>
> Signed-off-by: Darrell Ball
> ---
>
> v3: Removed an unnecessary change and limited scope of 2 others.
> v2: Found another one; will double check for others later.
>
> lib/conntrack.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index e1f4041..4c8d71b 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -748,6 +748,7 @@ conn_lookup(struct conntrack *ct, const struct
conn_key *key, long long now)
> {
> struct conn_lookup_ctx ctx;
> ctx.conn = NULL;
> +memset(, 0, sizeof ctx.key);
> ctx.key = *key;
> ctx.hash = conn_key_hash(key, ct->hash_basis);
> unsigned bucket = hash_to_bucket(ctx.hash);
We are talking about technical aspects of the C standard here. While
it's a somewhat petty distinction, padding bytes take unspecified
values, not undefined ones.
One can certainly assign values to the padding bytes with memset (or
otherwise using character types), but there's no guarantee (as far as I
know) that struct assignment doesn't overwrite the padding bytes with
unspecified values.
That is a very interesting interpretation of the standard.
My understanding is that there were only two possibilities
1/ Structure assignment copies the padding, which are always originally
initialized
2/ Structure assignment simply skips copying the padding, in which case the
destination padding bytes
are left as they were b4.
The third possibility of structure assignment overwriting the padding with
unspecified values is not
something I considered, because it seemed pointless to do that.
C11 6.2.6.1#11 essentially says that;
When a value is stored in an object of structure or union type,
including in a member object, the bytes of the object representation
that correspond to any padding bytes take unspecified values.51)
51) Thus, for example, structure assignment need not copy any
padding bits.
So I don't think there is value, standards-wise, to putting a memset
here. I don't know whether there is value, implementation-wise. Do you
know of a reason to believe it?
memcpy is certainly the first choice that came to mind
I chose memset because:
1/ It is highly optimized and combined with structure copy (also optimized) is
potentially faster than memcpy.
2/ memset zero and structure copy are easier to digest compared to memcpy,
although there are
two lines instead of one.
I am certainly fine either way though.
If we have a "delicate" type like this, then maybe we should use
memcpy() to copy it. (And be sure that we are somehow initializing
those padding bytes in the source of the memcpy().)
___
dev mailing list
d...@openvswitch.org
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-devdata=02%7C01%7Cdball%40vmware.com%7C8aea287cf6c24446a89e08d68ad51118%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636849045150275812sdata=Zx0550Y62amgeZi7wDTnw144hBYPCLJgp1vRrzSdf%2BY%3Dreserved=0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Pedi tu cuaderno con tu logo por $79
IMPRENTA DE MARKETING AL MÁS BAJO COSTO ( https://www.fineart.com.ar/cuadernos-promo/ ) CUADERNOS PERSONALIZADOS * Tus acciones de MKT mucho más duraderas. * 100% de usabilidad de tus clientes. * Publicitá tu empresa todos los días del año. * Amplio retorno de la inversión garantizado. ¡DISEÑO GRÁFICO BONIFICADO! PEDÍ LOS TUYOS ( https://www.fineart.com.ar/cuadernos-promo/ ) CUADERNOS PERSONALIZADOS TAMAÑO A5 (15cm x 21cm) 80 HOJAS PERSONALIZADAS ANILLADO RING WIRE TAPAS DURAS IMPRESAS TU MARCA EN TAPAS E INTERIOR OPCIONAL: CIERRE ELÁSTICO DESDE $79+IVA C/U PEDÍ LOS TUYOS ( https://www.fineart.com.ar/cuadernos-promo/ ) FINEART EN NÚMEROS 10+ AÑOS TRABAJANDO PARA EMPRESAS Business to Business 400+ EMPRESAS, PYMES Y ESTUDIOS Atendidos al año. 50.000+ AGENDAS PERSONALIZADAS Impresas al año. 400.000+ CUADERNOS, LIBRETAS Y ANOTADORES Impresos al año. CONTACTO Balcarce 225 · Ramos Mejía (54 11) 4653.6260 FINEART.COM.AR Este es un e-mail legal y contiene informacion de servicios y productos que consideramos pueden ser de su interes. De acuerdo con la nueva Ley argentina Nro. 26.032, la libre distribucion de este e-mail esta autorizada por tratarse de propositos de informacion, sin embargo, si le hemos causado alguna molestia por el mismo, le rogamos acepte nuestras disculpas. Si lo desea puede ser quitado de nuestra lista de correos solicitandolo a baj...@yopmail.com ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next V2 1/1] openvswitch: Declare ovs key structures using macros
On 2/3/2019 1:12 AM, Eli Britstein wrote:
Declare ovs key structures using macros as a pre-step towards to
enable retrieving fields information, as a work done in proposed
commit in the OVS tree https://patchwork.ozlabs.org/patch/1023406/
("odp-util: Do not rewrite fields with the same values as matched"),
with no functional change.
Signed-off-by: Eli Britstein
Reviewed-by: Roi Dayan
Obscuring the structures with these macros is awful. I'm opposed but I
see it has already been
accepted upstream so I guess that's that.
Ugh...
- Greg
---
include/uapi/linux/openvswitch.h | 102 ++-
1 file changed, 69 insertions(+), 33 deletions(-)
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index dbe0cbe4f1b7..dc8246f871fd 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -387,73 +387,109 @@ enum ovs_frag_type {
#define OVS_FRAG_TYPE_MAX (__OVS_FRAG_TYPE_MAX - 1)
+#define OVS_KEY_FIELD_ARR(type, name, elements) type name[elements];
+#define OVS_KEY_FIELD(type, name) type name;
+
+#define OVS_KEY_ETHERNET_FIELDS \
+OVS_KEY_FIELD_ARR(__u8, eth_src, ETH_ALEN) \
+OVS_KEY_FIELD_ARR(__u8, eth_dst, ETH_ALEN)
+
struct ovs_key_ethernet {
- __u8 eth_src[ETH_ALEN];
- __u8 eth_dst[ETH_ALEN];
+OVS_KEY_ETHERNET_FIELDS
};
struct ovs_key_mpls {
__be32 mpls_lse;
};
+#define OVS_KEY_IPV4_FIELDS \
+OVS_KEY_FIELD(__be32, ipv4_src) \
+OVS_KEY_FIELD(__be32, ipv4_dst) \
+OVS_KEY_FIELD(__u8, ipv4_proto) \
+OVS_KEY_FIELD(__u8, ipv4_tos) \
+OVS_KEY_FIELD(__u8, ipv4_ttl) \
+OVS_KEY_FIELD(__u8, ipv4_frag /* One of OVS_FRAG_TYPE_*. */)
+
struct ovs_key_ipv4 {
- __be32 ipv4_src;
- __be32 ipv4_dst;
- __u8 ipv4_proto;
- __u8 ipv4_tos;
- __u8 ipv4_ttl;
- __u8 ipv4_frag; /* One of OVS_FRAG_TYPE_*. */
+OVS_KEY_IPV4_FIELDS
};
+#define OVS_KEY_IPV6_FIELDS \
+OVS_KEY_FIELD_ARR(__be32, ipv6_src, 4) \
+OVS_KEY_FIELD_ARR(__be32, ipv6_dst, 4) \
+OVS_KEY_FIELD(__be32, ipv6_label /* 20-bits in least-significant bits. */)
\
+OVS_KEY_FIELD(__u8, ipv6_proto) \
+OVS_KEY_FIELD(__u8, ipv6_tclass) \
+OVS_KEY_FIELD(__u8, ipv6_hlimit) \
+OVS_KEY_FIELD(__u8, ipv6_frag /* One of OVS_FRAG_TYPE_*. */)
+
struct ovs_key_ipv6 {
- __be32 ipv6_src[4];
- __be32 ipv6_dst[4];
- __be32 ipv6_label; /* 20-bits in least-significant bits. */
- __u8 ipv6_proto;
- __u8 ipv6_tclass;
- __u8 ipv6_hlimit;
- __u8 ipv6_frag; /* One of OVS_FRAG_TYPE_*. */
+OVS_KEY_IPV6_FIELDS
};
+#define OVS_KEY_TCP_FIELDS \
+OVS_KEY_FIELD(__be16, tcp_src) \
+OVS_KEY_FIELD(__be16, tcp_dst)
+
struct ovs_key_tcp {
- __be16 tcp_src;
- __be16 tcp_dst;
+OVS_KEY_TCP_FIELDS
};
+#define OVS_KEY_UDP_FIELDS \
+OVS_KEY_FIELD(__be16, udp_src) \
+OVS_KEY_FIELD(__be16, udp_dst)
+
struct ovs_key_udp {
- __be16 udp_src;
- __be16 udp_dst;
+OVS_KEY_UDP_FIELDS
};
+#define OVS_KEY_SCTP_FIELDS \
+OVS_KEY_FIELD(__be16, sctp_src) \
+OVS_KEY_FIELD(__be16, sctp_dst)
+
struct ovs_key_sctp {
- __be16 sctp_src;
- __be16 sctp_dst;
+OVS_KEY_SCTP_FIELDS
};
+#define OVS_KEY_ICMP_FIELDS \
+OVS_KEY_FIELD(__u8, icmp_type) \
+OVS_KEY_FIELD(__u8, icmp_code)
+
struct ovs_key_icmp {
- __u8 icmp_type;
- __u8 icmp_code;
+OVS_KEY_ICMP_FIELDS
};
+#define OVS_KEY_ICMPV6_FIELDS \
+OVS_KEY_FIELD(__u8, icmpv6_type) \
+OVS_KEY_FIELD(__u8, icmpv6_code)
+
struct ovs_key_icmpv6 {
- __u8 icmpv6_type;
- __u8 icmpv6_code;
+OVS_KEY_ICMPV6_FIELDS
};
+#define OVS_KEY_ARP_FIELDS \
+OVS_KEY_FIELD(__be32, arp_sip) \
+OVS_KEY_FIELD(__be32, arp_tip) \
+OVS_KEY_FIELD(__be16, arp_op) \
+OVS_KEY_FIELD_ARR(__u8, arp_sha, ETH_ALEN) \
+OVS_KEY_FIELD_ARR(__u8, arp_tha, ETH_ALEN)
+
struct ovs_key_arp {
- __be32 arp_sip;
- __be32 arp_tip;
- __be16 arp_op;
- __u8 arp_sha[ETH_ALEN];
- __u8 arp_tha[ETH_ALEN];
+OVS_KEY_ARP_FIELDS
};
+#define OVS_KEY_ND_FIELDS \
+OVS_KEY_FIELD_ARR(__be32, nd_target, 4) \
+OVS_KEY_FIELD_ARR(__u8, nd_sll, ETH_ALEN) \
+OVS_KEY_FIELD_ARR(__u8, nd_tll, ETH_ALEN)
+
struct ovs_key_nd {
- __be32 nd_target[4];
- __u8nd_sll[ETH_ALEN];
- __u8nd_tll[ETH_ALEN];
+OVS_KEY_ND_FIELDS
};
+#undef OVS_KEY_FIELD_ARR
+#undef OVS_KEY_FIELD
+
#define OVS_CT_LABELS_LEN_32 4
#define OVS_CT_LABELS_LEN (OVS_CT_LABELS_LEN_32 * sizeof(__u32))
struct ovs_key_ct_labels {
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [branch 2.9] ofp-packet: Fix NXT_RESUME with geneve tunnel metadata
On Sat, Feb 02, 2019 at 12:27:51AM +0530, nusid...@redhat.com wrote: > From: Yi-Hung Wei > > The patch address vswitchd crash when it receives NXT_RESUME with geneve > tunnel metadata. The crash is due to segmentation fault with the > following stack trace, and it is observed only in kernel datapath. > A test is added to prevent regression. Thanks, Numan (and Yi-Hung). I applied this to branch-2.9. If it should be backported further, let me know. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] ovn-controller: Fix chassisredirect port flapping when ovs-vswitchd crashes
On Mon, Feb 04, 2019 at 10:20:31PM +0530, nusid...@redhat.com wrote: > From: Numan Siddique > > On a chassis when ovs-vswitchd crashes for some reason, the BFD status doesn't > get updated in the ovs db. ovn-controller will be reading the old BFD status > even though ovs-vswitchd is crashed. This results in the chassiredirect port > claim flapping between the master chassis and the chasiss with the next higher > priority if ovs-vswitchd crashes in the master chassis. > > All the other chassis notices the BFD status down with the master chassis > and hence the next higher priority claims the port. But according to > the master chassis, the BFD status is fine and it again claims back the > chassisredirect port. And this results in flapping. The issue gets resolved > when ovs-vswitchd comes back but until then it leads to lot of SB DB > transactions and high CPU usage in ovn-controller's. > > This patch fixes the issue by checking the OF connection status of the > ovn-controller with ovs-vswitchd and calculates the active bfd tunnels > only if it's connected. > > Signed-off-by: Numan Siddique > Acked-by: Mark Michelson Thanks. Applied to master and branch-2.11. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V2 1/2] acinclude: Include libverbs and libmlx5 when needed
On Tue, Jan 29, 2019 at 04:00:46PM +0200, Eli Britstein wrote: > DPDK 18.11 uses libverbs and libmlx5 when MLX5 PMD is enabled. > > This commit makes OVS to link to libverbs and libmlx5 when MLX5 PMD is > enabled on DPDK. > > Signed-off-by: Eli Britstein > Reviewed-by: Shahaf Shuler > Reviewed-by: Asaf Penso I don't see yet why this defines preprocessor symbols DPDK_MLX5 and DPDK_VERBS. Nothing in OVS uses them. This seems to be a preprocessor-only test, do you want to use AC_PREPROC_IFELSE? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v3 1/2] dp-packet: Add 'dp_packet_l3_size()'.
On Mon, Jan 28, 2019 at 10:49:03AM -0800, Darrell Ball wrote: > The new api will be used in a subsequent patch. > > Signed-off-by: Darrell Ball I think that this could use some comments on the functions, because "L3 size" (and L4 size) is ambiguous: does it refer to the size of the L3 header, or the size of the packet starting at the L3 header? In this case I guess it is the latter, but I had to look. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v3] conntrack: Fix possibly uninitialized memory.
On Sun, Feb 03, 2019 at 02:15:27PM -0800, Darrell Ball wrote:
> There are a few cases where padding may be undefined according to
> the C standard. Practically, it seems implementations don't have issue,
> but it is better to be safe. The code paths modified are not hot ones.
>
> Found by inspection.
>
> Signed-off-by: Darrell Ball
> ---
>
> v3: Removed an unnecessary change and limited scope of 2 others.
> v2: Found another one; will double check for others later.
>
> lib/conntrack.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index e1f4041..4c8d71b 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -748,6 +748,7 @@ conn_lookup(struct conntrack *ct, const struct conn_key
> *key, long long now)
> {
> struct conn_lookup_ctx ctx;
> ctx.conn = NULL;
> +memset(, 0, sizeof ctx.key);
> ctx.key = *key;
> ctx.hash = conn_key_hash(key, ct->hash_basis);
> unsigned bucket = hash_to_bucket(ctx.hash);
We are talking about technical aspects of the C standard here. While
it's a somewhat petty distinction, padding bytes take unspecified
values, not undefined ones.
One can certainly assign values to the padding bytes with memset (or
otherwise using character types), but there's no guarantee (as far as I
know) that struct assignment doesn't overwrite the padding bytes with
unspecified values. C11 6.2.6.1#11 essentially says that;
When a value is stored in an object of structure or union type,
including in a member object, the bytes of the object representation
that correspond to any padding bytes take unspecified values.51)
51) Thus, for example, structure assignment need not copy any
padding bits.
So I don't think there is value, standards-wise, to putting a memset
here. I don't know whether there is value, implementation-wise. Do you
know of a reason to believe it?
If we have a "delicate" type like this, then maybe we should use
memcpy() to copy it. (And be sure that we are somehow initializing
those padding bytes in the source of the memcpy().)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next V2 1/1] openvswitch: Declare ovs key structures using macros
On Sun, Feb 3, 2019 at 1:13 AM Eli Britstein wrote:
>
> Declare ovs key structures using macros as a pre-step towards to
> enable retrieving fields information, as a work done in proposed
> commit in the OVS tree https://patchwork.ozlabs.org/patch/1023406/
> ("odp-util: Do not rewrite fields with the same values as matched"),
> with no functional change.
>
> Signed-off-by: Eli Britstein
> Reviewed-by: Roi Dayan
> ---
> include/uapi/linux/openvswitch.h | 102
> ++-
> 1 file changed, 69 insertions(+), 33 deletions(-)
>
> diff --git a/include/uapi/linux/openvswitch.h
> b/include/uapi/linux/openvswitch.h
> index dbe0cbe4f1b7..dc8246f871fd 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -387,73 +387,109 @@ enum ovs_frag_type {
>
> #define OVS_FRAG_TYPE_MAX (__OVS_FRAG_TYPE_MAX - 1)
>
> +#define OVS_KEY_FIELD_ARR(type, name, elements) type name[elements];
> +#define OVS_KEY_FIELD(type, name) type name;
..
> +#define OVS_KEY_IPV6_FIELDS \
> +OVS_KEY_FIELD_ARR(__be32, ipv6_src, 4) \
> +OVS_KEY_FIELD_ARR(__be32, ipv6_dst, 4) \
> +OVS_KEY_FIELD(__be32, ipv6_label /* 20-bits in least-significant bits.
> */) \
> +OVS_KEY_FIELD(__u8, ipv6_proto) \
> +OVS_KEY_FIELD(__u8, ipv6_tclass) \
> +OVS_KEY_FIELD(__u8, ipv6_hlimit) \
> +OVS_KEY_FIELD(__u8, ipv6_frag /* One of OVS_FRAG_TYPE_*. */)
> +
> struct ovs_key_ipv6 {
> - __be32 ipv6_src[4];
> - __be32 ipv6_dst[4];
> - __be32 ipv6_label; /* 20-bits in least-significant bits. */
> - __u8 ipv6_proto;
> - __u8 ipv6_tclass;
> - __u8 ipv6_hlimit;
> - __u8 ipv6_frag; /* One of OVS_FRAG_TYPE_*. */
> +OVS_KEY_IPV6_FIELDS
> };
Hi Eli,
Thanks for the patch. In my personal opinion, I feel this patch makes
the header file harder to read.
For example, to see how 'struct ovs_key_ipv6' is defined, now we need
to trace how OVS_KEY_IPV6_FIELDS is defined, and how OVS_KEY_FIELD_ARR
and OVS_KEY_FIELD defined. I think it makes the header file to be
more complicated.
There are also some discussion on ovs-dev mailing list about this
patch: https://patchwork.ozlabs.org/cover/1023404/
Thanks,
-Yi-Hung
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Recuperación de cartera vencida
Cursos esenciales - Webinar Interactivo – Jueves 21 de Febrero Recuperación de cartera vencida Este webinar interactivo está diseñado para brindar a las empresas estrategias y técnicas para desarrollar mejores procedimientos de recuperación de cartera dentro del marco legar pertinente y en una relación de ganar- ganar con los clientes. Aprenderemos a manejar una cartera exitosa, recuperar el dinero y obtener ganancias. Objetivos Específicos: • El participante aprenderá a controlar su cartera vencida con ciertas medidas de cobranza que le permitirán suspender. • El participante podrá analizar el historial crediticio del cliente. • El participante mantendrá a sus finanzas organizadas. Para mayor información, responder sobre este correo con la palabra Cartera + los siguientes datos: NOMBRE: TELÉFONO: EMPRESA: Llamanos al (045) 55 1554 6630 www.Innovalearn.mx ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] system-dpdk-macros.at: Drop dpdk-socket-mem configuration.
> There are two reasons: > 1. OVS provides same default itself. > 2. socket-mem is not necessary with dynamic memory model in DPDK 18.11. > > Signed-off-by: Ilya Maximets Thanks Ilya, pushed to master and 2.11. Ian ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v2] conntrack: Fix possible uninitialized memory.
On 2/4/19, 8:53 AM, "ovs-dev-boun...@openvswitch.org on behalf of Aaron
Conole" wrote:
Darrell Ball writes:
> There are a few cases where padding may be undefined according to
> the C standard. Practically, it seems implementations don't have issue,
> but it is better to be safe. The code paths modified are not hot ones.
> Found by inspection.
Better to be safe how? What do implementations not have issue with?
I'm having trouble understanding this commit message as a rationale for
the change.
The conn keys are used in a hash calculation
> Signed-off-by: Darrell Ball
> ---
>
> v2: Found another one; will double check for others later.
Were there actual problems observed somewhere? From what I can see,
none of these accesses did any kind of bit-wise structure memcmp().
Rather, I see use of named fields when doing comparisons. This tells me
that the pad bits should be irrelevant. Probably I missed something.
OTOH, I guess you might also be interested in fixing:
'struct ct_addr first_addr = ct_addr;' on line 2181 of conntrack.c
Existing structure copy should work
and the 'process_ftp_ctl_v6' function. Those do possibly use memcmp()
with indeterminate pad bits, if I'm reading them correctly (but just
cursory reading).
Existing structure copy and the memset zero should work.
> lib/conntrack.c | 13 +
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index e1f4041..e7033a8 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -747,6 +747,7 @@ static struct conn *
> conn_lookup(struct conntrack *ct, const struct conn_key *key, long long
now)
> {
> struct conn_lookup_ctx ctx;
> +memset(, 0, sizeof ctx);
> ctx.conn = NULL;
> ctx.key = *key;
> ctx.hash = conn_key_hash(key, ct->hash_basis);
How does this help? Looking at conn_key_hash it seems to only use those
defined sections of the struct (ie: it doesn't walk the raw memory, it
pulls the fields it needs by assignment). conn_key_cmp also compares
named fields.
Again, For the hash; conn_key has padding
> @@ -896,6 +897,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet
*pkt,
>
> if (nat_action_info) {
> nc->nat_info = xmemdup(nat_action_info, sizeof
*nc->nat_info);
> +memset(conn_for_un_nat_copy, 0, sizeof
*conn_for_un_nat_copy);
Was there an observable bug related to the pad bits here?
No, it is just adhering to C standard
Implementations are 'very unlikely' to have issue.
I didn't walk this path deep enough, but in conntrack.c I didn't see any
related
memcmp() calls.
Again, for the hash later
> if (alg_exp) {
> if (alg_exp->nat_rpl_dst) {
> @@ -934,8 +936,6 @@ conn_not_found(struct conntrack *ct, struct dp_packet
*pkt,
> ct_rwlock_unlock(>resources_lock);
> }
> conn_for_un_nat_copy->conn_type = CT_CONN_TYPE_UN_NAT;
> -conn_for_un_nat_copy->nat_info = NULL;
> -conn_for_un_nat_copy->alg = NULL;
If this was needed here before, isn't it still needed now? There are a
interrim assignments from 'nc', and I think that won't be changed by
doing a memset() (ie: does this still cause a double free if you drop it?).
Not related to double free
Oops, this is still needed in present code base
> nat_packet(pkt, nc, ctx->icmp_related);
> }
> hmap_insert(>buckets[bucket].connections, >node,
ctx->hash);
> @@ -1262,6 +1262,7 @@ process_one(struct conntrack *ct, struct dp_packet
*pkt,
> ct->hash_basis,
> alg_src_ip_wc(ct_alg_ctl));
> if (alg_exp) {
> +memset(_exp_entry, 0, sizeof alg_exp_entry);
> alg_exp_entry = *alg_exp;
> alg_exp = _exp_entry;
> }
> @@ -2024,7 +2025,9 @@ conn_key_hash(const struct conn_key *key, uint32_t
basis)
> static void
> conn_key_reverse(struct conn_key *key)
> {
> -struct ct_endpoint tmp = key->src;
> +struct ct_endpoint tmp;
> +memset(, 0, sizeof tmp);
> +tmp = key->src;
I don't understand the reason to clear the pad bits of tmp here - you
only use the named fields and they should be of the same size (and if
not, the compiler should be performing integer widening / truncation
properly). Did I miss something?
This change was previously removed in V3.
> key->src = key->dst;
> key->dst = tmp;
> }
> @@ -2614,7 +2617,9 @@ static struct alg_exp_node *
>
Re: [ovs-dev] [patch v5 2/2] conntrack: Fix max size for inet_ntop() call.
On Thu, Jan 31, 2019 at 11:35:41PM -0800, Darrell Ball wrote:
> The call to inet_ntop() in repl_ftp_v6_addr() is 1 short to handle
> the maximum possible V6 address size for v4 mapping case.
>
> Found by inspection.
>
> Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.")
> Signed-off-by: Darrell Ball
Thanks for the bug fix patches. I applied these to master and
backported to 2.10 and 2.11. If they need to be backported further,
please submit backported versions.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v3] conntrack: Fix possibly uninitialized memory.
Darrell Ball writes: > There are a few cases where padding may be undefined according to > the C standard. Practically, it seems implementations don't have issue, > but it is better to be safe. The code paths modified are not hot ones. > > Found by inspection. > > Signed-off-by: Darrell Ball > --- My dumb brain had me review v2 instead of this one. Sorry. I think most of the comments still apply, though. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3] ovn-controller: Fix chassisredirect port flapping when ovs-vswitchd crashes
From: Numan Siddique
On a chassis when ovs-vswitchd crashes for some reason, the BFD status doesn't
get updated in the ovs db. ovn-controller will be reading the old BFD status
even though ovs-vswitchd is crashed. This results in the chassiredirect port
claim flapping between the master chassis and the chasiss with the next higher
priority if ovs-vswitchd crashes in the master chassis.
All the other chassis notices the BFD status down with the master chassis
and hence the next higher priority claims the port. But according to
the master chassis, the BFD status is fine and it again claims back the
chassisredirect port. And this results in flapping. The issue gets resolved
when ovs-vswitchd comes back but until then it leads to lot of SB DB
transactions and high CPU usage in ovn-controller's.
This patch fixes the issue by checking the OF connection status of the
ovn-controller with ovs-vswitchd and calculates the active bfd tunnels
only if it's connected.
Signed-off-by: Numan Siddique
Acked-by: Mark Michelson
---
v2 -> v3
* Addressed the review comment from Ilya Maximets in the test code
v1 -> v2
* Deleted unnecessary prints in the test case which were added during
debugging.
ovn/controller/ofctrl.c | 6 +
ovn/controller/ofctrl.h | 3 +++
ovn/controller/ovn-controller.c | 13 +-
tests/ovn.at| 45 -
4 files changed, 60 insertions(+), 7 deletions(-)
diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 218612787..95b95b607 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -1265,3 +1265,9 @@ ofctrl_inject_pkt(const struct ovsrec_bridge *br_int,
const char *flow_s,
return NULL;
}
+
+bool
+ofctrl_is_connected(void)
+{
+return rconn_is_connected(swconn);
+}
diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
index ae0cfa513..f7521801b 100644
--- a/ovn/controller/ofctrl.h
+++ b/ovn/controller/ofctrl.h
@@ -60,4 +60,7 @@ void ofctrl_check_and_add_flow(struct hmap *desired_flows,
uint8_t table_id,
const struct match *,
const struct ofpbuf *ofpacts,
bool log_duplicate_flow);
+
+bool ofctrl_is_connected(void);
+
#endif /* ovn/ofctrl.h */
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 4e9a5865f..2098f280c 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -689,7 +689,18 @@ main(int argc, char *argv[])
ovsrec_bridge_table_get(ovs_idl_loop.idl), br_int,
sbrec_chassis_table_get(ovnsb_idl_loop.idl), chassis_id,
sbrec_sb_global_first(ovnsb_idl_loop.idl));
-bfd_calculate_active_tunnels(br_int, _tunnels);
+
+if (ofctrl_is_connected()) {
+/* Calculate the active tunnels only if have an an active
+ * OpenFlow connection to br-int.
+ * If we don't have a connection to br-int, it could mean
+ * ovs-vswitchd is down for some reason and the BFD status
+ * in the Interface rows could be stale. So its better to
+ * consider 'active_tunnels' set to be empty if it's not
+ * connected. */
+bfd_calculate_active_tunnels(br_int, _tunnels);
+}
+
binding_run(ovnsb_idl_txn, ovs_idl_txn, sbrec_chassis_by_name,
sbrec_datapath_binding_by_key,
sbrec_port_binding_by_datapath,
diff --git a/tests/ovn.at b/tests/ovn.at
index f54f24c74..cfdbf412c 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -7989,6 +7989,7 @@ test_ip_packet()
{
local active_gw=$1
local backup_gw=$2
+local backup_vswitchd_dead=$3
# Send ip packet between foo1 and outside1
src_mac="f0010203" # foo1 mac
@@ -8027,7 +8028,11 @@ grep actions=mod_dl_dst:f0:00:00:01:02:04 | wc -l` -eq 1
echo $expected > ext1-vif1.expected
as $active_gw reset_pcap_file br-phys_n1 $active_gw/br-phys_n1
-as $backup_gw reset_pcap_file br-phys_n1 $backup_gw/br-phys_n1
+
+if test $backup_vswitchd_dead != 1; then
+# Reset the file only if vswitchd in backup gw is alive
+as $backup_gw reset_pcap_file br-phys_n1 $backup_gw/br-phys_n1
+fi
as ext1 reset_pcap_file ext1-vif1 ext1/vif1
# Resend packet from foo1 to outside1
@@ -8038,11 +8043,14 @@ grep actions=mod_dl_dst:f0:00:00:01:02:04 | wc -l` -eq 1
OVN_CHECK_PACKETS([ext1/vif1-tx.pcap], [ext1-vif1.expected])
$PYTHON "$top_srcdir/utilities/ovs-pcap.in" $active_gw/br-phys_n1-tx.pcap
> packets
AT_CHECK([grep $expected packets | sort], [0], [expout])
-$PYTHON "$top_srcdir/utilities/ovs-pcap.in" $backup_gw/br-phys_n1-tx.pcap
> packets
-AT_CHECK([grep $expected packets | sort], [0], [])
+if test
Re: [ovs-dev] [PATCH] ovs-tcpdump: Fix an undefined variable
Hyong Youb Kim via dev writes:
> From: Hyong Youb Kim
>
> Run ovs-tcpdump without --span, and it throws the following
> exception. Define mirror_select_all to avoid the error.
>
> Traceback (most recent call last):
> File "/usr/local/bin/ovs-tcpdump", line 488, in
> main()
> File "/usr/local/bin/ovs-tcpdump", line 454, in main
> mirror_select_all)
> UnboundLocalError: local variable 'mirror_select_all' referenced before
> assignment
>
> Fixes: 0475db71c650 ("ovs-tcpdump: Add --span to mirror all ports on bridge.")
>
> Signed-off-by: Hyong Youb Kim
> Acked-by: Ilya Maximets
> ---
Acked-by: Aaron Conole
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v2] conntrack: Fix possible uninitialized memory.
Darrell Ball writes:
> There are a few cases where padding may be undefined according to
> the C standard. Practically, it seems implementations don't have issue,
> but it is better to be safe. The code paths modified are not hot ones.
> Found by inspection.
Better to be safe how? What do implementations not have issue with?
I'm having trouble understanding this commit message as a rationale for
the change.
> Signed-off-by: Darrell Ball
> ---
>
> v2: Found another one; will double check for others later.
Were there actual problems observed somewhere? From what I can see,
none of these accesses did any kind of bit-wise structure memcmp().
Rather, I see use of named fields when doing comparisons. This tells me
that the pad bits should be irrelevant. Probably I missed something.
OTOH, I guess you might also be interested in fixing:
'struct ct_addr first_addr = ct_addr;' on line 2181 of conntrack.c
and the 'process_ftp_ctl_v6' function. Those do possibly use memcmp()
with indeterminate pad bits, if I'm reading them correctly (but just
cursory reading).
> lib/conntrack.c | 13 +
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index e1f4041..e7033a8 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -747,6 +747,7 @@ static struct conn *
> conn_lookup(struct conntrack *ct, const struct conn_key *key, long long now)
> {
> struct conn_lookup_ctx ctx;
> +memset(, 0, sizeof ctx);
> ctx.conn = NULL;
> ctx.key = *key;
> ctx.hash = conn_key_hash(key, ct->hash_basis);
How does this help? Looking at conn_key_hash it seems to only use those
defined sections of the struct (ie: it doesn't walk the raw memory, it
pulls the fields it needs by assignment). conn_key_cmp also compares
named fields.
> @@ -896,6 +897,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet
> *pkt,
>
> if (nat_action_info) {
> nc->nat_info = xmemdup(nat_action_info, sizeof *nc->nat_info);
> +memset(conn_for_un_nat_copy, 0, sizeof *conn_for_un_nat_copy);
Was there an observable bug related to the pad bits here? I didn't walk
this path deep enough, but in conntrack.c I didn't see any related
memcmp() calls.
> if (alg_exp) {
> if (alg_exp->nat_rpl_dst) {
> @@ -934,8 +936,6 @@ conn_not_found(struct conntrack *ct, struct dp_packet
> *pkt,
> ct_rwlock_unlock(>resources_lock);
> }
> conn_for_un_nat_copy->conn_type = CT_CONN_TYPE_UN_NAT;
> -conn_for_un_nat_copy->nat_info = NULL;
> -conn_for_un_nat_copy->alg = NULL;
If this was needed here before, isn't it still needed now? There are a
interrim assignments from 'nc', and I think that won't be changed by
doing a memset() (ie: does this still cause a double free if you drop it?).
> nat_packet(pkt, nc, ctx->icmp_related);
> }
> hmap_insert(>buckets[bucket].connections, >node, ctx->hash);
> @@ -1262,6 +1262,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
> ct->hash_basis,
> alg_src_ip_wc(ct_alg_ctl));
> if (alg_exp) {
> +memset(_exp_entry, 0, sizeof alg_exp_entry);
> alg_exp_entry = *alg_exp;
> alg_exp = _exp_entry;
> }
> @@ -2024,7 +2025,9 @@ conn_key_hash(const struct conn_key *key, uint32_t
> basis)
> static void
> conn_key_reverse(struct conn_key *key)
> {
> -struct ct_endpoint tmp = key->src;
> +struct ct_endpoint tmp;
> +memset(, 0, sizeof tmp);
> +tmp = key->src;
I don't understand the reason to clear the pad bits of tmp here - you
only use the named fields and they should be of the same size (and if
not, the compiler should be performing integer widening / truncation
properly). Did I miss something?
> key->src = key->dst;
> key->dst = tmp;
> }
> @@ -2614,7 +2617,9 @@ static struct alg_exp_node *
> expectation_lookup(struct hmap *alg_expectations, const struct conn_key *key,
> uint32_t basis, bool src_ip_wc)
> {
> -struct conn_key check_key = *key;
> +struct conn_key check_key;
> +memset(_key, 0, sizeof check_key);
> +check_key = *key;
> check_key.src.port = ALG_WC_SRC_PORT;
>
> if (src_ip_wc) {
Again, conn_key_hash and conn_key_cmp should be correct, no?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Need to retouch your photos?
Need to retouch your photos? Deep etching or masking for your photos? We are the studio who can do those service for your photos. Please send photos to start Thanks, Stacy Ingolsdtadt Nettetal ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] netdev-dpdk: Memset rte_flow_item on a need basis.
In netdev_dpdk_add_rte_flow_offload function different rte_flow_item are
created as part of the pattern matching.
For most of them, there is a check whether the wildcards are not zero.
In case of zero, nothing is being done with the rte_flow_item.
Befor the wildcard check, and regardless of the result, the
rte_flow_item is being memset.
The patch moves the memset function only if the condition of the
wildcard is true, thus saving cycles of memset if not needed.
Signed-off-by: Asaf Penso
---
v2 update coding-style compliant for sizeof operator
---
lib/netdev-dpdk.c | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 4bf0ca9..f07b10c 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -4570,18 +4570,18 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
/* Eth */
struct rte_flow_item_eth eth_spec;
struct rte_flow_item_eth eth_mask;
-memset(_spec, 0, sizeof(eth_spec));
-memset(_mask, 0, sizeof(eth_mask));
if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
!eth_addr_is_zero(match->wc.masks.dl_dst)) {
-rte_memcpy(_spec.dst, >flow.dl_dst, sizeof(eth_spec.dst));
-rte_memcpy(_spec.src, >flow.dl_src, sizeof(eth_spec.src));
+memset(_spec, 0, sizeof eth_spec);
+memset(_mask, 0, sizeof eth_mask);
+rte_memcpy(_spec.dst, >flow.dl_dst, sizeof eth_spec.dst);
+rte_memcpy(_spec.src, >flow.dl_src, sizeof eth_spec.src);
eth_spec.type = match->flow.dl_type;
rte_memcpy(_mask.dst, >wc.masks.dl_dst,
- sizeof(eth_mask.dst));
+ sizeof eth_mask.dst);
rte_memcpy(_mask.src, >wc.masks.dl_src,
- sizeof(eth_mask.src));
+ sizeof eth_mask.src);
eth_mask.type = match->wc.masks.dl_type;
add_flow_pattern(, RTE_FLOW_ITEM_TYPE_ETH,
@@ -4600,9 +4600,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
/* VLAN */
struct rte_flow_item_vlan vlan_spec;
struct rte_flow_item_vlan vlan_mask;
-memset(_spec, 0, sizeof(vlan_spec));
-memset(_mask, 0, sizeof(vlan_mask));
if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
+memset(_spec, 0, sizeof vlan_spec);
+memset(_mask, 0, sizeof vlan_mask);
vlan_spec.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
vlan_mask.tci = match->wc.masks.vlans[0].tci & ~htons(VLAN_CFI);
@@ -4617,9 +4617,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
uint8_t proto = 0;
struct rte_flow_item_ipv4 ipv4_spec;
struct rte_flow_item_ipv4 ipv4_mask;
-memset(_spec, 0, sizeof(ipv4_spec));
-memset(_mask, 0, sizeof(ipv4_mask));
if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
+memset(_spec, 0, sizeof ipv4_spec);
+memset(_mask, 0, sizeof ipv4_mask);
ipv4_spec.hdr.type_of_service = match->flow.nw_tos;
ipv4_spec.hdr.time_to_live= match->flow.nw_ttl;
@@ -4662,9 +4662,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
struct rte_flow_item_tcp tcp_spec;
struct rte_flow_item_tcp tcp_mask;
-memset(_spec, 0, sizeof(tcp_spec));
-memset(_mask, 0, sizeof(tcp_mask));
if (proto == IPPROTO_TCP) {
+memset(_spec, 0, sizeof tcp_spec);
+memset(_mask, 0, sizeof tcp_mask);
tcp_spec.hdr.src_port = match->flow.tp_src;
tcp_spec.hdr.dst_port = match->flow.tp_dst;
tcp_spec.hdr.data_off = ntohs(match->flow.tcp_flags) >> 8;
@@ -4687,9 +4687,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
struct rte_flow_item_udp udp_spec;
struct rte_flow_item_udp udp_mask;
-memset(_spec, 0, sizeof(udp_spec));
-memset(_mask, 0, sizeof(udp_mask));
if (proto == IPPROTO_UDP) {
+memset(_spec, 0, sizeof udp_spec);
+memset(_mask, 0, sizeof udp_mask);
udp_spec.hdr.src_port = match->flow.tp_src;
udp_spec.hdr.dst_port = match->flow.tp_dst;
@@ -4708,9 +4708,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
struct rte_flow_item_sctp sctp_spec;
struct rte_flow_item_sctp sctp_mask;
-memset(_spec, 0, sizeof(sctp_spec));
-memset(_mask, 0, sizeof(sctp_mask));
if (proto == IPPROTO_SCTP) {
+memset(_spec, 0, sizeof sctp_spec);
+memset(_mask, 0, sizeof sctp_mask);
sctp_spec.hdr.src_port = match->flow.tp_src;
sctp_spec.hdr.dst_port = match->flow.tp_dst;
@@ -4729,9 +4729,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
struct rte_flow_item_icmp icmp_spec;
struct rte_flow_item_icmp icmp_mask;
-memset(_spec, 0, sizeof(icmp_spec));
-memset(_mask, 0, sizeof(icmp_mask));
if (proto == IPPROTO_ICMP) {
+memset(_spec, 0, sizeof icmp_spec);
+memset(_mask, 0, sizeof icmp_mask);
icmp_spec.hdr.icmp_type = (uint8_t)ntohs(match->flow.tp_src);
Re: [ovs-dev] netdev-dpdk: Memset rte_flow_item on a need basis.
On 04.02.2019 18:50, Asaf Penso wrote:
>
>
> Regards,
> Asaf Penso
>
>> -Original Message-
>> From: Ilya Maximets
>> Sent: Monday, February 4, 2019 5:41 PM
>> To: Asaf Penso ; ovs-dev@openvswitch.org
>> Cc: Roni Bar Yanai ; Stokes, Ian
>>
>> Subject: Re: [ovs-dev] netdev-dpdk: Memset rte_flow_item on a need basis.
>>
>> On 04.02.2019 18:08, Asaf Penso wrote:
>>>
>>>
>>> Regards,
>>> Asaf Penso
>>>
-Original Message-
From: Ilya Maximets
Sent: Monday, February 4, 2019 4:14 PM
To: Asaf Penso ; ovs-dev@openvswitch.org
Cc: Roni Bar Yanai ; Stokes, Ian
Subject: Re: [ovs-dev] netdev-dpdk: Memset rte_flow_item on a need
>> basis.
On 04.02.2019 15:50, Asaf Penso wrote:
> In netdev_dpdk_add_rte_flow_offload function different
>> rte_flow_item
> are created as part of the pattern matching.
>
> For most of them, there is a check whether the wildcards are not zero.
> In case of zero, nothing is being done with the rte_flow_item.
>
> Befor the wildcard check, and regardless of the result, the
> rte_flow_item is being memset.
>
> The patch moves the memset function only if the condition of the
> wildcard is true, thus saving cycles of memset if not needed.
Structures are actually used only inside their 'if' blocks.
IMHO, it's better to move the definitions inside too.
>>>
>>> Inside each 'if' block there is a call to add_flow_pattern that updates the
>> pattern's pointers to these structs.
>>> Having them defined inside the block will cause their value to be corrupted
>> once we exit the block.
>>> They are needed all the way until rte_flow_create call.
>>
>> Oh. Sorry. You're right. I forget about that.
>>
>> BTW, as you touching this code, maybe you could make it a bit more coding-
>> style compliant ? It's about sizeof operator. Coding-style asks to not
>> parenthesize the operand. Like this:
>>
>> memset(_spec, 0, sizeof eth_spec);
>>
>> It'll be nice if you could fix that.
>> You may also fix that for the 'rte_memcpy' in the fist 'if'.
>
> Sure, no issue! I'll fix it for all the places in my patch, both in memset
> and in rte_memcpy.
Thanks.
> Anything else or can I upload v2?
No. Go ahead.
Looks good at the first glance.
>
>>
>>>
>>>
>
> Signed-off-by: Asaf Penso
> ---
> lib/netdev-dpdk.c | 28 ++--
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> 4bf0ca9..5216b74 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -4570,10 +4570,10 @@ netdev_dpdk_add_rte_flow_offload(struct
netdev *netdev,
> /* Eth */
> struct rte_flow_item_eth eth_spec;
> struct rte_flow_item_eth eth_mask;
> -memset(_spec, 0, sizeof(eth_spec));
> -memset(_mask, 0, sizeof(eth_mask));
> if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
> !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> +memset(_spec, 0, sizeof(eth_spec));
> +memset(_mask, 0, sizeof(eth_mask));
> rte_memcpy(_spec.dst, >flow.dl_dst,
sizeof(eth_spec.dst));
> rte_memcpy(_spec.src, >flow.dl_src,
sizeof(eth_spec.src));
> eth_spec.type = match->flow.dl_type; @@ -4600,9 +4600,9 @@
> netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
> /* VLAN */
> struct rte_flow_item_vlan vlan_spec;
> struct rte_flow_item_vlan vlan_mask;
> -memset(_spec, 0, sizeof(vlan_spec));
> -memset(_mask, 0, sizeof(vlan_mask));
> if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
> +memset(_spec, 0, sizeof(vlan_spec));
> +memset(_mask, 0, sizeof(vlan_mask));
> vlan_spec.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
> vlan_mask.tci = match->wc.masks.vlans[0].tci &
> ~htons(VLAN_CFI);
>
> @@ -4617,9 +4617,9 @@ netdev_dpdk_add_rte_flow_offload(struct
netdev *netdev,
> uint8_t proto = 0;
> struct rte_flow_item_ipv4 ipv4_spec;
> struct rte_flow_item_ipv4 ipv4_mask;
> -memset(_spec, 0, sizeof(ipv4_spec));
> -memset(_mask, 0, sizeof(ipv4_mask));
> if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
> +memset(_spec, 0, sizeof(ipv4_spec));
> +memset(_mask, 0, sizeof(ipv4_mask));
>
> ipv4_spec.hdr.type_of_service = match->flow.nw_tos;
> ipv4_spec.hdr.time_to_live= match->flow.nw_ttl;
> @@ -4662,9 +4662,9 @@ netdev_dpdk_add_rte_flow_offload(struct
netdev
> *netdev,
>
> struct rte_flow_item_tcp tcp_spec;
> struct rte_flow_item_tcp tcp_mask;
> -memset(_spec, 0, sizeof(tcp_spec));
> -memset(_mask, 0, sizeof(tcp_mask));
> if (proto == IPPROTO_TCP) {
> +memset(_spec, 0, sizeof(tcp_spec));
[ovs-dev] Bases de datos y tablas dinámicas
Cursos escenciales - Webinar Interactivo – Viernes 22 de Febrero Bases de datos y tablas dinámicas en Excel Nuestro curso está diseñado para enseñarte a importar o construir bases de datos correctamente estructuradas para la creación de tablas dinámicas y todas las herramientas que la herramienta nos ofrece para esta función. Identificaremos y aplicaremos las funciones y utilidades del software para tareas propias del área financiera más comunes en el ámbito empresarial. Ejes Temáticos: • Estructura y composición de una base de datos. • Asistente para creación de tablas dinámicas. • Campos, filtros, etiquetas y valores. • Diseño de tabla dinámica. • Herramientas de tabla. Para mayor información, responder sobre este correo con la palabra EXCEL + los siguientes datos: NOMBRE: TELÉFONO: EMPRESA: Llamanos al (045) 55 1554 6630 www.Innovalearn.mx ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] netdev-dpdk: Memset rte_flow_item on a need basis.
Regards,
Asaf Penso
> -Original Message-
> From: Ilya Maximets
> Sent: Monday, February 4, 2019 5:41 PM
> To: Asaf Penso ; ovs-dev@openvswitch.org
> Cc: Roni Bar Yanai ; Stokes, Ian
>
> Subject: Re: [ovs-dev] netdev-dpdk: Memset rte_flow_item on a need basis.
>
> On 04.02.2019 18:08, Asaf Penso wrote:
> >
> >
> > Regards,
> > Asaf Penso
> >
> >> -Original Message-
> >> From: Ilya Maximets
> >> Sent: Monday, February 4, 2019 4:14 PM
> >> To: Asaf Penso ; ovs-dev@openvswitch.org
> >> Cc: Roni Bar Yanai ; Stokes, Ian
> >>
> >> Subject: Re: [ovs-dev] netdev-dpdk: Memset rte_flow_item on a need
> basis.
> >>
> >> On 04.02.2019 15:50, Asaf Penso wrote:
> >>> In netdev_dpdk_add_rte_flow_offload function different
> rte_flow_item
> >>> are created as part of the pattern matching.
> >>>
> >>> For most of them, there is a check whether the wildcards are not zero.
> >>> In case of zero, nothing is being done with the rte_flow_item.
> >>>
> >>> Befor the wildcard check, and regardless of the result, the
> >>> rte_flow_item is being memset.
> >>>
> >>> The patch moves the memset function only if the condition of the
> >>> wildcard is true, thus saving cycles of memset if not needed.
> >>
> >> Structures are actually used only inside their 'if' blocks.
> >> IMHO, it's better to move the definitions inside too.
> >>
> >
> > Inside each 'if' block there is a call to add_flow_pattern that updates the
> pattern's pointers to these structs.
> > Having them defined inside the block will cause their value to be corrupted
> once we exit the block.
> > They are needed all the way until rte_flow_create call.
>
> Oh. Sorry. You're right. I forget about that.
>
> BTW, as you touching this code, maybe you could make it a bit more coding-
> style compliant ? It's about sizeof operator. Coding-style asks to not
> parenthesize the operand. Like this:
>
> memset(_spec, 0, sizeof eth_spec);
>
> It'll be nice if you could fix that.
> You may also fix that for the 'rte_memcpy' in the fist 'if'.
Sure, no issue! I'll fix it for all the places in my patch, both in memset and
in rte_memcpy.
Anything else or can I upload v2?
>
> >
> >
> >>>
> >>> Signed-off-by: Asaf Penso
> >>> ---
> >>> lib/netdev-dpdk.c | 28 ++--
> >>> 1 file changed, 14 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> >>> 4bf0ca9..5216b74 100644
> >>> --- a/lib/netdev-dpdk.c
> >>> +++ b/lib/netdev-dpdk.c
> >>> @@ -4570,10 +4570,10 @@ netdev_dpdk_add_rte_flow_offload(struct
> >> netdev *netdev,
> >>> /* Eth */
> >>> struct rte_flow_item_eth eth_spec;
> >>> struct rte_flow_item_eth eth_mask;
> >>> -memset(_spec, 0, sizeof(eth_spec));
> >>> -memset(_mask, 0, sizeof(eth_mask));
> >>> if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
> >>> !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> >>> +memset(_spec, 0, sizeof(eth_spec));
> >>> +memset(_mask, 0, sizeof(eth_mask));
> >>> rte_memcpy(_spec.dst, >flow.dl_dst,
> >> sizeof(eth_spec.dst));
> >>> rte_memcpy(_spec.src, >flow.dl_src,
> >> sizeof(eth_spec.src));
> >>> eth_spec.type = match->flow.dl_type; @@ -4600,9 +4600,9 @@
> >>> netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
> >>> /* VLAN */
> >>> struct rte_flow_item_vlan vlan_spec;
> >>> struct rte_flow_item_vlan vlan_mask;
> >>> -memset(_spec, 0, sizeof(vlan_spec));
> >>> -memset(_mask, 0, sizeof(vlan_mask));
> >>> if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
> >>> +memset(_spec, 0, sizeof(vlan_spec));
> >>> +memset(_mask, 0, sizeof(vlan_mask));
> >>> vlan_spec.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
> >>> vlan_mask.tci = match->wc.masks.vlans[0].tci &
> >>> ~htons(VLAN_CFI);
> >>>
> >>> @@ -4617,9 +4617,9 @@ netdev_dpdk_add_rte_flow_offload(struct
> >> netdev *netdev,
> >>> uint8_t proto = 0;
> >>> struct rte_flow_item_ipv4 ipv4_spec;
> >>> struct rte_flow_item_ipv4 ipv4_mask;
> >>> -memset(_spec, 0, sizeof(ipv4_spec));
> >>> -memset(_mask, 0, sizeof(ipv4_mask));
> >>> if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
> >>> +memset(_spec, 0, sizeof(ipv4_spec));
> >>> +memset(_mask, 0, sizeof(ipv4_mask));
> >>>
> >>> ipv4_spec.hdr.type_of_service = match->flow.nw_tos;
> >>> ipv4_spec.hdr.time_to_live= match->flow.nw_ttl;
> >>> @@ -4662,9 +4662,9 @@ netdev_dpdk_add_rte_flow_offload(struct
> >> netdev
> >>> *netdev,
> >>>
> >>> struct rte_flow_item_tcp tcp_spec;
> >>> struct rte_flow_item_tcp tcp_mask;
> >>> -memset(_spec, 0, sizeof(tcp_spec));
> >>> -memset(_mask, 0, sizeof(tcp_mask));
> >>> if (proto == IPPROTO_TCP) {
> >>> +memset(_spec, 0, sizeof(tcp_spec));
> >>> +memset(_mask, 0, sizeof(tcp_mask));
> >>> tcp_spec.hdr.src_port = match->flow.tp_src;
> >>>
Re: [ovs-dev] netdev-dpdk: Memset rte_flow_item on a need basis.
On 04.02.2019 18:08, Asaf Penso wrote:
>
>
> Regards,
> Asaf Penso
>
>> -Original Message-
>> From: Ilya Maximets
>> Sent: Monday, February 4, 2019 4:14 PM
>> To: Asaf Penso ; ovs-dev@openvswitch.org
>> Cc: Roni Bar Yanai ; Stokes, Ian
>>
>> Subject: Re: [ovs-dev] netdev-dpdk: Memset rte_flow_item on a need basis.
>>
>> On 04.02.2019 15:50, Asaf Penso wrote:
>>> In netdev_dpdk_add_rte_flow_offload function different rte_flow_item
>>> are created as part of the pattern matching.
>>>
>>> For most of them, there is a check whether the wildcards are not zero.
>>> In case of zero, nothing is being done with the rte_flow_item.
>>>
>>> Befor the wildcard check, and regardless of the result, the
>>> rte_flow_item is being memset.
>>>
>>> The patch moves the memset function only if the condition of the
>>> wildcard is true, thus saving cycles of memset if not needed.
>>
>> Structures are actually used only inside their 'if' blocks.
>> IMHO, it's better to move the definitions inside too.
>>
>
> Inside each 'if' block there is a call to add_flow_pattern that updates the
> pattern's pointers to these structs.
> Having them defined inside the block will cause their value to be corrupted
> once we exit the block.
> They are needed all the way until rte_flow_create call.
Oh. Sorry. You're right. I forget about that.
BTW, as you touching this code, maybe you could make it a bit more
coding-style compliant ? It's about sizeof operator. Coding-style
asks to not parenthesize the operand. Like this:
memset(_spec, 0, sizeof eth_spec);
It'll be nice if you could fix that.
You may also fix that for the 'rte_memcpy' in the fist 'if'.
>
>
>>>
>>> Signed-off-by: Asaf Penso
>>> ---
>>> lib/netdev-dpdk.c | 28 ++--
>>> 1 file changed, 14 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
>>> 4bf0ca9..5216b74 100644
>>> --- a/lib/netdev-dpdk.c
>>> +++ b/lib/netdev-dpdk.c
>>> @@ -4570,10 +4570,10 @@ netdev_dpdk_add_rte_flow_offload(struct
>> netdev *netdev,
>>> /* Eth */
>>> struct rte_flow_item_eth eth_spec;
>>> struct rte_flow_item_eth eth_mask;
>>> -memset(_spec, 0, sizeof(eth_spec));
>>> -memset(_mask, 0, sizeof(eth_mask));
>>> if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
>>> !eth_addr_is_zero(match->wc.masks.dl_dst)) {
>>> +memset(_spec, 0, sizeof(eth_spec));
>>> +memset(_mask, 0, sizeof(eth_mask));
>>> rte_memcpy(_spec.dst, >flow.dl_dst,
>> sizeof(eth_spec.dst));
>>> rte_memcpy(_spec.src, >flow.dl_src,
>> sizeof(eth_spec.src));
>>> eth_spec.type = match->flow.dl_type; @@ -4600,9 +4600,9 @@
>>> netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
>>> /* VLAN */
>>> struct rte_flow_item_vlan vlan_spec;
>>> struct rte_flow_item_vlan vlan_mask;
>>> -memset(_spec, 0, sizeof(vlan_spec));
>>> -memset(_mask, 0, sizeof(vlan_mask));
>>> if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
>>> +memset(_spec, 0, sizeof(vlan_spec));
>>> +memset(_mask, 0, sizeof(vlan_mask));
>>> vlan_spec.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
>>> vlan_mask.tci = match->wc.masks.vlans[0].tci &
>>> ~htons(VLAN_CFI);
>>>
>>> @@ -4617,9 +4617,9 @@ netdev_dpdk_add_rte_flow_offload(struct
>> netdev *netdev,
>>> uint8_t proto = 0;
>>> struct rte_flow_item_ipv4 ipv4_spec;
>>> struct rte_flow_item_ipv4 ipv4_mask;
>>> -memset(_spec, 0, sizeof(ipv4_spec));
>>> -memset(_mask, 0, sizeof(ipv4_mask));
>>> if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
>>> +memset(_spec, 0, sizeof(ipv4_spec));
>>> +memset(_mask, 0, sizeof(ipv4_mask));
>>>
>>> ipv4_spec.hdr.type_of_service = match->flow.nw_tos;
>>> ipv4_spec.hdr.time_to_live= match->flow.nw_ttl;
>>> @@ -4662,9 +4662,9 @@ netdev_dpdk_add_rte_flow_offload(struct
>> netdev
>>> *netdev,
>>>
>>> struct rte_flow_item_tcp tcp_spec;
>>> struct rte_flow_item_tcp tcp_mask;
>>> -memset(_spec, 0, sizeof(tcp_spec));
>>> -memset(_mask, 0, sizeof(tcp_mask));
>>> if (proto == IPPROTO_TCP) {
>>> +memset(_spec, 0, sizeof(tcp_spec));
>>> +memset(_mask, 0, sizeof(tcp_mask));
>>> tcp_spec.hdr.src_port = match->flow.tp_src;
>>> tcp_spec.hdr.dst_port = match->flow.tp_dst;
>>> tcp_spec.hdr.data_off = ntohs(match->flow.tcp_flags) >> 8;
>>> @@ -4687,9 +4687,9 @@ netdev_dpdk_add_rte_flow_offload(struct
>> netdev
>>> *netdev,
>>>
>>> struct rte_flow_item_udp udp_spec;
>>> struct rte_flow_item_udp udp_mask;
>>> -memset(_spec, 0, sizeof(udp_spec));
>>> -memset(_mask, 0, sizeof(udp_mask));
>>> if (proto == IPPROTO_UDP) {
>>> +memset(_spec, 0, sizeof(udp_spec));
>>> +memset(_mask, 0, sizeof(udp_mask));
>>> udp_spec.hdr.src_port = match->flow.tp_src;
>>> udp_spec.hdr.dst_port = match->flow.tp_dst;
Re: [ovs-dev] [PATCH] ovn-controller: Fix chassisredirect port flapping when ovs-vswitchd crashes
Acked-by: Mark Michelson
On 2/4/19 5:31 AM, nusid...@redhat.com wrote:
From: Numan Siddique
On a chassis when ovs-vswitchd crashes for some reason, the BFD status doesn't
get updated in the ovs db. ovn-controller will be reading the old BFD status
even though ovs-vswitchd is crashed. This results in the chassiredirect port
claim flapping between the master chassis and the chasiss with the next higher
priority if ovs-vswitchd crashes in the master chassis.
All the other chassis notices the BFD status down with the master chassis
and hence the next higher priority claims the port. But according to
the master chassis, the BFD status is fine and it again claims back the
chassisredirect port. And this results in flapping. The issue gets resolved
when ovs-vswitchd comes back but until then it leads to lot of SB DB
transactions and high CPU usage in ovn-controller's.
This patch fixes the issue by checking the OF connection status of the
ovn-controller with ovs-vswitchd and calculates the active bfd tunnels
only if it's connected.
Signed-off-by: Numan Siddique
---
ovn/controller/ofctrl.c | 6 ++
ovn/controller/ofctrl.h | 3 +++
ovn/controller/ovn-controller.c | 13 -
tests/ovn.at| 30 +-
4 files changed, 50 insertions(+), 2 deletions(-)
diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 218612787..95b95b607 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -1265,3 +1265,9 @@ ofctrl_inject_pkt(const struct ovsrec_bridge *br_int,
const char *flow_s,
return NULL;
}
+
+bool
+ofctrl_is_connected(void)
+{
+return rconn_is_connected(swconn);
+}
diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
index ae0cfa513..f7521801b 100644
--- a/ovn/controller/ofctrl.h
+++ b/ovn/controller/ofctrl.h
@@ -60,4 +60,7 @@ void ofctrl_check_and_add_flow(struct hmap *desired_flows,
uint8_t table_id,
const struct match *,
const struct ofpbuf *ofpacts,
bool log_duplicate_flow);
+
+bool ofctrl_is_connected(void);
+
#endif /* ovn/ofctrl.h */
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 4e9a5865f..b0f55a870 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -689,7 +689,18 @@ main(int argc, char *argv[])
ovsrec_bridge_table_get(ovs_idl_loop.idl), br_int,
sbrec_chassis_table_get(ovnsb_idl_loop.idl), chassis_id,
sbrec_sb_global_first(ovnsb_idl_loop.idl));
-bfd_calculate_active_tunnels(br_int, _tunnels);
+
+if (ofctrl_is_connected()) {
+/* Calculate the active tunnels only if have an an active
+ * OpenFlow connection to br-int.
+ * If we don't have a connection to br-int, it could mean
+ * ovs-vswitchd is down for some reason and the BFD status
+ * in the Interface rows could be stale. So its better to
+ * consider 'active_tunnels' set to be empty.
+ * */
+bfd_calculate_active_tunnels(br_int, _tunnels);
+}
+
binding_run(ovnsb_idl_txn, ovs_idl_txn, sbrec_chassis_by_name,
sbrec_datapath_binding_by_key,
sbrec_port_binding_by_datapath,
diff --git a/tests/ovn.at b/tests/ovn.at
index f54f24c74..feafe1f00 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -8055,7 +8055,35 @@ ovn-nbctl --timeout=3 --wait=hv \
test_ip_packet gw2 gw1
-OVN_CLEANUP([hv1],[gw1],[gw2],[ext1])
+# Both gw1 and gw2 at this point should have claimed the cr-alice
+# once each.
+gw1_claim_ct=`grep "cr-alice: Claiming" gw1/ovn-controller.log | wc -l`
+gw2_claim_ct=`grep "cr-alice: Claiming" gw2/ovn-controller.log | wc -l`
+
+# Kill ovs-vswitchd in gw2. gw1 should claim the gateway port.
+# So gw1 should claim twice and gw1 only once.
+
+kill `cat gw2/ovs-vswitchd.pid`
+
+# gw1 should claim the cr-alice
+gw1_claim_ct=$((gw1_claim_ct+1))
+
+OVS_WAIT_UNTIL([test $gw1_claim_ct = `cat gw1/ovn-controller.log \
+| grep -c "cr-alice: Claiming"`])
+
+AT_CHECK([test $gw2_claim_ct = `cat gw2/ovn-controller.log | \
+grep -c "cr-alice: Claiming"`])
+
+ovn-sbctl show
+test_ip_packet gw1 gw2
+
+as gw2
+ovs-appctl -t ovn-controller exit
+ovs-appctl -t ovs-vswitchd exit
+ps -aef | grep ovn-c
+
+OVN_CLEANUP([hv1],[gw1],[ext1])
+
AT_CLEANUP
AT_SETUP([ovn -- 4 HV, 3 LS, 2 LR, packet test with HA distributed router gateway port])
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] netdev-dpdk: Memset rte_flow_item on a need basis.
Regards,
Asaf Penso
> -Original Message-
> From: Ilya Maximets
> Sent: Monday, February 4, 2019 4:14 PM
> To: Asaf Penso ; ovs-dev@openvswitch.org
> Cc: Roni Bar Yanai ; Stokes, Ian
>
> Subject: Re: [ovs-dev] netdev-dpdk: Memset rte_flow_item on a need basis.
>
> On 04.02.2019 15:50, Asaf Penso wrote:
> > In netdev_dpdk_add_rte_flow_offload function different rte_flow_item
> > are created as part of the pattern matching.
> >
> > For most of them, there is a check whether the wildcards are not zero.
> > In case of zero, nothing is being done with the rte_flow_item.
> >
> > Befor the wildcard check, and regardless of the result, the
> > rte_flow_item is being memset.
> >
> > The patch moves the memset function only if the condition of the
> > wildcard is true, thus saving cycles of memset if not needed.
>
> Structures are actually used only inside their 'if' blocks.
> IMHO, it's better to move the definitions inside too.
>
Inside each 'if' block there is a call to add_flow_pattern that updates the
pattern's pointers to these structs.
Having them defined inside the block will cause their value to be corrupted
once we exit the block.
They are needed all the way until rte_flow_create call.
> >
> > Signed-off-by: Asaf Penso
> > ---
> > lib/netdev-dpdk.c | 28 ++--
> > 1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> > 4bf0ca9..5216b74 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -4570,10 +4570,10 @@ netdev_dpdk_add_rte_flow_offload(struct
> netdev *netdev,
> > /* Eth */
> > struct rte_flow_item_eth eth_spec;
> > struct rte_flow_item_eth eth_mask;
> > -memset(_spec, 0, sizeof(eth_spec));
> > -memset(_mask, 0, sizeof(eth_mask));
> > if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
> > !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> > +memset(_spec, 0, sizeof(eth_spec));
> > +memset(_mask, 0, sizeof(eth_mask));
> > rte_memcpy(_spec.dst, >flow.dl_dst,
> sizeof(eth_spec.dst));
> > rte_memcpy(_spec.src, >flow.dl_src,
> sizeof(eth_spec.src));
> > eth_spec.type = match->flow.dl_type; @@ -4600,9 +4600,9 @@
> > netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
> > /* VLAN */
> > struct rte_flow_item_vlan vlan_spec;
> > struct rte_flow_item_vlan vlan_mask;
> > -memset(_spec, 0, sizeof(vlan_spec));
> > -memset(_mask, 0, sizeof(vlan_mask));
> > if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
> > +memset(_spec, 0, sizeof(vlan_spec));
> > +memset(_mask, 0, sizeof(vlan_mask));
> > vlan_spec.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
> > vlan_mask.tci = match->wc.masks.vlans[0].tci &
> > ~htons(VLAN_CFI);
> >
> > @@ -4617,9 +4617,9 @@ netdev_dpdk_add_rte_flow_offload(struct
> netdev *netdev,
> > uint8_t proto = 0;
> > struct rte_flow_item_ipv4 ipv4_spec;
> > struct rte_flow_item_ipv4 ipv4_mask;
> > -memset(_spec, 0, sizeof(ipv4_spec));
> > -memset(_mask, 0, sizeof(ipv4_mask));
> > if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
> > +memset(_spec, 0, sizeof(ipv4_spec));
> > +memset(_mask, 0, sizeof(ipv4_mask));
> >
> > ipv4_spec.hdr.type_of_service = match->flow.nw_tos;
> > ipv4_spec.hdr.time_to_live= match->flow.nw_ttl;
> > @@ -4662,9 +4662,9 @@ netdev_dpdk_add_rte_flow_offload(struct
> netdev
> > *netdev,
> >
> > struct rte_flow_item_tcp tcp_spec;
> > struct rte_flow_item_tcp tcp_mask;
> > -memset(_spec, 0, sizeof(tcp_spec));
> > -memset(_mask, 0, sizeof(tcp_mask));
> > if (proto == IPPROTO_TCP) {
> > +memset(_spec, 0, sizeof(tcp_spec));
> > +memset(_mask, 0, sizeof(tcp_mask));
> > tcp_spec.hdr.src_port = match->flow.tp_src;
> > tcp_spec.hdr.dst_port = match->flow.tp_dst;
> > tcp_spec.hdr.data_off = ntohs(match->flow.tcp_flags) >> 8;
> > @@ -4687,9 +4687,9 @@ netdev_dpdk_add_rte_flow_offload(struct
> netdev
> > *netdev,
> >
> > struct rte_flow_item_udp udp_spec;
> > struct rte_flow_item_udp udp_mask;
> > -memset(_spec, 0, sizeof(udp_spec));
> > -memset(_mask, 0, sizeof(udp_mask));
> > if (proto == IPPROTO_UDP) {
> > +memset(_spec, 0, sizeof(udp_spec));
> > +memset(_mask, 0, sizeof(udp_mask));
> > udp_spec.hdr.src_port = match->flow.tp_src;
> > udp_spec.hdr.dst_port = match->flow.tp_dst;
> >
> > @@ -4708,9 +4708,9 @@ netdev_dpdk_add_rte_flow_offload(struct
> netdev
> > *netdev,
> >
> > struct rte_flow_item_sctp sctp_spec;
> > struct rte_flow_item_sctp sctp_mask;
> > -memset(_spec, 0, sizeof(sctp_spec));
> > -memset(_mask, 0, sizeof(sctp_mask));
> > if (proto == IPPROTO_SCTP) {
> > +memset(_spec, 0, sizeof(sctp_spec));
> > +memset(_mask, 0, sizeof(sctp_mask));
> >
Re: [ovs-dev] netdev-dpdk: Memset rte_flow_item on a need basis.
On 04.02.2019 15:50, Asaf Penso wrote:
> In netdev_dpdk_add_rte_flow_offload function different rte_flow_item are
> created as part of the pattern matching.
>
> For most of them, there is a check whether the wildcards are not zero.
> In case of zero, nothing is being done with the rte_flow_item.
>
> Befor the wildcard check, and regardless of the result, the
> rte_flow_item is being memset.
>
> The patch moves the memset function only if the condition of the
> wildcard is true, thus saving cycles of memset if not needed.
Structures are actually used only inside their 'if' blocks.
IMHO, it's better to move the definitions inside too.
>
> Signed-off-by: Asaf Penso
> ---
> lib/netdev-dpdk.c | 28 ++--
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 4bf0ca9..5216b74 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -4570,10 +4570,10 @@ netdev_dpdk_add_rte_flow_offload(struct netdev
> *netdev,
> /* Eth */
> struct rte_flow_item_eth eth_spec;
> struct rte_flow_item_eth eth_mask;
> -memset(_spec, 0, sizeof(eth_spec));
> -memset(_mask, 0, sizeof(eth_mask));
> if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
> !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> +memset(_spec, 0, sizeof(eth_spec));
> +memset(_mask, 0, sizeof(eth_mask));
> rte_memcpy(_spec.dst, >flow.dl_dst, sizeof(eth_spec.dst));
> rte_memcpy(_spec.src, >flow.dl_src, sizeof(eth_spec.src));
> eth_spec.type = match->flow.dl_type;
> @@ -4600,9 +4600,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
> /* VLAN */
> struct rte_flow_item_vlan vlan_spec;
> struct rte_flow_item_vlan vlan_mask;
> -memset(_spec, 0, sizeof(vlan_spec));
> -memset(_mask, 0, sizeof(vlan_mask));
> if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
> +memset(_spec, 0, sizeof(vlan_spec));
> +memset(_mask, 0, sizeof(vlan_mask));
> vlan_spec.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
> vlan_mask.tci = match->wc.masks.vlans[0].tci & ~htons(VLAN_CFI);
>
> @@ -4617,9 +4617,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
> uint8_t proto = 0;
> struct rte_flow_item_ipv4 ipv4_spec;
> struct rte_flow_item_ipv4 ipv4_mask;
> -memset(_spec, 0, sizeof(ipv4_spec));
> -memset(_mask, 0, sizeof(ipv4_mask));
> if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
> +memset(_spec, 0, sizeof(ipv4_spec));
> +memset(_mask, 0, sizeof(ipv4_mask));
>
> ipv4_spec.hdr.type_of_service = match->flow.nw_tos;
> ipv4_spec.hdr.time_to_live= match->flow.nw_ttl;
> @@ -4662,9 +4662,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
>
> struct rte_flow_item_tcp tcp_spec;
> struct rte_flow_item_tcp tcp_mask;
> -memset(_spec, 0, sizeof(tcp_spec));
> -memset(_mask, 0, sizeof(tcp_mask));
> if (proto == IPPROTO_TCP) {
> +memset(_spec, 0, sizeof(tcp_spec));
> +memset(_mask, 0, sizeof(tcp_mask));
> tcp_spec.hdr.src_port = match->flow.tp_src;
> tcp_spec.hdr.dst_port = match->flow.tp_dst;
> tcp_spec.hdr.data_off = ntohs(match->flow.tcp_flags) >> 8;
> @@ -4687,9 +4687,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
>
> struct rte_flow_item_udp udp_spec;
> struct rte_flow_item_udp udp_mask;
> -memset(_spec, 0, sizeof(udp_spec));
> -memset(_mask, 0, sizeof(udp_mask));
> if (proto == IPPROTO_UDP) {
> +memset(_spec, 0, sizeof(udp_spec));
> +memset(_mask, 0, sizeof(udp_mask));
> udp_spec.hdr.src_port = match->flow.tp_src;
> udp_spec.hdr.dst_port = match->flow.tp_dst;
>
> @@ -4708,9 +4708,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
>
> struct rte_flow_item_sctp sctp_spec;
> struct rte_flow_item_sctp sctp_mask;
> -memset(_spec, 0, sizeof(sctp_spec));
> -memset(_mask, 0, sizeof(sctp_mask));
> if (proto == IPPROTO_SCTP) {
> +memset(_spec, 0, sizeof(sctp_spec));
> +memset(_mask, 0, sizeof(sctp_mask));
> sctp_spec.hdr.src_port = match->flow.tp_src;
> sctp_spec.hdr.dst_port = match->flow.tp_dst;
>
> @@ -4729,9 +4729,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
>
> struct rte_flow_item_icmp icmp_spec;
> struct rte_flow_item_icmp icmp_mask;
> -memset(_spec, 0, sizeof(icmp_spec));
> -memset(_mask, 0, sizeof(icmp_mask));
> if (proto == IPPROTO_ICMP) {
> +memset(_spec, 0, sizeof(icmp_spec));
> +memset(_mask, 0, sizeof(icmp_mask));
> icmp_spec.hdr.icmp_type = (uint8_t)ntohs(match->flow.tp_src);
> icmp_spec.hdr.icmp_code = (uint8_t)ntohs(match->flow.tp_dst);
>
>
___
dev mailing list
d...@openvswitch.org
Re: [ovs-dev] [ovs-dev, v2] ovn-controller: Fix chassisredirect port flapping when ovs-vswitchd crashes
On Mon, Feb 4, 2019 at 6:52 PM Ilya Maximets wrote:
> On 04.02.2019 13:39, nusid...@redhat.com wrote:
> > From: Numan Siddique
> >
> > On a chassis when ovs-vswitchd crashes for some reason, the BFD status
> doesn't
> > get updated in the ovs db. ovn-controller will be reading the old BFD
> status
> > even though ovs-vswitchd is crashed. This results in the chassiredirect
> port
> > claim flapping between the master chassis and the chasiss with the next
> higher
> > priority if ovs-vswitchd crashes in the master chassis.
> >
> > All the other chassis notices the BFD status down with the master chassis
> > and hence the next higher priority claims the port. But according to
> > the master chassis, the BFD status is fine and it again claims back the
> > chassisredirect port. And this results in flapping. The issue gets
> resolved
> > when ovs-vswitchd comes back but until then it leads to lot of SB DB
> > transactions and high CPU usage in ovn-controller's.
> >
> > This patch fixes the issue by checking the OF connection status of the
> > ovn-controller with ovs-vswitchd and calculates the active bfd tunnels
> > only if it's connected.
> >
> > Signed-off-by: Numan Siddique
> > ---
> >
> > v1 -> v2
> > -
> > * Deleted unnecessary prints in the test case which were added during
> >debugging.
> >
> > ovn/controller/ofctrl.c | 6 ++
> > ovn/controller/ofctrl.h | 3 +++
> > ovn/controller/ovn-controller.c | 13 -
> > tests/ovn.at| 26 +-
> > 4 files changed, 46 insertions(+), 2 deletions(-)
> >
> > diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> > index 218612787..95b95b607 100644
> > --- a/ovn/controller/ofctrl.c
> > +++ b/ovn/controller/ofctrl.c
> > @@ -1265,3 +1265,9 @@ ofctrl_inject_pkt(const struct ovsrec_bridge
> *br_int, const char *flow_s,
> >
> > return NULL;
> > }
> > +
> > +bool
> > +ofctrl_is_connected(void)
> > +{
> > +return rconn_is_connected(swconn);
> > +}
> > diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
> > index ae0cfa513..f7521801b 100644
> > --- a/ovn/controller/ofctrl.h
> > +++ b/ovn/controller/ofctrl.h
> > @@ -60,4 +60,7 @@ void ofctrl_check_and_add_flow(struct hmap
> *desired_flows, uint8_t table_id,
> > const struct match *,
> > const struct ofpbuf *ofpacts,
> > bool log_duplicate_flow);
> > +
> > +bool ofctrl_is_connected(void);
> > +
> > #endif /* ovn/ofctrl.h */
> > diff --git a/ovn/controller/ovn-controller.c
> b/ovn/controller/ovn-controller.c
> > index 4e9a5865f..2098f280c 100644
> > --- a/ovn/controller/ovn-controller.c
> > +++ b/ovn/controller/ovn-controller.c
> > @@ -689,7 +689,18 @@ main(int argc, char *argv[])
> > ovsrec_bridge_table_get(ovs_idl_loop.idl), br_int,
> > sbrec_chassis_table_get(ovnsb_idl_loop.idl),
> chassis_id,
> > sbrec_sb_global_first(ovnsb_idl_loop.idl));
> > -bfd_calculate_active_tunnels(br_int, _tunnels);
> > +
> > +if (ofctrl_is_connected()) {
> > +/* Calculate the active tunnels only if have an an
> active
> > + * OpenFlow connection to br-int.
> > + * If we don't have a connection to br-int, it
> could mean
> > + * ovs-vswitchd is down for some reason and the BFD
> status
> > + * in the Interface rows could be stale. So its
> better to
> > + * consider 'active_tunnels' set to be empty if
> it's not
> > + * connected. */
> > +bfd_calculate_active_tunnels(br_int,
> _tunnels);
> > +}
> > +
> > binding_run(ovnsb_idl_txn, ovs_idl_txn,
> sbrec_chassis_by_name,
> > sbrec_datapath_binding_by_key,
> > sbrec_port_binding_by_datapath,
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index f54f24c74..fd558cb98 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -8055,7 +8055,31 @@ ovn-nbctl --timeout=3 --wait=hv \
> >
> > test_ip_packet gw2 gw1
> >
> > -OVN_CLEANUP([hv1],[gw1],[gw2],[ext1])
> > +# Get the claim count of both gw1 and gw2.
> > +gw1_claim_ct=`grep "cr-alice: Claiming" gw1/ovn-controller.log | wc -l`
> > +gw2_claim_ct=`grep "cr-alice: Claiming" gw2/ovn-controller.log | wc -l`
> > +
> > +# Kill ovs-vswitchd in gw2. gw1 should claim the gateway port.
> > +kill `cat gw2/ovs-vswitchd.pid`
>
> Is it necessary to hard kill the daemon ?
> Test works fine with OVS_APP_EXIT_AND_WAIT([ovs-vswitchd]) too.
>
I tried this but it didn't work for me. I will test again and update it.
> Anyway, simple 'kill' is not a hard kill. It's trapped by OVS and
> OVS executes some at_exit handlers. For example, pid file is removed,
> so, the below 'appctl exit' command always fails:
>
>
Re: [ovs-dev] [PATCH] netdev-dpdk: Flow validation refactoring.
Any thoughts on this?
Best regards, Ilya Maximets.
On 12.11.2018 12:28, Ilya Maximets wrote:
> * Dropped 'is_all_zero' function, which is equal to 'is_all_zeros'
> from util.h .
> * util.h added to includes. Includes re-sorted within their blocks.
> (it's hard to figure out where to put new one if there is no order.)
> Note: linux/if.h depends on sys/socket.h .
> * 'ovs_u128_is_zero' used instead of manual checking of fields.
> * Code simplified by using direct pointer to 'match->wc.masks'.
> * 'sizeof's rewritten to be coding-style complient.
>
> Signed-off-by: Ilya Maximets
> ---
> lib/netdev-dpdk.c | 83 +--
> 1 file changed, 30 insertions(+), 53 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 7e0a593d0..23f61c690 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -17,10 +17,10 @@
> #include
> #include "netdev-dpdk.h"
>
> -#include
> +#include
> #include
> #include
> -#include
> +#include
> #include
> #include
> #include
> @@ -32,13 +32,13 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> #include
> -#include
> #include
> -#include
> +#include
>
> #include "cmap.h"
> #include "dirs.h"
> @@ -51,20 +51,21 @@
> #include "odp-util.h"
> #include "openvswitch/dynamic-string.h"
> #include "openvswitch/list.h"
> +#include "openvswitch/match.h"
> #include "openvswitch/ofp-print.h"
> +#include "openvswitch/shash.h"
> #include "openvswitch/vlog.h"
> -#include "openvswitch/match.h"
> #include "ovs-numa.h"
> -#include "ovs-thread.h"
> #include "ovs-rcu.h"
> +#include "ovs-thread.h"
> #include "packets.h"
> -#include "openvswitch/shash.h"
> #include "smap.h"
> #include "sset.h"
> -#include "unaligned.h"
> #include "timeval.h"
> -#include "uuid.h"
> +#include "unaligned.h"
> #include "unixctl.h"
> +#include "util.h"
> +#include "uuid.h"
>
> enum {VIRTIO_RXQ, VIRTIO_TXQ, VIRTIO_QNUM};
>
> @@ -4635,39 +4636,24 @@ out:
> return ret;
> }
>
> -static bool
> -is_all_zero(const void *addr, size_t n) {
> -size_t i = 0;
> -const uint8_t *p = (uint8_t *)addr;
> -
> -for (i = 0; i < n; i++) {
> -if (p[i] != 0) {
> -return false;
> -}
> -}
> -
> -return true;
> -}
> -
> /*
> * Check if any unsupported flow patterns are specified.
> */
> static int
> netdev_dpdk_validate_flow(const struct match *match) {
> struct match match_zero_wc;
> +const struct flow *masks = >wc.masks;
>
> /* Create a wc-zeroed version of flow */
> match_init(_zero_wc, >flow, >wc);
>
> -if (!is_all_zero(_zero_wc.flow.tunnel,
> - sizeof(match_zero_wc.flow.tunnel))) {
> +if (!is_all_zeros(_zero_wc.flow.tunnel,
> + sizeof match_zero_wc.flow.tunnel)) {
> goto err;
> }
>
> -if (match->wc.masks.metadata ||
> -match->wc.masks.skb_priority ||
> -match->wc.masks.pkt_mark ||
> -match->wc.masks.dp_hash) {
> +if (masks->metadata || masks->skb_priority ||
> +masks->pkt_mark || masks->dp_hash) {
> goto err;
> }
>
> @@ -4676,38 +4662,31 @@ netdev_dpdk_validate_flow(const struct match *match) {
> goto err;
> }
>
> -if (match->wc.masks.ct_state ||
> -match->wc.masks.ct_nw_proto ||
> -match->wc.masks.ct_zone ||
> -match->wc.masks.ct_mark ||
> -match->wc.masks.ct_label.u64.hi ||
> -match->wc.masks.ct_label.u64.lo) {
> +if (masks->ct_state || masks->ct_nw_proto ||
> +masks->ct_zone || masks->ct_mark ||
> +!ovs_u128_is_zero(masks->ct_label)) {
> goto err;
> }
>
> -if (match->wc.masks.conj_id ||
> -match->wc.masks.actset_output) {
> +if (masks->conj_id || masks->actset_output) {
> goto err;
> }
>
> /* unsupported L2 */
> -if (!is_all_zero(>wc.masks.mpls_lse,
> - sizeof(match_zero_wc.flow.mpls_lse))) {
> +if (!is_all_zeros(masks->mpls_lse, sizeof masks->mpls_lse)) {
> goto err;
> }
>
> /* unsupported L3 */
> -if (match->wc.masks.ipv6_label ||
> -match->wc.masks.ct_nw_src ||
> -match->wc.masks.ct_nw_dst ||
> -!is_all_zero(>wc.masks.ipv6_src, sizeof(struct in6_addr)) ||
> -!is_all_zero(>wc.masks.ipv6_dst, sizeof(struct in6_addr)) ||
> -!is_all_zero(>wc.masks.ct_ipv6_src, sizeof(struct in6_addr))
> ||
> -!is_all_zero(>wc.masks.ct_ipv6_dst, sizeof(struct in6_addr))
> ||
> -!is_all_zero(>wc.masks.nd_target, sizeof(struct in6_addr)) ||
> -!is_all_zero(>wc.masks.nsh, sizeof(struct ovs_key_nsh)) ||
> -!is_all_zero(>wc.masks.arp_sha, sizeof(struct eth_addr)) ||
> -!is_all_zero(>wc.masks.arp_tha, sizeof(struct eth_addr))) {
> +if (masks->ipv6_label || masks->ct_nw_src || masks->ct_nw_dst ||
> +!is_all_zeros(>ipv6_src,
Re: [ovs-dev] [PATCH v2] lib/tc: Support optional tunnel id
On Mon, Feb 4, 2019 at 11:00 AM Simon Horman wrote:
>
>
>
> On Sun, 3 Feb 2019 at 12:03, Roi Dayan wrote:
>>
>> On Fri, Feb 1, 2019 at 4:05 PM Simon Horman
>> wrote:
>> >
>> > Thanks Roi,
>> >
>> > On Thu, 31 Jan 2019 at 15:52, Roi Dayan wrote:
>> >
>> > >
>> > >
>> > > On 31/01/2019 15:32, Roi Dayan wrote:
>> > > >
>> > > > On 31/01/2019 11:58, Simon Horman wrote:
>> > > >> On Mon, Jan 21, 2019 at 05:32:37PM +0200, Adi Nissim wrote:
>> > > >>> Currently the TC tunnel_key action is always
>> > > >>> initialized with the given tunnel id value. However,
>> > > >>> some tunneling protocols define the tunnel id as an optional field.
>> > > >>>
>> > > >>> This patch initializes the id field of tunnel_key:set and
>> > > tunnel_key:unset
>> > > >>> only if a value is provided.
>> > > >>>
>> > > >>> In the case that a tunnel key value is not provided by the user
>> > > >>> the key flag will not be set.
>> > > >>>
>> > > >>> Signed-off-by: Adi Nissim
>> > > >>> Acked-by: Paul Blakey
>> > > >>> ---
>> > > >>> v1->v2: check if mask.tunnel.id == OVS_BE64_MAX
>> > > >>> so we won't do match in the case of a partial mask.
>> > > >> I am still a bit concerned about the partial mask case.
>> > > >> It looks like the code will now silently not offload such matches.
>> > > >>
>> > > >> I think that a partial mask should either be offloaded or
>> > > >> offload of the entire flow should be rejected.
>> > > > thanks. you are right. I didn't notice it. partial masks should be
>> > > rejected
>> > > > to fallback to ovs dp instead of ignoring the mask.
>> > > >
>> > >
>> > >
>> > > Hi Simon,
>> > >
>> > > I did some checks and think the correct fix is to offload exact match.
>> > > if key is partial we can ignore the mask and offload exact match and
>> > > it will be correct as we do more strict matching.
>> > >
>> > > it is also documented that the kernel datapath is doing the same
>> > > (from datapath.rst)
>> > >
>> > > "The kernel can ignore the mask attribute, installing an exact
>> > > match flow"
>> > >
>> > > So I think the first patch V0 is actually correct as we
>> > > check the tunnel key flag exists and offload exact match if
>> > > there was any mask or offload without a key if the mask is 0
>> > > or no key.
>> > >
>> > > in netdev-tc-offloads.c
>> > >
>> > > +flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ?
>> > > + tnl_mask->tun_id : 0;
>> > >
>> >
>> > I think this is fine so long as tun_id is all-ones. Is that always the
>> > case?
>> > Should the code check that it is the case? Am I missing the point?
>> >
>>
>> it looks like tun_id mask is always set to all-ones.
>> but even if it won't be in the future, we shouldn't really care here.
>> tc adds exact match on the tun_id and ignores the tun_id mask.
>> this is considered ok as the matching is more strict.
>> if new match is needed with different tun_id then ovs will try to add
>> another rule for it.
>> so with tc we could have multiple rules vs 1 rule that support mask.
>
>
> Thanks for looking into this. That sounds find to me but I wonder if we
> should make
> this behaviour explicit.
>
> /*
> * Comment describing why the mask is 0 or all-ones
> */
> flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? UINT32_MAX :
> 0;
>
I think its nicer like this and symetric currently. here it's generic
and "use" mask.
in tc.c when we fill the netlink msg we ignore the mas and also when
parsing tc dump,
tun mask is set to here (tc.c) and not netdev-tc-offloads.c
>> >
>> > >
>> > >
>> > > in tc.c
>> > >
>> > > -nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);
>> > > +if (id_mask) {
>> > > +nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);
>> > > +}
>> > >
>> > >
>> > > let me know what you think.
>> > >
>> > > Thanks,
>> > > Roi
>> > >
>> > ___
>> > 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
Re: [ovs-dev] [ovs-dev, v2] ovn-controller: Fix chassisredirect port flapping when ovs-vswitchd crashes
On 04.02.2019 13:39, nusid...@redhat.com wrote:
> From: Numan Siddique
>
> On a chassis when ovs-vswitchd crashes for some reason, the BFD status doesn't
> get updated in the ovs db. ovn-controller will be reading the old BFD status
> even though ovs-vswitchd is crashed. This results in the chassiredirect port
> claim flapping between the master chassis and the chasiss with the next higher
> priority if ovs-vswitchd crashes in the master chassis.
>
> All the other chassis notices the BFD status down with the master chassis
> and hence the next higher priority claims the port. But according to
> the master chassis, the BFD status is fine and it again claims back the
> chassisredirect port. And this results in flapping. The issue gets resolved
> when ovs-vswitchd comes back but until then it leads to lot of SB DB
> transactions and high CPU usage in ovn-controller's.
>
> This patch fixes the issue by checking the OF connection status of the
> ovn-controller with ovs-vswitchd and calculates the active bfd tunnels
> only if it's connected.
>
> Signed-off-by: Numan Siddique
> ---
>
> v1 -> v2
> -
> * Deleted unnecessary prints in the test case which were added during
>debugging.
>
> ovn/controller/ofctrl.c | 6 ++
> ovn/controller/ofctrl.h | 3 +++
> ovn/controller/ovn-controller.c | 13 -
> tests/ovn.at| 26 +-
> 4 files changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> index 218612787..95b95b607 100644
> --- a/ovn/controller/ofctrl.c
> +++ b/ovn/controller/ofctrl.c
> @@ -1265,3 +1265,9 @@ ofctrl_inject_pkt(const struct ovsrec_bridge *br_int,
> const char *flow_s,
>
> return NULL;
> }
> +
> +bool
> +ofctrl_is_connected(void)
> +{
> +return rconn_is_connected(swconn);
> +}
> diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
> index ae0cfa513..f7521801b 100644
> --- a/ovn/controller/ofctrl.h
> +++ b/ovn/controller/ofctrl.h
> @@ -60,4 +60,7 @@ void ofctrl_check_and_add_flow(struct hmap *desired_flows,
> uint8_t table_id,
> const struct match *,
> const struct ofpbuf *ofpacts,
> bool log_duplicate_flow);
> +
> +bool ofctrl_is_connected(void);
> +
> #endif /* ovn/ofctrl.h */
> diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
> index 4e9a5865f..2098f280c 100644
> --- a/ovn/controller/ovn-controller.c
> +++ b/ovn/controller/ovn-controller.c
> @@ -689,7 +689,18 @@ main(int argc, char *argv[])
> ovsrec_bridge_table_get(ovs_idl_loop.idl), br_int,
> sbrec_chassis_table_get(ovnsb_idl_loop.idl), chassis_id,
> sbrec_sb_global_first(ovnsb_idl_loop.idl));
> -bfd_calculate_active_tunnels(br_int, _tunnels);
> +
> +if (ofctrl_is_connected()) {
> +/* Calculate the active tunnels only if have an an active
> + * OpenFlow connection to br-int.
> + * If we don't have a connection to br-int, it could mean
> + * ovs-vswitchd is down for some reason and the BFD
> status
> + * in the Interface rows could be stale. So its better to
> + * consider 'active_tunnels' set to be empty if it's not
> + * connected. */
> +bfd_calculate_active_tunnels(br_int, _tunnels);
> +}
> +
> binding_run(ovnsb_idl_txn, ovs_idl_txn,
> sbrec_chassis_by_name,
> sbrec_datapath_binding_by_key,
> sbrec_port_binding_by_datapath,
> diff --git a/tests/ovn.at b/tests/ovn.at
> index f54f24c74..fd558cb98 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -8055,7 +8055,31 @@ ovn-nbctl --timeout=3 --wait=hv \
>
> test_ip_packet gw2 gw1
>
> -OVN_CLEANUP([hv1],[gw1],[gw2],[ext1])
> +# Get the claim count of both gw1 and gw2.
> +gw1_claim_ct=`grep "cr-alice: Claiming" gw1/ovn-controller.log | wc -l`
> +gw2_claim_ct=`grep "cr-alice: Claiming" gw2/ovn-controller.log | wc -l`
> +
> +# Kill ovs-vswitchd in gw2. gw1 should claim the gateway port.
> +kill `cat gw2/ovs-vswitchd.pid`
Is it necessary to hard kill the daemon ?
Test works fine with OVS_APP_EXIT_AND_WAIT([ovs-vswitchd]) too.
Anyway, simple 'kill' is not a hard kill. It's trapped by OVS and
OVS executes some at_exit handlers. For example, pid file is removed,
so, the below 'appctl exit' command always fails:
2019-02-04T13:06:58Z|1|daemon_unix|WARN|/tests/testsuite.dir/2740/gw2/ovs-vswitchd.pid:
open: No such file or directory
ovs-appctl: cannot read pidfile
"/tests/testsuite.dir/2740/gw2/ovs-vswitchd.pid" (No such file or directory)
> +
> +# gw1 should claim the cr-alice and the claim count of gw1 should be
> +# incremented by 1.
>
[ovs-dev] [PATCH] netdev-dpdk: Memset rte_flow_item on a need basis.
In netdev_dpdk_add_rte_flow_offload function different rte_flow_item are
created as part of the pattern matching.
For most of them, there is a check whether the wildcards are not zero.
In case of zero, nothing is being done with the rte_flow_item.
Befor the wildcard check, and regardless of the result, the
rte_flow_item is being memset.
The patch moves the memset function only if the condition of the
wildcard is true, thus saving cycles of memset if not needed.
Signed-off-by: Asaf Penso
---
lib/netdev-dpdk.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 4bf0ca9..5216b74 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -4570,10 +4570,10 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
/* Eth */
struct rte_flow_item_eth eth_spec;
struct rte_flow_item_eth eth_mask;
-memset(_spec, 0, sizeof(eth_spec));
-memset(_mask, 0, sizeof(eth_mask));
if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
!eth_addr_is_zero(match->wc.masks.dl_dst)) {
+memset(_spec, 0, sizeof(eth_spec));
+memset(_mask, 0, sizeof(eth_mask));
rte_memcpy(_spec.dst, >flow.dl_dst, sizeof(eth_spec.dst));
rte_memcpy(_spec.src, >flow.dl_src, sizeof(eth_spec.src));
eth_spec.type = match->flow.dl_type;
@@ -4600,9 +4600,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
/* VLAN */
struct rte_flow_item_vlan vlan_spec;
struct rte_flow_item_vlan vlan_mask;
-memset(_spec, 0, sizeof(vlan_spec));
-memset(_mask, 0, sizeof(vlan_mask));
if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
+memset(_spec, 0, sizeof(vlan_spec));
+memset(_mask, 0, sizeof(vlan_mask));
vlan_spec.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
vlan_mask.tci = match->wc.masks.vlans[0].tci & ~htons(VLAN_CFI);
@@ -4617,9 +4617,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
uint8_t proto = 0;
struct rte_flow_item_ipv4 ipv4_spec;
struct rte_flow_item_ipv4 ipv4_mask;
-memset(_spec, 0, sizeof(ipv4_spec));
-memset(_mask, 0, sizeof(ipv4_mask));
if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
+memset(_spec, 0, sizeof(ipv4_spec));
+memset(_mask, 0, sizeof(ipv4_mask));
ipv4_spec.hdr.type_of_service = match->flow.nw_tos;
ipv4_spec.hdr.time_to_live= match->flow.nw_ttl;
@@ -4662,9 +4662,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
struct rte_flow_item_tcp tcp_spec;
struct rte_flow_item_tcp tcp_mask;
-memset(_spec, 0, sizeof(tcp_spec));
-memset(_mask, 0, sizeof(tcp_mask));
if (proto == IPPROTO_TCP) {
+memset(_spec, 0, sizeof(tcp_spec));
+memset(_mask, 0, sizeof(tcp_mask));
tcp_spec.hdr.src_port = match->flow.tp_src;
tcp_spec.hdr.dst_port = match->flow.tp_dst;
tcp_spec.hdr.data_off = ntohs(match->flow.tcp_flags) >> 8;
@@ -4687,9 +4687,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
struct rte_flow_item_udp udp_spec;
struct rte_flow_item_udp udp_mask;
-memset(_spec, 0, sizeof(udp_spec));
-memset(_mask, 0, sizeof(udp_mask));
if (proto == IPPROTO_UDP) {
+memset(_spec, 0, sizeof(udp_spec));
+memset(_mask, 0, sizeof(udp_mask));
udp_spec.hdr.src_port = match->flow.tp_src;
udp_spec.hdr.dst_port = match->flow.tp_dst;
@@ -4708,9 +4708,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
struct rte_flow_item_sctp sctp_spec;
struct rte_flow_item_sctp sctp_mask;
-memset(_spec, 0, sizeof(sctp_spec));
-memset(_mask, 0, sizeof(sctp_mask));
if (proto == IPPROTO_SCTP) {
+memset(_spec, 0, sizeof(sctp_spec));
+memset(_mask, 0, sizeof(sctp_mask));
sctp_spec.hdr.src_port = match->flow.tp_src;
sctp_spec.hdr.dst_port = match->flow.tp_dst;
@@ -4729,9 +4729,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
struct rte_flow_item_icmp icmp_spec;
struct rte_flow_item_icmp icmp_mask;
-memset(_spec, 0, sizeof(icmp_spec));
-memset(_mask, 0, sizeof(icmp_mask));
if (proto == IPPROTO_ICMP) {
+memset(_spec, 0, sizeof(icmp_spec));
+memset(_mask, 0, sizeof(icmp_mask));
icmp_spec.hdr.icmp_type = (uint8_t)ntohs(match->flow.tp_src);
icmp_spec.hdr.icmp_code = (uint8_t)ntohs(match->flow.tp_dst);
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] ovn-controller: Fix chassisredirect port flapping when ovs-vswitchd crashes
From: Numan Siddique
On a chassis when ovs-vswitchd crashes for some reason, the BFD status doesn't
get updated in the ovs db. ovn-controller will be reading the old BFD status
even though ovs-vswitchd is crashed. This results in the chassiredirect port
claim flapping between the master chassis and the chasiss with the next higher
priority if ovs-vswitchd crashes in the master chassis.
All the other chassis notices the BFD status down with the master chassis
and hence the next higher priority claims the port. But according to
the master chassis, the BFD status is fine and it again claims back the
chassisredirect port. And this results in flapping. The issue gets resolved
when ovs-vswitchd comes back but until then it leads to lot of SB DB
transactions and high CPU usage in ovn-controller's.
This patch fixes the issue by checking the OF connection status of the
ovn-controller with ovs-vswitchd and calculates the active bfd tunnels
only if it's connected.
Signed-off-by: Numan Siddique
---
v1 -> v2
-
* Deleted unnecessary prints in the test case which were added during
debugging.
ovn/controller/ofctrl.c | 6 ++
ovn/controller/ofctrl.h | 3 +++
ovn/controller/ovn-controller.c | 13 -
tests/ovn.at| 26 +-
4 files changed, 46 insertions(+), 2 deletions(-)
diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 218612787..95b95b607 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -1265,3 +1265,9 @@ ofctrl_inject_pkt(const struct ovsrec_bridge *br_int,
const char *flow_s,
return NULL;
}
+
+bool
+ofctrl_is_connected(void)
+{
+return rconn_is_connected(swconn);
+}
diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
index ae0cfa513..f7521801b 100644
--- a/ovn/controller/ofctrl.h
+++ b/ovn/controller/ofctrl.h
@@ -60,4 +60,7 @@ void ofctrl_check_and_add_flow(struct hmap *desired_flows,
uint8_t table_id,
const struct match *,
const struct ofpbuf *ofpacts,
bool log_duplicate_flow);
+
+bool ofctrl_is_connected(void);
+
#endif /* ovn/ofctrl.h */
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 4e9a5865f..2098f280c 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -689,7 +689,18 @@ main(int argc, char *argv[])
ovsrec_bridge_table_get(ovs_idl_loop.idl), br_int,
sbrec_chassis_table_get(ovnsb_idl_loop.idl), chassis_id,
sbrec_sb_global_first(ovnsb_idl_loop.idl));
-bfd_calculate_active_tunnels(br_int, _tunnels);
+
+if (ofctrl_is_connected()) {
+/* Calculate the active tunnels only if have an an active
+ * OpenFlow connection to br-int.
+ * If we don't have a connection to br-int, it could mean
+ * ovs-vswitchd is down for some reason and the BFD status
+ * in the Interface rows could be stale. So its better to
+ * consider 'active_tunnels' set to be empty if it's not
+ * connected. */
+bfd_calculate_active_tunnels(br_int, _tunnels);
+}
+
binding_run(ovnsb_idl_txn, ovs_idl_txn, sbrec_chassis_by_name,
sbrec_datapath_binding_by_key,
sbrec_port_binding_by_datapath,
diff --git a/tests/ovn.at b/tests/ovn.at
index f54f24c74..fd558cb98 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -8055,7 +8055,31 @@ ovn-nbctl --timeout=3 --wait=hv \
test_ip_packet gw2 gw1
-OVN_CLEANUP([hv1],[gw1],[gw2],[ext1])
+# Get the claim count of both gw1 and gw2.
+gw1_claim_ct=`grep "cr-alice: Claiming" gw1/ovn-controller.log | wc -l`
+gw2_claim_ct=`grep "cr-alice: Claiming" gw2/ovn-controller.log | wc -l`
+
+# Kill ovs-vswitchd in gw2. gw1 should claim the gateway port.
+kill `cat gw2/ovs-vswitchd.pid`
+
+# gw1 should claim the cr-alice and the claim count of gw1 should be
+# incremented by 1.
+gw1_claim_ct=$((gw1_claim_ct+1))
+
+OVS_WAIT_UNTIL([test $gw1_claim_ct = `cat gw1/ovn-controller.log \
+| grep -c "cr-alice: Claiming"`])
+
+AT_CHECK([test $gw2_claim_ct = `cat gw2/ovn-controller.log | \
+grep -c "cr-alice: Claiming"`])
+
+test_ip_packet gw1 gw2
+
+as gw2
+ovs-appctl -t ovn-controller exit
+ovs-appctl -t ovs-vswitchd exit
+
+OVN_CLEANUP([hv1],[gw1],[ext1])
+
AT_CLEANUP
AT_SETUP([ovn -- 4 HV, 3 LS, 2 LR, packet test with HA distributed router
gateway port])
--
2.20.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] ovn-controller: Fix chassisredirect port flapping when ovs-vswitchd crashes
From: Numan Siddique
On a chassis when ovs-vswitchd crashes for some reason, the BFD status doesn't
get updated in the ovs db. ovn-controller will be reading the old BFD status
even though ovs-vswitchd is crashed. This results in the chassiredirect port
claim flapping between the master chassis and the chasiss with the next higher
priority if ovs-vswitchd crashes in the master chassis.
All the other chassis notices the BFD status down with the master chassis
and hence the next higher priority claims the port. But according to
the master chassis, the BFD status is fine and it again claims back the
chassisredirect port. And this results in flapping. The issue gets resolved
when ovs-vswitchd comes back but until then it leads to lot of SB DB
transactions and high CPU usage in ovn-controller's.
This patch fixes the issue by checking the OF connection status of the
ovn-controller with ovs-vswitchd and calculates the active bfd tunnels
only if it's connected.
Signed-off-by: Numan Siddique
---
ovn/controller/ofctrl.c | 6 ++
ovn/controller/ofctrl.h | 3 +++
ovn/controller/ovn-controller.c | 13 -
tests/ovn.at| 30 +-
4 files changed, 50 insertions(+), 2 deletions(-)
diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 218612787..95b95b607 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -1265,3 +1265,9 @@ ofctrl_inject_pkt(const struct ovsrec_bridge *br_int,
const char *flow_s,
return NULL;
}
+
+bool
+ofctrl_is_connected(void)
+{
+return rconn_is_connected(swconn);
+}
diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
index ae0cfa513..f7521801b 100644
--- a/ovn/controller/ofctrl.h
+++ b/ovn/controller/ofctrl.h
@@ -60,4 +60,7 @@ void ofctrl_check_and_add_flow(struct hmap *desired_flows,
uint8_t table_id,
const struct match *,
const struct ofpbuf *ofpacts,
bool log_duplicate_flow);
+
+bool ofctrl_is_connected(void);
+
#endif /* ovn/ofctrl.h */
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 4e9a5865f..b0f55a870 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -689,7 +689,18 @@ main(int argc, char *argv[])
ovsrec_bridge_table_get(ovs_idl_loop.idl), br_int,
sbrec_chassis_table_get(ovnsb_idl_loop.idl), chassis_id,
sbrec_sb_global_first(ovnsb_idl_loop.idl));
-bfd_calculate_active_tunnels(br_int, _tunnels);
+
+if (ofctrl_is_connected()) {
+/* Calculate the active tunnels only if have an an active
+ * OpenFlow connection to br-int.
+ * If we don't have a connection to br-int, it could mean
+ * ovs-vswitchd is down for some reason and the BFD status
+ * in the Interface rows could be stale. So its better to
+ * consider 'active_tunnels' set to be empty.
+ * */
+bfd_calculate_active_tunnels(br_int, _tunnels);
+}
+
binding_run(ovnsb_idl_txn, ovs_idl_txn, sbrec_chassis_by_name,
sbrec_datapath_binding_by_key,
sbrec_port_binding_by_datapath,
diff --git a/tests/ovn.at b/tests/ovn.at
index f54f24c74..feafe1f00 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -8055,7 +8055,35 @@ ovn-nbctl --timeout=3 --wait=hv \
test_ip_packet gw2 gw1
-OVN_CLEANUP([hv1],[gw1],[gw2],[ext1])
+# Both gw1 and gw2 at this point should have claimed the cr-alice
+# once each.
+gw1_claim_ct=`grep "cr-alice: Claiming" gw1/ovn-controller.log | wc -l`
+gw2_claim_ct=`grep "cr-alice: Claiming" gw2/ovn-controller.log | wc -l`
+
+# Kill ovs-vswitchd in gw2. gw1 should claim the gateway port.
+# So gw1 should claim twice and gw1 only once.
+
+kill `cat gw2/ovs-vswitchd.pid`
+
+# gw1 should claim the cr-alice
+gw1_claim_ct=$((gw1_claim_ct+1))
+
+OVS_WAIT_UNTIL([test $gw1_claim_ct = `cat gw1/ovn-controller.log \
+| grep -c "cr-alice: Claiming"`])
+
+AT_CHECK([test $gw2_claim_ct = `cat gw2/ovn-controller.log | \
+grep -c "cr-alice: Claiming"`])
+
+ovn-sbctl show
+test_ip_packet gw1 gw2
+
+as gw2
+ovs-appctl -t ovn-controller exit
+ovs-appctl -t ovs-vswitchd exit
+ps -aef | grep ovn-c
+
+OVN_CLEANUP([hv1],[gw1],[ext1])
+
AT_CLEANUP
AT_SETUP([ovn -- 4 HV, 3 LS, 2 LR, packet test with HA distributed router
gateway port])
--
2.20.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] lib/tc: Support optional tunnel id
On Sun, 3 Feb 2019 at 12:03, Roi Dayan wrote:
> On Fri, Feb 1, 2019 at 4:05 PM Simon Horman
> wrote:
> >
> > Thanks Roi,
> >
> > On Thu, 31 Jan 2019 at 15:52, Roi Dayan wrote:
> >
> > >
> > >
> > > On 31/01/2019 15:32, Roi Dayan wrote:
> > > >
> > > > On 31/01/2019 11:58, Simon Horman wrote:
> > > >> On Mon, Jan 21, 2019 at 05:32:37PM +0200, Adi Nissim wrote:
> > > >>> Currently the TC tunnel_key action is always
> > > >>> initialized with the given tunnel id value. However,
> > > >>> some tunneling protocols define the tunnel id as an optional field.
> > > >>>
> > > >>> This patch initializes the id field of tunnel_key:set and
> > > tunnel_key:unset
> > > >>> only if a value is provided.
> > > >>>
> > > >>> In the case that a tunnel key value is not provided by the user
> > > >>> the key flag will not be set.
> > > >>>
> > > >>> Signed-off-by: Adi Nissim
> > > >>> Acked-by: Paul Blakey
> > > >>> ---
> > > >>> v1->v2: check if mask.tunnel.id == OVS_BE64_MAX
> > > >>> so we won't do match in the case of a partial mask.
> > > >> I am still a bit concerned about the partial mask case.
> > > >> It looks like the code will now silently not offload such matches.
> > > >>
> > > >> I think that a partial mask should either be offloaded or
> > > >> offload of the entire flow should be rejected.
> > > > thanks. you are right. I didn't notice it. partial masks should be
> > > rejected
> > > > to fallback to ovs dp instead of ignoring the mask.
> > > >
> > >
> > >
> > > Hi Simon,
> > >
> > > I did some checks and think the correct fix is to offload exact match.
> > > if key is partial we can ignore the mask and offload exact match and
> > > it will be correct as we do more strict matching.
> > >
> > > it is also documented that the kernel datapath is doing the same
> > > (from datapath.rst)
> > >
> > > "The kernel can ignore the mask attribute, installing an exact
> > > match flow"
> > >
> > > So I think the first patch V0 is actually correct as we
> > > check the tunnel key flag exists and offload exact match if
> > > there was any mask or offload without a key if the mask is 0
> > > or no key.
> > >
> > > in netdev-tc-offloads.c
> > >
> > > +flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ?
> > > + tnl_mask->tun_id : 0;
> > >
> >
> > I think this is fine so long as tun_id is all-ones. Is that always the
> case?
> > Should the code check that it is the case? Am I missing the point?
> >
>
> it looks like tun_id mask is always set to all-ones.
> but even if it won't be in the future, we shouldn't really care here.
> tc adds exact match on the tun_id and ignores the tun_id mask.
> this is considered ok as the matching is more strict.
> if new match is needed with different tun_id then ovs will try to add
> another rule for it.
> so with tc we could have multiple rules vs 1 rule that support mask.
>
Thanks for looking into this. That sounds find to me but I wonder if we
should make
this behaviour explicit.
/*
* Comment describing why the mask is 0 or all-ones
*/
flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? UINT32_MAX
: 0;
>
> > >
> > >
> > > in tc.c
> > >
> > > -nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);
> > > +if (id_mask) {
> > > +nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);
> > > +}
> > >
> > >
> > > let me know what you think.
> > >
> > > Thanks,
> > > Roi
> > >
> > ___
> > 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
