> On 24 Dec 2020, at 3:16 am, Scott Cheloha <scottchel...@gmail.com> wrote:
>
> 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.
There's been no need to tweak them for years. I'd bet people would reach for
the compiler before ddb for tuning these anyway.
> Looking for OKs on the attached patch.
OK by me.
>
> 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++;
> }
