[ovs-dev] [PATCH v3 1/1] ovs-thread: Fix thread id for threads not started with ovs_thread_create()

Eelco Chaudron echaudro at redhat.com
Mon Jun 4 08:08:21 UTC 2018


Thanks Ilya for the comments below! I've send out a v4.

//Eelco

On 01/06/18 18:07, Ilya Maximets wrote:
> On 01.06.2018 16:18, Eelco Chaudron wrote:
>> When ping-pong'in a live VM migration between two machines running
>> OVS-DPDK every now and then the ping misses would increase
>> dramatically. For example:
>>
>> ===========Stream Rate: 3Mpps===========
>> No Stream_Rate Downtime Totaltime Ping_Loss Moongen_Loss
>>   0       3Mpps      128     13974       115      7168374
>>   1       3Mpps      145     13620        17      1169770
>>   2       3Mpps      140     14499       116      7141175
>>   3       3Mpps      142     13358        16      1150606
>>   4       3Mpps      136     14004        16      1124020
>>   5       3Mpps      139     15494       214     13170452
>>   6       3Mpps      136     15610       217     13282413
>>   7       3Mpps      146     13194        17      1167512
>>   8       3Mpps      148     12871        16      1162655
>>   9       3Mpps      137     15615       214     13170656
>>
>> I identified this issue being introduced in OVS commit
>> https://github.com/openvswitch/ovs/commit/f3e7ec254738364101eed8f04b1d954cb510615c
>> and more specific due to DPDK commit
>> http://dpdk.org/browse/dpdk/commit/?id=af14759181240120f76c82f894982e8f33f0ba2a
> Please, use following format for mentioning commits:
> git log -1 --pretty=format:"%h (\"%s\")" --abbrev=12 <HASH>
>
>> The combined changes no longer have OVS start the vhost socket polling
>> thread at startup, but DPDK will do it on its own when the first vhost
>> client is started.
>>
>> Figuring out the reason why this happens kept me puzzled for quite some time...
>> What happens is that the callbacks called from the vhost thread are
>> calling ovsrcu_synchronize() as part of destroy_device(). This will
>> end-up calling seq_wait__(), and the problem is with this part:
>>
>>     176static void
>>     177seq_wait__(struct seq *seq, uint64_t value, const char *where)
>>     178    OVS_REQUIRES(seq_mutex)
>>     179{
>>>> 180    unsigned int id = ovsthread_id_self();
>>     181    uint32_t hash = hash_int(id, 0);
>>     182    struct seq_waiter *waiter;
>>     183
>>     184    HMAP_FOR_EACH_IN_BUCKET (waiter, hmap_node, hash, &seq->waiters) {
>>>> 185        if (waiter->ovsthread_id == id) {
>>     186            if (waiter->value != value) {
>>     187                /* The current value is different from the value we've already
>>     188                 * waited for, */
>>     189                poll_immediate_wake_at(where);
>>     190            } else {
>>     191                /* Already waiting on 'value', nothing more to do. */
>>     192            }
>>>> 193            return;
>>     194        }
>>     195    }
>>     196
>>
> I'm not sure about including the full code snippet in commit-message.
> The text here already describes the issue well enough.
>
>> By default, all created threads outside of OVS will get thread id 0,
>> which is equal to the main ovs thread. So for example in the function
>> above if the main thread is waiting already we won't add ourselves as
>> a waiter.
>>
>> The fix below assigns UINT_MAX to none OVS created threads, which will
> Actually, it assigns OVSTHREAD_ID_UNSET to all threads.
>
>> get updated to a valid ID on the first call to ovsthread_id_self().
>>
>> v1 -> v2:
>> =========
>> Included Ilya's suggestion to automatically assign an ID in
>> ovsthread_id_self().
>>
>> v2 -> v3:
>> =========
>> Changed ovsthread_id_init() to assert when called twice
> Version history should go after '---' near to stats because it's not
> the information we need in the commit-message.
>
> Following tag needed here:
> Fixes: f3e7ec254738 ("Update relevant artifacts to add support for DPDK 17.05.1.")
>> Signed-off-by: Eelco Chaudron <echaudro at redhat.com>
>> ---
>>   lib/ovs-thread.c        | 16 +++++++++++-----
>>   lib/ovs-thread.h        | 12 +++++++++++-
>>   vswitchd/ovs-vswitchd.c |  2 ++
>>   3 files changed, 24 insertions(+), 6 deletions(-)
> The code in general looks good to me. One minor comment below.
>
>> diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
>> index f8bc06d38..c72bc543b 100644
>> --- a/lib/ovs-thread.c
>> +++ b/lib/ovs-thread.c
>> @@ -315,7 +315,7 @@ ovs_barrier_block(struct ovs_barrier *barrier)
>>       }
>>   }
>>   
>> -DEFINE_EXTERN_PER_THREAD_DATA(ovsthread_id, 0);
>> +DEFINE_EXTERN_PER_THREAD_DATA(ovsthread_id, OVSTHREAD_ID_UNSET);
>>   
>>   struct ovsthread_aux {
>>       void *(*start)(void *);
>> @@ -323,17 +323,23 @@ struct ovsthread_aux {
>>       char name[16];
>>   };
>>   
>> +unsigned int
>> +ovsthread_id_init(void)
>> +{
>> +    static atomic_count next_id = ATOMIC_COUNT_INIT(0);
>> +
>> +    ovs_assert(*ovsthread_id_get() == OVSTHREAD_ID_UNSET);
>> +    return *ovsthread_id_get() = atomic_count_inc(&next_id);
>> +}
>> +
>>   static void *
>>   ovsthread_wrapper(void *aux_)
>>   {
>> -    static atomic_count next_id = ATOMIC_COUNT_INIT(1);
>> -
>>       struct ovsthread_aux *auxp = aux_;
>>       struct ovsthread_aux aux;
>>       unsigned int id;
>>   
>> -    id = atomic_count_inc(&next_id);
>> -    *ovsthread_id_get() = id;
>> +    id = ovsthread_id_init();
>>   
>>       aux = *auxp;
>>       free(auxp);
>> diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
>> index 03fd80439..6b8c285a8 100644
>> --- a/lib/ovs-thread.h
>> +++ b/lib/ovs-thread.h
>> @@ -465,14 +465,24 @@ void *ovsthread_getspecific(ovsthread_key_t);
>>    * This thread ID avoids the problem.
>>    */
>>   
>> +#define OVSTHREAD_ID_UNSET UINT_MAX
>>   DECLARE_EXTERN_PER_THREAD_DATA(unsigned int, ovsthread_id);
>>   
>> +/* Initializes the unique per thread identifier if not done already */
> As we're not allowing to call init if id already initialized,
> "if not done already" should be removed from the comment.
>
>> +unsigned int ovsthread_id_init(void);
>> +
>>   /* Returns a per-thread identifier unique within the lifetime of the
>>    * process. */
>>   static inline unsigned int
>>   ovsthread_id_self(void)
>>   {
>> -    return *ovsthread_id_get();
>> +    unsigned int id = *ovsthread_id_get();
>> +
>> +    if (OVS_UNLIKELY(id == OVSTHREAD_ID_UNSET)) {
>> +        id = ovsthread_id_init();
>> +    }
>> +
>> +    return id;
>>   }
>>   
>>   /* Simulated global counter.
>> diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c
>> index 414b54780..46da45db9 100644
>> --- a/vswitchd/ovs-vswitchd.c
>> +++ b/vswitchd/ovs-vswitchd.c
>> @@ -39,6 +39,7 @@
>>   #include "ovsdb-idl.h"
>>   #include "ovs-rcu.h"
>>   #include "ovs-router.h"
>> +#include "ovs-thread.h"
>>   #include "openvswitch/poll-loop.h"
>>   #include "simap.h"
>>   #include "stream-ssl.h"
>> @@ -78,6 +79,7 @@ main(int argc, char *argv[])
>>       int retval;
>>   
>>       set_program_name(argv[0]);
>> +    ovsthread_id_init();
>>   
>>       ovs_cmdl_proctitle_init(argc, argv);
>>       service_start(&argc, &argv);
>>



More information about the dev mailing list