Changeset 015b147 in mainline

Timestamp:

2019-08-17T13:52:32Z (5 years ago)

Author:

Matthieu Riolo <matthieu.riolo@…>

Children:

be07995

Parents:

31ef7c1

git-author:

Michal Koutný <xm.koutny+hos@…> (2016-01-10 17:17:19)

git-committer:

Matthieu Riolo <matthieu.riolo@…> (2019-08-17 13:52:32)

Message:

sysman: Refactored unit repo iteration and locking

Location:

uspace/srv/sysman

Files:

• connection_ctl.c (modified) (1 diff)
• job_closure.c (modified) (2 diffs)
• repo.c (modified) (12 diffs)
• repo.h (modified) (2 diffs)
• sm_task.c (modified) (1 diff)
• units/unit_cfg.c (modified) (1 diff)

uspace/srv/sysman/connection_ctl.c

@@ -197 +197 @@
         size_t total = 0;
         repo_rlock();
-        list_foreach(units, units, unit_t, u) {
+        repo_foreach(u) {
                 if (filled < to_fill) {
                         buffer[filled++] = u->handle;

uspace/srv/sysman/job_closure.c

@@ -205 +205 @@
 {
         /* Check invariant */
-        list_foreach(units, units, unit_t, u) {
+        repo_foreach(u) {
                 assert(u->bfs_tag == false);
         }
@@ -211 +211 @@
 
         /* Clean after ourselves (BFS tag jobs) */
-        list_foreach(units, units, unit_t, u) {
+        repo_foreach(u) {
                 u->bfs_tag = false;
         }

uspace/srv/sysman/repo.c

@@ -38 +38 @@
 #include "log.h"
 
-LIST_INITIALIZE(units);
+LIST_INITIALIZE(units_);
 
 static hash_table_t units_by_name;
@@ -119 +119 @@
 static void repo_remove_unit_internal(unit_t *u)
 {
+        assert(fibril_rwlock_is_write_locked(&repo_lock));
+
         hash_table_remove_item(&units_by_name, &u->units_by_name);
         hash_table_remove_item(&units_by_handle, &u->units_by_handle);
@@ -125 +127 @@
         // TODO decrease refcount of unit
         // unit may be referenced e.g. from running job, thus we cannot simply destroy it
+}
+
+static unit_t *repo_find_unit_by_name_internal(const char *name, bool lock)
+{
+        sysman_log(LVL_DEBUG2, "%s(%s, %i)", __func__, name, lock);
+        if (lock) fibril_rwlock_read_lock(&repo_lock);
+        ht_link_t *ht_link = hash_table_find(&units_by_name, (void *)name);
+        if (lock) fibril_rwlock_read_unlock(&repo_lock);
+
+        if (ht_link != NULL) {
+                return hash_table_get_inst(ht_link, unit_t, units_by_name);
+        } else {
+                return NULL;
+        }
 }
 
@@ -139 +155 @@
         assert(unit->handle == 0);
         assert(unit->name != NULL);
+        assert(fibril_rwlock_is_write_locked(&repo_lock));
         sysman_log(LVL_DEBUG2, "%s('%s')", __func__, unit_name(unit));
 
-        fibril_rwlock_write_lock(&repo_lock);
         if (hash_table_insert_unique(&units_by_name, &unit->units_by_name)) {
                 /* Pointers are same size as unit_handle_t both on 32b and 64b */
@@ -147 +163 @@
 
                 hash_table_insert(&units_by_handle, &unit->units_by_handle);
-                list_append(&unit->units, &units);
-                fibril_rwlock_write_unlock(&repo_lock);
+                list_append(&unit->units, &units_);
                 return EOK;
         } else {
-                fibril_rwlock_write_unlock(&repo_lock);
                 return EEXISTS;
         }
@@ -162 +176 @@
 }
 
-void repo_begin_update(void) {
-        sysman_log(LVL_DEBUG2, "%s", __func__);
+void repo_begin_update_(void) {
+        sysman_log(LVL_DEBUG2, "%s", __func__);
+        fibril_rwlock_write_lock(&repo_lock);
 }
 
@@ -196 +211 @@
          */
         hash_table_apply(&units_by_name, &repo_commit_unit, NULL);
+        fibril_rwlock_write_unlock(&repo_lock);
 }
 
@@ -228 +244 @@
 
         hash_table_apply(&units_by_name, &repo_rollback_unit, NULL);
+        fibril_rwlock_write_unlock(&repo_lock);
 }
 
@@ -243 +260 @@
 
                 unit_t *output =
-                    repo_find_unit_by_name(e->output_name);
+                    repo_find_unit_by_name_unsafe(e->output_name);
                 if (output == NULL) {
                         sysman_log(LVL_ERROR,
@@ -273 +290 @@
 }
 
-unit_t *repo_find_unit_by_name(const char *name)
-{
-        fibril_rwlock_read_lock(&repo_lock);
-        ht_link_t *ht_link = hash_table_find(&units_by_name, (void *)name);
-        fibril_rwlock_read_unlock(&repo_lock);
-
-        if (ht_link != NULL) {
-                return hash_table_get_inst(ht_link, unit_t, units_by_name);
-        } else {
-                return NULL;
-        }
-}
-
+/**
+ * The function can be safely called from non-event loop fibrils
+ */
+unit_t *repo_find_unit_by_name_(const char *name)
+{
+        return repo_find_unit_by_name_internal(name, true);
+}
+
+/**
+ * @note Caller must hold repo_lock (at least reader)
+ */
+unit_t *repo_find_unit_by_name_unsafe(const char *name)
+{
+        return repo_find_unit_by_name_internal(name, false);
+}
+
+/**
+ * The function can be safely called from non-event loop fibrils
+ */
 unit_t *repo_find_unit_by_handle(unit_handle_t handle)
 {
+        sysman_log(LVL_DEBUG2, "%s", __func__);
         fibril_rwlock_read_lock(&repo_lock);
         ht_link_t *ht_link = hash_table_find(&units_by_handle, &handle);
@@ -301 +325 @@
 void repo_rlock(void)
 {
+        sysman_log(LVL_DEBUG2, "%s", __func__);
         fibril_rwlock_read_lock(&repo_lock);
 }
@@ -306 +331 @@
 void repo_runlock(void)
 {
+        sysman_log(LVL_DEBUG2, "%s", __func__);
         fibril_rwlock_read_unlock(&repo_lock);
 }

uspace/srv/sysman/repo.h

@@ -38 +38 @@
 #define ANONYMOUS_SERVICE_MASK "service_%" PRIu64
 
-/*
- * If you access units out of the main event-loop fibril call repo_rlock(),
- * repo_runlock().
+/**
+ * Don't access this structure directly, use repo_foreach instead.
  */
-extern list_t units;
+extern list_t units_;
+
+/**
+ * If you iterate units out of the main event-loop fibril, wrap the foreach
+ * into repo_rlock() and repo_runlock().
+ *
+ * @param  it  name of unit_t * loop iterator variable
+ */
+#define repo_foreach(_it) \
+        list_foreach(units_, units, unit_t, _it)
+
+/**
+ * @see repo_foreach
+ *
+ * @param type  unit_type_t  restrict to units of type only
+ * @param it                 name of unit_t * loop iterator variable
+ */
+#define repo_foreach_t(_type, _it) \
+        list_foreach(units_, units, unit_t, _it) \
+                if (_it->type == (_type))
 
 extern void repo_init(void);
@@ -58 +76 @@
 
 extern unit_t *repo_find_unit_by_name(const char *);
+extern unit_t *repo_find_unit_by_name_unsafe(const char *);
 extern unit_t *repo_find_unit_by_handle(unit_handle_t);
 

uspace/srv/sysman/sm_task.c

@@ -75 +75 @@
          * instead of specialized structures.
          */
-        list_foreach(units, units, unit_t, u) {
-                if (u->type != UNIT_SERVICE) {
-                        continue;
-                }
+        repo_foreach_t(UNIT_SERVICE, u) {
                 if (CAST_SVC(u)->main_task_id == tid) {
                         return CAST_SVC(u);

uspace/srv/sysman/units/unit_cfg.c

@@ -82 +82 @@
         }
         
-        unit_t *u = repo_find_unit_by_name(unit_name);
+        /* We parse files as part of ongoing repo transaction (locked). */
+        unit_t *u = repo_find_unit_by_name_unsafe(unit_name);
         if (u != NULL) {
                 // TODO allow updating configuration of existing unit