On 08/07/16 14:02, Paolo Bonzini wrote: >> On 08/07/16 11:40, Paolo Bonzini wrote: >>> Even better: add a "bool *tb_locked" argument to tb_find_slow, and >>> don't move the mmap_lock release. Then tb_find_fast knows directly >>> whether tb_lock is taken, and you don't need any of tb_lock_reset >>> or mmap_lock_reset. >> I think we can do even better. One option is using a separate tiny lock >> to protect direct jump set/reset instead of tb_lock. > If you have to use a separate tiny lock, you don't gain anything compared > to the two critical sections, do you?
If we have a separate lock for direct jump set/reset then we can do fast TB lookup + direct jump patching without taking tb_lock at all. How much this would reduce lock contention largely depends on the workload we use. > > In any case, this seems to be more complex than necessary. The "bool *" > convention is pretty common in Linux for example---it works well. It could work, no doubts. > > The one below is even more complicated. I'm all for simple lock-free > stacks (e.g. QSLIST_INSERT_HEAD_ATOMIC and QSLIST_MOVE_ATOMIC), but lock-free > lists are too much, especially if with the complicated first/next mechanism > of TCG's chained block lists. Direct jump handling code is pretty isolated and self-contained. It would require to back out of tb_remove_from_jmp_list() and sprinkle a couple of atomic_rcu_read()/atomic_rcu_set() with some comments, I think. Maybe it could be easier to justify looking at actual patches. Thanks, Sergey > > Paolo > >> Another option which I've had in my mind for some time is to make direct >> jump set/reset thread-safe. We already have thread-safe TB patching. The >> only question is the right order of operations and handling >> jmp_list_next/jmp_list_first safely. I think that could be done by >> removing tb_remove_from_jmp_list() and making RCU-like manipulation with >> jmp_list_next/jmp_list_first. What do you think? >> >> Kind regards, >> Sergey >>