On 11/12/20(Fri) 12:52, Mark Kettenis wrote: > > Date: Thu, 10 Dec 2020 16:13:22 -0600 > > From: Scott Cheloha <scottchel...@gmail.com> > > > > Hi, > > > > We looked at removing the ticks from subr_pool.c a while back but it > > got shelved. That may or may not have been my fault. I don't > > remember. > > > > Anyway, I would normally suggest switching to getuptime(9) here, but > > getuptime(9) counts in seconds and we're working with a 1 second > > timeout in this code (pool_wait_free) so that's too coarse an > > interface for this job. > > > > The next best thing I could come up with was introducing a coarse > > sub-second interface for use in this file, "getnsecuptime()", which > > calls getnanouptime(9) and converts the result to a 64-bit count of > > nanoseconds. This is relatively fast (we don't read the underlying > > timecounter hardware) and causes a minimal amount of code change (we > > can use it inline because it returns an integral value). > > > > >From there the changes are simple: > > > > - Renames: ph_tick -> ph_timestamp, pr_cache_tick -> pr_cache_timestamp > > > > - Call getnsecuptime(9) wherever we read 'ticks'. > > > > - Change pool_wait_gc and pool_wait_free to counts of nanoseconds. > > They could be macros, e.g. > > > > #define POOL_WAIT_GC 8000000000ULL > > > > but I'll leave that for a second diff to keep things simple. > > > > This compiles and I haven't changed any logic so I assume it isn't > > broken. > > > > We could move getnsecuptime() into kern_tc.c but it isn't used > > anywhere else yet so I'm hesitant to do so. > > > > Thoughts? > > Specifying the timeouts in nanoseconds isn't particularly useful I'd > say. But I see we can't use SEC_TO_NSEC here because of the overflow > check...
I'm not sure to understand, can't we do: pool_wait_free = SEC_TO_NSEC(1); pool_wait_gc = SEC_TO_NSEC(8); or are you pointing at something else? One comment below > > 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 10 Dec 2020 22:08:33 -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; /* uptime when last modified */ > > }; > > #define POOL_MAGICBIT (1 << 3) /* keep away from perturbed low bits */ > > #define POOL_PHPOISON(ph) ISSET((ph)->ph_magic, POOL_MAGICBIT) > > @@ -266,8 +267,18 @@ 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; > > +uint64_t pool_wait_free = 1000000000ULL; /* nanoseconds */ > > +uint64_t pool_wait_gc = 8000000000ULL; /* nanoseconds */ > > + > > +/* XXX where do I put this? */ > > +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 +808,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 +875,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 +1577,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 +1737,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 +1840,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 +2017,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 +2026,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++; > > } > > 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 10 Dec 2020 22:08:33 -0000 > > @@ -201,7 +201,7 @@ 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 */ > > + uint64_t pr_cache_timestamp; /* when idle list was empty */ > > int pr_cache_nout; Do you see a change in of size in the struct? If so, does moving `pr_cache_nout' after `pr_cache_ngc' helps? > > uint64_t pr_cache_ngc; /* # of times the gc released a list */ > > > >