[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC 7/7] watchpoints: put watchpoints under RCU contro
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [RFC 7/7] watchpoints: put watchpoints under RCU control |
Date: |
Fri, 17 Jun 2016 19:10:31 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 |
On 17/06/2016 18:33, Alex Bennée wrote:
> Each time watchpoints 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 <address@hidden>
> ---
> cpu-exec.c | 7 ++-
> exec.c | 193
> ++++++++++++++++++++++++++++++++++++++++++++-----------------
> 2 files changed, 144 insertions(+), 56 deletions(-)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 2736a27..7fcb500 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -387,12 +387,13 @@ static inline bool cpu_handle_halt(CPUState *cpu)
> static inline void cpu_handle_debug_exception(CPUState *cpu)
> {
> CPUClass *cc = CPU_GET_CLASS(cpu);
> + GArray *wpts = atomic_rcu_read(&cpu->watchpoints);
> CPUWatchpoint *wp;
> int i;
>
> - if (!cpu->watchpoint_hit && cpu->watchpoints) {
> - for (i = 0; i < cpu->watchpoints->len; i++) {
> - wp = &g_array_index(cpu->watchpoints, CPUWatchpoint, i);
> + if (!cpu->watchpoint_hit && wpts) {
> + for (i = 0; i < wpts->len; i++) {
> + wp = &g_array_index(wpts, CPUWatchpoint, i);
> wp->flags &= ~BP_WATCHPOINT_HIT;
> }
> }
> diff --git a/exec.c b/exec.c
> index d1d14c1..b9167ec 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -735,6 +735,18 @@ static void breakpoint_invalidate(CPUState *cpu,
> target_ulong pc)
> }
> #endif
>
> +struct FreeDebugRCU {
> + struct rcu_head rcu;
> + GArray *debug;
> +};
Deja vu? ;)
> +/* RCU reclaim step */
> +static void rcu_debug_free(struct FreeDebugRCU *rcu_free)
> +{
> + g_array_free(rcu_free->debug, false);
> + g_free(rcu_free);
> +}
> +
> #if defined(CONFIG_USER_ONLY)
> void cpu_watchpoint_remove_all(CPUState *cpu, int mask)
>
> @@ -766,22 +778,72 @@ CPUWatchpoint *cpu_watchpoint_get_by_ref(CPUState *cpu,
> int ref)
> return NULL;
> }
> #else
> -/* Find watchpoint with external reference */
> -CPUWatchpoint *cpu_watchpoint_get_by_ref(CPUState *cpu, int ref)
> +/* Create a working copy of the watchpoints.
> + *
> + * 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.
Same as patch 5.
> + */
> +
> +static GArray *rcu_copy_watchpoints(CPUState *cpu)
> {
> - CPUWatchpoint *wp = NULL;
> + GArray *old, *new;
> +
> + rcu_read_lock();
> + old = atomic_rcu_read(&cpu->watchpoints);
> + if (old) {
> + new = g_array_sized_new(false, false, sizeof(CPUWatchpoint),
> old->len);
> + memcpy(new->data, old->data, old->len * sizeof(CPUWatchpoint));
> + new->len = old->len;
> + } else {
> + new = g_array_new(false, false, sizeof(CPUWatchpoint));
> + }
> + rcu_read_unlock();
> +
> + return new;
> +}
> +
> +/* Called with update lock held */
> +static void rcu_update_watchpoints(CPUState *cpu, GArray *new_watchpts)
> +{
> + GArray *wpts = atomic_rcu_read(&cpu->watchpoints);
> + atomic_rcu_set(&cpu->watchpoints, new_watchpts);
> + if (wpts) {
> + struct FreeDebugRCU *rcu_free = g_malloc(sizeof(*rcu_free));
> + rcu_free->debug = wpts;
> + call_rcu(rcu_free, rcu_debug_free, rcu);
> + }
> +}
> +
> +/* Find watchpoint with external reference, only valid for duration of
> + * rcu_read_lock */
> +static CPUWatchpoint *find_watch_with_ref(GArray *wpts, int ref)
watchpoint_get_by_ref.
BTW, should the "ref" be unsigned?
> +{
> + CPUWatchpoint *wp;
> int i = 0;
> +
> do {
> - wp = &g_array_index(cpu->watchpoints, CPUWatchpoint, i++);
> - } while (i < cpu->watchpoints->len && wp && wp->ref != ref);
> + wp = &g_array_index(wpts, CPUWatchpoint, i);
> + if (wp->ref == ref) {
> + return wp;
> + }
> + } while (i++ < wpts->len);
> +
> + return NULL;
> +}
>
> - return wp;
> +/* Find watchpoint with external reference */
> +CPUWatchpoint *cpu_watchpoint_get_by_ref(CPUState *cpu, int ref)
> +{
> + GArray *wpts = atomic_rcu_read(&cpu->watchpoints);
> + return find_watch_with_ref(wpts, ref);
> }
>
> /* Add a watchpoint. */
> int cpu_watchpoint_insert_with_ref(CPUState *cpu, vaddr addr, vaddr len,
> int flags, int ref)
> {
> + GArray *wpts;
> CPUWatchpoint *wp = NULL;
>
> /* forbid ranges which are empty or run off the end of the address space
> */
> @@ -791,14 +853,14 @@ int cpu_watchpoint_insert_with_ref(CPUState *cpu, vaddr
> addr, vaddr len,
> return -EINVAL;
> }
>
> - /* Allocate if no previous watchpoints */
> - if (!cpu->watchpoints) {
> - cpu->watchpoints = g_array_new(false, true, sizeof(CPUWatchpoint));
> - }
> + qemu_mutex_lock(&cpu->update_debug_lock);
> +
> + /* This will allocate if no previous breakpoints */
^^^^^^^^^^^
watchpoints
> + wpts = rcu_copy_watchpoints(cpu);
In Linux it's usual to just call the array "new" (and if you need it
"old"). I like it more than bpts and wpts.
>
> /* Find old watchpoint */
> if (ref != BPWP_NOREF) {
> - wp = cpu_watchpoint_get_by_ref(cpu, ref);
> + wp = find_watch_with_ref(wpts, ref);
> }
>
> if (wp) {
> @@ -815,15 +877,18 @@ int cpu_watchpoint_insert_with_ref(CPUState *cpu, vaddr
> addr, vaddr len,
>
> /* keep all GDB-injected watchpoints in front */
> if (flags & BP_GDB) {
> - g_array_prepend_val(cpu->watchpoints, watch);
> + g_array_prepend_val(wpts, watch);
> } else {
> - g_array_append_val(cpu->watchpoints, watch);
> + g_array_append_val(wpts, watch);
> }
> }
>
>
> tlb_flush_page(cpu, addr);
>
> + rcu_update_watchpoints(cpu, wpts);
> + qemu_mutex_unlock(&cpu->update_debug_lock);
> +
> return 0;
> }
>
> @@ -832,69 +897,101 @@ int cpu_watchpoint_insert(CPUState *cpu, vaddr addr,
> vaddr len, int flags)
> return cpu_watchpoint_insert_with_ref(cpu, addr, len, flags, BPWP_NOREF);
> }
>
> -static void cpu_watchpoint_delete(CPUState *cpu, int index)
> +static void cpu_watchpoint_delete(CPUState *cpu, GArray *wpts, int index)
Same here, just call the GArray "new".
> {
> CPUWatchpoint *wp;
> - wp = &g_array_index(cpu->watchpoints, CPUWatchpoint, index);
> + wp = &g_array_index(wpts, CPUWatchpoint, index);
> tlb_flush_page(cpu, wp->vaddr);
> - g_array_remove_index(cpu->watchpoints, index);
> + g_array_remove_index(wpts, index);
> }
>
> /* Remove a specific watchpoint. */
> int cpu_watchpoint_remove(CPUState *cpu, vaddr addr, vaddr len,
> int flags)
> {
> + GArray *wpts = atomic_rcu_read(&cpu->watchpoints);
old...
> CPUWatchpoint *wp;
> - int i = 0;
> + int retval = -ENOENT;
> +
> + if (unlikely(wpts) && unlikely(wpts->len)) {
If this function is called, it's not so unlikely that cpu->watchpoints
is NULL!
> + int i = 0;
> +
> + qemu_mutex_lock(&cpu->update_debug_lock);
> + wpts = rcu_copy_watchpoints(cpu);
... and new.
>
> - if (unlikely(cpu->watchpoints) && unlikely(cpu->watchpoints->len)) {
> do {
> wp = &g_array_index(cpu->watchpoints, CPUWatchpoint, i);
> if (wp && addr == wp->vaddr && len == wp->len
> && flags == (wp->flags & ~BP_WATCHPOINT_HIT)) {
> - cpu_watchpoint_delete(cpu, i);
> - return 0;
> + cpu_watchpoint_delete(cpu, wpts, i);
> + retval = 0;
> + } else {
> + i++;
> }
> - } while (i++ < cpu->watchpoints->len);
> + } while (i < wpts->len);
Please write the iteration in the same way from the beginning, to keep
this patch as small as possible.
> + rcu_update_watchpoints(cpu, wpts);
Or free and set it to NULL if there are no watchpoints at all?
> + qemu_mutex_unlock(&cpu->update_debug_lock);
> +
> }
>
> - return -ENOENT;
> + return retval;
> }
>
> /* Remove a specific watchpoint by external reference. */
> int cpu_watchpoint_remove_by_ref(CPUState *cpu, int ref)
> {
> + GArray *wpts = atomic_rcu_read(&cpu->watchpoints);
> CPUWatchpoint *wp;
> - int i = 0;
> + int retval = -ENOENT;
> +
> + if (unlikely(wpts) && unlikely(wpts->len)) {
> + int i = 0;
> +
> + qemu_mutex_lock(&cpu->update_debug_lock);
> + wpts = rcu_copy_watchpoints(cpu);
>
> - if (unlikely(cpu->watchpoints) && unlikely(cpu->watchpoints->len)) {
> do {
> - wp = &g_array_index(cpu->watchpoints, CPUWatchpoint, i);
> + wp = &g_array_index(wpts, CPUWatchpoint, i);
> if (wp && wp->ref == ref) {
> - cpu_watchpoint_delete(cpu, i);
> - return 0;
> + cpu_watchpoint_delete(cpu, wpts, i);
> + retval = 0;
> + } else {
> + i++;
> }
> - } while (wp && i++ < cpu->watchpoints->len);
> + } while (i < wpts->len);
> +
> + rcu_update_watchpoints(cpu, wpts);
Same here, free and set it to NULL if the last watchpoint has been removed?
A few of these suggestions probably apply to breakpoints as well.
> + qemu_mutex_unlock(&cpu->update_debug_lock);
> +
> }
>
> - return -ENOENT;
> + return retval;
> }
>
> /* Remove all matching watchpoints. */
> void cpu_watchpoint_remove_all(CPUState *cpu, int mask)
> {
> + GArray *wpts = atomic_rcu_read(&cpu->watchpoints);
> CPUWatchpoint *wp;
>
> - if (unlikely(cpu->watchpoints) && unlikely(cpu->watchpoints->len)) {
> - int i = cpu->watchpoints->len;
> + if (unlikely(wpts) && unlikely(wpts->len)) {
> + int i = 0;
> +
> + qemu_mutex_lock(&cpu->update_debug_lock);
> + wpts = rcu_copy_watchpoints(cpu);
> +
> do {
> - wp = &g_array_index(cpu->watchpoints, CPUWatchpoint, i);
> + wp = &g_array_index(wpts, CPUWatchpoint, i);
> if (wp->flags & mask) {
> - cpu_watchpoint_delete(cpu, i);
> + cpu_watchpoint_delete(cpu, wpts, i);
> } else {
> - i--;
> + i++;
> }
> - } while (cpu->watchpoints->len && i >= 0);
> + } while (i < wpts->len);
> +
> + rcu_update_watchpoints(cpu, wpts);
> + qemu_mutex_unlock(&cpu->update_debug_lock);
> }
> }
>
> @@ -945,18 +1042,6 @@ static GArray *rcu_copy_breakpoints(CPUState *cpu)
> return new;
> }
>
> -struct BreakRCU {
> - 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);
> - g_free(rcu_free);
> -}
> -
> /* Called with update lock held */
> static void rcu_update_breakpoints(CPUState *cpu, GArray *new_bkpts)
> {
> @@ -964,9 +1049,9 @@ static void rcu_update_breakpoints(CPUState *cpu, GArray
> *new_bkpts)
> 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);
> + struct FreeDebugRCU *rcu_free = g_malloc(sizeof(*rcu_free));
> + rcu_free->debug = bpts;
> + call_rcu(rcu_free, rcu_debug_free, rcu);
> }
> }
>
> @@ -2296,6 +2381,7 @@ static void check_watchpoint(int offset, int len,
> MemTxAttrs attrs, int flags)
> target_ulong pc, cs_base;
> target_ulong vaddr;
> uint32_t cpu_flags;
> + GArray *wpts;
>
> if (cpu->watchpoint_hit) {
> /* We re-entered the check after replacing the TB. Now raise
> @@ -2306,11 +2392,12 @@ static void check_watchpoint(int offset, int len,
> MemTxAttrs attrs, int flags)
> }
> vaddr = (cpu->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
>
> - if (unlikely(cpu->watchpoints) && unlikely(cpu->watchpoints->len)) {
> + wpts = atomic_rcu_read(&cpu->watchpoints);
> + if (unlikely(wpts) && unlikely(wpts->len)) {
> CPUWatchpoint *wp;
> int i;
> - for (i = 0; i < cpu->watchpoints->len; i++) {
> - wp = &g_array_index(cpu->watchpoints, CPUWatchpoint, i);
> + for (i = 0; i < wpts->len; i++) {
> + wp = &g_array_index(wpts, CPUWatchpoint, i);
> if (cpu_watchpoint_address_matches(wp, vaddr, len)
> && (wp->flags & flags)) {
> if (flags == BP_MEM_READ) {
>
- [Qemu-devel] [RFC 5/7] breakpoints: put breakpoints under RCU control, (continued)
- [Qemu-devel] [RFC 5/7] breakpoints: put breakpoints under RCU control, Alex Bennée, 2016/06/17
- [Qemu-devel] [RFC 3/7] exec: keep CPUBreakpoint references internal, Alex Bennée, 2016/06/17
- [Qemu-devel] [RFC 6/7] linux-user: don't clone watchpoints, Alex Bennée, 2016/06/17
- [Qemu-devel] [RFC 4/7] break/watchpoints: store inside array, Alex Bennée, 2016/06/17
- [Qemu-devel] [RFC 2/7] exec: keep CPUWatchpoint references internal, Alex Bennée, 2016/06/17
- [Qemu-devel] [RFC 7/7] watchpoints: put watchpoints under RCU control, Alex Bennée, 2016/06/17
- Re: [Qemu-devel] [RFC 7/7] watchpoints: put watchpoints under RCU control,
Paolo Bonzini <=
- Re: [Qemu-devel] [RFC 0/7] Safe watch and breakpoint manipulation, Paolo Bonzini, 2016/06/17
- Re: [Qemu-devel] [RFC 0/7] Safe watch and breakpoint manipulation, Sergey Fedorov, 2016/06/20