From 81758004fd4c243ac044ebbb9121ddeaed69f0f9 Mon Sep 17 00:00:00 2001 From: Esben Haabendal Date: Fri, 26 Jan 2018 12:05:10 +0100 Subject: [PATCH] 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 | 7 ++-- master/ioctl.c | 107 ++++++++++++++++++++++++++++++------------------ master/master.c | 15 ++++--- master/master.h | 1 + 4 files changed, 82 insertions(+), 48 deletions(-) diff --git a/master/domain.c b/master/domain.c index b303ea97..b1c7aa5c 100755 --- a/master/domain.c +++ b/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); } /*****************************************************************************/ diff --git a/master/ioctl.c b/master/ioctl.c index 957b9aa7..95c3cefd 100755 --- a/master/ioctl.c +++ b/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; diff --git a/master/master.c b/master/master.c index bfaa893e..81d56383 100644 --- a/master/master.c +++ b/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 diff --git a/master/master.h b/master/master.h index c51d79fb..9541df5b 100644 --- a/master/master.h +++ b/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. */