| Message ID | 20220928081517.734954-27-laurent@vivier.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,01/38] linux-user: use 'max' instead of 'qemu32' / 'qemu64' by default | expand |
Hello Laurent, hello Peter, I see the Maximum number of active timers ist still 32. I hope it is not too late, but for my application this is not enough. Could this define be bigger i.e. 32*4? Thanks for your support! I really appreciate it! Jon Laurent Vivier <laurent@vivier.eu> igorleak hau idatzi zuen (2022 ira. 28, az. 10:15): > From: Peter Maydell <peter.maydell@linaro.org> > > For handling guest POSIX timers, we currently use an array > g_posix_timers[], whose entries are a host timer_t value, or 0 for > "this slot is unused". When the guest calls the timer_create syscall > we look through the array for a slot containing 0, and use that for > the new timer. > > This scheme assumes that host timer_t values can never be zero. This > is unfortunately not a valid assumption -- for some host libc > versions, timer_t values are simply indexes starting at 0. When > using this kind of host libc, the effect is that the first and second > timers end up sharing a slot, and so when the guest tries to operate > on the first timer it changes the second timer instead. > > Rework the timer allocation code, so that: > * the 'slot in use' indication uses a separate array from the > host timer_t array > * we grab the free slot atomically, to avoid races when multiple > threads call timer_create simultaneously > * releasing an allocated slot is abstracted out into a new > free_host_timer_slot() function called in the correct places > > This fixes: > * problems on hosts where timer_t 0 is valid > * the FIXME in next_free_host_timer() about locking > * bugs in the error paths in timer_create where we forgot to release > the slot we grabbed, or forgot to free the host timer > > Reported-by: Jon Alduan <jon.alduan@gmail.com> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > Message-Id: <20220725110035.1273441-1-peter.maydell@linaro.org> > Signed-off-by: Laurent Vivier <laurent@vivier.eu> > --- > linux-user/syscall.c | 24 ++++++++++++++++-------- > 1 file changed, 16 insertions(+), 8 deletions(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 54b29f3b406a..e0e0f058121f 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -525,20 +525,25 @@ _syscall4(int, sys_prlimit64, pid_t, pid, int, > resource, > > #if defined(TARGET_NR_timer_create) > /* Maximum of 32 active POSIX timers allowed at any one time. */ > -static timer_t g_posix_timers[32] = { 0, } ; > +#define GUEST_TIMER_MAX 32 > +static timer_t g_posix_timers[GUEST_TIMER_MAX]; > +static int g_posix_timer_allocated[GUEST_TIMER_MAX]; > > static inline int next_free_host_timer(void) > { > - int k ; > - /* FIXME: Does finding the next free slot require a lock? */ > - for (k = 0; k < ARRAY_SIZE(g_posix_timers); k++) { > - if (g_posix_timers[k] == 0) { > - g_posix_timers[k] = (timer_t) 1; > + int k; > + for (k = 0; k < ARRAY_SIZE(g_posix_timer_allocated); k++) { > + if (qatomic_xchg(g_posix_timer_allocated + k, 1) == 0) { > return k; > } > } > return -1; > } > + > +static inline void free_host_timer_slot(int id) > +{ > + qatomic_store_release(g_posix_timer_allocated + id, 0); > +} > #endif > > static inline int host_to_target_errno(int host_errno) > @@ -12896,15 +12901,18 @@ static abi_long do_syscall1(CPUArchState > *cpu_env, int num, abi_long arg1, > phost_sevp = &host_sevp; > ret = target_to_host_sigevent(phost_sevp, arg2); > if (ret != 0) { > + free_host_timer_slot(timer_index); > return ret; > } > } > > ret = get_errno(timer_create(clkid, phost_sevp, phtimer)); > if (ret) { > - phtimer = NULL; > + free_host_timer_slot(timer_index); > } else { > if (put_user(TIMER_MAGIC | timer_index, arg3, > target_timer_t)) { > + timer_delete(*phtimer); > + free_host_timer_slot(timer_index); > return -TARGET_EFAULT; > } > } > @@ -13040,7 +13048,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, > int num, abi_long arg1, > } else { > timer_t htimer = g_posix_timers[timerid]; > ret = get_errno(timer_delete(htimer)); > - g_posix_timers[timerid] = 0; > + free_host_timer_slot(timerid); > } > return ret; > } > -- > 2.37.3 > >
On Wed, 28 Sept 2022 at 09:23, Jon Alduan <jon.alduan@gmail.com> wrote: > > Hello Laurent, hello Peter, > > I see the Maximum number of active timers ist still 32. I hope it is not too late, but for my application this is not enough. Could this define be bigger i.e. 32*4? This is a separate issue from the "0 ID values" bug -- the limit defined in QEMU has always been 32, and this patch doesn't change that. There's no particular reason why we shouldn't raise the limit, though. If you file a bug we're less likely to forget about it. thanks -- PMM
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 54b29f3b406a..e0e0f058121f 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -525,20 +525,25 @@ _syscall4(int, sys_prlimit64, pid_t, pid, int, resource, #if defined(TARGET_NR_timer_create) /* Maximum of 32 active POSIX timers allowed at any one time. */ -static timer_t g_posix_timers[32] = { 0, } ; +#define GUEST_TIMER_MAX 32 +static timer_t g_posix_timers[GUEST_TIMER_MAX]; +static int g_posix_timer_allocated[GUEST_TIMER_MAX]; static inline int next_free_host_timer(void) { - int k ; - /* FIXME: Does finding the next free slot require a lock? */ - for (k = 0; k < ARRAY_SIZE(g_posix_timers); k++) { - if (g_posix_timers[k] == 0) { - g_posix_timers[k] = (timer_t) 1; + int k; + for (k = 0; k < ARRAY_SIZE(g_posix_timer_allocated); k++) { + if (qatomic_xchg(g_posix_timer_allocated + k, 1) == 0) { return k; } } return -1; } + +static inline void free_host_timer_slot(int id) +{ + qatomic_store_release(g_posix_timer_allocated + id, 0); +} #endif static inline int host_to_target_errno(int host_errno) @@ -12896,15 +12901,18 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1, phost_sevp = &host_sevp; ret = target_to_host_sigevent(phost_sevp, arg2); if (ret != 0) { + free_host_timer_slot(timer_index); return ret; } } ret = get_errno(timer_create(clkid, phost_sevp, phtimer)); if (ret) { - phtimer = NULL; + free_host_timer_slot(timer_index); } else { if (put_user(TIMER_MAGIC | timer_index, arg3, target_timer_t)) { + timer_delete(*phtimer); + free_host_timer_slot(timer_index); return -TARGET_EFAULT; } } @@ -13040,7 +13048,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1, } else { timer_t htimer = g_posix_timers[timerid]; ret = get_errno(timer_delete(htimer)); - g_posix_timers[timerid] = 0; + free_host_timer_slot(timerid); } return ret; }