diff mbox

[ovs-dev,v2,6/6] vlog: Use log_file_mutex to more consistently protect vlog_modules.

Message ID 1454536211-12354-7-git-send-email-blp@ovn.org
State Accepted
Headers show

Commit Message

Ben Pfaff Feb. 3, 2016, 9:50 p.m. UTC 
This could be beneficial if we ever insert a new module at runtime
(currently we only insert them at startup) and it should have negligible
cost.

Suggested-by: Russell Bryant <russell@ovn.org>
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 lib/vlog.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

Comments

Russell Bryant Feb. 10, 2016, 9:32 p.m. UTC | #1 
On 02/03/2016 04:50 PM, Ben Pfaff wrote:
> This could be beneficial if we ever insert a new module at runtime
> (currently we only insert them at startup) and it should have negligible
> cost.
> 
> Suggested-by: Russell Bryant <russell@ovn.org>
> Signed-off-by: Ben Pfaff <blp@ovn.org>

Acked-by: Russell Bryant <russell@ovn.org>

> @@ -104,13 +101,18 @@ DEFINE_STATIC_PER_THREAD_DATA(unsigned int, msg_num, 0);
>   *
>   * All of the following is protected by 'log_file_mutex', which nests inside
>   * pattern_rwlock. */
> -static struct ovs_mutex log_file_mutex = OVS_MUTEX_INITIALIZER;
> +static struct ovs_mutex log_file_mutex OVS_ACQ_AFTER(pattern_rwlock)
> +    = OVS_MUTEX_INITIALIZER;

btw, I *love* these macros enabling thread safety attributes from clang.
 They probably would have saved me a lot of pain in a previous life.
Ben Pfaff Feb. 10, 2016, 9:38 p.m. UTC | #2 
On Wed, Feb 10, 2016 at 04:32:33PM -0500, Russell Bryant wrote:
> On 02/03/2016 04:50 PM, Ben Pfaff wrote:
> > This could be beneficial if we ever insert a new module at runtime
> > (currently we only insert them at startup) and it should have negligible
> > cost.
> > 
> > Suggested-by: Russell Bryant <russell@ovn.org>
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> 
> Acked-by: Russell Bryant <russell@ovn.org>

Thanks.  I'll apply patches 4, 5, and 6 to master in a minute, then.

> > @@ -104,13 +101,18 @@ DEFINE_STATIC_PER_THREAD_DATA(unsigned int, msg_num, 0);
> >   *
> >   * All of the following is protected by 'log_file_mutex', which nests inside
> >   * pattern_rwlock. */
> > -static struct ovs_mutex log_file_mutex = OVS_MUTEX_INITIALIZER;
> > +static struct ovs_mutex log_file_mutex OVS_ACQ_AFTER(pattern_rwlock)
> > +    = OVS_MUTEX_INITIALIZER;
> 
> btw, I *love* these macros enabling thread safety attributes from clang.
>  They probably would have saved me a lot of pain in a previous life.

Beware: current versions of Clang don't actually honor OVS_ACQ_AFTER and
OVS_ACQ_BEFORE.  I still find them helpful as in-code documentation.
diff mbox

Patch

diff --git a/lib/vlog.c b/lib/vlog.c
index 49260d8..bbc6eb6 100644
--- a/lib/vlog.c
+++ b/lib/vlog.c
@@ -77,9 +77,6 @@  VLOG_LEVELS
  * used for LOG_LOCAL0. */
 BUILD_ASSERT_DECL(LOG_LOCAL0 == (16 << 3));
 
-/* The log modules. */
-static struct ovs_list vlog_modules = OVS_LIST_INITIALIZER(&vlog_modules);
-
 /* Protects the 'pattern' in all "struct destination"s, so that a race between
  * changing and reading the pattern does not cause an access to freed
  * memory. */
@@ -104,13 +101,18 @@  DEFINE_STATIC_PER_THREAD_DATA(unsigned int, msg_num, 0);
  *
  * All of the following is protected by 'log_file_mutex', which nests inside
  * pattern_rwlock. */
-static struct ovs_mutex log_file_mutex = OVS_MUTEX_INITIALIZER;
+static struct ovs_mutex log_file_mutex OVS_ACQ_AFTER(pattern_rwlock)
+    = OVS_MUTEX_INITIALIZER;
 static char *log_file_name OVS_GUARDED_BY(log_file_mutex) = NULL;
 static int log_fd OVS_GUARDED_BY(log_file_mutex) = -1;
 static struct async_append *log_writer OVS_GUARDED_BY(log_file_mutex);
 static bool log_async OVS_GUARDED_BY(log_file_mutex);
 static struct syslogger *syslogger = NULL;
 
+/* The log modules. */
+static struct ovs_list vlog_modules OVS_GUARDED_BY(log_file_mutex)
+    = OVS_LIST_INITIALIZER(&vlog_modules);
+
 /* Syslog export configuration. */
 static int syslog_fd OVS_GUARDED_BY(pattern_rwlock) = -1;
 
@@ -212,7 +214,9 @@  vlog_get_destination_val(const char *name)
 void
 vlog_insert_module(struct ovs_list *vlog)
 {
+    ovs_mutex_lock(&log_file_mutex);
     list_insert(&vlog_modules, vlog);
+    ovs_mutex_unlock(&log_file_mutex);
 }
 
 /* Returns the name for logging module 'module'. */
@@ -229,11 +233,14 @@  vlog_module_from_name(const char *name)
 {
     struct vlog_module *mp;
 
+    ovs_mutex_lock(&log_file_mutex);
     LIST_FOR_EACH (mp, list, &vlog_modules) {
         if (!strcasecmp(name, mp->name)) {
+            ovs_mutex_unlock(&log_file_mutex);
             return mp;
         }
     }
+    ovs_mutex_unlock(&log_file_mutex);
 
     return NULL;
 }
@@ -704,9 +711,11 @@  set_all_rate_limits(bool enable)
 {
     struct vlog_module *mp;
 
+    ovs_mutex_lock(&log_file_mutex);
     LIST_FOR_EACH (mp, list, &vlog_modules) {
         mp->honor_rate_limits = enable;
     }
+    ovs_mutex_unlock(&log_file_mutex);
 }
 
 static void
@@ -836,6 +845,7 @@  vlog_get_levels(void)
     ds_put_format(&s, "                 console    syslog    file\n");
     ds_put_format(&s, "                 -------    ------    ------\n");
 
+    ovs_mutex_lock(&log_file_mutex);
     LIST_FOR_EACH (mp, list, &vlog_modules) {
         struct ds line;
 
@@ -852,6 +862,7 @@  vlog_get_levels(void)
 
         svec_add_nocopy(&lines, ds_steal_cstr(&line));
     }
+    ovs_mutex_unlock(&log_file_mutex);
 
     svec_sort(&lines);
     SVEC_FOR_EACH (i, line, &lines) {