Re: [ovs-dev] [patch v3 1/4] conntrack: Skip ephemeral ports fallback for DNAT.
On Mon, Dec 10, 2018 at 3:53 PM Ben Pfaff wrote:
> On Mon, Nov 26, 2018 at 08:48:37AM -0800, Darrell Ball wrote:
> > Ephemeral port fallback is being done for DNAT and the code could be hit
> in
> > some special cases and testing configurations. Also good packets are
> > expected to be persistently dropped in this case, which is not a common
> > user goal. Regardless, this is incorrect, so filter this out. Also,
> rename
> > the variable used for checking whether ephemeral ports need to be
> checked.
> >
> > Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-dev/2018-August/351629.html
> > Fixes: 286de2729955 ("dpdk: Userspace Datapath: Introduce NAT Support.")
> > Signed-off-by: Darrell Ball
>
> Does the following change actually have a behavioral difference? I see
> that there's a renaming but the code flow change looks to me like it
> would have the same behavior before and after. If so, could you please
> just leave the code the same?
>
The switch to positive sense for the selection statement is purely
cosmetic; I don't mind omitting it.
>
> > -if (!original_ports_tried) {
> > -original_ports_tried = true;
> > +if (ephemeral_ports_tried) {
> > +break;
> > +} else {
> > +ephemeral_ports_tried = true;
> > ct_addr = conn->nat_info->min_addr;
> > min_port = MIN_NAT_EPHEMERAL_PORT;
> > max_port = MAX_NAT_EPHEMERAL_PORT;
> > -} else {
> > -break;
> > }
>
> Thanks,
>
> Ben.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v3 1/4] conntrack: Stop exporting internal datastructures.
On Mon, Dec 10, 2018 at 5:22 PM Ben Pfaff wrote:
> On Mon, Dec 10, 2018 at 03:47:17PM -0800, Ben Pfaff wrote:
> > On Sun, Dec 02, 2018 at 09:17:16PM -0800, Darrell Ball wrote:
> > > Remove the exporting of the main internal conntrack datastructure.
> > > These are made static. Also stop passing around a pointer parameter
> > > to all the internal datastructures; only one or two is used
> > > for a given code path and these can be referenced directly and passed
> > > specifically where appropriate.
> > >
> > > Signed-off-by: Darrell Ball
> >
> > Seems fine, I applied this to master. Thank you!
>
> Actually I had to un-apply this because:
>
> 1099: dpctl - add-dp del-dp FAILED (
> ovs-macros.at:193)
> 1100: dpctl - add-if set-if del-ifFAILED (
> ovs-macros.at:193)
>
> due to the following Address Sanitizer reports. The following is for
> 1099 but the one for 1100 is almost identical:
>
> =
> ==17824==ERROR: AddressSanitizer: SEGV on unknown address 0xeafffba8 (pc
> 0xf657d67d bp 0x sp 0xffc65150 T0)
> ==17824==The signal is caused by a READ memory access.
> #0 0xf657d67c in __GI___pthread_timedjoin_ex
> (/lib/i386-linux-gnu/libpthread.so.0+0x767c)
> #1 0xf657d5c3 in pthread_join
> (/lib/i386-linux-gnu/libpthread.so.0+0x75c3)
> #2 0xf7522c24 in conntrack_destroy ../lib/conntrack.c:378
>
I don't see this using the address sanitizer; I am using
*../configure** CFLAGS="-g -O2 **-march=native** -fsanitize=address
-fno-omit-frame-pointer -fno-common" **--with-linux=/lib/modules/`uname
-r`/build CC=gcc **--enable-Werror*
Furthermore, I don't see a functional change here, but I'll double check.
I do get a different "report" though
../lib/odp-util.c: In function 'parse_odp_key_mask_attr':
../lib/odp-util.c:5188:30: error: 'mask_offset' may be used uninitialized
in this function [-Werror=maybe-uninitialized]
nl_msg_end_nested(mask, mask_offset); \
^
../lib/odp-util.c:5176:28: note: 'mask_offset' was declared here
size_t key_offset, mask_offset;\
^
../lib/odp-util.c:5452:5: note: in expansion of macro 'SCAN_BEGIN_NESTED'
SCAN_BEGIN_NESTED("tunnel(", OVS_KEY_ATTR_TUNNEL) {
^
cc1: all warnings being treated as errors
> #3 0xf714e568 in dp_netdev_free ../lib/dpif-netdev.c:1614
> #4 0xf714f1ce in dp_netdev_unref ../lib/dpif-netdev.c:1650
> #5 0xf714f2a5 in dp_netdev_unref ../lib/dpif-netdev.c:1645
> #6 0xf714f2a5 in dpif_netdev_close ../lib/dpif-netdev.c:1661
> #7 0xf7170832 in dpif_uninit ../lib/dpif.c:1690
> #8 0xf71709b3 in dpif_close ../lib/dpif.c:456
> #9 0xf70147e2 in close_dpif_backer ../ofproto/ofproto-dpif.c:683
> #10 0xf7024d05 in destruct ../ofproto/ofproto-dpif.c:1614
> #11 0xf6ff9499 in ofproto_destroy ../ofproto/ofproto.c:1663
> #12 0xf6f9acb0 in bridge_destroy ../vswitchd/bridge.c:3298
> #13 0xf6fb399b in bridge_exit ../vswitchd/bridge.c:507
> #14 0xf6f89653 in main ../vswitchd/ovs-vswitchd.c:141
> #15 0xf61c3e80 in __libc_start_main
> (/lib/i386-linux-gnu/libc.so.6+0x18e80)
> #16 0xf6f8f245
> (/home/blp/nicira/ovs/_build/vswitchd/ovs-vswitchd+0x67245)
>
> AddressSanitizer can not provide additional info.
> SUMMARY: AddressSanitizer: SEGV
> (/lib/i386-linux-gnu/libpthread.so.0+0x767c) in __GI___pthread_timedjoin_ex
> ==17824==ABORTING
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v9 05/11] ovs-atomic: Add 64 bit apis.
On Mon, Dec 10, 2018 at 5:50 PM Ben Pfaff wrote: > On Mon, Nov 19, 2018 at 11:09:24AM -0800, Darrell Ball wrote: > > Signed-off-by: Darrell Ball > > Please write 1ull as 1ULL because in some fonts the former is almost > unreadable. > Makes sense > > Thanks, > > Ben. > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v9 02/11] flow: Enhance parse_ipv6_ext_hdrs.
On Mon, Dec 10, 2018 at 5:48 PM Ben Pfaff wrote: > On Mon, Nov 19, 2018 at 11:09:21AM -0800, Darrell Ball wrote: > > Acked-by: Justin Pettit > > Signed-off-by: Darrell Ball > > Thanks for adding the documentation comment to parse_ipv6_ext_hdrs(). > Please update the documentation comment to describe the return value. > sure > > I don't see anything that sets *frag_hdr to NULL if there is no fragment > header. This seems risky at best. > Agreed; there is no guarantee the user would check 'nw_frag' before accessing frag_hdr and/or set *frag_hdr to NULL in the caller. > > Thanks, > > Ben. > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v3 3/4] conntrack: Enforce conn_type for flush tuple.
On Mon, Dec 10, 2018 at 5:42 PM Ben Pfaff wrote: > On Mon, Nov 26, 2018 at 08:48:39AM -0800, Darrell Ball wrote: > > The user should only reference a conntrack entry by the forward > > direction context, as per 'conntrack_flush()', enforce this by > > checking for 'default' conn_type. The likelihood of a user > > not using the original tuple is low, but it should be guarded > > against, logged and documented. > > > > Signed-off-by: Darrell Ball > > --- > > > > Backport to 2.9. > > > > v3: Move backport hint out of commit message. > > Remove warning log conditional for now. > > Would it be more user-friendly to translate these into the forward > equivalent and flush that one? > If there were a practical application to try to flush a tuple using the dynamic/random NAT tuple assignment, yes; but there is not and it is also not worth the added complexity to handle the race b/w buckets for this purpose. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v3 2/4] conntrack: Check all addresses for ephemeral ports.
On Mon, Dec 10, 2018 at 5:39 PM Ben Pfaff wrote:
> On Mon, Nov 26, 2018 at 08:48:38AM -0800, Darrell Ball wrote:
> > When fallback to ephemeral ports triggers to find a NAT translation,
> > it may happen that the full address range is not explored; i.e. if
> > all ephemeral ports are being used for the address range >= the
> > first address checked and there are other addresses in the
> > available range, then they would not be explored for availability.
> > The likelihood of hitting this condition is rare. The fix is to
> > reset the first address to the minimum address when starting to
> > search ephemeral ports. Found by inspection.
> >
> > Fixes: 286de2729955 ("dpdk: Userspace Datapath: Introduce NAT Support.")
> > Signed-off-by: Darrell Ball
>
> Looks good, thanks.
>
> Can't be applied without that other series.
>
The other series is meant to be rebased after this long pending series; I
had
added comments to the other series cover letter.
"These patches presently include an updated version of the changes
for this series
https://patchwork.ozlabs.org/project/openvswitch/list/?series=78059
but those parts (approx 20 lines) will be later dropped from this
series and handled with the above mentioned series."
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v9 05/11] ovs-atomic: Add 64 bit apis.
On Mon, Nov 19, 2018 at 11:09:24AM -0800, Darrell Ball wrote: > Signed-off-by: Darrell Ball Please write 1ull as 1ULL because in some fonts the former is almost unreadable. Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v9 02/11] flow: Enhance parse_ipv6_ext_hdrs.
On Mon, Nov 19, 2018 at 11:09:21AM -0800, Darrell Ball wrote: > Acked-by: Justin Pettit > Signed-off-by: Darrell Ball Thanks for adding the documentation comment to parse_ipv6_ext_hdrs(). Please update the documentation comment to describe the return value. I don't see anything that sets *frag_hdr to NULL if there is no fragment header. This seems risky at best. Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v3 1/4] conntrack: Stop exporting internal datastructures.
On Mon, Dec 10, 2018 at 03:47:17PM -0800, Ben Pfaff wrote: > On Sun, Dec 02, 2018 at 09:17:16PM -0800, Darrell Ball wrote: > > Remove the exporting of the main internal conntrack datastructure. > > These are made static. Also stop passing around a pointer parameter > > to all the internal datastructures; only one or two is used > > for a given code path and these can be referenced directly and passed > > specifically where appropriate. > > > > Signed-off-by: Darrell Ball > > Seems fine, I applied this to master. Thank you! Actually I had to un-apply this because: 1099: dpctl - add-dp del-dp FAILED (ovs-macros.at:193) 1100: dpctl - add-if set-if del-ifFAILED (ovs-macros.at:193) due to the following Address Sanitizer reports. The following is for 1099 but the one for 1100 is almost identical: = ==17824==ERROR: AddressSanitizer: SEGV on unknown address 0xeafffba8 (pc 0xf657d67d bp 0x sp 0xffc65150 T0) ==17824==The signal is caused by a READ memory access. #0 0xf657d67c in __GI___pthread_timedjoin_ex (/lib/i386-linux-gnu/libpthread.so.0+0x767c) #1 0xf657d5c3 in pthread_join (/lib/i386-linux-gnu/libpthread.so.0+0x75c3) #2 0xf7522c24 in conntrack_destroy ../lib/conntrack.c:378 #3 0xf714e568 in dp_netdev_free ../lib/dpif-netdev.c:1614 #4 0xf714f1ce in dp_netdev_unref ../lib/dpif-netdev.c:1650 #5 0xf714f2a5 in dp_netdev_unref ../lib/dpif-netdev.c:1645 #6 0xf714f2a5 in dpif_netdev_close ../lib/dpif-netdev.c:1661 #7 0xf7170832 in dpif_uninit ../lib/dpif.c:1690 #8 0xf71709b3 in dpif_close ../lib/dpif.c:456 #9 0xf70147e2 in close_dpif_backer ../ofproto/ofproto-dpif.c:683 #10 0xf7024d05 in destruct ../ofproto/ofproto-dpif.c:1614 #11 0xf6ff9499 in ofproto_destroy ../ofproto/ofproto.c:1663 #12 0xf6f9acb0 in bridge_destroy ../vswitchd/bridge.c:3298 #13 0xf6fb399b in bridge_exit ../vswitchd/bridge.c:507 #14 0xf6f89653 in main ../vswitchd/ovs-vswitchd.c:141 #15 0xf61c3e80 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x18e80) #16 0xf6f8f245 (/home/blp/nicira/ovs/_build/vswitchd/ovs-vswitchd+0x67245) AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV (/lib/i386-linux-gnu/libpthread.so.0+0x767c) in __GI___pthread_timedjoin_ex ==17824==ABORTING ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1] ofp-monitor: Added support for OpenFlow 1.4+ Flow Monitor
Thanks for the feedback. Would revert back shortly with replies and few questions followed by the v2 patch. On Mon, Dec 10, 2018 at 1:54 PM Ben Pfaff wrote: > On Wed, Nov 28, 2018 at 10:30:07AM -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 a lot for working on this. The OF1.4+ flow monitor support was > based on the OVS design for the same feature, so it's a shame that we > didn't implement it early on. > > In openflow-1.4.h, please align the comments on members of structs and > enumerations to some common column, as is the usual practice in OVS > code, e.g. instead of this: > > /* struct ofp14_flow_update_full. */ > OFPFME_INITIAL = 0, /* Flow present when flow monitor created. */ > OFPFME_ADDED = 1, /* Flow was added. */ > OFPFME_REMOVED = 2, /* Flow was removed. */ > OFPFME_MODIFIED = 3, /* Flow instructions were changed. */ > /* struct ofp14_flow_update_abbrev. */ > OFPFME_ABBREV = 4, /* Abbreviated reply. */ > /* struct ofp14_flow_update_header. */ > OFPFME_PAUSED = 5, /* Monitoring paused (out of buffer space). */ > OFPFME_RESUMED = 6, /* Monitoring resumed. */ > > more like this (also adding some blank lines for clarity): > > /* struct ofp14_flow_update_full. */ > OFPFME_INITIAL = 0, /* Flow present when flow monitor created. > */ > OFPFME_ADDED = 1, /* Flow was added. */ > OFPFME_REMOVED = 2, /* Flow was removed. */ > OFPFME_MODIFIED = 3,/* Flow instructions were changed. */ > > /* struct ofp14_flow_update_abbrev. */ > OFPFME_ABBREV = 4, /* Abbreviated reply. */ > > /* struct ofp14_flow_update_header. */ > OFPFME_PAUSED = 5, /* Monitoring paused (out of buffer > space). */ > OFPFME_RESUMED = 6, /* Monitoring resumed. */ > > Also please use the "14" infix for these enumeration members, > e.g. OFPFME14_INITIAL. > > The OFPUTIL_FMF_* flags aren't really abstracted from NXFMF_* and > OFPFMF14_*; they're just a copy of the OFPFMF14_* ones with the names > changed. I think it might be better to just use OFPFMF14_* with a note > that NXFMF_* doesn't have NXFMF_ONLY_OWN. > > It's usually unnecessary to use a union of an ofp_port_t and an > ofp11_port_t, because OVS can usually reject unsupported ports when it > parses them from OpenFlow using ofputil_port_from_ofp11(). Maybe there > is some exception here but that's not obvious to me yet. > > I don't see how a piece of code that looks at an > ofputil_flow_monitor_request can tell whether to look at > out_port.ofp_port or out_port.ofp11_port. > > Our usual style is to start a comment with a capital letter and end it > with a period: > > /* there is one to one mapping between ofp14_flow_update_event and > * ofputil_flow_update_event */ > > The name ofputil_get_util_flow_update_event_from_nx_event() is pretty > long and I don't know what the _util_ part of it is there for. > > In new code, please try to declare (and initialize) variables at their > first use, when possible, rather than the older style we used of > declaring variables at the top of a block. > > I think that ofputil_start_flow_update() should always use > NXST_FLOW_MONITOR_REPLY for OpenFlow 1.0, so it seems to me that it > should check for OFPUTIL_P_OF10_ANY rather than OFPUTIL_P_OF10_STD_ANY. > (Or it could accept an OpenFlow version instead of a protocol.) > > I don't understand why ofputil_append_flow_update() checks for > OFPUTIL_P_OF10_STD in particular. It seems like any OF1.0 protocol > would make sense. (Or it could accept an OpenFlow version instead of a > protocol.) > > In general I'm not sure of the rationale here for how OpenFlow versions > are handled. Currently, it looks like OVS supports flow monitoring in > OF1.0 only. The OpenFlow specification defines flow monitoring for > OF1.4 and later and for OF1.3 as an "official extension" in extensions > pack 2 (which you can find at > > https://www.opennetworking.org/wp-content/uploads/2014/10/openflow-extensions-1.3.x-pack2.zip > ). > So we should use the official specs for 1.3 and later. For 1.1 and 1.2, > we have a choice of supporting our extension or the OpenFlow version. I > don't have a particular preference. > > Thanks! I'll look forward to the next revision. > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v1 1/1] dpdk: Update to use DPDK 18.11.
This commit adds support for DPDK v18.11, it includes the following
changes.
1. Enable compilation and linkage with dpdk 18.11.0
The following dpdk commits which were introduced after dpdk 17.11.x
require OVS updates to accommodate to the dpdk changes.
- ce17edde ("ethdev: introduce Rx queue offloads API")
- ab3ce1e0 ("ethdev: remove old offload API")
- c06ddf96 ("meter: add configuration profile")
- e58638c3 ("ethdev: fix TPID handling in flow API")
- cd8c7c7c ("ethdev: replace bus specific struct with generic dev")
- ac8d22de ("ethdev: flatten RSS configuration in flow API")
2. Limit configured rss hash functions to only those supported
by the eth device.
3. Set default RSS key in struct action_rss_data, required by OVS
commit- e8a2b5bf ("netdev-dpdk: implement flow offload with rte flow")
when configured with "other_config:hw-offload=true".
4. DEV_RX_OFFLOAD_CRC_STRIP has been removed from DPDK 18.11.
DEV_RX_OFFLOAD_KEEP_CRC can now be used to keep the CRC.
Use the correct flag and check it is supported.
5. rte_eth_dev_attach/detach have been removed from DPDK 18.11.
Replace them with rte_dev_probe/remove.
6. redhat: change variable used for non-root user support from
$HOME to $XDG_RUNTIME_DIR.
7. Update docs and travis to use DPDK18.11.
This commit squashes the following commits present on the dpdk-latest
branch:
7f021f902bb3 ("netdev-dpdk: Upgrade to dpdk v18.08")
270d9216f1ed ("netdev-dpdk: Set scatter based on capabilities")
bef2cdc8f412 ("netdev-dpdk: Fix returning the field of malloced struct.")
73c1a65167fc ("redhat: change variable used for non-root user support")
eb485f60ce44 ("dpdk: Update to use DPDK 18.11.")
For credit all authors of the original commits above have been added as
co-authors for this commmit.
Signed-off-by: Ophir Munk
Co-authored-by: Ophir Munk
Signed-off-by: Kevin Traynor
Co-authored-by: Kevin Traynor
Signed-off-by: Ilya Maximets
Co-authored-by: Ilya Maximets
Signed-off-by: Timothy Redaelli
Co-authored-by: Timothy Redaelli
Signed-off-by: Ian Stokes
---
.travis/linux-build.sh | 8 +-
Documentation/intro/install/dpdk.rst | 15 +-
Documentation/topics/dpdk/ring.rst | 3 +-
Documentation/topics/dpdk/vhost-user.rst | 8 +-
NEWS | 1 +
lib/netdev-dpdk.c | 176 +
.../usr_lib_systemd_system_ovs-vswitchd.service.in | 2 +-
7 files changed, 132 insertions(+), 81 deletions(-)
diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh
index 1fe5bbfa9..5f4d838a9 100755
--- a/.travis/linux-build.sh
+++ b/.travis/linux-build.sh
@@ -56,9 +56,9 @@ function install_dpdk()
cd dpdk-$1
git checkout tags/v$1
else
-wget http://fast.dpdk.org/rel/dpdk-$1.tar.gz
-tar xzvf dpdk-$1.tar.gz > /dev/null
-DIR_NAME=$(tar -tf dpdk-$1.tar.gz | head -1 | cut -f1 -d"/")
+wget https://fast.dpdk.org/rel/dpdk-$1.tar.xz
+tar xvf dpdk-$1.tar.xz > /dev/null
+DIR_NAME=$(tar -tf dpdk-$1.tar.xz | head -1 | cut -f1 -d"/")
if [ $DIR_NAME != "dpdk-$1" ]; then mv $DIR_NAME dpdk-$1; fi
cd dpdk-$1
fi
@@ -83,7 +83,7 @@ fi
if [ "$DPDK" ]; then
if [ -z "$DPDK_VER" ]; then
-DPDK_VER="17.11.4"
+DPDK_VER="18.11"
fi
install_dpdk $DPDK_VER
if [ "$CC" = "clang" ]; then
diff --git a/Documentation/intro/install/dpdk.rst
b/Documentation/intro/install/dpdk.rst
index 13546bb72..61307cb7c 100644
--- a/Documentation/intro/install/dpdk.rst
+++ b/Documentation/intro/install/dpdk.rst
@@ -42,7 +42,7 @@ Build requirements
In addition to the requirements described in :doc:`general`, building Open
vSwitch with DPDK will require the following:
-- DPDK 17.11.4
+- DPDK 18.11
- A `DPDK supported NIC`_
@@ -71,9 +71,9 @@ Install DPDK
#. Download the `DPDK sources`_, extract the file and set ``DPDK_DIR``::
$ cd /usr/src/
- $ wget http://fast.dpdk.org/rel/dpdk-17.11.4.tar.xz
- $ tar xf dpdk-17.11.4.tar.xz
- $ export DPDK_DIR=/usr/src/dpdk-stable-17.11.4
+ $ wget http://fast.dpdk.org/rel/dpdk-18.11.tar.xz
+ $ tar xf dpdk-18.11.tar.xz
+ $ export DPDK_DIR=/usr/src/dpdk-18.11
$ cd $DPDK_DIR
#. (Optional) Configure DPDK as a shared library
@@ -283,9 +283,9 @@ with either the ovs-vswitchd logs, or by running either of
the commands::
$ ovs-vswitchd --version
ovs-vswitchd (Open vSwitch) 2.9.0
- DPDK 17.11.0
+ DPDK 18.08.0
$ ovs-vsctl get Open_vSwitch . dpdk_version
- "DPDK 17.11.0"
+ "DPDK 18.08.0"
At this point you can use ovs-vsctl to set up bridges and other Open vSwitch
features. Seeing as we've configured the DPDK datapath, we will use DPDK-type
@@ -672,7 +672,8 @@ Limitations
The latest list of validated firmware versions can be found in the `DPDK
release notes`_.
-.. _DPDK release notes:
Re: [ovs-dev] [patch v3 1/4] conntrack: Skip ephemeral ports fallback for DNAT.
On Mon, Nov 26, 2018 at 08:48:37AM -0800, Darrell Ball wrote:
> Ephemeral port fallback is being done for DNAT and the code could be hit in
> some special cases and testing configurations. Also good packets are
> expected to be persistently dropped in this case, which is not a common
> user goal. Regardless, this is incorrect, so filter this out. Also, rename
> the variable used for checking whether ephemeral ports need to be checked.
>
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-dev/2018-August/351629.html
> Fixes: 286de2729955 ("dpdk: Userspace Datapath: Introduce NAT Support.")
> Signed-off-by: Darrell Ball
Does the following change actually have a behavioral difference? I see
that there's a renaming but the code flow change looks to me like it
would have the same behavior before and after. If so, could you please
just leave the code the same?
> -if (!original_ports_tried) {
> -original_ports_tried = true;
> +if (ephemeral_ports_tried) {
> +break;
> +} else {
> +ephemeral_ports_tried = true;
> ct_addr = conn->nat_info->min_addr;
> min_port = MIN_NAT_EPHEMERAL_PORT;
> max_port = MAX_NAT_EPHEMERAL_PORT;
> -} else {
> -break;
> }
Thanks,
Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v3 1/4] conntrack: Stop exporting internal datastructures.
On Sun, Dec 02, 2018 at 09:17:16PM -0800, Darrell Ball wrote: > Remove the exporting of the main internal conntrack datastructure. > These are made static. Also stop passing around a pointer parameter > to all the internal datastructures; only one or two is used > for a given code path and these can be referenced directly and passed > specifically where appropriate. > > Signed-off-by: Darrell Ball Seems fine, I applied this to master. Thank you! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v2 0/5] conntrack: Optimize and refactor.
On Mon, Dec 10, 2018 at 03:34:05PM -0800, Ben Pfaff wrote: > Do you think you could post a v3 with Aaron's comments accounted for? Oh, never mind, I see v3. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v2 0/5] conntrack: Optimize and refactor.
Do you think you could post a v3 with Aaron's comments accounted for? Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1] ofp-monitor: Added support for OpenFlow 1.4+ Flow Monitor
On Wed, Nov 28, 2018 at 10:30:07AM -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 a lot for working on this. The OF1.4+ flow monitor support was based on the OVS design for the same feature, so it's a shame that we didn't implement it early on. In openflow-1.4.h, please align the comments on members of structs and enumerations to some common column, as is the usual practice in OVS code, e.g. instead of this: /* struct ofp14_flow_update_full. */ OFPFME_INITIAL = 0, /* Flow present when flow monitor created. */ OFPFME_ADDED = 1, /* Flow was added. */ OFPFME_REMOVED = 2, /* Flow was removed. */ OFPFME_MODIFIED = 3, /* Flow instructions were changed. */ /* struct ofp14_flow_update_abbrev. */ OFPFME_ABBREV = 4, /* Abbreviated reply. */ /* struct ofp14_flow_update_header. */ OFPFME_PAUSED = 5, /* Monitoring paused (out of buffer space). */ OFPFME_RESUMED = 6, /* Monitoring resumed. */ more like this (also adding some blank lines for clarity): /* struct ofp14_flow_update_full. */ OFPFME_INITIAL = 0, /* Flow present when flow monitor created. */ OFPFME_ADDED = 1, /* Flow was added. */ OFPFME_REMOVED = 2, /* Flow was removed. */ OFPFME_MODIFIED = 3,/* Flow instructions were changed. */ /* struct ofp14_flow_update_abbrev. */ OFPFME_ABBREV = 4, /* Abbreviated reply. */ /* struct ofp14_flow_update_header. */ OFPFME_PAUSED = 5, /* Monitoring paused (out of buffer space). */ OFPFME_RESUMED = 6, /* Monitoring resumed. */ Also please use the "14" infix for these enumeration members, e.g. OFPFME14_INITIAL. The OFPUTIL_FMF_* flags aren't really abstracted from NXFMF_* and OFPFMF14_*; they're just a copy of the OFPFMF14_* ones with the names changed. I think it might be better to just use OFPFMF14_* with a note that NXFMF_* doesn't have NXFMF_ONLY_OWN. It's usually unnecessary to use a union of an ofp_port_t and an ofp11_port_t, because OVS can usually reject unsupported ports when it parses them from OpenFlow using ofputil_port_from_ofp11(). Maybe there is some exception here but that's not obvious to me yet. I don't see how a piece of code that looks at an ofputil_flow_monitor_request can tell whether to look at out_port.ofp_port or out_port.ofp11_port. Our usual style is to start a comment with a capital letter and end it with a period: /* there is one to one mapping between ofp14_flow_update_event and * ofputil_flow_update_event */ The name ofputil_get_util_flow_update_event_from_nx_event() is pretty long and I don't know what the _util_ part of it is there for. In new code, please try to declare (and initialize) variables at their first use, when possible, rather than the older style we used of declaring variables at the top of a block. I think that ofputil_start_flow_update() should always use NXST_FLOW_MONITOR_REPLY for OpenFlow 1.0, so it seems to me that it should check for OFPUTIL_P_OF10_ANY rather than OFPUTIL_P_OF10_STD_ANY. (Or it could accept an OpenFlow version instead of a protocol.) I don't understand why ofputil_append_flow_update() checks for OFPUTIL_P_OF10_STD in particular. It seems like any OF1.0 protocol would make sense. (Or it could accept an OpenFlow version instead of a protocol.) In general I'm not sure of the rationale here for how OpenFlow versions are handled. Currently, it looks like OVS supports flow monitoring in OF1.0 only. The OpenFlow specification defines flow monitoring for OF1.4 and later and for OF1.3 as an "official extension" in extensions pack 2 (which you can find at https://www.opennetworking.org/wp-content/uploads/2014/10/openflow-extensions-1.3.x-pack2.zip). So we should use the official specs for 1.3 and later. For 1.1 and 1.2, we have a choice of supporting our extension or the OpenFlow version. I don't have a particular preference. Thanks! I'll look forward to the next revision. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovs-ctl: fix system-id.conf owner
On Thu, Nov 22, 2018 at 04:37:57PM +0100, David Marchand wrote: > As far as RPMs are concerned, system-id.conf file is declared as being > owned by openvswitch. > At the first ovs startup, ovs-ctl creates this file if none exists without > ensuring this. > > We end up with an inconsistency: > $ rpm -V openvswitch > .UG.. c /etc/openvswitch/system-id.conf > > Fix this when ovs-ctl is the one who creates the file. > > Note: this issue ends up being hidden after a RPM upgrade, since the > openvswitch user is enforced on the whole /etc/openvswitch directory as a > %post operation. > > Signed-off-by: David Marchand Thanks! Applied to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] rhel: Don't ship static libraries
On Tue, Dec 04, 2018 at 10:34:43AM -0200, Flavio Leitner wrote:
> On Tue, Nov 20, 2018 at 07:40:50PM +0100, Timothy Redaelli wrote:
> > Since commit bc4fd439586f ("rhel: Ship ovs shared libraries, fedora")
> > openvswitch-devel RPM package includes both static and shared library.
> > This is against the Fedora Packaging Guidelines [1].
> >
> > This commit prevent the static libraries and libtool archives to be shipped.
> >
> > [1]
> > https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries
> > Signed-off-by: Timothy Redaelli
> > ---
>
> Acked-by: Flavio Leitner
Applied to master, thanks Timothy (and Flavio).
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5] pinctrl: Check requested IP in DHCPREQUEST messages
On Wed, Dec 05, 2018 at 07:10:13PM +, Gregory Smith wrote: > See RFC 2131, section 4.3.2. When handling a DHCPREQUEST message, the > server should validate that the client's requested IP matches the > offered IP. If not, the server should reply with a DHCPNAK. The client's > requested IP is either specified as the Requested IP Address (option > 50), or as the ciaddr, depending on the client's state. > > Signed-off-by: Gregory Smith > --- > > v4 -> v5 > > * Fixed clang cast alignment warnings. > > v3 -> v4 > > * Reworked the option-parsing while loop. > > v2 -> v3 > > * Fixed long line. > > v1 -> v2 > > * Refactored DHCP option parsing and packet boundary checks. > > lib/dhcp.h | 3 + > ovn/controller/pinctrl.c | 129 +--- > ovn/lib/ovn-l7.h | 9 +++ > tests/ovn.at | 188 > +-- > 4 files changed, 264 insertions(+), 65 deletions(-) Thanks for the patch. This code looks pretty optimistic about things that I'm not sure it should depend on. It uses ALIGNED_CAST in multiple places although I don't see a reason to assume that the data in question is actually aligned. It also appears to read out the DHCP magic cookie before it checks whether the packet is long enough to contain it. Please take another look and make sure that this code is validating everything properly. Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] ossfuzz: Regression testing with fuzzer generated corpus
On Fri, Nov 30, 2018 at 01:17:39PM +0100, Bhargava Shastry wrote: > Hi all, > > oss-fuzz corpus (test inputs synthesized by the fuzzer) comprises two > classes of inputs: crashing and non-crashing-but-new-coverage-yielding. > > At the moment, Open vSwitch performs regression testing using > **crashing** test inputs only [1]. > > [1]: https://github.com/openvswitch/ovs/tree/master/tests/fuzz-regression > > However, adding non-crashing test inputs generated by the fuzzer to this > set may be useful to catch bugs that are not necessarily regressions of > known bugs but bugs in program paths that have already been covered > during fuzz testing. > > If you like this idea, I have an initial proposal. What we could do is > use this "driver" [2] for each of the fuzzer targets to drive regression > testing on the entire fuzzer corpus. > > [2]: > https://github.com/llvm-mirror/compiler-rt/blob/master/lib/fuzzer/standalone/StandaloneFuzzTargetMain.c > > The fuzzer corpus may be downloaded by oss-fuzz contact points (e.g., > Ben Pfaff, Justin Pettit etc.) from Google Cloud via a program called > gsutil that is shipped with Google Cloud SDK. This would need to be > updated from time to time, but this is very easy (`gsutil sync` is > sufficient). > > The plan is to have a PR that includes the corpus obtained via Google > cloud, standalone drivers, and some sort of regression test automation > for all the fuzzer targets. > > I am interested in contributing to this effort, should you decide to go > forward with it. Looking forward to feedback. It sounds like a good idea to me. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v1] rhel: Add 'SYSTEMD_NO_WRAP=yes' in ovs init script for SLES
The variable equivalent to RHEL's 'SYSTEMCTL_SKIP_REDIRECT=yes' on SLES 12 is 'SYSTEMD_NO_WRAP=yes' VMware-BZ: #2245358 Signed-off-by: Martin Xu CC: Markos Chandras CC: Ansis Atteka CC: Ben Pfaff --- rhel/etc_init.d_openvswitch | 1 + 1 file changed, 1 insertion(+) diff --git a/rhel/etc_init.d_openvswitch b/rhel/etc_init.d_openvswitch index 20d556803..7a4cfbab5 100755 --- a/rhel/etc_init.d_openvswitch +++ b/rhel/etc_init.d_openvswitch @@ -28,6 +28,7 @@ ### END INIT INFO SYSTEMCTL_SKIP_REDIRECT=yes +SYSTEMD_NO_WRAP=yes . /usr/share/openvswitch/scripts/ovs-lib || exit 1 test -e /etc/sysconfig/openvswitch && . /etc/sysconfig/openvswitch -- 2.12.3 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 0/8] datapath: Pull in upstream fixes and updates
On Wed, Dec 05, 2018 at 10:55:44AM -0800, Greg Rose wrote: > Pull in recent Linux upstream kernel updates for openvswitch core. > > Travis build is here: > https://travis-ci.org/gvrose8192/ovs-experimental/builds/463951223 It looks like this series is fully acked. Greg, would you mind reposting with the acks included? Thanks a lot. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] tests: Simplify and improve the daemon tests.
Bleep bloop. Greetings Ben Pfaff, 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. checkpatch: ERROR: Author Scott Cheloha needs to sign off. WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Ben Pfaff Lines checked: 676, Warnings: 1, Errors: 1 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 8/8] datapath: fix spelling mistake "execeeds" -> "exceeds"
On Wed, Dec 5, 2018 at 10:56 AM Greg Rose wrote: > > From: Colin Ian King > > Upstream commit: > commit 43d0e96022ae3c66743c01bba6c18a3afec7b578 > Author: Colin Ian King > Date: Tue Nov 27 14:37:17 2018 + > > openvswitch: fix spelling mistake "execeeds" -> "exceeds" > > There is a spelling mistake in a net_warn_ratelimited message, fix this. > > Signed-off-by: Colin Ian King > Reviewed-by: Simon Horman > Signed-off-by: David S. Miller > > CC: Colin Ian King > Signed-off-by: Greg Rose > --- Acked-by: William Tu ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] tests: Simplify and improve the daemon tests.
From: Scott Cheloha
The daemon tests used files a lot when shell variables were easier to use
and easier to understand. This commit changes that.
The tests created empty databases that aren't really needed anymore. This
commit changes them to use the ovsdb-server --no-db option instead.
The tests had a lot of common code for checking the ancestry of processes.
This commit factors out a new shell function check_ancestors.
The tests tended to use random pidfile names. This switches to just using
the defaults, which are fine.
The tests didn't check the names of the child processes. This adds those
checks using the new check_process_name shell function. This should avoid
regression of the bug fixed by commit 266f79e32c60 ("daemon-unix: Use
same name for original or restarted children.")
Other minor improvements too.
I only made small updates to the Windows-specific test, because it is hard
for me to verify.
Acked-by: Alin Gabriel Serdean
Signed-off-by: Ben Pfaff
---
v1->v2: Rebased, added ack.
tests/daemon-py.at | 218 +
tests/daemon.at| 212 ---
tests/odp.at | 12 +--
3 files changed, 224 insertions(+), 218 deletions(-)
diff --git a/tests/daemon-py.at b/tests/daemon-py.at
index b7236b1f3622..6adea3c85dfc 100644
--- a/tests/daemon-py.at
+++ b/tests/daemon-py.at
@@ -6,20 +6,23 @@ m4_define([DAEMON_PYN],
# Skip this test for Windows, echo $! gives shell pid instead of parent
process
AT_SKIP_IF([test "$IS_WIN32" = "yes"])
AT_KEYWORDS([python daemon])
- AT_CAPTURE_FILE([pid])
- AT_CAPTURE_FILE([expected])
+
+ on_exit 'kill $(cat *.pid)'
+ pidfile=test-daemon.py.pid
+
# Start the daemon and wait for the pidfile to get created
# and that its contents are the correct pid.
- AT_CHECK([$3 $srcdir/test-daemon.py --pidfile=pid& echo $! > expected], [0])
- OVS_WAIT_UNTIL([test -s pid], [kill `cat expected`])
- AT_CHECK(
- [pid=`cat pid` && expected=`cat expected` && test "$pid" = "$expected"],
- [0], [], [], [kill `cat expected`])
- AT_CHECK([kill -0 `cat pid`], [0], [], [], [kill `cat expected`])
+ AT_CHECK([$3 $srcdir/test-daemon.py --pidfile & echo $!], [0], [stdout])
+ pid=$(cat stdout)
+
+ OVS_WAIT_UNTIL([test -s $pidfile], [kill $pid])
+ AT_CHECK([test $pid = $(cat $pidfile)])
+ AT_CHECK([kill -0 $pid])
+
# Kill the daemon and make sure that the pidfile gets deleted.
- kill `cat expected`
- OVS_WAIT_WHILE([kill -0 `cat expected`])
- AT_CHECK([test ! -e pid])
+ kill $pid
+ OVS_WAIT_WHILE([kill -0 $pid])
+ AT_CHECK([test ! -e $pidfile])
AT_CLEANUP])
DAEMON_PYN([Python2], [$HAVE_PYTHON2], [$PYTHON2])
@@ -28,42 +31,38 @@ DAEMON_PYN([Python3], [$HAVE_PYTHON3], [$PYTHON3])
m4_define([DAEMON_MONITOR_PYN],
[AT_SETUP([daemon --monitor - $1])
AT_SKIP_IF([test $2 = no])
+
# Skip this test for Windows, echo $! gives shell pid instead of parent
process
AT_SKIP_IF([test "$IS_WIN32" = "yes"])
- AT_CAPTURE_FILE([pid])
- AT_CAPTURE_FILE([parent])
- AT_CAPTURE_FILE([parentpid])
- AT_CAPTURE_FILE([newpid])
+
+ on_exit 'kill $(cat *.pid)'
+ pidfile=test-daemon.py.pid
+
# Start the daemon and wait for the pidfile to get created.
- AT_CHECK([$3 $srcdir/test-daemon.py --pidfile=pid --monitor& echo $! >
parent], [0])
- on_exit 'kill `cat parent`'
- OVS_WAIT_UNTIL([test -s pid])
+ AT_CHECK([$3 $srcdir/test-daemon.py --pidfile --monitor & echo $!], [0],
[stdout])
+ monitor=$(cat stdout)
+ OVS_WAIT_UNTIL([test -s $pidfile])
+ child=$(cat $pidfile)
+
# Check that the pidfile names a running process,
# and that the parent process of that process is our child process.
- AT_CHECK([kill -0 `cat pid`])
- AT_CHECK([parent_pid `cat pid` > parentpid])
- AT_CHECK(
- [parentpid=`cat parentpid` &&
- parent=`cat parent` &&
- test $parentpid = $parent])
+ check_ancestors $child $monitor
+
# Kill the daemon process, making it look like a segfault,
# and wait for a new child process to get spawned.
- AT_CHECK([cp pid oldpid])
- AT_CHECK([kill -SEGV `cat pid`], [0], [], [ignore])
- OVS_WAIT_WHILE([kill -0 `cat oldpid`])
- OVS_WAIT_UNTIL([test -s pid && test `cat pid` != `cat oldpid`])
- AT_CHECK([cp pid newpid])
+ AT_CHECK([kill -SEGV $child])
+ OVS_WAIT_WHILE([kill -0 $child])
+ OVS_WAIT_UNTIL([test -s $pidfile && test $(cat $pidfile) != $child])
+ child2=$(cat $pidfile)
+
# Check that the pidfile names a running process,
# and that the parent process of that process is our child process.
- AT_CHECK([parent_pid `cat pid` > parentpid])
- AT_CHECK(
- [parentpid=`cat parentpid` &&
- parent=`cat parent` &&
- test $parentpid = $parent])
+ check_ancestors $child2 $monitor
+
# Kill the daemon process with SIGTERM, and wait for the daemon
# and the monitor processes to go away and the pidfile to get deleted.
-
Re: [ovs-dev] [PATCH] tests: Simplify and improve the daemon tests.
On Thu, Dec 06, 2018 at 03:58:34PM +0200, Alin Gabriel Serdean wrote:
>
>
> > On 29 Oct 2018, at 18:25, Ben Pfaff wrote:
> >
> > The daemon tests used files a lot when shell variables were easier to use
> > and easier to understand. This commit changes that.
> >
> > The tests created empty databases that aren't really needed anymore. This
> > commit changes them to use the ovsdb-server --no-db option instead.
> >
> > The tests had a lot of common code for checking the ancestry of processes.
> > This commit factors out a new shell function check_ancestors.
> >
> > The tests tended to use random pidfile names. This switches to just using
> > the defaults, which are fine.
> >
> > The tests didn't check the names of the child processes. This adds those
> > checks using the new check_process_name shell function. This should avoid
> > regression of the bug fixed by commit 266f79e32c60 ("daemon-unix: Use
> > same name for original or restarted children.")
> >
> > Other minor improvements too.
> >
> > I only made small updates to the Windows-specific test, because it is hard
> > for me to verify.
> >
> > Signed-off-by: Ben Pfaff
> > ---
> > tests/daemon-py.at | 218
> > +
> > tests/daemon.at| 214
> > +---
> > 2 files changed, 222 insertions(+), 210 deletions(-)
> >
>
>
> This looks good to me but it needs a rebase.
>
> I tested with the patch applied on top of
> commit 4123c2123a0aba7752d881dbd220437407d52698
> and everything seems ok.
>
> I can do another sanity check once the patch is rebased.
>
> Acked-by: Alin Gabriel Serdean mailto:aserd...@ovn.org>>
Thanks. I rebased and posted v2:
https://patchwork.ozlabs.org/patch/1010564/
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tests: keep some datapath parsing tests from hanging
On 10.12.2018 20:25, Ben Pfaff wrote: > On Fri, Dec 07, 2018 at 06:15:29PM +0300, Ilya Maximets wrote: >>> Whoops, looks like you dropped the list. No problem. I've re-added it. >>> >>> Scott Cheloha writes: >>> On Thu, Nov 29, 2018 at 01:15:24PM -0500, Aaron Conole wrote: > Scott Cheloha writes: > >> On Thu, Nov 29, 2018 at 11:11:31AM -0500, Aaron Conole wrote: >>> Hi Scott, >>> >>> Scott Cheloha writes: >>> The arguments to sed(1) need to be on the same line in the shell script or it will just sit there awaiting input. Signed-off-by: Scott Cheloha --- This is my first submission so I'm not sure if I'm doing this correctly. tests/odp.at | 12 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/tests/odp.at b/tests/odp.at index 1cff727ae..05dc76dbf 100644 --- a/tests/odp.at +++ b/tests/odp.at @@ -83,13 +83,11 @@ sed 's/^/skb_priority(0),skb_mark(0),recirc_id(0),dp_hash(0),/' odp-base.txt | s echo echo '# Valid forms with tunnel and ERSPAN v1 headers.' - sed - 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=1,idx=0x7),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/' odp-base.txt + sed 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=1,idx=0x7),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/' odp-base.txt echo echo '# Valid forms with tunnel and ERSPAN v2 headers.' - sed - 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=2,dir=0x1,hwid=0x7),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/' odp-base.txt + sed 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=2,dir=0x1,hwid=0x7),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/' odp-base.txt ) > odp-in.txt AT_CAPTURE_FILE([odp-in.txt]) @@ -186,13 +184,11 @@ sed -n 's/,frag=no),.*/,frag=later)/p' odp-base.txt echo echo '# Valid forms with tunnel and ERSPAN v1 headers.' - sed - 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=1/0,idx=0x7/0xf),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/' odp-base.txt + sed 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=1/0,idx=0x7/0xf),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/' odp-base.txt echo echo '# Valid forms with tunnel and ERSPAN v2 headers.' - sed - 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=2,dir=0x1,hwid=0x7/0xf),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/' odp-base.txt + sed 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=2,dir=0x1,hwid=0x7/0xf),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/' odp-base.txt ) > odp.txt AT_CAPTURE_FILE([odp.txt]) AT_CHECK_UNQUOTED([ovstest test-odp parse-wc-keys < odp.txt], [0], [`cat odp.txt` >>> >>> I see the following error on a RHEL7 system: >>> >>>485: OVS datapath key parsing and formatting - valid forms FAILED >>> (odp.at:107) >>> >>> Let me know if you want a zip of the testsuite directory. I can make it >>> available. >> >> I'm getting nearly the same error here on OpenBSD 6.4: >> >> 485: OVS datapath key parsing and formatting - valid forms FAILED >> (odp.at:108) >> >> But before digging into the meat of the test itself, can you try running >> that >> test without my patch? When I invoke sed(1) without arguments on >> RHEL7.6 it >> exits with status 4 and prints a usage statement, which makes me think >> that >> the test is failing/broken without this patch. (The fact that the test >> hangs >> here with OpenBSD's sed seems to be a historical difference in behavior >> between >> the two implementations.) > > I looked at the last few builds on the build server, and this series is > the only one that fails this test suite. > > Thanks! Sure thing. So, uh, what do I need to do next? >>> >>> Can you reproduce this error locally? Meaning, if you run a 'make >>> distcheck' do you see it? Can you try on CentOS7 or using a RHEL7 >>> Developer license install? Maybe
Re: [ovs-dev] [PATCH 6/8] datapath: fix return type of ndo_start_xmit function
On Wed, Dec 5, 2018 at 10:56 AM Greg Rose wrote: > > From: YueHaibing > > Upstream commit: > commit eddf11e18dff0e8671e06ce54e64cfc843303ab9 > Author: YueHaibing > Date: Wed Sep 26 17:15:38 2018 +0800 > > net: ovs: fix return type of ndo_start_xmit function > > The method ndo_start_xmit() is defined as returning an 'netdev_tx_t', > which is a typedef for an enum type, so make sure the implementation in > this driver has returns 'netdev_tx_t' value, and change the function > return type to netdev_tx_t. > > Found by coccinelle. > > Signed-off-by: YueHaibing > Signed-off-by: David S. Miller > > CC: YueHaibing > Signed-off-by: Greg Rose > --- LGTM. Acked-by: William Tu ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 4/8] datapath: check tunnel option type in tunnel flags
On Wed, Dec 5, 2018 at 10:56 AM Greg Rose wrote: > > From: Pieter Jansen van Vuuren > > Upstream commit: > commit 256c87c17c53e60882a43dcf3e98f3bf859eaf6f > Author: Pieter Jansen van Vuuren > Date: Tue Jun 26 21:39:36 2018 -0700 > > net: check tunnel option type in tunnel flags > > Check the tunnel option type stored in tunnel flags when creating options > for tunnels. Thereby ensuring we do not set geneve, vxlan or erspan tunnel > options on interfaces that are not associated with them. > > Make sure all users of the infrastructure set correct flags, for the BPF > helper we have to set all bits to keep backward compatibility. > > Signed-off-by: Pieter Jansen van Vuuren > > Signed-off-by: Jakub Kicinski > Signed-off-by: David S. Miller > > CC: Pieter Jansen van Vuuren > Signed-off-by: Greg Rose > --- LGTM. Acked-by: William Tu ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/8] datapath: kzalloc() -> kcalloc()
On Wed, Dec 5, 2018 at 10:56 AM Greg Rose wrote: > > From: Kees Cook > > Upstream commit: > commit 6396bb221514d2876fd6dc0aa2a1f240d99b37bb > Author: Kees Cook > Date: Tue Jun 12 14:03:40 2018 -0700 > > treewide: kzalloc() -> kcalloc() > > The kzalloc() function has a 2-factor argument form, kcalloc(). This > patch replaces cases of: > > kzalloc(a * b, gfp) > > with: > kcalloc(a * b, gfp) > > as well as handling cases of: > > kzalloc(a * b * c, gfp) > > with: > > kzalloc(array3_size(a, b, c), gfp) > > as it's slightly less ugly than: > > kzalloc_array(array_size(a, b), c, gfp) > > This does, however, attempt to ignore constant size factors like: > > kzalloc(4 * 1024, gfp) > > though any constants defined via macros get caught up in the conversion. > > Any factors with a sizeof() of "unsigned char", "char", and "u8" were > dropped, since they're redundant. > > The Coccinelle script used for this was: > > // Fix redundant parens around sizeof(). > @@ > type TYPE; > expression THING, E; > @@ > > ( > kzalloc( > - (sizeof(TYPE)) * E > + sizeof(TYPE) * E > , ...) > | > kzalloc( > - (sizeof(THING)) * E > + sizeof(THING) * E > , ...) > ) > > // Drop single-byte sizes and redundant parens. > @@ > expression COUNT; > typedef u8; > typedef __u8; > @@ > > ( > kzalloc( > - sizeof(u8) * (COUNT) > + COUNT > , ...) > | > kzalloc( > - sizeof(__u8) * (COUNT) > + COUNT > , ...) > | > kzalloc( > - sizeof(char) * (COUNT) > + COUNT > , ...) > | > kzalloc( > - sizeof(unsigned char) * (COUNT) > + COUNT > , ...) > | > kzalloc( > - sizeof(u8) * COUNT > + COUNT > , ...) > | > kzalloc( > - sizeof(__u8) * COUNT > + COUNT > , ...) > | > kzalloc( > - sizeof(char) * COUNT > + COUNT > , ...) > | > kzalloc( > - sizeof(unsigned char) * COUNT > + COUNT > , ...) > ) > > // 2-factor product with sizeof(type/expression) and identifier or > constant. > @@ > type TYPE; > expression THING; > identifier COUNT_ID; > constant COUNT_CONST; > @@ > > ( > - kzalloc > + kcalloc > ( > - sizeof(TYPE) * (COUNT_ID) > + COUNT_ID, sizeof(TYPE) > , ...) > | > - kzalloc > + kcalloc > ( > - sizeof(TYPE) * COUNT_ID > + COUNT_ID, sizeof(TYPE) > , ...) > | > - kzalloc > + kcalloc > ( > - sizeof(TYPE) * (COUNT_CONST) > + COUNT_CONST, sizeof(TYPE) > , ...) > | > - kzalloc > + kcalloc > ( > - sizeof(TYPE) * COUNT_CONST > + COUNT_CONST, sizeof(TYPE) > , ...) > | > - kzalloc > + kcalloc > ( > - sizeof(THING) * (COUNT_ID) > + COUNT_ID, sizeof(THING) > , ...) > | > - kzalloc > + kcalloc > ( > - sizeof(THING) * COUNT_ID > + COUNT_ID, sizeof(THING) > , ...) > | > - kzalloc > + kcalloc > ( > - sizeof(THING) * (COUNT_CONST) > + COUNT_CONST, sizeof(THING) > , ...) > | > - kzalloc > + kcalloc > ( > - sizeof(THING) * COUNT_CONST > + COUNT_CONST, sizeof(THING) > , ...) > ) > > // 2-factor product, only identifiers. > @@ > identifier SIZE, COUNT; > @@ > > - kzalloc > + kcalloc > ( > - SIZE * COUNT > + COUNT, SIZE > , ...) > > // 3-factor product with 1 sizeof(type) or sizeof(expression), with > // redundant parens removed. > @@ > expression THING; > identifier STRIDE, COUNT; > type TYPE; > @@ > > ( > kzalloc( > - sizeof(TYPE) * (COUNT) * (STRIDE) > + array3_size(COUNT, STRIDE, sizeof(TYPE)) > , ...) > | > kzalloc( > - sizeof(TYPE) * (COUNT) * STRIDE > + array3_size(COUNT, STRIDE, sizeof(TYPE)) > , ...) > | > kzalloc( > - sizeof(TYPE) * COUNT * (STRIDE) > + array3_size(COUNT, STRIDE, sizeof(TYPE)) > , ...) > | > kzalloc( > - sizeof(TYPE) * COUNT * STRIDE > + array3_size(COUNT, STRIDE, sizeof(TYPE)) > , ...) > | > kzalloc( > - sizeof(THING) * (COUNT) * (STRIDE) > + array3_size(COUNT, STRIDE, sizeof(THING)) > , ...) > | > kzalloc( > - sizeof(THING) * (COUNT) * STRIDE > + array3_size(COUNT, STRIDE, sizeof(THING)) > , ...) > | > kzalloc( > - sizeof(THING) * COUNT * (STRIDE) > + array3_size(COUNT, STRIDE, sizeof(THING)) > , ...) > | > kzalloc( > - sizeof(THING) * COUNT * STRIDE > + array3_size(COUNT, STRIDE, sizeof(THING)) >
Re: [ovs-dev] [PATCH 1/4] configure.ac: More enhanced check for pthread library.
On Mon, Dec 10, 2018 at 08:05:20PM +0300, Ilya Maximets wrote: > FreeBSD 12 supports 'pthread_rwlock_tryrdlock' without 'pthread' > library. Let's add check for more rare function. > OTOH, Travis-CI environment supports 'pthread_rwlockattr_destroy', > but does not support 'pthread_rwlock_tryrdlock' without 'pthread'. > So, both checks needed. > > Signed-off-by: Ilya Maximets Applied to master, thanks! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/4] tests: Remove redzone flag for FreeBSD 12+.
On Mon, Dec 10, 2018 at 08:05:21PM +0300, Ilya Maximets wrote: > 'redzone' not supported in new versions of jemalloc > (since jemalloc 5.0.0). > > Signed-off-by: Ilya Maximets Applied to master, thanks! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] treewide: Wider use of packet batch APIs.
On Mon, Dec 10, 2018 at 08:17:53PM +0300, Ilya Maximets wrote: > This patch replaces most of direct accesses to the dp_packet_batch > internal components by appropriate APIs. > > Signed-off-by: Ilya Maximets I looked over this one and didn't see a problem, but, Ian, I'll leave it to you because you are probably more familiar with packet batching. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] tests: Fix syntax in another ODP test.
Reported-by: Ilya Maximets Signed-off-by: Ben Pfaff --- tests/odp.at | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/odp.at b/tests/odp.at index 178877dc811f..96b4b47dc45a 100644 --- a/tests/odp.at +++ b/tests/odp.at @@ -188,7 +188,7 @@ sed -n 's/,frag=no),.*/,frag=later)/p' odp-base.txt echo echo '# Valid forms with tunnel and ERSPAN v2 headers.' - sed 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=2,dir=0x1,hwid=0x7/0xf),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/' odp-base.txt + sed 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=2,dir=1,hwid=0x7/0xf),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/' odp-base.txt ) > odp.txt AT_CAPTURE_FILE([odp.txt]) AT_CHECK_UNQUOTED([ovstest test-odp parse-wc-keys < odp.txt], [0], [`cat odp.txt` -- 2.16.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] tests: Simplify and improve the daemon tests.
Obviously this version is screwed up because I accidentally squashed two unrelated patches. I'll post v3. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3] tests: Simplify and improve the daemon tests.
The daemon tests used files a lot when shell variables were easier to use
and easier to understand. This commit changes that.
The tests created empty databases that aren't really needed anymore. This
commit changes them to use the ovsdb-server --no-db option instead.
The tests had a lot of common code for checking the ancestry of processes.
This commit factors out a new shell function check_ancestors.
The tests tended to use random pidfile names. This switches to just using
the defaults, which are fine.
The tests didn't check the names of the child processes. This adds those
checks using the new check_process_name shell function. This should avoid
regression of the bug fixed by commit 266f79e32c60 ("daemon-unix: Use
same name for original or restarted children.")
Other minor improvements too.
I only made small updates to the Windows-specific test, because it is hard
for me to verify.
Acked-by: Alin Gabriel Serdean
Signed-off-by: Ben Pfaff
---
v1->v2: Rebased, added ack.
v2->v3: Fix some mixups in v2 from muddling two patches together.
tests/daemon-py.at | 218 +
tests/daemon.at| 212 ---
2 files changed, 220 insertions(+), 210 deletions(-)
diff --git a/tests/daemon-py.at b/tests/daemon-py.at
index b7236b1f3622..6adea3c85dfc 100644
--- a/tests/daemon-py.at
+++ b/tests/daemon-py.at
@@ -6,20 +6,23 @@ m4_define([DAEMON_PYN],
# Skip this test for Windows, echo $! gives shell pid instead of parent
process
AT_SKIP_IF([test "$IS_WIN32" = "yes"])
AT_KEYWORDS([python daemon])
- AT_CAPTURE_FILE([pid])
- AT_CAPTURE_FILE([expected])
+
+ on_exit 'kill $(cat *.pid)'
+ pidfile=test-daemon.py.pid
+
# Start the daemon and wait for the pidfile to get created
# and that its contents are the correct pid.
- AT_CHECK([$3 $srcdir/test-daemon.py --pidfile=pid& echo $! > expected], [0])
- OVS_WAIT_UNTIL([test -s pid], [kill `cat expected`])
- AT_CHECK(
- [pid=`cat pid` && expected=`cat expected` && test "$pid" = "$expected"],
- [0], [], [], [kill `cat expected`])
- AT_CHECK([kill -0 `cat pid`], [0], [], [], [kill `cat expected`])
+ AT_CHECK([$3 $srcdir/test-daemon.py --pidfile & echo $!], [0], [stdout])
+ pid=$(cat stdout)
+
+ OVS_WAIT_UNTIL([test -s $pidfile], [kill $pid])
+ AT_CHECK([test $pid = $(cat $pidfile)])
+ AT_CHECK([kill -0 $pid])
+
# Kill the daemon and make sure that the pidfile gets deleted.
- kill `cat expected`
- OVS_WAIT_WHILE([kill -0 `cat expected`])
- AT_CHECK([test ! -e pid])
+ kill $pid
+ OVS_WAIT_WHILE([kill -0 $pid])
+ AT_CHECK([test ! -e $pidfile])
AT_CLEANUP])
DAEMON_PYN([Python2], [$HAVE_PYTHON2], [$PYTHON2])
@@ -28,42 +31,38 @@ DAEMON_PYN([Python3], [$HAVE_PYTHON3], [$PYTHON3])
m4_define([DAEMON_MONITOR_PYN],
[AT_SETUP([daemon --monitor - $1])
AT_SKIP_IF([test $2 = no])
+
# Skip this test for Windows, echo $! gives shell pid instead of parent
process
AT_SKIP_IF([test "$IS_WIN32" = "yes"])
- AT_CAPTURE_FILE([pid])
- AT_CAPTURE_FILE([parent])
- AT_CAPTURE_FILE([parentpid])
- AT_CAPTURE_FILE([newpid])
+
+ on_exit 'kill $(cat *.pid)'
+ pidfile=test-daemon.py.pid
+
# Start the daemon and wait for the pidfile to get created.
- AT_CHECK([$3 $srcdir/test-daemon.py --pidfile=pid --monitor& echo $! >
parent], [0])
- on_exit 'kill `cat parent`'
- OVS_WAIT_UNTIL([test -s pid])
+ AT_CHECK([$3 $srcdir/test-daemon.py --pidfile --monitor & echo $!], [0],
[stdout])
+ monitor=$(cat stdout)
+ OVS_WAIT_UNTIL([test -s $pidfile])
+ child=$(cat $pidfile)
+
# Check that the pidfile names a running process,
# and that the parent process of that process is our child process.
- AT_CHECK([kill -0 `cat pid`])
- AT_CHECK([parent_pid `cat pid` > parentpid])
- AT_CHECK(
- [parentpid=`cat parentpid` &&
- parent=`cat parent` &&
- test $parentpid = $parent])
+ check_ancestors $child $monitor
+
# Kill the daemon process, making it look like a segfault,
# and wait for a new child process to get spawned.
- AT_CHECK([cp pid oldpid])
- AT_CHECK([kill -SEGV `cat pid`], [0], [], [ignore])
- OVS_WAIT_WHILE([kill -0 `cat oldpid`])
- OVS_WAIT_UNTIL([test -s pid && test `cat pid` != `cat oldpid`])
- AT_CHECK([cp pid newpid])
+ AT_CHECK([kill -SEGV $child])
+ OVS_WAIT_WHILE([kill -0 $child])
+ OVS_WAIT_UNTIL([test -s $pidfile && test $(cat $pidfile) != $child])
+ child2=$(cat $pidfile)
+
# Check that the pidfile names a running process,
# and that the parent process of that process is our child process.
- AT_CHECK([parent_pid `cat pid` > parentpid])
- AT_CHECK(
- [parentpid=`cat parentpid` &&
- parent=`cat parent` &&
- test $parentpid = $parent])
+ check_ancestors $child2 $monitor
+
# Kill the daemon process with SIGTERM, and wait for the daemon
# and the monitor processes to go away and the pidfile to get deleted.
Re: [ovs-dev] [PATCH] tests: keep some datapath parsing tests from hanging
On Fri, Dec 07, 2018 at 06:15:29PM +0300, Ilya Maximets wrote: > > Whoops, looks like you dropped the list. No problem. I've re-added it. > > > > Scott Cheloha writes: > > > >> On Thu, Nov 29, 2018 at 01:15:24PM -0500, Aaron Conole wrote: > >>> Scott Cheloha writes: > >>> > >>> > On Thu, Nov 29, 2018 at 11:11:31AM -0500, Aaron Conole wrote: > >>> >> Hi Scott, > >>> >> > >>> >> Scott Cheloha writes: > >>> >> > >>> >> > The arguments to sed(1) need to be on the same line in the shell > >>> >> > script or it will just sit there awaiting input. > >>> >> > > >>> >> > Signed-off-by: Scott Cheloha > >>> >> > --- > >>> >> > > >>> >> > This is my first submission so I'm not sure if I'm doing this > >>> >> > correctly. > >>> >> > > >>> >> > tests/odp.at | 12 > >>> >> > 1 file changed, 4 insertions(+), 8 deletions(-) > >>> >> > > >>> >> > diff --git a/tests/odp.at b/tests/odp.at > >>> >> > index 1cff727ae..05dc76dbf 100644 > >>> >> > --- a/tests/odp.at > >>> >> > +++ b/tests/odp.at > >>> >> > @@ -83,13 +83,11 @@ sed > >>> >> > 's/^/skb_priority(0),skb_mark(0),recirc_id(0),dp_hash(0),/' > >>> >> > odp-base.txt | s > >>> >> > > >>> >> > echo > >>> >> > echo '# Valid forms with tunnel and ERSPAN v1 headers.' > >>> >> > - sed > >>> >> > - > >>> >> > 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=1,idx=0x7),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/' > >>> >> > odp-base.txt > >>> >> > + sed > >>> >> > 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=1,idx=0x7),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/' > >>> >> > odp-base.txt > >>> >> > > >>> >> > echo > >>> >> > echo '# Valid forms with tunnel and ERSPAN v2 headers.' > >>> >> > - sed > >>> >> > - > >>> >> > 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=2,dir=0x1,hwid=0x7),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/' > >>> >> > odp-base.txt > >>> >> > + sed > >>> >> > 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=2,dir=0x1,hwid=0x7),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/' > >>> >> > odp-base.txt > >>> >> > ) > odp-in.txt > >>> >> > AT_CAPTURE_FILE([odp-in.txt]) > >>> >> > > >>> >> > @@ -186,13 +184,11 @@ sed -n 's/,frag=no),.*/,frag=later)/p' > >>> >> > odp-base.txt > >>> >> > > >>> >> > echo > >>> >> > echo '# Valid forms with tunnel and ERSPAN v1 headers.' > >>> >> > - sed > >>> >> > - > >>> >> > 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=1/0,idx=0x7/0xf),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/' > >>> >> > odp-base.txt > >>> >> > + sed > >>> >> > 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=1/0,idx=0x7/0xf),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/' > >>> >> > odp-base.txt > >>> >> > > >>> >> > echo > >>> >> > echo '# Valid forms with tunnel and ERSPAN v2 headers.' > >>> >> > - sed > >>> >> > - > >>> >> > 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=2,dir=0x1,hwid=0x7/0xf),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/' > >>> >> > odp-base.txt > >>> >> > + sed > >>> >> > 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=2,dir=0x1,hwid=0x7/0xf),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/' > >>> >> > odp-base.txt > >>> >> > ) > odp.txt > >>> >> > AT_CAPTURE_FILE([odp.txt]) > >>> >> > AT_CHECK_UNQUOTED([ovstest test-odp parse-wc-keys < odp.txt], [0], > >>> >> > [`cat odp.txt` > >>> >> > >>> >> I see the following error on a RHEL7 system: > >>> >> > >>> >>485: OVS datapath key parsing and formatting - valid forms FAILED > >>> >> (odp.at:107) > >>> >> > >>> >> Let me know if you want a zip of the testsuite directory. I can make > >>> >> it > >>> >> available. > >>> > > >>> > I'm getting nearly the same error here on OpenBSD 6.4: > >>> > > >>> > 485: OVS datapath key parsing and formatting - valid forms FAILED > >>> > (odp.at:108) > >>> > > >>> > But before digging into the meat of the test itself, can you try > >>> > running that > >>> > test without my patch? When I invoke sed(1) without arguments on > >>> > RHEL7.6 it > >>> > exits with status 4 and prints a usage statement, which makes me think > >>> > that > >>> > the test is failing/broken without this patch. (The fact that the test > >>> > hangs > >>> > here with OpenBSD's sed seems to be a historical difference in behavior > >>> > between > >>> > the two implementations.) > >>> > >>> I looked at the last few builds on the build server, and this series is > >>> the only one that fails this test suite. > >>> > >>> Thanks! > >> > >> Sure thing. > >> > >> So, uh, what do I need to do next? > > > > Can you reproduce this error
[ovs-dev] [PATCH] treewide: Wider use of packet batch APIs.
This patch replaces most of direct accesses to the dp_packet_batch
internal components by appropriate APIs.
Signed-off-by: Ilya Maximets
---
lib/dpif.c | 2 +-
lib/netdev-bsd.c | 11 +--
lib/netdev-dummy.c | 5 ++---
lib/odp-execute.c | 2 +-
tests/test-conntrack.c | 4 ++--
5 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/lib/dpif.c b/lib/dpif.c
index 59aa1dc7c..e35f11147 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1166,7 +1166,7 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch
*packets_,
int type = nl_attr_type(action);
struct dp_packet *packet = packets_->packets[0];
-ovs_assert(packets_->count == 1);
+ovs_assert(dp_packet_batch_size(packets_) == 1);
switch ((enum ovs_action_attr)type) {
case OVS_ACTION_ATTR_METER:
diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index 99d4f4842..46698d547 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -640,8 +640,7 @@ netdev_bsd_rxq_recv(struct netdev_rxq *rxq_, struct
dp_packet_batch *batch,
if (retval) {
dp_packet_delete(packet);
} else {
-batch->packets[0] = packet;
-batch->count = 1;
+dp_packet_batch_init_packet(batch, packet);
}
if (qfill) {
@@ -690,8 +689,8 @@ netdev_bsd_send(struct netdev *netdev_, int qid OVS_UNUSED,
{
struct netdev_bsd *dev = netdev_bsd_cast(netdev_);
const char *name = netdev_get_name(netdev_);
+struct dp_packet *packet;
int error;
-int i;
ovs_mutex_lock(>mutex);
if (dev->tap_fd < 0 && !dev->pcap) {
@@ -700,9 +699,9 @@ netdev_bsd_send(struct netdev *netdev_, int qid OVS_UNUSED,
error = 0;
}
-for (i = 0; i < batch->count; i++) {
-const void *data = dp_packet_data(batch->packets[i]);
-size_t size = dp_packet_size(batch->packets[i]);
+DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
+const void *data = dp_packet_data(packet);
+size_t size = dp_packet_size(packet);
while (!error) {
ssize_t retval;
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 23817d121..72b4f7adc 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -1036,8 +1036,7 @@ netdev_dummy_rxq_recv(struct netdev_rxq *rxq_, struct
dp_packet_batch *batch,
netdev->custom_stats[1].value++;
ovs_mutex_unlock(>mutex);
-batch->packets[0] = packet;
-batch->count = 1;
+dp_packet_batch_init_packet(batch, packet);
if (qfill) {
*qfill = -ENOTSUP;
@@ -1091,7 +1090,7 @@ netdev_dummy_send(struct netdev *netdev, int qid
OVS_UNUSED,
const void *buffer = dp_packet_data(packet);
size_t size = dp_packet_size(packet);
-if (batch->packets[i]->packet_type != htonl(PT_ETH)) {
+if (packet->packet_type != htonl(PT_ETH)) {
error = EPFNOSUPPORT;
break;
}
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 5831d1fdb..3b6890e95 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -717,7 +717,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch
*batch, bool steal,
dp_execute_action(dp, batch, a, should_steal);
-if (last_action || batch->count == 0) {
+if (last_action || dp_packet_batch_is_empty(batch)) {
/* We do not need to free the packets.
* Either dp_execute_actions() has stolen them
* or the batch is freed due to errors. In either
diff --git a/tests/test-conntrack.c b/tests/test-conntrack.c
index 24d0bb442..07a4857cf 100644
--- a/tests/test-conntrack.c
+++ b/tests/test-conntrack.c
@@ -177,7 +177,7 @@ pcap_batch_execute_conntrack(struct conntrack *ct_,
NULL, NULL, 0, 0, NULL, NULL, now);
dp_packet_batch_init(_batch);
}
-new_batch.packets[new_batch.count++] = packet;;
+dp_packet_batch_add(_batch, packet);
}
if (!dp_packet_batch_is_empty(_batch)) {
@@ -226,7 +226,7 @@ test_pcap(struct ovs_cmdl_context *ctx)
}
dp_packet_batch_add(batch, packet);
}
-if (!batch->count) {
+if (dp_packet_batch_is_empty(batch)) {
break;
}
pcap_batch_execute_conntrack(, batch);
--
2.17.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 4/4] ovs-thread: Add thread safety annotation to cond_wait.
This fixes build with clang on FreeBSD:
lib/ovs-thread.c:266:13: error:
calling function 'pthread_cond_wait' requires holding mutex \
'mutex->lock' exclusively [-Werror,-Wthread-safety-analysis]
error = pthread_cond_wait(cond, >lock);
^
Fixes: 97be153858b4 ("clang: Add annotations for thread safety check.")
Signed-off-by: Ilya Maximets
---
include/openvswitch/thread.h | 3 ++-
lib/ovs-thread.c | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/openvswitch/thread.h b/include/openvswitch/thread.h
index 5ffc29067..2987db37c 100644
--- a/include/openvswitch/thread.h
+++ b/include/openvswitch/thread.h
@@ -68,7 +68,8 @@ int ovs_mutex_trylock_at(const struct ovs_mutex *mutex, const
char *where)
#define ovs_mutex_trylock(mutex) \
ovs_mutex_trylock_at(mutex, OVS_SOURCE_LOCATOR)
-void ovs_mutex_cond_wait(pthread_cond_t *, const struct ovs_mutex *);
+void ovs_mutex_cond_wait(pthread_cond_t *, const struct ovs_mutex *mutex)
+OVS_REQUIRES(mutex);
/* Convenient once-only execution.
*
diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
index b5f7424b7..c8d92bc1b 100644
--- a/lib/ovs-thread.c
+++ b/lib/ovs-thread.c
@@ -257,6 +257,7 @@ ovs_rwlock_init(const struct ovs_rwlock *l_)
* call with calls to ovsrcu_quiesce_start() and ovsrcu_quiesce_end(). */
void
ovs_mutex_cond_wait(pthread_cond_t *cond, const struct ovs_mutex *mutex_)
+OVS_NO_THREAD_SAFETY_ANALYSIS
{
struct ovs_mutex *mutex = CONST_CAST(struct ovs_mutex *, mutex_);
int error;
--
2.17.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 3/4] ovs-thread: Drop xpthread_meutex_{un}lock finctions.
There are no users of these functions.
This change fixes clang build on FreeBSD:
lib/ovs-thread.c:158:1: error: \
mutex 'mutex' is still held at the end of function \
[-Werror,-Wthread-safety-analysis]
XPTHREAD_FUNC1(pthread_mutex_lock, pthread_mutex_t *);
^
lib/ovs-thread.c:138:5: note: expanded from macro 'XPTHREAD_FUNC1'
}
^
Fixes: 4dff0893c376 ("ovs-atomic-pthreads: Use global shared locks for
atomic_flag also.")
Signed-off-by: Ilya Maximets
---
lib/ovs-thread.c | 2 --
lib/ovs-thread.h | 5 -
2 files changed, 7 deletions(-)
diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
index c72bc543b..b5f7424b7 100644
--- a/lib/ovs-thread.c
+++ b/lib/ovs-thread.c
@@ -155,8 +155,6 @@ UNLOCK_FUNCTION(rwlock, destroy, NULL);
} \
}
-XPTHREAD_FUNC1(pthread_mutex_lock, pthread_mutex_t *);
-XPTHREAD_FUNC1(pthread_mutex_unlock, pthread_mutex_t *);
XPTHREAD_FUNC1(pthread_mutexattr_init, pthread_mutexattr_t *);
XPTHREAD_FUNC1(pthread_mutexattr_destroy, pthread_mutexattr_t *);
XPTHREAD_FUNC2(pthread_mutexattr_settype, pthread_mutexattr_t *, int);
diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
index 0f9663324..1050fc29a 100644
--- a/lib/ovs-thread.h
+++ b/lib/ovs-thread.h
@@ -33,11 +33,6 @@ struct ovs_barrier {
struct seq *seq;
};
-/* Wrappers for pthread_mutex_*() that abort the process on any error.
- * This is still needed when ovs-atomic-pthreads.h is used. */
-void xpthread_mutex_lock(pthread_mutex_t *mutex);
-void xpthread_mutex_unlock(pthread_mutex_t *mutex);
-
/* Wrappers for pthread_mutexattr_*() that abort the process on any error. */
void xpthread_mutexattr_init(pthread_mutexattr_t *);
void xpthread_mutexattr_destroy(pthread_mutexattr_t *);
--
2.17.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 2/4] tests: Remove redzone flag for FreeBSD 12+.
'redzone' not supported in new versions of jemalloc (since jemalloc 5.0.0). Signed-off-by: Ilya Maximets --- tests/atlocal.in | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/atlocal.in b/tests/atlocal.in index e186fc555..388f79ed8 100644 --- a/tests/atlocal.in +++ b/tests/atlocal.in @@ -78,9 +78,12 @@ FreeBSD) [789].*) MALLOC_CONF=AJ ;; -*) +1[01].*) MALLOC_CONF=abort:true,junk:true,redzone:true ;; +*) +MALLOC_CONF=abort:true,junk:true +;; esac export MALLOC_CONF esac -- 2.17.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 1/4] configure.ac: More enhanced check for pthread library.
FreeBSD 12 supports 'pthread_rwlock_tryrdlock' without 'pthread' library. Let's add check for more rare function. OTOH, Travis-CI environment supports 'pthread_rwlockattr_destroy', but does not support 'pthread_rwlock_tryrdlock' without 'pthread'. So, both checks needed. Signed-off-by: Ilya Maximets --- configure.ac | 1 + 1 file changed, 1 insertion(+) diff --git a/configure.ac b/configure.ac index 3e97a750c..71ca90782 100644 --- a/configure.ac +++ b/configure.ac @@ -78,6 +78,7 @@ AC_SEARCH_LIBS([pow], [m]) AC_SEARCH_LIBS([clock_gettime], [rt]) AC_SEARCH_LIBS([timer_create], [rt]) AC_SEARCH_LIBS([pthread_rwlock_tryrdlock], [pthread]) +AC_SEARCH_LIBS([pthread_rwlockattr_destroy], [pthread]) AC_FUNC_STRERROR_R OVS_CHECK_ESX -- 2.17.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 0/4] Various build/test fixes.
Found while trying to build on FreeBSD-12-rc3.
Ilya Maximets (4):
configure.ac: More enhanced check for pthread library.
tests: Remove redzone flag for FreeBSD 12+.
ovs-thread: Drop xpthread_meutex_{un}lock finctions.
ovs-thread: Add thread safety annotation to cond_wait.
configure.ac | 1 +
include/openvswitch/thread.h | 3 ++-
lib/ovs-thread.c | 3 +--
lib/ovs-thread.h | 5 -
tests/atlocal.in | 5 -
5 files changed, 8 insertions(+), 9 deletions(-)
--
2.17.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] [PATCH] rcu: reduce RCU block time
I mean that the seq_wait() call should cause ovsrcu_synchronize() to wake up immediately when it can return. If it doesn't, something is wrong. On Mon, Dec 10, 2018 at 07:30:09AM +, Lilijun (Jerry, Cloud Networking) wrote: > Hi Ben, > > Do you mean that there some threads in in ovsrcu_threads list were wrongly > using seq_wait or rcu functions? > > Thanks, I will do some more research on this problem. > > > -Original Message- > > From: Ben Pfaff [mailto:b...@ovn.org] > > Sent: Monday, December 10, 2018 10:08 AM > > To: Lilijun (Jerry, Cloud Networking) > > Cc: d...@openvswitch.org > > Subject: Re: [ovs-dev] [PATCH] [PATCH] rcu: reduce RCU block time > > > > If that's the case, then there's something wrong with seq_wait() or the use > > of it in this context. The timer logic here is just to issue log messages. > > > > It would be better to figure out the real problem, because that would reduce > > the 100 ms (or 1000 ms) latency to near-zero. > > > > On Mon, Dec 10, 2018 at 01:50:41AM +, Lilijun (Jerry, Cloud Networking) > > wrote: > > > Hi, Ben > > > > > > Thanks for your reply. > > > > > > Do you mean that the function poll_timer_wait_until(start + > > warning_threshold) call only wakeup to do some need log messages and > > ovsrcu_synchronize() will wait for seq_wait()? > > > > > > According to my understanding, ovsrcu_synchronize() will return until > > every thread in ovsrcu_threads list have change their seqno to target_seqno. > > When some thread call ovsrcu_quiesce(), global_seqno is changed. Then > > seq_wait() will check if global_seqno is changed and call > > poll_immediate_wake_at() to finish the poll_block()'s waiting. > > > > > > We assume that the first call of seq_wait() in this loop check > > > global_seqno > > failed for other thread hasn't been ready to call ovsrcu_quiesce(). Then the > > loop will sleep 1000ms(set by warning_threshold) by poll_block() and check > > the thread's seqno or global_seqno again when no other threads finish the > > poll_block()'s waiting in early. > > > > > > If we set warning_threshold to 100ms, the look will check the seqno more > > frequently and may break earlier. I have test it myself, when > > warning_threshold is set to 100 as default, the loop 's time indeed reduce > > to > > 100ms, that's maybe some evidence. > > > > > > > > > > > > > -Original Message- > > > > From: Ben Pfaff [mailto:b...@ovn.org] > > > > Sent: Friday, December 07, 2018 10:37 PM > > > > To: Lilijun (Jerry, Cloud Networking) > > > > Cc: d...@openvswitch.org > > > > Subject: Re: [ovs-dev] [PATCH] [PATCH] rcu: reduce RCU block time > > > > > > > > On Fri, Dec 07, 2018 at 02:03:27PM +0800, Lilijun wrote: > > > > > When calling ovsrcu_synchronize(), it always block 1000ms because > > > > > we poll block until elapsed time become greater than > > > > > warning_threshold(1000).That's too long for some configuration > > commands. > > > > > So this patch reduces warning_threshold's default value to 100 and > > > > > print logs after it have elapsed more than 1000ms. > > > > > > > > > > Signed-off-by: Lilijun > > > > > > > > This only makes any sense if the seq_wait() doesn't work. Do you > > > > have evidence that is the case? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
