[vlc-commits] [Git][videolan/vlc][master] 5 commits: config: make config_dirty atomic

Rémi Denis-Courmont (@Courmisch) gitlab at videolan.org
Sat Nov 27 08:35:26 UTC 2021



Rémi Denis-Courmont pushed to branch master at VideoLAN / VLC


Commits:
b282e134 by Rémi Denis-Courmont at 2021-11-27T08:21:48+00:00
config: make config_dirty atomic

- - - - -
fd2a9dfd by Rémi Denis-Courmont at 2021-11-27T08:21:48+00:00
config: remove recursive lock on auto-save

This was only there to handle the dirty flag atomically with the actual
saving of the configuration file.

- - - - -
04e27dea by Rémi Denis-Courmont at 2021-11-27T08:21:48+00:00
config/help: fix printed default for booleans

- - - - -
dbc43dfb by Rémi Denis-Courmont at 2021-11-27T08:21:48+00:00
config: add atomic values for int and float items

The non-atomic copies are left for backward compatibility.

- - - - -
d78c6262 by Rémi Denis-Courmont at 2021-11-27T08:21:48+00:00
config: read ints and floats as atomic

This makes config_GetInt() and config_GetFloat() completely lock-less.

- - - - -


6 changed files:

- src/config/configuration.h
- src/config/core.c
- src/config/file.c
- src/config/help.c
- src/modules/cache.c
- src/modules/entry.c


Changes:

=====================================
src/config/configuration.h
=====================================
@@ -26,6 +26,11 @@
 struct vlc_plugin_t;
 
 struct vlc_param {
+    union {
+        _Atomic int64_t i; /**< Current value (if integer or boolean) */
+        _Atomic float f; /**< Current value (if floating point) */
+    } value;
+
     struct vlc_plugin_t *owner;
     unsigned char shortname; /**< Optional short option name */
     unsigned internal:1; /**< Hidden from preferences and help */
@@ -56,7 +61,7 @@ int config_SortConfig (void);
 void config_UnsortConfig (void);
 
 extern vlc_rwlock_t config_lock;
-extern bool config_dirty;
+extern _Atomic bool config_dirty;
 
 bool config_IsSafe (const char *);
 


=====================================
src/config/core.c
=====================================
@@ -24,6 +24,7 @@
 # include "config.h"
 #endif
 
+#include <stdatomic.h>
 #include <vlc_common.h>
 #include <vlc_actions.h>
 #include <vlc_modules.h>
@@ -38,7 +39,7 @@
 #include "modules/modules.h"
 
 vlc_rwlock_t config_lock = VLC_STATIC_RWLOCK;
-bool config_dirty = false;
+atomic_bool config_dirty = ATOMIC_VAR_INIT(false);
 
 static inline char *strdupnull (const char *src)
 {
@@ -84,38 +85,26 @@ static module_config_t * config_FindConfigChecked( const char *psz_name )
     return p_config;
 }
 
-int64_t config_GetInt(const char *psz_name)
+int64_t config_GetInt(const char *name)
 {
-    module_config_t *p_config = config_FindConfigChecked( psz_name );
+    const struct vlc_param *param = vlc_param_Find(name);
 
     /* sanity checks */
-    assert(p_config != NULL);
-    assert(IsConfigIntegerType(p_config->i_type));
+    assert(param != NULL);
+    assert(IsConfigIntegerType(param->item.i_type));
 
-    int64_t val;
-
-    vlc_rwlock_rdlock (&config_lock);
-    val = p_config->value.i;
-    vlc_rwlock_unlock (&config_lock);
-    return val;
+    return atomic_load_explicit(&param->value.i, memory_order_relaxed);
 }
 
-float config_GetFloat(const char *psz_name)
+float config_GetFloat(const char *name)
 {
-    module_config_t *p_config;
-
-    p_config = config_FindConfigChecked( psz_name );
+    const struct vlc_param *param = vlc_param_Find(name);
 
     /* sanity checks */
-    assert(p_config != NULL);
-    assert(IsConfigFloatType(p_config->i_type));
+    assert(param != NULL);
+    assert(IsConfigFloatType(param->item.i_type));
 
-    float val;
-
-    vlc_rwlock_rdlock (&config_lock);
-    val = p_config->value.f;
-    vlc_rwlock_unlock (&config_lock);
-    return val;
+    return atomic_load_explicit(&param->value.f, memory_order_relaxed);
 }
 
 char *config_GetPsz(const char *psz_name)
@@ -151,38 +140,41 @@ void config_PutPsz(const char *psz_name, const char *psz_value)
     vlc_rwlock_wrlock (&config_lock);
     oldstr = (char *)p_config->value.psz;
     p_config->value.psz = str;
-    config_dirty = true;
     vlc_rwlock_unlock (&config_lock);
+    atomic_store_explicit(&config_dirty, true, memory_order_release);
 
     free (oldstr);
 }
 
-void config_PutInt(const char *psz_name, int64_t i_value )
+void config_PutInt(const char *name, int64_t i_value)
 {
-    module_config_t *p_config = config_FindConfigChecked( psz_name );
+    struct vlc_param *param = vlc_param_Find(name);
+    module_config_t *p_config = &param->item;
 
     /* sanity checks */
-    assert(p_config != NULL);
-    assert(IsConfigIntegerType(p_config->i_type));
+    assert(param != NULL);
+    assert(IsConfigIntegerType(param->item.i_type));
 
     if (i_value < p_config->min.i)
         i_value = p_config->min.i;
     if (i_value > p_config->max.i)
         i_value = p_config->max.i;
 
+    atomic_store_explicit(&param->value.i, i_value, memory_order_relaxed);
     vlc_rwlock_wrlock (&config_lock);
     p_config->value.i = i_value;
-    config_dirty = true;
     vlc_rwlock_unlock (&config_lock);
+    atomic_store_explicit(&config_dirty, true, memory_order_release);
 }
 
-void config_PutFloat(const char *psz_name, float f_value)
+void config_PutFloat(const char *name, float f_value)
 {
-    module_config_t *p_config = config_FindConfigChecked( psz_name );
+    struct vlc_param *param = vlc_param_Find(name);
+    module_config_t *p_config = &param->item;
 
     /* sanity checks */
-    assert(p_config != NULL);
-    assert(IsConfigFloatType(p_config->i_type));
+    assert(param != NULL);
+    assert(IsConfigFloatType(param->item.i_type));
 
     /* if f_min == f_max == 0, then do not use them */
     if ((p_config->min.f == 0.f) && (p_config->max.f == 0.f))
@@ -192,10 +184,11 @@ void config_PutFloat(const char *psz_name, float f_value)
     else if (f_value > p_config->max.f)
         f_value = p_config->max.f;
 
+    atomic_store_explicit(&param->value.f, f_value, memory_order_relaxed);
     vlc_rwlock_wrlock (&config_lock);
     p_config->value.f = f_value;
-    config_dirty = true;
     vlc_rwlock_unlock (&config_lock);
+    atomic_store_explicit(&config_dirty, true, memory_order_release);
 }
 
 ssize_t config_GetIntChoices(const char *name,
@@ -507,10 +500,18 @@ void config_ResetAll(void)
             module_config_t *p_config = &param->item;
 
             if (IsConfigIntegerType (p_config->i_type))
+            {
+                atomic_store_explicit(&param->value.i, p_config->orig.i,
+                                      memory_order_relaxed);
                 p_config->value.i = p_config->orig.i;
+            }
             else
             if (IsConfigFloatType (p_config->i_type))
+            {
+                atomic_store_explicit(&param->value.f, p_config->orig.f,
+                                      memory_order_relaxed);
                 p_config->value.f = p_config->orig.f;
+            }
             else
             if (IsConfigStringType (p_config->i_type))
             {
@@ -520,6 +521,6 @@ void config_ResetAll(void)
             }
         }
     }
-    config_dirty = true;
     vlc_rwlock_unlock (&config_lock);
+    atomic_store_explicit(&config_dirty, true, memory_order_release);
 }


=====================================
src/config/file.c
=====================================
@@ -27,6 +27,7 @@
 #include <errno.h>                                                  /* errno */
 #include <assert.h>
 #include <limits.h>
+#include <stdatomic.h>
 #include <fcntl.h>
 #include <sys/stat.h>
 #ifdef __APPLE__
@@ -230,15 +231,25 @@ int config_LoadConfigFile( vlc_object_t *p_this )
                               psz_option_value, psz_option_name,
                               vlc_strerror_c(errno));
                 else
+                {
+                    atomic_store_explicit(&param->value.i, l,
+                                          memory_order_relaxed);
                     item->value.i = l;
+                }
                 break;
             }
 
             case CONFIG_ITEM_FLOAT:
+            {
                 if (!*psz_option_value)
                     break;                    /* ignore empty option */
-                item->value.f = (float)atof (psz_option_value);
+
+                float f = (float)atof(psz_option_value);
+                atomic_store_explicit(&param->value.f, f,
+                                      memory_order_relaxed);
+                item->value.f = f;
                 break;
+            }
 
             default:
                 free (item->value.psz);
@@ -452,7 +463,8 @@ int config_SaveConfigFile (vlc_object_t *p_this)
 
             if (IsConfigIntegerType (p_item->i_type))
             {
-                int64_t val = p_item->value.i;
+                int64_t val = atomic_load_explicit(&param->value.i,
+                                                   memory_order_relaxed);
                 config_Write (file, p_item->psz_text,
                              (CONFIG_CLASS(p_item->i_type) == CONFIG_ITEM_BOOL)
                                   ? N_("boolean") : N_("integer"),
@@ -462,7 +474,8 @@ int config_SaveConfigFile (vlc_object_t *p_this)
             else
             if (IsConfigFloatType (p_item->i_type))
             {
-                float val = p_item->value.f;
+                float val = atomic_load_explicit(&param->value.f,
+                                                 memory_order_relaxed);
                 config_Write (file, p_item->psz_text, N_("float"),
                               val == p_item->orig.f,
                               p_item->psz_name, "%f", val);
@@ -530,18 +543,20 @@ error:
 
 int config_AutoSaveConfigFile( vlc_object_t *p_this )
 {
-    int ret = 0;
-
     assert( p_this );
 
-    vlc_rwlock_rdlock (&config_lock);
-    if (config_dirty)
-    {
-        /* Note: this will get the read lock recursively. Ok. */
-        ret = config_SaveConfigFile (p_this);
-        config_dirty = (ret != 0);
-    }
-    vlc_rwlock_unlock (&config_lock);
+    if (!atomic_exchange_explicit(&config_dirty, false, memory_order_acquire))
+        return 0;
+
+    int ret = config_SaveConfigFile (p_this);
+
+    if (unlikely(ret != 0))
+        /*
+         * On write failure, set the dirty flag back again. While it looks
+         * racy, it really means to retry later in hope that it does not
+         * fail again. Concurrent write attempts would not succeed anyway.
+         */
+        atomic_store_explicit(&config_dirty, true, memory_order_relaxed);
 
     return ret;
 }


=====================================
src/config/help.c
=====================================
@@ -22,6 +22,7 @@
 # include "config.h"
 #endif
 
+#include <stdatomic.h>
 #include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
@@ -485,8 +486,8 @@ static void print_item(const module_t *m, const struct vlc_param *param,
         case CONFIG_ITEM_BOOL:
             bra = type = ket = "";
             prefix = ", --no-";
-            suffix = item->value.i ? _("(default enabled)")
-                                   : _("(default disabled)");
+            suffix = item->orig.i ? _("(default enabled)")
+                                  : _("(default disabled)");
             break;
        default:
             return;


=====================================
src/modules/cache.c
=====================================
@@ -28,6 +28,7 @@
 #endif
 
 #include <stdalign.h>
+#include <stdatomic.h>
 #include <stdlib.h>
 #include <stdio.h>
 #include <string.h>
@@ -216,6 +217,12 @@ static int vlc_cache_load_config(struct vlc_param *param, block_t *file)
         LOAD_IMMEDIATE (cfg->orig);
         LOAD_IMMEDIATE (cfg->min);
         LOAD_IMMEDIATE (cfg->max);
+        if (IsConfigFloatType(cfg->i_type))
+            atomic_store_explicit(&param->value.f, cfg->orig.f,
+                                  memory_order_relaxed);
+        else
+            atomic_store_explicit(&param->value.i, cfg->orig.i,
+                                  memory_order_relaxed);
         cfg->value = cfg->orig;
 
         if (cfg->list_count)


=====================================
src/modules/entry.c
=====================================
@@ -23,6 +23,7 @@
 # include "config.h"
 #endif
 
+#include <stdatomic.h>
 #include <vlc_common.h>
 #include <vlc_plugin.h>
 #include <assert.h>
@@ -329,14 +330,18 @@ static int vlc_plugin_desc_cb(void *ctx, void *tgt, int propid, ...)
             if (IsConfigIntegerType(item->i_type)
              || !CONFIG_ITEM(item->i_type))
             {
-                item->orig.i =
-                item->value.i = va_arg (ap, int64_t);
+                item->orig.i = va_arg(ap, int64_t);
+                item->value.i = item->orig.i;
+                atomic_store_explicit(&param->value.i, item->orig.i,
+                                      memory_order_relaxed);
             }
             else
             if (IsConfigFloatType (item->i_type))
             {
-                item->orig.f =
-                item->value.f = va_arg (ap, double);
+                item->orig.f = va_arg(ap, double);
+                item->value.f = item->orig.f;
+                atomic_store_explicit(&param->value.f, item->orig.f,
+                                      memory_order_relaxed);
             }
             else
             if (IsConfigStringType (item->i_type))



View it on GitLab: https://code.videolan.org/videolan/vlc/-/compare/a38f2a139ee64df6b54fba83489f660eee1f9d38...d78c6262c05b435056817657baecfcab2d9d71fc

-- 
View it on GitLab: https://code.videolan.org/videolan/vlc/-/compare/a38f2a139ee64df6b54fba83489f660eee1f9d38...d78c6262c05b435056817657baecfcab2d9d71fc
You're receiving this email because of your account on code.videolan.org.




More information about the vlc-commits mailing list