[ovs-dev] [PATCH 1/1] ovs-thread: Fix thread id for threads not started with ovs_thread_create()
Ilya Maximets
i.maximets at samsung.com
Fri May 25 15:05:57 UTC 2018
On 25.05.2018 17:39, Eelco Chaudron wrote:
> On 25/05/18 15:30, Ilya Maximets 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
>>>
>>> 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
>>>
>>> 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.
>> Good catch. Thanks for working on this. I guess, this issue could even cause
>> a crash, because vhost device could be freed before other threads stop
>> working with it.
>>
>>> The fix below assigns UINT_MAX to none OVS created threads, which will
>>> fix this specific issue. However if more none OVS threads gets added
>>> the issue might arise again.
>>>
>>> Currently, I do not see another solution that will work unless DPDK is
>>> adding some framework/callback support when new threads get created.
>> What do you think about allocating ids on demand inside 'ovsthread_id_self()'?
> I was thinking about this also, but was not sure where to put it... Not sure why ovsthread_id_self() did not come to mind :)
>> This will work for any number of threads. Something like this:
>> ------------------------------------------------------------------------
>> diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
>> index f8bc06d..ff3a1df 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, UINT_MAX);
> Guess I'll add a #define for clearity on the UINT_MAX usage.
Yes, this should be useful.
>> struct ovsthread_aux {
>> void *(*start)(void *);
>> @@ -323,24 +323,28 @@ struct ovsthread_aux {
>> char name[16];
>> };
>> +void
>> +ovsthread_id_init(void)
>> +{
>> + static atomic_count next_id = ATOMIC_COUNT_INIT(0);
>> +
>> + unsigned int id = atomic_count_inc(&next_id);
>> +
>> + *ovsthread_id_get() = 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;
>>
> I would still call ovsthread_id_init() here explicitly, as its more clear.
ok.
>> aux = *auxp;
>> free(auxp);
>> /* The order of the following calls is important, because
>> * ovsrcu_quiesce_end() saves a copy of the thread name. */
>> - char *subprogram_name = xasprintf("%s%u", aux.name, id);
>> + char *subprogram_name = xasprintf("%s%u", aux.name, ovsthread_id_self());
>> set_subprogram_name(subprogram_name);
>> free(subprogram_name);
>> ovsrcu_quiesce_end();
>> diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
>> index 03fd804..ada09d1 100644
>> --- a/lib/ovs-thread.h
>> +++ b/lib/ovs-thread.h
>> @@ -467,12 +467,21 @@ void *ovsthread_getspecific(ovsthread_key_t);
>> DECLARE_EXTERN_PER_THREAD_DATA(unsigned int, ovsthread_id);
>> +void 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 == UINT_MAX)) {
>> + ovsthread_id_init();
>> + id = *ovsthread_id_get();
>> + }
>> +
>> + return id;
>> }
>>
>> /* Simulated global counter.
>> ------------------------------------------------------------------------
>
> Let me know and I can rework your above code/idea into a V2 patch.
Sure, thanks.
More information about the dev
mailing list