| Message ID | 20230722113507.78332-2-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | accel/tcg: Take mmap_lock in load_atomic*_or_exit | expand |
On Sat, 22 Jul 2023 at 12:35, Richard Henderson <richard.henderson@linaro.org> wrote: > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/exec/exec-all.h | 10 ++++++++++ > bsd-user/mmap.c | 1 + > linux-user/mmap.c | 1 + > 3 files changed, 12 insertions(+) > > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index 5fa0687cd2..d02517e95f 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -629,6 +629,15 @@ void TSA_NO_TSA mmap_lock(void); > void TSA_NO_TSA mmap_unlock(void); > bool have_mmap_lock(void); > > +static inline void mmap_unlock_guard(void *unused) > +{ > + mmap_unlock(); > +} > + > +#define WITH_MMAP_LOCK_GUARD() \ > + for (int _mmap_lock_iter __attribute__((cleanup(mmap_unlock_guard))) \ > + = (mmap_lock(), 0); _mmap_lock_iter == 0; _mmap_lock_iter = 1) All our other WITH_FOO macros seem to use g_autoptr rather than a raw attribute((cleanup)); is it worth being consistent? (This one also doesn't allow nested uses, I think.) Either way Reviewed-by: Peter Maydell <peter.maydell@linaro.org> since it would be nice to fix this for the next rc. thanks -- PMM
On 7/23/23 15:18, Peter Maydell wrote: > On Sat, 22 Jul 2023 at 12:35, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> include/exec/exec-all.h | 10 ++++++++++ >> bsd-user/mmap.c | 1 + >> linux-user/mmap.c | 1 + >> 3 files changed, 12 insertions(+) >> >> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h >> index 5fa0687cd2..d02517e95f 100644 >> --- a/include/exec/exec-all.h >> +++ b/include/exec/exec-all.h >> @@ -629,6 +629,15 @@ void TSA_NO_TSA mmap_lock(void); >> void TSA_NO_TSA mmap_unlock(void); >> bool have_mmap_lock(void); >> >> +static inline void mmap_unlock_guard(void *unused) >> +{ >> + mmap_unlock(); >> +} >> + >> +#define WITH_MMAP_LOCK_GUARD() \ >> + for (int _mmap_lock_iter __attribute__((cleanup(mmap_unlock_guard))) \ >> + = (mmap_lock(), 0); _mmap_lock_iter == 0; _mmap_lock_iter = 1) > > All our other WITH_FOO macros seem to use g_autoptr rather than > a raw attribute((cleanup)); is it worth being consistent? I didn't think it worthwhile, no, since that requires even more boilerplate. > (This one also doesn't allow nested uses, I think.) It does, since each variable will shadow the next within each context. r~
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index 5fa0687cd2..d02517e95f 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -629,6 +629,15 @@ void TSA_NO_TSA mmap_lock(void); void TSA_NO_TSA mmap_unlock(void); bool have_mmap_lock(void); +static inline void mmap_unlock_guard(void *unused) +{ + mmap_unlock(); +} + +#define WITH_MMAP_LOCK_GUARD() \ + for (int _mmap_lock_iter __attribute__((cleanup(mmap_unlock_guard))) \ + = (mmap_lock(), 0); _mmap_lock_iter == 0; _mmap_lock_iter = 1) + /** * adjust_signal_pc: * @pc: raw pc from the host signal ucontext_t. @@ -683,6 +692,7 @@ G_NORETURN void cpu_loop_exit_sigbus(CPUState *cpu, target_ulong addr, #else static inline void mmap_lock(void) {} static inline void mmap_unlock(void) {} +#define WITH_MMAP_LOCK_GUARD() void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length); void tlb_set_dirty(CPUState *cpu, vaddr addr); diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c index aca8764356..74ed00b9fe 100644 --- a/bsd-user/mmap.c +++ b/bsd-user/mmap.c @@ -32,6 +32,7 @@ void mmap_lock(void) void mmap_unlock(void) { + assert(mmap_lock_count > 0); if (--mmap_lock_count == 0) { pthread_mutex_unlock(&mmap_mutex); } diff --git a/linux-user/mmap.c b/linux-user/mmap.c index 44b53bd446..a5dfb56545 100644 --- a/linux-user/mmap.c +++ b/linux-user/mmap.c @@ -36,6 +36,7 @@ void mmap_lock(void) void mmap_unlock(void) { + assert(mmap_lock_count > 0); if (--mmap_lock_count == 0) { pthread_mutex_unlock(&mmap_mutex); }
Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- include/exec/exec-all.h | 10 ++++++++++ bsd-user/mmap.c | 1 + linux-user/mmap.c | 1 + 3 files changed, 12 insertions(+)