Message ID | 1494414906-71269-1-git-send-email-nic@opencloud.tech |
---|---|
State | Changes Requested |
Headers | show |
On Wed, May 10, 2017 at 04:15:02AM -0700, nickcooper-zhangtonghao wrote: > * This patch will be used for next patch. The 'rstp/show' command, > which uses the mutex, calls functions which also use the mutex. > We should init it as a recursive mutex. > > * Some rstp tests of OvS, which run with ovstest, does not run rstp_init > in the bridge_init. We should call rstp_init when creating the rstp > and stp also do that, otherwise tests fail. > > * This patch remove the rstp_mutex in header file and make it static > in c file because we only use it in lib/rstp.c > > Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech> Thanks for working on the rstp code! This patch causes a huge number of build failures with Clang, like this: In file included from ../lib/rstp.c:32: ../lib/rstp.h:135:18: error: use of undeclared identifier 'rstp_mutex' ../include/openvswitch/compiler.h:136:57: note: expanded from macro 'OVS_EXCLUDED' I think that's the only reason that rstp_mutex is global.
> On May 19, 2017, at 6:47 AM, Ben Pfaff <blp@ovn.org> wrote: > > On Wed, May 10, 2017 at 04:15:02AM -0700, nickcooper-zhangtonghao wrote: >> * This patch will be used for next patch. The 'rstp/show' command, >> which uses the mutex, calls functions which also use the mutex. >> We should init it as a recursive mutex. >> >> * Some rstp tests of OvS, which run with ovstest, does not run rstp_init >> in the bridge_init. We should call rstp_init when creating the rstp >> and stp also do that, otherwise tests fail. >> >> * This patch remove the rstp_mutex in header file and make it static >> in c file because we only use it in lib/rstp.c >> >> Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech> > > Thanks for working on the rstp code! > > This patch causes a huge number of build failures with Clang, like this: > > In file included from ../lib/rstp.c:32: > ../lib/rstp.h:135:18: error: use of undeclared identifier 'rstp_mutex' > ../include/openvswitch/compiler.h:136:57: note: expanded from macro 'OVS_EXCLUDED' > > I think that's the only reason that rstp_mutex is global. Thanks for your review, the v3 patches don’t remove the rstp_mutex from header file and we can build it with gcc and clang. Because of recursive mutex, I remove the OVS_EXCLUDED in the lib/rstp.[ch] files. Thanks very much.
diff --git a/lib/rstp.c b/lib/rstp.c index 907a907..6f1c1e3 100644 --- a/lib/rstp.c +++ b/lib/rstp.c @@ -50,7 +50,7 @@ VLOG_DEFINE_THIS_MODULE(rstp); -struct ovs_mutex rstp_mutex = OVS_MUTEX_INITIALIZER; +static struct ovs_mutex rstp_mutex; static struct ovs_list all_rstps__ = OVS_LIST_INITIALIZER(&all_rstps__); static struct ovs_list *const all_rstps OVS_GUARDED_BY(rstp_mutex) = &all_rstps__; @@ -239,8 +239,15 @@ void rstp_init(void) OVS_EXCLUDED(rstp_mutex) { - unixctl_command_register("rstp/tcn", "[bridge]", 0, 1, rstp_unixctl_tcn, - NULL); + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; + + if (ovsthread_once_start(&once)) { + ovs_mutex_init_recursive(&rstp_mutex); + + unixctl_command_register("rstp/tcn", "[bridge]", 0, 1, rstp_unixctl_tcn, + NULL); + ovsthread_once_done(&once); + } } /* Creates and returns a new RSTP instance that initially has no ports. */ @@ -255,6 +262,8 @@ rstp_create(const char *name, rstp_identifier bridge_address, VLOG_DBG("Creating RSTP instance"); + rstp_init(); + rstp = xzalloc(sizeof *rstp); rstp->name = xstrdup(name); diff --git a/lib/rstp.h b/lib/rstp.h index 4942d59..78e07fb 100644 --- a/lib/rstp.h +++ b/lib/rstp.h @@ -36,12 +36,6 @@ #include "compiler.h" #include "util.h" -/* Thread Safety: Callers passing in RSTP and RSTP port object - * pointers must hold a reference to the passed object to ensure that - * the object does not become stale while it is being accessed. */ - -extern struct ovs_mutex rstp_mutex; - #define RSTP_MAX_PORTS 4095 struct dp_packet;
* This patch will be used for next patch. The 'rstp/show' command, which uses the mutex, calls functions which also use the mutex. We should init it as a recursive mutex. * Some rstp tests of OvS, which run with ovstest, does not run rstp_init in the bridge_init. We should call rstp_init when creating the rstp and stp also do that, otherwise tests fail. * This patch remove the rstp_mutex in header file and make it static in c file because we only use it in lib/rstp.c Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech> --- lib/rstp.c | 15 ++++++++++++--- lib/rstp.h | 6 ------ 2 files changed, 12 insertions(+), 9 deletions(-)