Re: ratecheck mutex
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.c16 May 2021 15:10:20 - 1.146
> +++ kern/kern_malloc.c3 May 2022 21:51:21 -
> @@ -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(_lasterr, _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 - 1.154
> +++ kern/kern_time.c 3 May 2022 21:51:21 -
> @@ -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();
>
> + mtx_enter();
> timersub(, lasttime, );
>
> /*
> @@ -798,6 +800,7 @@ ratecheck(struct timeval *lasttime, cons
> *lasttime = tv;
> rv = 1;
> }
> + mtx_leave();
>
> 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();
>
> + mtx_enter();
> timersub(, lasttime, );
>
> /*
> @@ -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();
>
> return (rv);
> }
>
--
:wq Claudio
Re: ratecheck mutex
On Wed, May 04, 2022 at 04:42:21AM -0500, Scott Cheloha wrote: > > On May 3, 2022, at 17:16, 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. > > Why not declare a struct ratecheck with > a per-struct mutex? Because that diff is more work. It is the cleaner solution, but touches a lot of code. > It seems odd to be heading toward more > parallel processing in e.g. the networking > stack and introduce a global point of > contention. I don't expect contention on the rate limit. Make things stable, run in parallel, tweak performance. That's why I have chosen this aproach. But if it is too dirty, I can create the larger diff. bluhm
Re: ratecheck mutex
> On May 3, 2022, at 17:16, 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. Why not declare a struct ratecheck with a per-struct mutex? It seems odd to be heading toward more parallel processing in e.g. the networking stack and introduce a global point of contention.
ratecheck mutex
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?
bluhm
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 - 1.146
+++ kern/kern_malloc.c 3 May 2022 21:51:21 -
@@ -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(_lasterr, _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.c18 Jun 2021 15:59:14 - 1.154
+++ kern/kern_time.c3 May 2022 21:51:21 -
@@ -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();
+ mtx_enter();
timersub(, lasttime, );
/*
@@ -798,6 +800,7 @@ ratecheck(struct timeval *lasttime, cons
*lasttime = tv;
rv = 1;
}
+ mtx_leave();
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();
+ mtx_enter();
timersub(, lasttime, );
/*
@@ -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();
return (rv);
}
