On Tue, Dec 11, 2018 at 6:55 PM Ben Pfaff <[email protected]> wrote: > On Tue, Dec 11, 2018 at 06:05:11PM -0800, Darrell Ball wrote: > > On Tue, Dec 11, 2018 at 8:51 AM Ben Pfaff <[email protected]> wrote: > > > > > On Mon, Dec 10, 2018 at 07:18:43PM -0800, Darrell Ball wrote: > > > > On Mon, Dec 10, 2018 at 5:22 PM Ben Pfaff <[email protected]> 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 <[email protected]> > > > > > > > > > > > > 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-if FAILED ( > > > > > 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 0x00000000 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. > > > > > > When I apply the following patch: > > > > > > diff --git a/lib/conntrack.c b/lib/conntrack.c > > > index a69026d6f32f..f9bdfb6c2aa3 100644 > > > --- a/lib/conntrack.c > > > +++ b/lib/conntrack.c > > > @@ -338,6 +338,7 @@ ct_print_conn_info(const struct conn *c, const char > > > *log_msg, > > > void > > > conntrack_init(void) > > > { > > > + VLOG_WARN("%s:%d", __FILE__, __LINE__); > > > long long now = time_msec(); > > > > > > ct_rwlock_init(&resources_lock); > > > @@ -374,6 +375,7 @@ conntrack_init(void) > > > void > > > conntrack_destroy(void) > > > { > > > + VLOG_WARN("%s:%d", __FILE__, __LINE__); > > > latch_set(&clean_thread_exit); > > > pthread_join(clean_thread, NULL); > > > latch_destroy(&clean_thread_exit); > > > > > > The test shows the following logging: > > > > > > --- /dev/null 2018-11-19 11:45:05.360009223 -0800 > > > +++ > /home/blp/nicira/ovs/_build/tests/testsuite.dir/at-groups/1099/stdout > > > 2018-12-11 08:50:04.623158278 -0800 > > > @@ -0,0 +1,3 @@ > > > +2018-12-11T16:50:04.582Z|00008|conntrack|WARN|../lib/conntrack.c:341 > > > +2018-12-11T16:50:04.596Z|00031|conntrack|WARN|../lib/conntrack.c:341 > > > +2018-12-11T16:50:04.613Z|00038|conntrack|WARN|../lib/conntrack.c:378 > > > > > > which indicates that conntrack_init() is being called more than once. > > > > > > > > > Thanks; I am glad your running of the address sanitizer flagged this at > > least. > > All the built-in testing, including Valgrind and manual testing even > with 2 > > bridges did not, providing a false sense of security. > > Missed the forest for PPS tress. > > You're welcome! (Really I got lucky ;-) >
Since we only support one datapath per datapath type, the only way to encounter a real issue here is for one instance of vswitchd to have both dummy and netdev datapath types, which is 'unlikely'. One option was to simply explicitly not support that with a simple check in the userspace datapath. However, there may be some future work where this may change, so I reinstated the multiple userspace datapaths, aka, multiple userspace datapath types support. The same holds for fragmentation support. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
