On Mon, Jun 19, 2023 at 04:45:03PM -0500, Scott Cheloha wrote:
> On Mon, Jun 19, 2023 at 10:22:56AM +0200, Claudio Jeker wrote:
> > On Sun, Jun 18, 2023 at 12:43:18PM -0500, Scott Cheloha wrote:
> > > On Sun, Jun 18, 2023 at 12:36:07PM -0500, Scott Cheloha wrote:
> > > > On Sun, Jun 18, 2023 at 07:32:56PM +0200, Mark Kettenis wrote:
> > > > > > Date: Sun, 18 Jun 2023 12:27:17 -0500
> > > > > > From: Scott Cheloha <scottchel...@gmail.com>
> > > > > >
> > > > > > The intent here is to update the load averages every five seconds.
> > > > > > However:
> > > > > >
> > > > > > 1. Measuring elapsed time with the UTC clock is unreliable because
> > > > > > of
> > > > > > settimeofday(2).
> > > > > >
> > > > > > 2. "Call uvm_loadav() no more than once every five seconds", is not
> > > > > > equivalent to "call uvm_loadav() if the current second is equal
> > > > > > to zero, modulo five".
> > > > > >
> > > > > > Not hard to imagine edge cases where timeouts are delayed and
> > > > > > the load averages are not updated.
> > > > > >
> > > > > > So, (1) use the monotonic clock, and (2) keep the next uvm_loadav()
> > > > > > call time in a static value.
> > > > > >
> > > > > > ok?
> > > > >
> > > > > I really don't see why the calculatin of something vague like the load
> > > > > average warrants complicating the code like this.
> > > >
> > > > Aren't load averages used to make decisions about thread placement in
> > > > the scheduler?
> > > >
> > > > Regardless, the code is still wrong. At minimum you should use
> > > > getuptime(9).
> > >
> > > Maybe I misunderstood. Are you suggesting this?
> > >
> > >
> > > now = getuptime();
> > > if (now >= next_uvm_loadav) {
> > > next_uvm_loadav = now + 5;
> > > uvm_loadav(...);
> > > }
> > >
> > > The patch I posted preserves the current behavior. It is equivalent
> > > to:
> > >
> > > while (next_uvm_loadav <= now)
> > > next_uvm_loadav += 5;
> > >
> > > Changing it to (now + 5) changes the behavior.
> >
> > To be honest, I think the uvm_meter should be called via timeout(9) and
> > not be called via the current path using schedcpu().
> > At some point schedcpu() may be removed and then we need to fix this
> > proper anyway.
>
> See attached.
>
> This changes the wakeup interval for the proc0 (the swapper) on
> patched kernels where maxslp is patched (highly unusual).
>
> On default kernels, maxslp is 20, which divides evenly into 5, so
> the patch does not change the proc0 wakeup interval.
>
> Another approach would be to run the uvm_meter() timeout every second
> and track the uvm_loadav() deadline in a static variable.
>
> > Running the lbolt wakeup inside schedcpu() has the same issue.
>
> Eliminating lbolt has been a goal of mine for several years. Tried
> to remove as many users as possible a few years ago. The tricky cases
> are in the sys/kern tty code.
>
> --
>
> Index: kern/sched_bsd.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/sched_bsd.c,v
> retrieving revision 1.74
> diff -u -p -r1.74 sched_bsd.c
> --- kern/sched_bsd.c 4 Feb 2023 19:33:03 -0000 1.74
> +++ kern/sched_bsd.c 19 Jun 2023 21:35:22 -0000
> @@ -234,7 +234,6 @@ schedcpu(void *arg)
> }
> SCHED_UNLOCK(s);
> }
> - uvm_meter();
> wakeup(&lbolt);
> timeout_add_sec(to, 1);
> }
> @@ -669,6 +668,7 @@ scheduler_start(void)
>
> rrticks_init = hz / 10;
> schedcpu(&schedcpu_to);
> + uvm_meter_start();
>
> #ifndef SMALL_KERNEL
> if (perfpolicy == PERFPOL_AUTO)
> Index: uvm/uvm_meter.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_meter.c,v
> retrieving revision 1.42
> diff -u -p -r1.42 uvm_meter.c
> --- uvm/uvm_meter.c 28 Dec 2020 14:01:23 -0000 1.42
> +++ uvm/uvm_meter.c 19 Jun 2023 21:35:22 -0000
> @@ -42,6 +42,7 @@
> #include <sys/percpu.h>
> #include <sys/proc.h>
> #include <sys/sysctl.h>
> +#include <sys/timeout.h>
> #include <sys/vmmeter.h>
> #include <uvm/uvm.h>
> #include <uvm/uvm_ddb.h>
> @@ -65,6 +66,9 @@
> int maxslp = MAXSLP; /* patchable ... */
> struct loadavg averunnable;
>
> +/* Update load averages every five seconds. */
> +#define UVM_METER_INTVL 5
> +
> /*
> * constants for averages over 1, 5, and 15 minutes when sampling at
> * 5 second intervals.
> @@ -78,17 +82,29 @@ static fixpt_t cexp[3] = {
>
>
> static void uvm_loadav(struct loadavg *);
> +void uvm_meter(void *);
> void uvm_total(struct vmtotal *);
> void uvmexp_read(struct uvmexp *);
>
> +void
> +uvm_meter_start(void)
> +{
> + static struct timeout to = TIMEOUT_INITIALIZER(uvm_meter, &to);
> +
> + uvm_meter(&to);
> +}
> +
> /*
> * uvm_meter: calculate load average and wake up the swapper (if needed)
> */
> void
> -uvm_meter(void)
> +uvm_meter(void *arg)
> {
> - if ((gettime() % 5) == 0)
> - uvm_loadav(&averunnable);
> + struct timeout *to = arg;
> +
> + timeout_add_sec(to, UVM_METER_INTVL);
> +
> + uvm_loadav(&averunnable);
> if (proc0.p_slptime > (maxslp / 2))
> wakeup(&proc0);
> }
Why add uvm_meter_start() using a static global value and then pass that
value around. This code could just be:
struct timeout uvm_meter_to = TIMEOUT_INITIALIZER(uvm_meter, NULL);
void
uvm_meter(void *arg)
{
timeout_add_sec(&uvm_meter_to, UVM_METER_INTVL);
uvm_loadav(&averunnable);
}
and then just call uvm_meter() once in scheduler_start().
I don't understand why all extra this indirection is needed it does not
make the code better..
Apart from that and the fact that the proc0 wakeup and go I'm OK with this
diff.
> Index: uvm/uvm_extern.h
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_extern.h,v
> retrieving revision 1.168
> diff -u -p -r1.168 uvm_extern.h
> --- uvm/uvm_extern.h 30 May 2023 08:30:01 -0000 1.168
> +++ uvm/uvm_extern.h 19 Jun 2023 21:35:22 -0000
> @@ -414,7 +414,7 @@ void uvmspace_free(struct vmspace *);
> struct vmspace *uvmspace_share(struct process *);
> int uvm_share(vm_map_t, vaddr_t, vm_prot_t,
> vm_map_t, vaddr_t, vsize_t);
> -void uvm_meter(void);
> +void uvm_meter_start(void);
> int uvm_sysctl(int *, u_int, void *, size_t *,
> void *, size_t, struct proc *);
> struct vm_page *uvm_pagealloc(struct uvm_object *,
>
--
:wq Claudio
