> 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()'? 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); 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; 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. ------------------------------------------------------------------------ _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev