On 17/06/2016 18:33, Alex Bennée wrote: > Each time breakpoints are added/removed from the array it's done using > an read-copy-update cycle. Simultaneous writes are protected by the > debug_update_lock. > > Signed-off-by: Alex Bennée <alex.ben...@linaro.org> > --- > cpus.c | 3 + > exec.c | 167 > ++++++++++++++++++++++++++++++++++++++++++++---------- > include/qom/cpu.h | 42 ++++++++++---- > linux-user/main.c | 11 +--- > 4 files changed, 175 insertions(+), 48 deletions(-) > > diff --git a/cpus.c b/cpus.c > index 84c3520..b76300b 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -1441,6 +1441,9 @@ void qemu_init_vcpu(CPUState *cpu) > cpu_address_space_init(cpu, as, 0); > } > > + /* protects updates to debug info */
No need to comment here. > + qemu_mutex_init(&cpu->update_debug_lock); Missing qemu_mutex_destroy. > if (kvm_enabled()) { > qemu_kvm_start_vcpu(cpu); > } else if (tcg_enabled()) { > diff --git a/exec.c b/exec.c > index c8e8823..d1d14c1 100644 > --- a/exec.c > +++ b/exec.c > @@ -919,31 +919,94 @@ static inline bool > cpu_watchpoint_address_matches(CPUWatchpoint *wp, > } > > #endif > -/* Find watchpoint with external reference */ > -CPUBreakpoint *cpu_breakpoint_get_by_ref(CPUState *cpu, int ref) > + > +/* Create a working copy of the breakpoints. > + * > + * The rcu_read_lock() may already be held depending on where this > + * update has been triggered from. However it is safe to nest > + * rcu_read_locks() so we do the copy under the lock here. More precisely, the rcu_read_lock() and atomic_rcu_read() are unnecessary in update paths because these already take the update lock. However it is safe to nest rcu_read_lock() within itself or within the update lock, so your code is fine. > + */ > + > +static GArray *rcu_copy_breakpoints(CPUState *cpu) > { > - CPUBreakpoint *bp = NULL; > + GArray *old, *new; > + > + rcu_read_lock(); > + old = atomic_rcu_read(&cpu->breakpoints); > + if (old) { > + new = g_array_sized_new(false, false, sizeof(CPUBreakpoint), > old->len); > + memcpy(new->data, old->data, old->len * sizeof(CPUBreakpoint)); > + new->len = old->len; > + } else { > + new = g_array_new(false, false, sizeof(CPUBreakpoint)); > + } > + rcu_read_unlock(); > + > + return new; > +} > + > +struct BreakRCU { What about creating a generic g_array_free_rcu(GArray *array, gboolean free_segment)? Or better g_array_unref_rcu(GArray *array) since we require glib 2.22. > + struct rcu_head rcu; > + GArray *bkpts; > +}; > + > +/* RCU reclaim step */ > +static void rcu_free_breakpoints(struct BreakRCU *rcu_free) > +{ > + g_array_free(rcu_free->bkpts, false); Why false? Doesn't this leak? (g_array_unref is another valid replacement, as mentioned above). > + g_free(rcu_free); > +} > + > +/* Called with update lock held */ > +static void rcu_update_breakpoints(CPUState *cpu, GArray *new_bkpts) > +{ > + GArray *bpts = atomic_rcu_read(&cpu->breakpoints); > + atomic_rcu_set(&cpu->breakpoints, new_bkpts); > + > + if (bpts) { > + struct BreakRCU *rcu_free = g_malloc(sizeof(*rcu_free)); > + rcu_free->bkpts = bpts; > + call_rcu(rcu_free, rcu_free_breakpoints, rcu); > + } > +} > + > +/* Find watchpoint with external reference, only valid for duration of > + * rcu_read_lock */ > +static CPUBreakpoint *find_bkpt_with_ref(GArray *bkpts, int ref) breakpoint_get_by_ref, please. > +{ > + CPUBreakpoint *bp; > int i = 0; > + > do { > - bp = &g_array_index(cpu->breakpoints, CPUBreakpoint, i++); > - } while (i < cpu->breakpoints->len && bp && bp->ref != ref); > + bp = &g_array_index(bkpts, CPUBreakpoint, i); > + if (bp->ref == ref) { > + return bp; > + } > + } while (i++ < bkpts->len); > > - return bp; > + return NULL; > +} > + > +CPUBreakpoint *cpu_breakpoint_get_by_ref(CPUState *cpu, int ref) > +{ > + GArray *bkpts = atomic_rcu_read(&cpu->breakpoints); > + return find_bkpt_with_ref(bkpts, ref); > } > > /* Add a breakpoint. */ > int cpu_breakpoint_insert_with_ref(CPUState *cpu, vaddr pc, int flags, int > ref) > { > + GArray *bkpts; > CPUBreakpoint *bp = NULL; > > - /* Allocate if no previous breakpoints */ > - if (!cpu->breakpoints) { > - cpu->breakpoints = g_array_new(false, true, sizeof(CPUBreakpoint)); > - } > + qemu_mutex_lock(&cpu->update_debug_lock); > + > + /* This will allocate if no previous breakpoints */ > + bkpts = rcu_copy_breakpoints(cpu); > > /* Find old watchpoint */ > if (ref != BPWP_NOREF) { > - bp = cpu_breakpoint_get_by_ref(cpu, ref); > + bp = find_bkpt_with_ref(bkpts, ref); > } > > if (bp) { > @@ -958,14 +1021,17 @@ int cpu_breakpoint_insert_with_ref(CPUState *cpu, > vaddr pc, int flags, int ref) > > /* keep all GDB-injected breakpoints in front */ > if (flags & BP_GDB) { > - g_array_prepend_val(cpu->breakpoints, brk); > + g_array_prepend_val(bkpts, brk); > } else { > - g_array_append_val(cpu->breakpoints, brk); > + g_array_append_val(bkpts, brk); > } > } > > breakpoint_invalidate(cpu, pc); > > + rcu_update_breakpoints(cpu, bkpts); > + qemu_mutex_unlock(&cpu->update_debug_lock); > + > return 0; > } > > @@ -974,67 +1040,110 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, > int flags) > return cpu_breakpoint_insert_with_ref(cpu, pc, flags, BPWP_NOREF); > } > > -static void cpu_breakpoint_delete(CPUState *cpu, int index) > +/* Called with update_debug_lock held */ > +static void cpu_breakpoint_delete(CPUState *cpu, GArray *bkpts, int index) > { > CPUBreakpoint *bp; > - bp = &g_array_index(cpu->breakpoints, CPUBreakpoint, index); > + bp = &g_array_index(bkpts, CPUBreakpoint, index); > breakpoint_invalidate(cpu, bp->pc); > - g_array_remove_index(cpu->breakpoints, index); > + g_array_remove_index(bkpts, index); > } > > /* Remove a specific breakpoint. */ > int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags) > { > + GArray *bkpts = atomic_rcu_read(&cpu->breakpoints); > CPUBreakpoint *bp; > + int retval = -ENOENT; > > - if (unlikely(cpu->breakpoints) && unlikely(cpu->breakpoints->len)) { > + if (unlikely(bkpts) && unlikely(bkpts->len)) { > int i = 0; > + > + qemu_mutex_lock(&cpu->update_debug_lock); > + bkpts = rcu_copy_breakpoints(cpu); > + > do { > - bp = &g_array_index(cpu->breakpoints, CPUBreakpoint, i); > + bp = &g_array_index(bkpts, CPUBreakpoint, i); > if (bp && bp->pc == pc && bp->flags == flags) { > - cpu_breakpoint_delete(cpu, i); > + cpu_breakpoint_delete(cpu, bkpts, i); > + retval = 0; > } else { > i++; > } > - } while (i < cpu->breakpoints->len); > + } while (i < bkpts->len); > + > + rcu_update_breakpoints(cpu, bkpts); > + qemu_mutex_unlock(&cpu->update_debug_lock); > + > } > > - return -ENOENT; > + return retval; > } > > +#ifdef CONFIG_USER_ONLY > +void cpu_breakpoints_clone(CPUState *old_cpu, CPUState *new_cpu) > +{ > + GArray *bkpts = atomic_rcu_read(&old_cpu->breakpoints); > + > + if (unlikely(bkpts) && unlikely(bkpts->len)) { > + qemu_mutex_lock(&new_cpu->update_debug_lock); > + bkpts = rcu_copy_breakpoints(old_cpu); > + rcu_update_breakpoints(new_cpu, bkpts); > + qemu_mutex_unlock(&new_cpu->update_debug_lock); > + } > +} > +#endif > + > + > /* Remove a specific breakpoint by reference. */ > void cpu_breakpoint_remove_by_ref(CPUState *cpu, int ref) > { > + GArray *bkpts = atomic_rcu_read(&cpu->breakpoints); > CPUBreakpoint *bp; > > - if (unlikely(cpu->breakpoints) && unlikely(cpu->breakpoints->len)) { > + if (unlikely(bkpts) && unlikely(bkpts->len)) { > int i = 0; > + > + qemu_mutex_lock(&cpu->update_debug_lock); > + bkpts = rcu_copy_breakpoints(cpu); > + > do { > - bp = &g_array_index(cpu->breakpoints, CPUBreakpoint, i); > + bp = &g_array_index(bkpts, CPUBreakpoint, i); > if (bp && bp->ref == ref) { > - cpu_breakpoint_delete(cpu, i); > + cpu_breakpoint_delete(cpu, bkpts, i); > } else { > i++; > } > - } while (i < cpu->breakpoints->len); > + } while (i < bkpts->len); > + > + rcu_update_breakpoints(cpu, bkpts); > + qemu_mutex_unlock(&cpu->update_debug_lock); > } > } > > /* Remove all matching breakpoints. */ > void cpu_breakpoint_remove_all(CPUState *cpu, int mask) > { > + GArray *bkpts = atomic_rcu_read(&cpu->breakpoints); > CPUBreakpoint *bp; > > - if (unlikely(cpu->breakpoints) && unlikely(cpu->breakpoints->len)) { > + if (unlikely(bkpts) && unlikely(bkpts->len)) { > int i = 0; > + > + qemu_mutex_lock(&cpu->update_debug_lock); > + bkpts = rcu_copy_breakpoints(cpu); > + > do { > - bp = &g_array_index(cpu->breakpoints, CPUBreakpoint, i); > + bp = &g_array_index(bkpts, CPUBreakpoint, i); > if (bp->flags & mask) { > - cpu_breakpoint_delete(cpu, i); > + cpu_breakpoint_delete(cpu, bkpts, i); > } else { > i++; > } > - } while (i < cpu->breakpoints->len); > + } while (i < bkpts->len); > + > + rcu_update_breakpoints(cpu, bkpts); > + qemu_mutex_unlock(&cpu->update_debug_lock); > } > } > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index f695afb..51ca28c 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -326,8 +326,11 @@ struct CPUState { > /* Debugging support: > * > * Both the gdbstub and architectural debug support will add > - * references to these arrays. > + * references to these arrays. The list of breakpoints and > + * watchpoints are updated with RCU. The update_debug_lock is only > + * required for updating, not just reading. > */ > + QemuMutex update_debug_lock; > GArray *breakpoints; > GArray *watchpoints; > CPUWatchpoint *watchpoint_hit; > @@ -849,12 +852,13 @@ int cpu_breakpoint_insert_with_ref(CPUState *cpu, vaddr > pc, int flags, int ref); > * @cpu: CPU to monitor > * @ref: unique reference > * > - * @return: a pointer to working copy of the breakpoint. > + * @return: a pointer to working copy of the breakpoint or NULL. > * > - * Return a working copy of the current referenced breakpoint. This > - * obviously only works for breakpoints inserted with a reference. The > - * lifetime of this objected will be limited and should not be kept > - * beyond its immediate use. Otherwise return NULL. > + * Return short term working copy of the a breakpoint marked with an > + * external reference. This obviously only works for breakpoints > + * inserted with a reference. The lifetime of this object is the > + * duration of the rcu_read_lock() and it may be released when > + * rcu_synchronize is called. "and it may be released..." is implicit. > */ > CPUBreakpoint *cpu_breakpoint_get_by_ref(CPUState *cpu, int ref); > > @@ -871,14 +875,32 @@ void cpu_breakpoint_remove_by_ref(CPUState *cpu, int > ref); > > void cpu_breakpoint_remove_all(CPUState *cpu, int mask); > > -/* Return true if PC matches an installed breakpoint. */ > +#ifdef CONFIG_USER_ONLY > +/** > + * cpu_breakpoints_clone: > + * > + * @old_cpu: source of breakpoints > + * @new_cpu: destination for breakpoints > + * > + * When a new user-mode thread is created the CPU structure behind it > + * needs a copy of the exiting breakpoint conditions. This helper does > + * the copying. > + */ > +void cpu_breakpoints_clone(CPUState *old_cpu, CPUState *new_cpu); > +#endif > + > +/* Return true if PC matches an installed breakpoint. > + * Called while the RCU read lock is held. The "standard" phrasing is "Called from RCU critical section". > + */ > static inline bool cpu_breakpoint_test(CPUState *cpu, vaddr pc, int mask) > { > - if (unlikely(cpu->breakpoints) && unlikely(cpu->breakpoints->len)) { > + GArray *bkpts = atomic_rcu_read(&cpu->breakpoints); > + > + if (unlikely(bkpts) && unlikely(bkpts->len)) { > CPUBreakpoint *bp; > int i; > - for (i = 0; i < cpu->breakpoints->len; i++) { > - bp = &g_array_index(cpu->breakpoints, CPUBreakpoint, i); > + for (i = 0; i < bkpts->len; i++) { > + bp = &g_array_index(bkpts, CPUBreakpoint, i); > if (bp->pc == pc && (bp->flags & mask)) { > return true; > } > diff --git a/linux-user/main.c b/linux-user/main.c > index 179f2f2..2901626 100644 > --- a/linux-user/main.c > +++ b/linux-user/main.c > @@ -3808,17 +3808,10 @@ CPUArchState *cpu_copy(CPUArchState *env) > > memcpy(new_env, env, sizeof(CPUArchState)); > > - /* Clone all break/watchpoints. > + /* Clone all breakpoints. > Note: Once we support ptrace with hw-debug register access, make sure > BP_CPU break/watchpoints are handled correctly on clone. */ > - if (unlikely(cpu->breakpoints) && unlikely(cpu->breakpoints->len)) { > - CPUBreakpoint *bp; > - int i; > - for (i = 0; i < cpu->breakpoints->len; i++) { > - bp = &g_array_index(cpu->breakpoints, CPUBreakpoint, i); > - cpu_breakpoint_insert(new_cpu, bp->pc, bp->flags); > - } > - } > + cpu_breakpoints_clone(cpu, new_cpu); > if (unlikely(cpu->watchpoints) && unlikely(cpu->watchpoints->len)) { > CPUWatchpoint *wp; > int i; >