On Wed, May 04, 2022 at 12:14:01AM +0200, Alexander Bluhm wrote:
> Hi,
>
> We have one comment that locking for ratecheck(9) is missing. In
> all other places locking status of the struct timeval *lasttime
> is unclear.
>
> The easiest fix is a global mutex for all lasttime in ratecheck().
> This covers the usual usecase of the function.
>
> Same for ppsratecheck(9), lasttime and curpps are protected.
>
> Remove a useless #if 1 while there.
>
> ok?
For now this is the quickest way to move forward. OK claudio@
It would be nice to make ratecheck() and ppsratecheck() only require a
lock in the unusual case of hitting the limit. At least in the common case
these calls should be as cheap as possible.
This is a nice little project for someone to explore how to implement this
in a fast way that does not introduce bus locks or other slow operations.
> Index: kern/kern_malloc.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_malloc.c,v
> retrieving revision 1.146
> diff -u -p -r1.146 kern_malloc.c
> --- kern/kern_malloc.c 16 May 2021 15:10:20 -0000 1.146
> +++ kern/kern_malloc.c 3 May 2022 21:51:21 -0000
> @@ -188,7 +188,6 @@ malloc(size_t size, int type, int flags)
> if (size > 65535 * PAGE_SIZE) {
> if (flags & M_CANFAIL) {
> #ifndef SMALL_KERNEL
> - /* XXX lock */
> if (ratecheck(&malloc_lasterr, &malloc_errintvl))
> printf("malloc(): allocation too large, "
> "type = %d, size = %lu\n", type, size);
> Index: kern/kern_time.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_time.c,v
> retrieving revision 1.154
> diff -u -p -r1.154 kern_time.c
> --- kern/kern_time.c 18 Jun 2021 15:59:14 -0000 1.154
> +++ kern/kern_time.c 3 May 2022 21:51:21 -0000
> @@ -782,11 +782,13 @@ itimerdecr(struct itimerspec *itp, long
> int
> ratecheck(struct timeval *lasttime, const struct timeval *mininterval)
> {
> + static struct mutex mtx = MUTEX_INITIALIZER(IPL_HIGH);
> struct timeval tv, delta;
> int rv = 0;
>
> getmicrouptime(&tv);
>
> + mtx_enter(&mtx);
> timersub(&tv, lasttime, &delta);
>
> /*
> @@ -798,6 +800,7 @@ ratecheck(struct timeval *lasttime, cons
> *lasttime = tv;
> rv = 1;
> }
> + mtx_leave(&mtx);
>
> return (rv);
> }
> @@ -808,11 +811,13 @@ ratecheck(struct timeval *lasttime, cons
> int
> ppsratecheck(struct timeval *lasttime, int *curpps, int maxpps)
> {
> + static struct mutex mtx = MUTEX_INITIALIZER(IPL_HIGH);
> struct timeval tv, delta;
> int rv;
>
> microuptime(&tv);
>
> + mtx_enter(&mtx);
> timersub(&tv, lasttime, &delta);
>
> /*
> @@ -837,20 +842,11 @@ ppsratecheck(struct timeval *lasttime, i
> else
> rv = 0;
>
> -#if 1 /*DIAGNOSTIC?*/
> /* be careful about wrap-around */
> if (*curpps + 1 > *curpps)
> *curpps = *curpps + 1;
> -#else
> - /*
> - * assume that there's not too many calls to this function.
> - * not sure if the assumption holds, as it depends on *caller's*
> - * behavior, not the behavior of this function.
> - * IMHO it is wrong to make assumption on the caller's behavior,
> - * so the above #if is #if 1, not #ifdef DIAGNOSTIC.
> - */
> - *curpps = *curpps + 1;
> -#endif
> +
> + mtx_leave(&mtx);
>
> return (rv);
> }
>
--
:wq Claudio
