Improve synchronization of master->domains list

Introduce a new lock for synchronization of the master->domains list, and use
that instead of the big master_sem lock, improving real-time performance of
fx. ecrt_domain_process() when EoE thread is being shut down (or other
use-cases where master_sem is held for extended periods).

The following locking order is used:

* master->master_sem
* master->domains_lock
* master->io_sem
* domain->datagram_pairs_lock

Meaning that it is allowed to take domains_lock while holding master_sem, and
it is allowed to hold domains_lock while locking io_sem and
datagram_pairs_lock.

[RV: For now, the lockdep_assert_held() annotations are commented out,
since they don't compile with CONFIG_PROVE_LOCKING and (the default)
--disable-rtmutex.]

master/domain.c

@@ -99,6 +99,7 @@ void ec_domain_clear(ec_domain_t *domain /**< EtherCAT domain */)
 {
     ec_datagram_pair_t *datagram_pair, *next_pair;
 
+    // lockdep_assert_held(&domain->master->domains_lock);
     ec_lock_down(&domain->datagram_pairs_lock);
     // dequeue and free datagrams
     list_for_each_entry_safe(datagram_pair, next_pair,
@@ -119,6 +120,7 @@ void ec_domain_clear_data(
         ec_domain_t *domain /**< EtherCAT domain. */
         )
 {
+    // lockdep_assert_held(&domain->master->domains_lock);
     if (domain->data_origin == EC_ORIG_INTERNAL && domain->data) {
         kfree(domain->data);
     }
@@ -519,14 +521,11 @@ void ecrt_domain_external_memory(ec_domain_t *domain, uint8_t *mem)
     EC_MASTER_DBG(domain->master, 1, "ecrt_domain_external_memory("
             "domain = 0x%p, mem = 0x%p)\n", domain, mem);
 
-    ec_lock_down(&domain->master->master_sem);
-
+    // lockdep_assert_held(&domain->master->domains_lock);
     ec_domain_clear_data(domain);
 
     domain->data = mem;
     domain->data_origin = EC_ORIG_EXTERNAL;
-
-    ec_lock_up(&domain->master->master_sem);
 }
 
 /*****************************************************************************/

master/ioctl.c

@@ -488,11 +488,11 @@ static ATTRIBUTES int ec_ioctl_domain(
         return -EFAULT;
     }
 
-    if (ec_lock_down_interruptible(&master->master_sem))
+    if (ec_lock_down_interruptible(&master->domains_lock))
         return -EINTR;
 
     if (!(domain = ec_master_find_domain_const(master, data.index))) {
-        ec_lock_up(&master->master_sem);
+        ec_lock_up(&master->domains_lock);
         EC_MASTER_ERR(master, "Domain %u does not exist!\n", data.index);
         return -EINVAL;
     }
@@ -506,7 +506,7 @@ static ATTRIBUTES int ec_ioctl_domain(
     data.expected_working_counter = domain->expected_working_counter;
     data.fmmu_count = ec_domain_fmmu_count(domain);
 
-    ec_lock_up(&master->master_sem);
+    ec_lock_up(&master->domains_lock);
 
     if (copy_to_user((void __user *) arg, &data, sizeof(data)))
         return -EFAULT;
@@ -533,18 +533,18 @@ static ATTRIBUTES int ec_ioctl_domain_fmmu(
         return -EFAULT;
     }
 
-    if (ec_lock_down_interruptible(&master->master_sem))
+    if (ec_lock_down_interruptible(&master->domains_lock))
         return -EINTR;
 
     if (!(domain = ec_master_find_domain_const(master, data.domain_index))) {
-        ec_lock_up(&master->master_sem);
+        ec_lock_up(&master->domains_lock);
         EC_MASTER_ERR(master, "Domain %u does not exist!\n",
                 data.domain_index);
         return -EINVAL;
     }
 
     if (!(fmmu = ec_domain_find_fmmu(domain, data.fmmu_index))) {
-        ec_lock_up(&master->master_sem);
+        ec_lock_up(&master->domains_lock);
         EC_MASTER_ERR(master, "Domain %u has less than %u"
                 " fmmu configurations.\n",
                 data.domain_index, data.fmmu_index + 1);
@@ -558,7 +558,7 @@ static ATTRIBUTES int ec_ioctl_domain_fmmu(
     data.logical_address = fmmu->domain->logical_base_address + fmmu->logical_domain_offset;
     data.data_size = fmmu->data_size;
 
-    ec_lock_up(&master->master_sem);
+    ec_lock_up(&master->domains_lock);
 
     if (copy_to_user((void __user *) arg, &data, sizeof(data)))
         return -EFAULT;
@@ -584,18 +584,18 @@ static ATTRIBUTES int ec_ioctl_domain_data(
         return -EFAULT;
     }
 
-    if (ec_lock_down_interruptible(&master->master_sem))
+    if (ec_lock_down_interruptible(&master->domains_lock))
         return -EINTR;
 
     if (!(domain = ec_master_find_domain_const(master, data.domain_index))) {
-        ec_lock_up(&master->master_sem);
+        ec_lock_up(&master->domains_lock);
         EC_MASTER_ERR(master, "Domain %u does not exist!\n",
                 data.domain_index);
         return -EINVAL;
     }
 
     if (domain->data_size != data.data_size) {
-        ec_lock_up(&master->master_sem);
+        ec_lock_up(&master->domains_lock);
         EC_MASTER_ERR(master, "Data size mismatch %u/%zu!\n",
                 data.data_size, domain->data_size);
         return -EFAULT;
@@ -603,11 +603,11 @@ static ATTRIBUTES int ec_ioctl_domain_data(
 
     if (copy_to_user((void __user *) data.target, domain->data,
                 domain->data_size)) {
-        ec_lock_up(&master->master_sem);
+        ec_lock_up(&master->domains_lock);
         return -EFAULT;
     }
 
-    ec_lock_up(&master->master_sem);
+    ec_lock_up(&master->domains_lock);
     return 0;
 }
 
@@ -1875,15 +1875,13 @@ static ATTRIBUTES int ec_ioctl_setup_domain_memory(
 
         ctx->process_data_size = 0;
 
-        if (ec_lock_down_interruptible(&master->master_sem))
+        if (ec_lock_down_interruptible(&master->domains_lock))
             return -EINTR;
 
         list_for_each_entry(domain, &master->domains, list) {
             ctx->process_data_size += ecrt_domain_size(domain);
         }
 
-        ec_lock_up(&master->master_sem);
-
         if (ctx->process_data_size) {
             ctx->process_data = vmalloc(ctx->process_data_size);
             if (!ctx->process_data) {
@@ -1901,6 +1899,8 @@ static ATTRIBUTES int ec_ioctl_setup_domain_memory(
                 offset += ecrt_domain_size(domain);
             }
 
+            ec_lock_up(&master->domains_lock);
+
 #ifdef EC_IOCTL_RTDM
             /* RTDM uses a different approach for memory-mapping, which has to be
              * initiated by the kernel.
@@ -1912,6 +1912,8 @@ static ATTRIBUTES int ec_ioctl_setup_domain_memory(
                 return ret;
             }
 #endif
+        } else {
+            ec_lock_up(&master->domains_lock);
         }
 
         io.process_data_size = ctx->process_data_size;
@@ -1959,15 +1961,13 @@ static ATTRIBUTES int ec_ioctl_activate(
 
         ctx->process_data_size = 0;
 
-        if (ec_lock_down_interruptible(&master->master_sem))
+        if (ec_lock_down_interruptible(&master->domains_lock))
             return -EINTR;
 
         list_for_each_entry(domain, &master->domains, list) {
             ctx->process_data_size += ecrt_domain_size(domain);
         }
 
-        ec_lock_up(&master->master_sem);
-
         if (ctx->process_data_size) {
             ctx->process_data = vmalloc(ctx->process_data_size);
             if (!ctx->process_data) {
@@ -1998,6 +1998,8 @@ static ATTRIBUTES int ec_ioctl_activate(
 #endif
         }
 
+        ec_lock_up(&master->domains_lock);
+
         io.process_data_size = ctx->process_data_size;
     }
     else
@@ -2775,12 +2777,24 @@ static ATTRIBUTES int ec_ioctl_sc_reg_pdo_entry(
         return -ENOENT;
     }
 
+    ec_lock_up(&master->master_sem);
+
+    if (ec_lock_down_interruptible(&master->domains_lock))
+        return -EINTR;
     if (!(domain = ec_master_find_domain(master, data.domain_index))) {
-        ec_lock_up(&master->master_sem);
+        ec_lock_up(&master->domains_lock);
         return -ENOENT;
     }
+    ec_lock_up(&master->domains_lock);
 
-    ec_lock_up(&master->master_sem); /** \todo sc or domain could be invalidated */
+    /** \todo sc or domain could be invalidated */
+
+    /* FIXME: As ecrt_slave_config_reg_pdo_entry() calls
+     * ec_slave_config_prepare_fmmu() which locks master_sem we must release
+     * domains_lock, as it would break lock ordering, and thus cause
+     * deadlocks.  Unfortunately, this leaves a race condition, as the domain
+     * could be cleared/deleted while ecrt_slave_config_reg_pdo_entry() is
+     * called. So the above todo statement still holds :( */
 
     ret = ecrt_slave_config_reg_pdo_entry(sc, data.entry_index,
             data.entry_subindex, domain, &data.bit_position);
@@ -2824,13 +2838,24 @@ static ATTRIBUTES int ec_ioctl_sc_reg_pdo_pos(
         ec_lock_up(&master->master_sem);
         return -ENOENT;
     }
+    ec_lock_up(&master->master_sem);
 
+    if (ec_lock_down_interruptible(&master->domains_lock))
+        return -EINTR;
     if (!(domain = ec_master_find_domain(master, io.domain_index))) {
-        ec_lock_up(&master->master_sem);
+        ec_lock_up(&master->domains_lock);
         return -ENOENT;
     }
+    ec_lock_up(&master->domains_lock);
 
-    ec_lock_up(&master->master_sem); /** \todo sc or domain could be invalidated */
+    /** \todo sc or domain could be invalidated */
+
+    /* FIXME: As ecrt_slave_config_reg_pdo_entry_pos() calls
+     * ec_slave_config_prepare_fmmu() which locks master_sem we must release
+     * domains_lock, as it would break lock ordering, and thus cause
+     * deadlocks.  Unfortunately, this leaves a race condition, as the domain
+     * could be cleared/deleted while ecrt_slave_config_reg_pdo_entry() is
+     * called. So the above todo statement still holds :( */
 
     ret = ecrt_slave_config_reg_pdo_entry_pos(sc, io.sync_index,
             io.pdo_pos, io.entry_pos, domain, &io.bit_position);
@@ -3421,19 +3446,19 @@ static ATTRIBUTES int ec_ioctl_domain_size(
         return -EPERM;
     }
 
-    if (ec_lock_down_interruptible(&master->master_sem)) {
+    if (ec_lock_down_interruptible(&master->domains_lock)) {
         return -EINTR;
     }
 
     list_for_each_entry(domain, &master->domains, list) {
         if (domain->index == (unsigned long) arg) {
             size_t size = ecrt_domain_size(domain);
-            ec_lock_up(&master->master_sem);
+            ec_lock_up(&master->domains_lock);
             return size;
         }
     }
 
-    ec_lock_up(&master->master_sem);
+    ec_lock_up(&master->domains_lock);
     return -ENOENT;
 }
 
@@ -3455,19 +3480,19 @@ static ATTRIBUTES int ec_ioctl_domain_offset(
     if (unlikely(!ctx->requested))
         return -EPERM;
 
-    if (ec_lock_down_interruptible(&master->master_sem)) {
+    if (ec_lock_down_interruptible(&master->domains_lock)) {
         return -EINTR;
     }
 
     list_for_each_entry(domain, &master->domains, list) {
         if (domain->index == (unsigned long) arg) {
-            ec_lock_up(&master->master_sem);
+            ec_lock_up(&master->domains_lock);
             return offset;
         }
         offset += ecrt_domain_size(domain);
     }
 
-    ec_lock_up(&master->master_sem);
+    ec_lock_up(&master->domains_lock);
     return -ENOENT;
 }
 
@@ -3490,17 +3515,17 @@ static ATTRIBUTES int ec_ioctl_domain_process(
 
     /* Locking added as domain processing is likely to be used by more than
        one application tasks */
-    if (ec_ioctl_lock_down_interruptible(&master->master_sem)) {
+    if (ec_ioctl_lock_down_interruptible(&master->domains_lock)) {
         return -EINTR;
     }
 
     if (!(domain = ec_master_find_domain(master, (unsigned long) arg))) {
-        ec_ioctl_lock_up(&master->master_sem);
+        ec_ioctl_lock_up(&master->domains_lock);
         return -ENOENT;
     }
 
     ecrt_domain_process(domain);
-    ec_ioctl_lock_up(&master->master_sem);
+    ec_ioctl_lock_up(&master->domains_lock);
     return 0;
 }
 
@@ -3523,21 +3548,22 @@ static ATTRIBUTES int ec_ioctl_domain_queue(
 
     /* Locking added as domain queing is likely to be used by more than
        one application tasks */
-    if (ec_ioctl_lock_down_interruptible(&master->master_sem))
+    if (ec_ioctl_lock_down_interruptible(&master->domains_lock))
         return -EINTR;
-
     if (!(domain = ec_master_find_domain(master, (unsigned long) arg))) {
-        ec_ioctl_lock_up(&master->master_sem);
+        ec_ioctl_lock_up(&master->domains_lock);
         return -ENOENT;
     }
 
-    ec_ioctl_lock_up(&master->master_sem);
-
-    if (ec_ioctl_lock_down_interruptible(&master->io_sem))
+    if (ec_ioctl_lock_down_interruptible(&master->io_sem)) {
+        ec_ioctl_lock_up(&master->domains_lock);
         return -EINTR;
+    }
     ecrt_domain_queue(domain);
     ec_ioctl_lock_up(&master->io_sem);
 
+    ec_ioctl_lock_up(&master->domains_lock);
+
     return 0;
 }
 
@@ -3564,15 +3590,18 @@ static ATTRIBUTES int ec_ioctl_domain_state(
         return -EFAULT;
     }
 
-    /* no locking of master_sem needed, because domain will not be deleted in
-     * the meantime. */
+    if (ec_ioctl_lock_down_interruptible(&master->domains_lock))
+        return -EINTR;
 
     if (!(domain = ec_master_find_domain_const(master, data.domain_index))) {
+        ec_ioctl_lock_up(&master->domains_lock);
         return -ENOENT;
     }
 
     ecrt_domain_state(domain, &state);
 
+    ec_ioctl_lock_up(&master->domains_lock);
+
     if (copy_to_user((void __user *) data.state, &state, sizeof(state)))
         return -EFAULT;
 

master/master.c

@@ -186,6 +186,7 @@ int ec_master_init(ec_master_t *master, /**< EtherCAT master */
 
     INIT_LIST_HEAD(&master->configs);
     INIT_LIST_HEAD(&master->domains);
+	ec_lock_init(&master->domains_lock);
 
     master->app_time = 0ULL;
     master->dc_ref_time = 0ULL;
@@ -534,11 +535,13 @@ void ec_master_clear_domains(ec_master_t *master)
 {
     ec_domain_t *domain, *next;
 
+    ec_lock_down(&master->domains_lock);
     list_for_each_entry_safe(domain, next, &master->domains, list) {
         list_del(&domain->list);
         ec_domain_clear(domain);
         kfree(domain);
     }
+    ec_lock_up(&master->domains_lock);
 }
 
 /*****************************************************************************/
@@ -2089,9 +2092,11 @@ unsigned int ec_master_domain_count(
     const ec_domain_t *domain;
     unsigned int count = 0;
 
+    ec_lock_down(&master->domains_lock);
     list_for_each_entry(domain, &master->domains, list) {
         count++;
     }
+    ec_lock_up(&master->domains_lock);
 
     return count;
 }
@@ -2437,7 +2442,7 @@ ec_domain_t *ecrt_master_create_domain_err(
         return ERR_PTR(-ENOMEM);
     }
 
-    ec_lock_down(&master->master_sem);
+    ec_lock_down(&master->domains_lock);
 
     if (list_empty(&master->domains)) {
         index = 0;
@@ -2449,7 +2454,7 @@ ec_domain_t *ecrt_master_create_domain_err(
     ec_domain_init(domain, master, index);
     list_add_tail(&domain->list, &master->domains);
 
-    ec_lock_up(&master->master_sem);
+    ec_lock_up(&master->domains_lock);
 
     EC_MASTER_DBG(master, 1, "Created domain %u.\n", domain->index);
 
@@ -2492,21 +2497,21 @@ int ecrt_master_activate(ec_master_t *master)
         return 0;
     }
 
-    ec_lock_down(&master->master_sem);
+    ec_lock_down(&master->domains_lock);
 
     // finish all domains
     domain_offset = 0;
     list_for_each_entry(domain, &master->domains, list) {
         ret = ec_domain_finish(domain, domain_offset);
         if (ret < 0) {
-            ec_lock_up(&master->master_sem);
+            ec_lock_up(&master->domains_lock);
             EC_MASTER_ERR(master, "Failed to finish domain 0x%p!\n", domain);
             return ret;
         }
         domain_offset += domain->data_size;
     }
 
-    ec_lock_up(&master->master_sem);
+    ec_lock_up(&master->domains_lock);
 
     // restart EoE process and master thread with new locking
 

master/master.h

@@ -229,6 +229,7 @@ struct ec_master {
     /* Configuration applied by the application. */
     struct list_head configs; /**< List of slave configurations. */
     struct list_head domains; /**< List of domains. */
+    ec_lock_t domains_lock; /**< Lock for access to domains list. */
 
     u64 app_time; /**< Time of the last ecrt_master_sync() call. */
     u64 dc_ref_time; /**< Common reference timestamp for DC start times. */