On 01/07/21(Thu) 13:53, Anindya Mukherjee wrote:
> Hi,
>
> I noticed that if I leave the system running for more than about a month, some
> of the counters in the uvm view of systat(1) overflow and become negative.
> This
> is because the members of struct uvmexp in sys/uvm/uvmexp.h are ints. The
> kernel's internal counters are of course uint64_t so they don't overflow. It
> only happens during the uvm_sysctl(9) call which casts the numbers to
> integers.
> The function is uvmexp_read.
>
> In the attached diff I took the path of least resistance and promoted some of
> the counters to unsigned int. Ideally I would have liked to use int64_t or
> even
> uint64_t, but I hit an issue in some of the architecture dependent code. An
> example is:
> /usr/src/sys/arch/alpha/alpha/trap.c:536 atomic_add_int(&uvmexp.syscalls, 1);
> In other places the ++ operator is used to increment the counters and the 64
> bit
> types can be used.
>
> I am not completely sure this is the best way to proceed, but even if this
> diff
> is horrifying, I'd appreciate some feedback and advice, thanks!
I wonder if we shouldn't use uint64_t for those and embrace the ABI
break, that would at least simplify the kernel side and remove a
truncation.
Do you have an idea of the different consumers of the "struct uvmexp"
apart from systat(1)? What is the impact in userland & ports of such
change?
> Index: sys/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
> --- sys/uvm/uvm_meter.c 28 Dec 2020 14:01:23 -0000 1.42
> +++ sys/uvm/uvm_meter.c 28 Jun 2021 05:24:56 -0000
> @@ -329,7 +329,7 @@ uvmexp_read(struct uvmexp *uexp)
> counters_read(uvmexp_counters, counters, exp_ncounters);
>
> /* stat counters */
> - uexp->faults = (int)counters[faults];
> + uexp->faults = (unsigned int)counters[faults];
> uexp->pageins = (int)counters[pageins];
>
> /* fault subcounters */
> @@ -379,10 +379,10 @@ uvmexp_print(int (*pr)(const char *, ...
> (*pr)(" freemin=%d, free-target=%d, inactive-target=%d, "
> "wired-max=%d\n", uexp.freemin, uexp.freetarg, uexp.inactarg,
> uexp.wiredmax);
> - (*pr)(" faults=%d, traps=%d, intrs=%d, ctxswitch=%d fpuswitch=%d\n",
> + (*pr)(" faults=%u, traps=%u, intrs=%u, ctxswitch=%u fpuswitch=%d\n",
> uexp.faults, uexp.traps, uexp.intrs, uexp.swtch,
> uexp.fpswtch);
> - (*pr)(" softint=%d, syscalls=%d, kmapent=%d\n",
> + (*pr)(" softint=%u, syscalls=%u, kmapent=%d\n",
> uexp.softs, uexp.syscalls, uexp.kmapent);
>
> (*pr)(" fault counts:\n");
> Index: sys/uvm/uvmexp.h
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvmexp.h,v
> retrieving revision 1.9
> diff -u -p -r1.9 uvmexp.h
> --- sys/uvm/uvmexp.h 4 Mar 2021 09:00:03 -0000 1.9
> +++ sys/uvm/uvmexp.h 28 Jun 2021 05:24:56 -0000
> @@ -90,12 +90,12 @@ struct uvmexp {
> int unused06; /* formerly nfreeanon */
>
> /* stat counters */
> - int faults; /* page fault count */
> - int traps; /* trap count */
> - int intrs; /* interrupt count */
> - int swtch; /* context switch count */
> - int softs; /* software interrupt count */
> - int syscalls; /* system calls */
> + unsigned int faults; /* page fault count */
> + unsigned int traps; /* trap count */
> + unsigned int intrs; /* interrupt count */
> + unsigned int swtch; /* context switch count */
> + unsigned int softs; /* software interrupt count */
> + unsigned int syscalls; /* system calls */
> int pageins; /* pagein operation count */
> /* pageouts are in pdpageouts below */
> int unused07; /* formerly obsolete_swapins */
> Index: usr.bin/systat/uvm.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/systat/uvm.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 uvm.c
> --- usr.bin/systat/uvm.c 28 Jun 2019 13:35:04 -0000 1.5
> +++ usr.bin/systat/uvm.c 28 Jun 2021 05:24:57 -0000
> @@ -37,22 +37,23 @@ void print_uvm(void);
> int read_uvm(void);
> int select_uvm(void);
>
> -void print_uvmexp_field(field_def *, field_def *, int *, int *, const char
> *);
> +void print_uvmexp_field(field_def *, field_def *, unsigned int *,
> + unsigned int *, const char *);
> void print_uvmexp_line(int);
>
> struct uvmexp uvmexp;
> struct uvmexp last_uvmexp;
>
> struct uvmline {
> - int *v1;
> - int *ov1;
> - char *n1;
> - int *v2;
> - int *ov2;
> - char *n2;
> - int *v3;
> - int *ov3;
> - char *n3;
> + unsigned int *v1;
> + unsigned int *ov1;
> + char *n1;
> + unsigned int *v2;
> + unsigned int *ov2;
> + char *n2;
> + unsigned int *v3;
> + unsigned int *ov3;
> + char *n3;
> };
>
> struct uvmline uvmline[] = {
> @@ -214,8 +215,8 @@ read_uvm(void)
> }
>
> void
> -print_uvmexp_field(field_def *fvalue, field_def *fname, int *new, int *old,
> - const char *name)
> +print_uvmexp_field(field_def *fvalue, field_def *fname, unsigned int *new,
> + unsigned int *old, const char *name)
> {
> char *uppername;
> size_t len, i;
>
