Re: uvmexp & per-CPU counters
> Date: Wed, 23 Dec 2020 10:58:16 -0300
> From: Martin Pieuchot
>
> On 22/12/20(Tue) 23:43, Mark Kettenis wrote:
> > > Date: Mon, 21 Dec 2020 16:46:32 -0300
> > > From: Martin Pieuchot
> > >
> > > During a page fault multiples counters are updated. They fall into two
> > > categories "fault counters" and "global statistics" both of which are
> > > currently represented by int-sized fields inside a global: `uvmexp'.
> > >
> > > Diff below makes use of the per-CPU counters_inc(9) API to make sure no
> > > update is lost with an unlocked fault handler. I only converted the
> > > fields touched by uvm_fault() to have a working solution and start a
> > > discussion.
> > >
> > > - Should we keep a single enum for all fields inside `uvmexp' or do we
> > > want to separate "statistics counters" which are mostly used sys/arch
> > > from "fault counters" which are only used in uvm/uvm_fault.c?
I don't know. Keep it simple for now, so maybe stick with what you have.
> > > - The counter_add(9) API deals with uint64_t and currently uvmexp uses
> > > int. Should we truncate or change the size of uvmexp fields or do
> > > something else?
Truncating should be good enough.
> > > Comments?
> >
> > I think this breaks "show uvmexp" in ddb.
>
> Updated diff below fixes that. Any comment on the issues raised above?
>
> > You fear that using atomic operations for these counters would lead to
> > too much bus contention on systems with a large number of CPUs?
>
> I don't know. I don't see the point of using atomic operations for
> "real" counters that are not used to make any decision. Atomic
> operations have a high cost, bus contention might be one of them.
Sure. If dlg@ thinks this is an appropriate use of the per-CPU
counters, this is ok with me.
> Index: kern/init_main.c
> ===
> RCS file: /cvs/src/sys/kern/init_main.c,v
> retrieving revision 1.302
> diff -u -p -r1.302 init_main.c
> --- kern/init_main.c 7 Dec 2020 16:55:28 - 1.302
> +++ kern/init_main.c 21 Dec 2020 19:37:13 -
> @@ -432,6 +432,7 @@ main(void *framep)
> #endif
>
> mbcpuinit();/* enable per cpu mbuf data */
> + uvm_init_percpu();
>
> /* init exec and emul */
> init_exec();
> Index: uvm/uvm_extern.h
> ===
> RCS file: /cvs/src/sys/uvm/uvm_extern.h,v
> retrieving revision 1.155
> diff -u -p -r1.155 uvm_extern.h
> --- uvm/uvm_extern.h 1 Dec 2020 13:56:22 - 1.155
> +++ uvm/uvm_extern.h 21 Dec 2020 19:37:13 -
> @@ -289,6 +289,7 @@ void uvm_vsunlock_device(struct proc
> *
> void *);
> void uvm_pause(void);
> void uvm_init(void);
> +void uvm_init_percpu(void);
> int uvm_io(vm_map_t, struct uio *, int);
>
> #define UVM_IO_FIXPROT 0x01
> Index: uvm/uvm_fault.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_fault.c,v
> retrieving revision 1.109
> diff -u -p -r1.109 uvm_fault.c
> --- uvm/uvm_fault.c 8 Dec 2020 12:26:31 - 1.109
> +++ uvm/uvm_fault.c 21 Dec 2020 19:37:13 -
> @@ -35,6 +35,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -271,7 +272,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
> int result;
>
> result = 0; /* XXX shut up gcc */
> - uvmexp.fltanget++;
> + counters_inc(uvmexp_counters, flt_anget);
> /* bump rusage counters */
> if (anon->an_page)
> curproc->p_ru.ru_minflt++;
> @@ -295,7 +296,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
> if ((pg->pg_flags & (PG_BUSY|PG_RELEASED)) == 0)
> return (VM_PAGER_OK);
> atomic_setbits_int(>pg_flags, PG_WANTED);
> - uvmexp.fltpgwait++;
> + counters_inc(uvmexp_counters, flt_pgwait);
>
> /*
>* the last unlock must be an atomic unlock+wait on
> @@ -310,7 +311,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
>
> if (pg == NULL) { /* out of RAM. */
> uvmfault_unlockall(ufi, amap, NULL);
> - uvmexp.fltnoram++;
> + counters_inc(uvmexp_counters, flt_noram);
> uvm_wait("flt_noram1");
> /* ready to relock and try again */
> } else {
> @@ -325,7 +326,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
>* it is ok to read an_swslot here because
>* we hold PG_BUSY on the page.
>*/
> - uvmexp.pageins++;
> +
Re: uvmexp & per-CPU counters
On 22/12/20(Tue) 23:43, Mark Kettenis wrote:
> > Date: Mon, 21 Dec 2020 16:46:32 -0300
> > From: Martin Pieuchot
> >
> > During a page fault multiples counters are updated. They fall into two
> > categories "fault counters" and "global statistics" both of which are
> > currently represented by int-sized fields inside a global: `uvmexp'.
> >
> > Diff below makes use of the per-CPU counters_inc(9) API to make sure no
> > update is lost with an unlocked fault handler. I only converted the
> > fields touched by uvm_fault() to have a working solution and start a
> > discussion.
> >
> > - Should we keep a single enum for all fields inside `uvmexp' or do we
> > want to separate "statistics counters" which are mostly used sys/arch
> > from "fault counters" which are only used in uvm/uvm_fault.c?
> >
> > - The counter_add(9) API deals with uint64_t and currently uvmexp uses
> > int. Should we truncate or change the size of uvmexp fields or do
> > something else?
> >
> > Comments?
>
> I think this breaks "show uvmexp" in ddb.
Updated diff below fixes that. Any comment on the issues raised above?
> You fear that using atomic operations for these counters would lead to
> too much bus contention on systems with a large number of CPUs?
I don't know. I don't see the point of using atomic operations for
"real" counters that are not used to make any decision. Atomic
operations have a high cost, bus contention might be one of them.
Index: kern/init_main.c
===
RCS file: /cvs/src/sys/kern/init_main.c,v
retrieving revision 1.302
diff -u -p -r1.302 init_main.c
--- kern/init_main.c7 Dec 2020 16:55:28 - 1.302
+++ kern/init_main.c21 Dec 2020 19:37:13 -
@@ -432,6 +432,7 @@ main(void *framep)
#endif
mbcpuinit();/* enable per cpu mbuf data */
+ uvm_init_percpu();
/* init exec and emul */
init_exec();
Index: uvm/uvm_extern.h
===
RCS file: /cvs/src/sys/uvm/uvm_extern.h,v
retrieving revision 1.155
diff -u -p -r1.155 uvm_extern.h
--- uvm/uvm_extern.h1 Dec 2020 13:56:22 - 1.155
+++ uvm/uvm_extern.h21 Dec 2020 19:37:13 -
@@ -289,6 +289,7 @@ voiduvm_vsunlock_device(struct proc
*
void *);
void uvm_pause(void);
void uvm_init(void);
+void uvm_init_percpu(void);
intuvm_io(vm_map_t, struct uio *, int);
#defineUVM_IO_FIXPROT 0x01
Index: uvm/uvm_fault.c
===
RCS file: /cvs/src/sys/uvm/uvm_fault.c,v
retrieving revision 1.109
diff -u -p -r1.109 uvm_fault.c
--- uvm/uvm_fault.c 8 Dec 2020 12:26:31 - 1.109
+++ uvm/uvm_fault.c 21 Dec 2020 19:37:13 -
@@ -35,6 +35,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -271,7 +272,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
int result;
result = 0; /* XXX shut up gcc */
- uvmexp.fltanget++;
+ counters_inc(uvmexp_counters, flt_anget);
/* bump rusage counters */
if (anon->an_page)
curproc->p_ru.ru_minflt++;
@@ -295,7 +296,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
if ((pg->pg_flags & (PG_BUSY|PG_RELEASED)) == 0)
return (VM_PAGER_OK);
atomic_setbits_int(>pg_flags, PG_WANTED);
- uvmexp.fltpgwait++;
+ counters_inc(uvmexp_counters, flt_pgwait);
/*
* the last unlock must be an atomic unlock+wait on
@@ -310,7 +311,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
if (pg == NULL) { /* out of RAM. */
uvmfault_unlockall(ufi, amap, NULL);
- uvmexp.fltnoram++;
+ counters_inc(uvmexp_counters, flt_noram);
uvm_wait("flt_noram1");
/* ready to relock and try again */
} else {
@@ -325,7 +326,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
* it is ok to read an_swslot here because
* we hold PG_BUSY on the page.
*/
- uvmexp.pageins++;
+ counters_inc(uvmexp_counters, pageins);
result = uvm_swap_get(pg, anon->an_swslot,
PGO_SYNCIO);
@@ -369,7 +370,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
uvm_anfree(anon); /* frees page for us */
if (locked)
Re: uvmexp & per-CPU counters
> Date: Mon, 21 Dec 2020 16:46:32 -0300
> From: Martin Pieuchot
>
> During a page fault multiples counters are updated. They fall into two
> categories "fault counters" and "global statistics" both of which are
> currently represented by int-sized fields inside a global: `uvmexp'.
>
> Diff below makes use of the per-CPU counters_inc(9) API to make sure no
> update is lost with an unlocked fault handler. I only converted the
> fields touched by uvm_fault() to have a working solution and start a
> discussion.
>
> - Should we keep a single enum for all fields inside `uvmexp' or do we
> want to separate "statistics counters" which are mostly used sys/arch
> from "fault counters" which are only used in uvm/uvm_fault.c?
>
> - The counter_add(9) API deals with uint64_t and currently uvmexp uses
> int. Should we truncate or change the size of uvmexp fields or do
> something else?
>
> Comments?
I think this breaks "show uvmexp" in ddb.
You fear that using atomic operations for these counters would lead to
too much bus contention on systems with a large number of CPUs?
> Index: kern/init_main.c
> ===
> RCS file: /cvs/src/sys/kern/init_main.c,v
> retrieving revision 1.302
> diff -u -p -r1.302 init_main.c
> --- kern/init_main.c 7 Dec 2020 16:55:28 - 1.302
> +++ kern/init_main.c 21 Dec 2020 19:37:13 -
> @@ -432,6 +432,7 @@ main(void *framep)
> #endif
>
> mbcpuinit();/* enable per cpu mbuf data */
> + uvm_init_percpu();
>
> /* init exec and emul */
> init_exec();
> Index: uvm/uvm_extern.h
> ===
> RCS file: /cvs/src/sys/uvm/uvm_extern.h,v
> retrieving revision 1.155
> diff -u -p -r1.155 uvm_extern.h
> --- uvm/uvm_extern.h 1 Dec 2020 13:56:22 - 1.155
> +++ uvm/uvm_extern.h 21 Dec 2020 19:37:13 -
> @@ -289,6 +289,7 @@ void uvm_vsunlock_device(struct proc
> *
> void *);
> void uvm_pause(void);
> void uvm_init(void);
> +void uvm_init_percpu(void);
> int uvm_io(vm_map_t, struct uio *, int);
>
> #define UVM_IO_FIXPROT 0x01
> Index: uvm/uvm_fault.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_fault.c,v
> retrieving revision 1.109
> diff -u -p -r1.109 uvm_fault.c
> --- uvm/uvm_fault.c 8 Dec 2020 12:26:31 - 1.109
> +++ uvm/uvm_fault.c 21 Dec 2020 19:37:13 -
> @@ -35,6 +35,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -271,7 +272,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
> int result;
>
> result = 0; /* XXX shut up gcc */
> - uvmexp.fltanget++;
> + counters_inc(uvmexp_counters, flt_anget);
> /* bump rusage counters */
> if (anon->an_page)
> curproc->p_ru.ru_minflt++;
> @@ -295,7 +296,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
> if ((pg->pg_flags & (PG_BUSY|PG_RELEASED)) == 0)
> return (VM_PAGER_OK);
> atomic_setbits_int(>pg_flags, PG_WANTED);
> - uvmexp.fltpgwait++;
> + counters_inc(uvmexp_counters, flt_pgwait);
>
> /*
>* the last unlock must be an atomic unlock+wait on
> @@ -310,7 +311,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
>
> if (pg == NULL) { /* out of RAM. */
> uvmfault_unlockall(ufi, amap, NULL);
> - uvmexp.fltnoram++;
> + counters_inc(uvmexp_counters, flt_noram);
> uvm_wait("flt_noram1");
> /* ready to relock and try again */
> } else {
> @@ -325,7 +326,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
>* it is ok to read an_swslot here because
>* we hold PG_BUSY on the page.
>*/
> - uvmexp.pageins++;
> + counters_inc(uvmexp_counters, pageins);
> result = uvm_swap_get(pg, anon->an_swslot,
> PGO_SYNCIO);
>
> @@ -369,7 +370,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
> uvm_anfree(anon); /* frees page for us */
> if (locked)
> uvmfault_unlockall(ufi, amap, NULL);
> - uvmexp.fltpgrele++;
> + counters_inc(uvmexp_counters, flt_pgrele);
> return (VM_PAGER_REFAULT); /* refault! */
> }
>
> @@ -426,7 +427,7 @@
uvmexp & per-CPU counters
During a page fault multiples counters are updated. They fall into two
categories "fault counters" and "global statistics" both of which are
currently represented by int-sized fields inside a global: `uvmexp'.
Diff below makes use of the per-CPU counters_inc(9) API to make sure no
update is lost with an unlocked fault handler. I only converted the
fields touched by uvm_fault() to have a working solution and start a
discussion.
- Should we keep a single enum for all fields inside `uvmexp' or do we
want to separate "statistics counters" which are mostly used sys/arch
from "fault counters" which are only used in uvm/uvm_fault.c?
- The counter_add(9) API deals with uint64_t and currently uvmexp uses
int. Should we truncate or change the size of uvmexp fields or do
something else?
Comments?
Index: kern/init_main.c
===
RCS file: /cvs/src/sys/kern/init_main.c,v
retrieving revision 1.302
diff -u -p -r1.302 init_main.c
--- kern/init_main.c7 Dec 2020 16:55:28 - 1.302
+++ kern/init_main.c21 Dec 2020 19:37:13 -
@@ -432,6 +432,7 @@ main(void *framep)
#endif
mbcpuinit();/* enable per cpu mbuf data */
+ uvm_init_percpu();
/* init exec and emul */
init_exec();
Index: uvm/uvm_extern.h
===
RCS file: /cvs/src/sys/uvm/uvm_extern.h,v
retrieving revision 1.155
diff -u -p -r1.155 uvm_extern.h
--- uvm/uvm_extern.h1 Dec 2020 13:56:22 - 1.155
+++ uvm/uvm_extern.h21 Dec 2020 19:37:13 -
@@ -289,6 +289,7 @@ voiduvm_vsunlock_device(struct proc
*
void *);
void uvm_pause(void);
void uvm_init(void);
+void uvm_init_percpu(void);
intuvm_io(vm_map_t, struct uio *, int);
#defineUVM_IO_FIXPROT 0x01
Index: uvm/uvm_fault.c
===
RCS file: /cvs/src/sys/uvm/uvm_fault.c,v
retrieving revision 1.109
diff -u -p -r1.109 uvm_fault.c
--- uvm/uvm_fault.c 8 Dec 2020 12:26:31 - 1.109
+++ uvm/uvm_fault.c 21 Dec 2020 19:37:13 -
@@ -35,6 +35,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -271,7 +272,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
int result;
result = 0; /* XXX shut up gcc */
- uvmexp.fltanget++;
+ counters_inc(uvmexp_counters, flt_anget);
/* bump rusage counters */
if (anon->an_page)
curproc->p_ru.ru_minflt++;
@@ -295,7 +296,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
if ((pg->pg_flags & (PG_BUSY|PG_RELEASED)) == 0)
return (VM_PAGER_OK);
atomic_setbits_int(>pg_flags, PG_WANTED);
- uvmexp.fltpgwait++;
+ counters_inc(uvmexp_counters, flt_pgwait);
/*
* the last unlock must be an atomic unlock+wait on
@@ -310,7 +311,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
if (pg == NULL) { /* out of RAM. */
uvmfault_unlockall(ufi, amap, NULL);
- uvmexp.fltnoram++;
+ counters_inc(uvmexp_counters, flt_noram);
uvm_wait("flt_noram1");
/* ready to relock and try again */
} else {
@@ -325,7 +326,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
* it is ok to read an_swslot here because
* we hold PG_BUSY on the page.
*/
- uvmexp.pageins++;
+ counters_inc(uvmexp_counters, pageins);
result = uvm_swap_get(pg, anon->an_swslot,
PGO_SYNCIO);
@@ -369,7 +370,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
uvm_anfree(anon); /* frees page for us */
if (locked)
uvmfault_unlockall(ufi, amap, NULL);
- uvmexp.fltpgrele++;
+ counters_inc(uvmexp_counters, flt_pgrele);
return (VM_PAGER_REFAULT); /* refault! */
}
@@ -426,7 +427,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
}
/* try it again! */
- uvmexp.fltanretry++;
+ counters_inc(uvmexp_counters, flt_anretry);
continue;
} /* while (1) */
@@ -547,7 +548,7 @@ uvm_fault_check(struct uvm_faultinfo *uf
/* need to clear */
