| Message ID | 1491016286-86218-4-git-send-email-nic@opencloud.tech |
|---|---|
| State | Changes Requested |
| Headers | show |
> On Mar 31, 2017, at 8:11 PM, nickcooper-zhangtonghao <nic@opencloud.tech> wrote: > > This patch will be used for next patch. I don’t see exactly what in the following patch(es) need this. Could you elaborate? > > Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech> > --- > lib/rstp.c | 15 ++++++++++++--- > lib/rstp.h | 6 ------ > 2 files changed, 12 insertions(+), 9 deletions(-) > > 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_init() is already called earlier from the bridge_init(), so I see little point calling it from here. Not having multiple call sites would also remove the need for most of the changes above. > 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; > - This change, if needed, should be in a separate patch with it’s own commit message. Jarno > #define RSTP_MAX_PORTS 4095 > > struct dp_packet; > -- > 1.8.3.1 > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> On Apr 21, 2017, at 8:59 AM, Jarno Rajahalme <jarno@ovn.org <mailto:jarno@ovn.org>> wrote: > >> >> On Mar 31, 2017, at 8:11 PM, nickcooper-zhangtonghao <nic@opencloud.tech <mailto:nic@opencloud.tech>> wrote: >> >> This patch will be used for next patch. > > I don’t see exactly what in the following patch(es) need this. Could you elaborate? The next patch 6/6 will call some functions which use the mutex and the ‘rstp/show’ use it agin. we should init it as a recursive mutex. > >> >> Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech <mailto:nic@opencloud.tech>> >> --- >> lib/rstp.c | 15 ++++++++++++--- >> lib/rstp.h | 6 ------ >> 2 files changed, 12 insertions(+), 9 deletions(-) >> >> 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_init() is already called earlier from the bridge_init(), so I see little point calling it from here. Not having multiple call sites would also remove the need for most of the changes above. Yes, but some rstp testes, which run with ovstest, will not run rstp_init in the bridge_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; >> - > > This change, if needed, should be in a separate patch with it’s own commit message. > > Jarno Yes, If other patches will be ok, I will put it to a separate patch.
> On Apr 21, 2017, at 8:59 AM, Jarno Rajahalme <jarno@ovn.org <mailto:jarno@ovn.org>> wrote: > >> >> On Mar 31, 2017, at 8:11 PM, nickcooper-zhangtonghao <nic@opencloud.tech <mailto:nic@opencloud.tech>> wrote: >> >> This patch will be used for next patch. > > I don’t see exactly what in the following patch(es) need this. Could you elaborate? The next patch 6/6 will call some functions which use the mutex and the ‘rstp/show’ use it agin. we should init it as a recursive mutex. > >> >> Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech <mailto:nic@opencloud.tech>> >> --- >> lib/rstp.c | 15 ++++++++++++--- >> lib/rstp.h | 6 ------ >> 2 files changed, 12 insertions(+), 9 deletions(-) >> >> 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_init() is already called earlier from the bridge_init(), so I see little point calling it from here. Not having multiple call sites would also remove the need for most of the changes above. Yes, but some rstp testes, which run with ovstest, will not run rstp_init in the bridge_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; >> - > > This change, if needed, should be in a separate patch with it’s own commit message. > > Jarno Yes, If other patches will be ok, I will put it to a separate patch.
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. Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech> --- lib/rstp.c | 15 ++++++++++++--- lib/rstp.h | 6 ------ 2 files changed, 12 insertions(+), 9 deletions(-)