On 07.05.2019 16:46, Eelco Chaudron wrote:
>
>
> On 30 Apr 2019, at 14:17, David Marchand wrote:
>
>> pmd reloads are currently serialised in each steps calling
>> reload_affected_pmds.
>> Any pmd processing packets, waiting on a mutex etc... will make other
>> pmd threads wait for a delay that can be undeterministic when syscalls
>> adds up.
>>
>> Switch to a little busy loop on the control thread using an atomic
>> count.
>>
>> Signed-off-by: David Marchand <david.march...@redhat.com>
>> ---
>> lib/dpif-netdev.c | 43 ++++++++++++++++++++++++++++++-------------
>> 1 file changed, 30 insertions(+), 13 deletions(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 30774ed..b2b21fd 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -335,6 +335,9 @@ struct dp_netdev {
>> /* The time that a packet can wait in output batch for sending. */
>> atomic_uint32_t tx_flush_interval;
>>
>> + /* Count of pmds currently reloading */
>> + atomic_count reloading_pmds;
>> +
>> /* Meters. */
>> struct ovs_mutex meter_locks[N_METER_LOCKS];
>> struct dp_meter *meters[MAX_METERS]; /* Meter bands. */
>> @@ -647,9 +650,6 @@ struct dp_netdev_pmd_thread {
>> struct ovs_refcount ref_cnt; /* Every reference must be refcount'ed.
>> */
>> struct cmap_node node; /* In 'dp->poll_threads'. */
>>
>> - pthread_cond_t cond; /* For synchronizing pmd thread reload.
>> */
>> - struct ovs_mutex cond_mutex; /* Mutex for condition variable. */
>> -
>> /* Per thread exact-match cache. Note, the instance for cpu core
>> * NON_PMD_CORE_ID can be accessed by multiple threads, and thusly
>> * need to be protected by 'non_pmd_mutex'. Every other instance
>> @@ -1525,6 +1525,8 @@ create_dp_netdev(const char *name, const struct
>> dpif_class *class,
>> atomic_init(&dp->emc_insert_min, DEFAULT_EM_FLOW_INSERT_MIN);
>> atomic_init(&dp->tx_flush_interval, DEFAULT_TX_FLUSH_INTERVAL);
>>
>> + atomic_count_init(&dp->reloading_pmds, 0);
>> +
>> cmap_init(&dp->poll_threads);
>> dp->pmd_rxq_assign_cyc = true;
>>
>> @@ -1754,11 +1756,8 @@ dp_netdev_reload_pmd__(struct dp_netdev_pmd_thread
>> *pmd)
>> return;
>> }
>>
>> - ovs_mutex_lock(&pmd->cond_mutex);
>> seq_change(pmd->reload_seq);
>> atomic_store_relaxed(&pmd->reload, true);
>> - ovs_mutex_cond_wait(&pmd->cond, &pmd->cond_mutex);
>> - ovs_mutex_unlock(&pmd->cond_mutex);
>> }
>>
>> static uint32_t
>> @@ -4641,9 +4640,27 @@ rxq_scheduling(struct dp_netdev *dp, bool pinned)
>> OVS_REQUIRES(dp->port_mutex)
>> }
>>
>> static void
>> +wait_reloading_pmds(struct dp_netdev *dp)
>> +{
>> + while (atomic_count_get(&dp->reloading_pmds) != 0) {
>> + }
>> +}
>> +
>> +static void
>> reload_affected_pmds(struct dp_netdev *dp)
>> {
>> struct dp_netdev_pmd_thread *pmd;
>> + unsigned int pmd_count = 0;
>> +
>> + CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
>> + if (pmd->core_id == NON_PMD_CORE_ID) {
>> + continue;
>> + }
>> + if (pmd->need_reload) {
>> + pmd_count++;
>> + }
>> + }
>> + atomic_count_set(&dp->reloading_pmds, pmd_count);
>> CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
>> if (pmd->need_reload) {
>> @@ -4652,6 +4669,10 @@ reload_affected_pmds(struct dp_netdev *dp)
>
> The above atomic_count_set() is a relaxed set, so is the
> atomic_store_relaxed(&pmd->reload, true) in dp_netdev_reload_pmd__(pmd).
> Which could lead to the PMDs decreasing the dp->reloading_pmd before it’s
> set. Guess if the correct memory_order is selected for pmd->reload as
> suggested by Ilya in patch 1/5 it should work.
relaxed memory ordering does not imply any synchronization and will
not be synchronized with 'reload' regardless of its memory ordering.
To guarantee the order, atomic_count_set() must be replaced with
non-relaxed version along with 'reload'.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
