On Fri, Dec 11, 2020 at 05:32:54PM -0600, Scott Cheloha wrote: > On Fri, Dec 11, 2020 at 07:52:45PM +0100, Mark Kettenis wrote: > > > Date: Fri, 11 Dec 2020 11:51:54 -0600 > > > From: Scott Cheloha <scottchel...@gmail.com> > > > > > > On Fri, Dec 11, 2020 at 09:49:07AM -0300, Martin Pieuchot wrote: > > > > > > > > I'm not sure to understand, can't we do: > > > > > > > > pool_wait_free = SEC_TO_NSEC(1); > > > > pool_wait_gc = SEC_TO_NSEC(8); > > > > > > > [...] > > > > > > We can do that at runtime but not at compile time. SEC_TO_NSEC(1) > > > isn't a constant so that won't compile (I just tried). > > > > > > We _could_ do something like this: > > > > > > #define POOL_WAIT_FREE SEC_TO_NSEC(1) > > > > > > I think the compiler will probably inline the result and elide the > > > overflow check because the input is a constant. I don't know how to > > > verify this, but my limited understanding of compilers suggests that > > > this is totally possible. > > > > Yes. The consequence of that is that the values are no longer > > patchable. That may not be very important though (I never really use > > that possibility). > > What do you mean by "patchable"? I assume you don't mean the source > code. > > (Also, you did not comment on the struct stuff below so I'm proceeding > with the impression there's nothing at issue there.)
Hearing nothing after two weeks I assume nobody cares if timeouts are no longer patchable. Looking for OKs on the attached patch. CC tedu@/dlg@, who added these timeouts to pool(9) in the first place: https://github.com/openbsd/src/commit/786f6c84e747ccb9777ad35d2b01160679aec089 Index: sys/pool.h =================================================================== RCS file: /cvs/src/sys/sys/pool.h,v retrieving revision 1.77 diff -u -p -r1.77 pool.h --- sys/pool.h 19 Jul 2019 09:03:03 -0000 1.77 +++ sys/pool.h 23 Dec 2020 17:06:19 -0000 @@ -201,9 +201,9 @@ struct pool { u_int pr_cache_items; /* target list length */ u_int pr_cache_contention; u_int pr_cache_contention_prev; - int pr_cache_tick; /* time idle list was empty */ - int pr_cache_nout; + uint64_t pr_cache_timestamp; /* time idle list was empty */ uint64_t pr_cache_ngc; /* # of times the gc released a list */ + int pr_cache_nout; u_int pr_align; u_int pr_maxcolors; /* Cache coloring */ Index: kern/subr_pool.c =================================================================== RCS file: /cvs/src/sys/kern/subr_pool.c,v retrieving revision 1.230 diff -u -p -r1.230 subr_pool.c --- kern/subr_pool.c 24 Jan 2020 06:31:17 -0000 1.230 +++ kern/subr_pool.c 23 Dec 2020 17:06:20 -0000 @@ -41,6 +41,7 @@ #include <sys/syslog.h> #include <sys/sysctl.h> #include <sys/task.h> +#include <sys/time.h> #include <sys/timeout.h> #include <sys/percpu.h> @@ -148,7 +149,7 @@ struct pool_page_header { caddr_t ph_page; /* this page's address */ caddr_t ph_colored; /* page's colored address */ unsigned long ph_magic; - int ph_tick; + uint64_t ph_timestamp; }; #define POOL_MAGICBIT (1 << 3) /* keep away from perturbed low bits */ #define POOL_PHPOISON(ph) ISSET((ph)->ph_magic, POOL_MAGICBIT) @@ -266,8 +267,22 @@ void pool_gc_sched(void *); struct timeout pool_gc_tick = TIMEOUT_INITIALIZER(pool_gc_sched, NULL); void pool_gc_pages(void *); struct task pool_gc_task = TASK_INITIALIZER(pool_gc_pages, NULL); -int pool_wait_free = 1; -int pool_wait_gc = 8; + +#define POOL_WAIT_FREE SEC_TO_NSEC(1) +#define POOL_WAIT_GC SEC_TO_NSEC(8) + +/* + * TODO Move getnsecuptime() to kern_tc.c and document it when we + * have callers in other modules. + */ +static uint64_t +getnsecuptime(void) +{ + struct timespec now; + + getnanouptime(&now); + return TIMESPEC_TO_NSEC(&now); +} RBT_PROTOTYPE(phtree, pool_page_header, ph_node, phtree_compare); @@ -797,7 +812,7 @@ pool_put(struct pool *pp, void *v) /* is it time to free a page? */ if (pp->pr_nidle > pp->pr_maxpages && (ph = TAILQ_FIRST(&pp->pr_emptypages)) != NULL && - (ticks - ph->ph_tick) > (hz * pool_wait_free)) { + getnsecuptime() - ph->ph_timestamp > POOL_WAIT_FREE) { freeph = ph; pool_p_remove(pp, freeph); } @@ -864,7 +879,7 @@ pool_do_put(struct pool *pp, void *v) */ pp->pr_nidle++; - ph->ph_tick = ticks; + ph->ph_timestamp = getnsecuptime(); TAILQ_REMOVE(&pp->pr_partpages, ph, ph_entry); TAILQ_INSERT_TAIL(&pp->pr_emptypages, ph, ph_entry); pool_update_curpage(pp); @@ -1566,7 +1581,7 @@ pool_gc_pages(void *null) /* is it time to free a page? */ if (pp->pr_nidle > pp->pr_minpages && (ph = TAILQ_FIRST(&pp->pr_emptypages)) != NULL && - (ticks - ph->ph_tick) > (hz * pool_wait_gc)) { + getnsecuptime() - ph->ph_timestamp > POOL_WAIT_GC) { freeph = ph; pool_p_remove(pp, freeph); } else @@ -1726,7 +1741,7 @@ pool_cache_init(struct pool *pp) arc4random_buf(pp->pr_cache_magic, sizeof(pp->pr_cache_magic)); TAILQ_INIT(&pp->pr_cache_lists); pp->pr_cache_nitems = 0; - pp->pr_cache_tick = ticks; + pp->pr_cache_timestamp = getnsecuptime(); pp->pr_cache_items = 8; pp->pr_cache_contention = 0; pp->pr_cache_ngc = 0; @@ -1829,7 +1844,7 @@ pool_cache_list_free(struct pool *pp, st { pool_list_enter(pp); if (TAILQ_EMPTY(&pp->pr_cache_lists)) - pp->pr_cache_tick = ticks; + pp->pr_cache_timestamp = getnsecuptime(); pp->pr_cache_nitems += POOL_CACHE_ITEM_NITEMS(ci); TAILQ_INSERT_TAIL(&pp->pr_cache_lists, ci, ci_nextl); @@ -2006,7 +2021,7 @@ pool_cache_gc(struct pool *pp) { unsigned int contention, delta; - if ((ticks - pp->pr_cache_tick) > (hz * pool_wait_gc) && + if (getnsecuptime() - pp->pr_cache_timestamp > POOL_WAIT_GC && !TAILQ_EMPTY(&pp->pr_cache_lists) && pl_enter_try(pp, &pp->pr_cache_lock)) { struct pool_cache_item *pl = NULL; @@ -2015,7 +2030,7 @@ pool_cache_gc(struct pool *pp) if (pl != NULL) { TAILQ_REMOVE(&pp->pr_cache_lists, pl, ci_nextl); pp->pr_cache_nitems -= POOL_CACHE_ITEM_NITEMS(pl); - pp->pr_cache_tick = ticks; + pp->pr_cache_timestamp = getnsecuptime(); pp->pr_cache_ngc++; }
