From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43204) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDxI2-000114-G0 for qemu-devel@nongnu.org; Fri, 17 Jun 2016 13:10:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bDxI0-0002SF-7y for qemu-devel@nongnu.org; Fri, 17 Jun 2016 13:10:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52748) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDxHz-0002Rx-PQ for qemu-devel@nongnu.org; Fri, 17 Jun 2016 13:10:40 -0400 References: <1466181227-14934-1-git-send-email-alex.bennee@linaro.org> <1466181227-14934-8-git-send-email-alex.bennee@linaro.org> From: Paolo Bonzini Message-ID: Date: Fri, 17 Jun 2016 19:10:31 +0200 MIME-Version: 1.0 In-Reply-To: <1466181227-14934-8-git-send-email-alex.bennee@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC 7/7] watchpoints: put watchpoints under RCU control List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Alex_Benn=c3=a9e?= , mttcg@greensocs.com, qemu-devel@nongnu.org, fred.konrad@greensocs.com, a.rigo@virtualopensystems.com, serge.fdrv@gmail.com, cota@braap.org, bobby.prani@gmail.com Cc: mark.burton@greensocs.com, jan.kiszka@siemens.com, rth@twiddle.net, peter.maydell@linaro.org, claudio.fontana@huawei.com, Peter Crosthwaite On 17/06/2016 18:33, Alex Benn=C3=A9e 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. >=20 > Signed-off-by: Alex Benn=C3=A9e > --- > cpu-exec.c | 7 ++- > exec.c | 193 ++++++++++++++++++++++++++++++++++++++++++++---------= -------- > 2 files changed, 144 insertions(+), 56 deletions(-) >=20 > 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 =3D CPU_GET_CLASS(cpu); > + GArray *wpts =3D atomic_rcu_read(&cpu->watchpoints); > CPUWatchpoint *wp; > int i; > =20 > - if (!cpu->watchpoint_hit && cpu->watchpoints) { > - for (i =3D 0; i < cpu->watchpoints->len; i++) { > - wp =3D &g_array_index(cpu->watchpoints, CPUWatchpoint, i); > + if (!cpu->watchpoint_hit && wpts) { > + for (i =3D 0; i < wpts->len; i++) { > + wp =3D &g_array_index(wpts, CPUWatchpoint, i); > wp->flags &=3D ~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, t= arget_ulong pc) > } > #endif > =20 > +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) > =20 > @@ -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 =3D NULL; > + GArray *old, *new; > + > + rcu_read_lock(); > + old =3D atomic_rcu_read(&cpu->watchpoints); > + if (old) { > + new =3D g_array_sized_new(false, false, sizeof(CPUWatchpoint),= old->len); > + memcpy(new->data, old->data, old->len * sizeof(CPUWatchpoint))= ; > + new->len =3D old->len; > + } else { > + new =3D 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 =3D atomic_rcu_read(&cpu->watchpoints); > + atomic_rcu_set(&cpu->watchpoints, new_watchpts); > + if (wpts) { > + struct FreeDebugRCU *rcu_free =3D g_malloc(sizeof(*rcu_free)); > + rcu_free->debug =3D 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 =3D 0; > + > do { > - wp =3D &g_array_index(cpu->watchpoints, CPUWatchpoint, i++); > - } while (i < cpu->watchpoints->len && wp && wp->ref !=3D ref); > + wp =3D &g_array_index(wpts, CPUWatchpoint, i); > + if (wp->ref =3D=3D ref) { > + return wp; > + } > + } while (i++ < wpts->len); > + > + return NULL; > +} > =20 > - return wp; > +/* Find watchpoint with external reference */ > +CPUWatchpoint *cpu_watchpoint_get_by_ref(CPUState *cpu, int ref) > +{ > + GArray *wpts =3D atomic_rcu_read(&cpu->watchpoints); > + return find_watch_with_ref(wpts, ref); > } > =20 > /* Add a watchpoint. */ > int cpu_watchpoint_insert_with_ref(CPUState *cpu, vaddr addr, vaddr le= n, > int flags, int ref) > { > + GArray *wpts; > CPUWatchpoint *wp =3D NULL; > =20 > /* 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; > } > =20 > - /* Allocate if no previous watchpoints */ > - if (!cpu->watchpoints) { > - cpu->watchpoints =3D g_array_new(false, true, sizeof(CPUWatchp= oint)); > - } > + qemu_mutex_lock(&cpu->update_debug_lock); > + > + /* This will allocate if no previous breakpoints */ ^^^^^^^^^^^ watchpoints > + wpts =3D 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. > =20 > /* Find old watchpoint */ > if (ref !=3D BPWP_NOREF) { > - wp =3D cpu_watchpoint_get_by_ref(cpu, ref); > + wp =3D find_watch_with_ref(wpts, ref); > } > =20 > if (wp) { > @@ -815,15 +877,18 @@ int cpu_watchpoint_insert_with_ref(CPUState *cpu,= vaddr addr, vaddr len, > =20 > /* 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); > } > } > =20 > =20 > tlb_flush_page(cpu, addr); > =20 > + rcu_update_watchpoints(cpu, wpts); > + qemu_mutex_unlock(&cpu->update_debug_lock); > + > return 0; > } > =20 > @@ -832,69 +897,101 @@ int cpu_watchpoint_insert(CPUState *cpu, vaddr a= ddr, vaddr len, int flags) > return cpu_watchpoint_insert_with_ref(cpu, addr, len, flags, BPWP_= NOREF); > } > =20 > -static void cpu_watchpoint_delete(CPUState *cpu, int index) > +static void cpu_watchpoint_delete(CPUState *cpu, GArray *wpts, int ind= ex) Same here, just call the GArray "new". > { > CPUWatchpoint *wp; > - wp =3D &g_array_index(cpu->watchpoints, CPUWatchpoint, index); > + wp =3D &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); > } > =20 > /* Remove a specific watchpoint. */ > int cpu_watchpoint_remove(CPUState *cpu, vaddr addr, vaddr len, > int flags) > { > + GArray *wpts =3D atomic_rcu_read(&cpu->watchpoints); old... > CPUWatchpoint *wp; > - int i =3D 0; > + int retval =3D -ENOENT; > + > + if (unlikely(wpts) && unlikely(wpts->len)) { If this function is called, it's not so unlikely that cpu->watchpoints is NULL! > + int i =3D 0; > + > + qemu_mutex_lock(&cpu->update_debug_lock); > + wpts =3D rcu_copy_watchpoints(cpu); ... and new. > =20 > - if (unlikely(cpu->watchpoints) && unlikely(cpu->watchpoints->len))= { > do { > wp =3D &g_array_index(cpu->watchpoints, CPUWatchpoint, i); > if (wp && addr =3D=3D wp->vaddr && len =3D=3D wp->len > && flags =3D=3D (wp->flags & ~BP_WATCHPOINT_HIT)) { > - cpu_watchpoint_delete(cpu, i); > - return 0; > + cpu_watchpoint_delete(cpu, wpts, i); > + retval =3D 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); > + > } > =20 > - return -ENOENT; > + return retval; > } > =20 > /* Remove a specific watchpoint by external reference. */ > int cpu_watchpoint_remove_by_ref(CPUState *cpu, int ref) > { > + GArray *wpts =3D atomic_rcu_read(&cpu->watchpoints); > CPUWatchpoint *wp; > - int i =3D 0; > + int retval =3D -ENOENT; > + > + if (unlikely(wpts) && unlikely(wpts->len)) { > + int i =3D 0; > + > + qemu_mutex_lock(&cpu->update_debug_lock); > + wpts =3D rcu_copy_watchpoints(cpu); > =20 > - if (unlikely(cpu->watchpoints) && unlikely(cpu->watchpoints->len))= { > do { > - wp =3D &g_array_index(cpu->watchpoints, CPUWatchpoint, i); > + wp =3D &g_array_index(wpts, CPUWatchpoint, i); > if (wp && wp->ref =3D=3D ref) { > - cpu_watchpoint_delete(cpu, i); > - return 0; > + cpu_watchpoint_delete(cpu, wpts, i); > + retval =3D 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 remove= d? A few of these suggestions probably apply to breakpoints as well. > + qemu_mutex_unlock(&cpu->update_debug_lock); > + > } > =20 > - return -ENOENT; > + return retval; > } > =20 > /* Remove all matching watchpoints. */ > void cpu_watchpoint_remove_all(CPUState *cpu, int mask) > { > + GArray *wpts =3D atomic_rcu_read(&cpu->watchpoints); > CPUWatchpoint *wp; > =20 > - if (unlikely(cpu->watchpoints) && unlikely(cpu->watchpoints->len))= { > - int i =3D cpu->watchpoints->len; > + if (unlikely(wpts) && unlikely(wpts->len)) { > + int i =3D 0; > + > + qemu_mutex_lock(&cpu->update_debug_lock); > + wpts =3D rcu_copy_watchpoints(cpu); > + > do { > - wp =3D &g_array_index(cpu->watchpoints, CPUWatchpoint, i); > + wp =3D &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 >=3D 0); > + } while (i < wpts->len); > + > + rcu_update_watchpoints(cpu, wpts); > + qemu_mutex_unlock(&cpu->update_debug_lock); > } > } > =20 > @@ -945,18 +1042,6 @@ static GArray *rcu_copy_breakpoints(CPUState *cpu= ) > return new; > } > =20 > -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); > =20 > if (bpts) { > - struct BreakRCU *rcu_free =3D g_malloc(sizeof(*rcu_free)); > - rcu_free->bkpts =3D bpts; > - call_rcu(rcu_free, rcu_free_breakpoints, rcu); > + struct FreeDebugRCU *rcu_free =3D g_malloc(sizeof(*rcu_free)); > + rcu_free->debug =3D bpts; > + call_rcu(rcu_free, rcu_debug_free, rcu); > } > } > =20 > @@ -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; > =20 > 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 le= n, MemTxAttrs attrs, int flags) > } > vaddr =3D (cpu->mem_io_vaddr & TARGET_PAGE_MASK) + offset; > =20 > - if (unlikely(cpu->watchpoints) && unlikely(cpu->watchpoints->len))= { > + wpts =3D atomic_rcu_read(&cpu->watchpoints); > + if (unlikely(wpts) && unlikely(wpts->len)) { > CPUWatchpoint *wp; > int i; > - for (i =3D 0; i < cpu->watchpoints->len; i++) { > - wp =3D &g_array_index(cpu->watchpoints, CPUWatchpoint, i); > + for (i =3D 0; i < wpts->len; i++) { > + wp =3D &g_array_index(wpts, CPUWatchpoint, i); > if (cpu_watchpoint_address_matches(wp, vaddr, len) > && (wp->flags & flags)) { > if (flags =3D=3D BP_MEM_READ) { >=20