Re: uvm_swap: introduce uvm_swap_data_lock
On Wed, Aug 17, 2022 at 03:12:22PM +0200, Martin Pieuchot wrote:
> On 16/01/22(Sun) 15:35, Martin Pieuchot wrote:
> > On 30/12/21(Thu) 23:38, Theo Buehler wrote:
> > > The diff below does two things: it adds a uvm_swap_data_lock mutex and
> > > trades it for the KERNEL_LOCK in uvm_swapisfull() and uvm_swap_markbad()
> >
> > Why is it enough? Which fields is the lock protecting in these
> > function? Is it `uvmexp.swpages', could that be documented?
>
> It is documented in the diff below.
>
> >
> > What about `nswapdev'? Why is the rwlock grabbed before reading it in
> > sys_swapctl()?i
>
> Because it is always modified with the lock, I added some documentation.
>
> > What about `swpginuse'?
>
> This is still under KERNEL_LOCK(), documented below.
>
> > If the mutex/rwlock are used to protect the global `swap_priority' could
> > that be also documented? Once this is documented it should be trivial to
> > see that some places are missing some locking. Is it intentional?
> >
> > > The uvm_swap_data_lock protects all swap data structures, so needs to be
> > > grabbed a few times, many of them already documented in the comments.
> > >
> > > For review, I suggest comparing to what NetBSD did and also going
> > > through the consumers (swaplist_insert, swaplist_find, swaplist_trim)
> > > and check that they are properly locked when called, or that there is
> > > the KERNEL_LOCK() in place when swap data structures are manipulated.
> >
> > I'd suggest using the KASSERT(rw_write_held()) idiom to further reduce
> > the differences with NetBSD.
>
> Done.
>
> > > In swapmount() I introduced locking since that's needed to be able to
> > > assert that the proper locks are held in swaplist_{insert,find,trim}.
> >
> > Could the KERNEL_LOCK() in uvm_swap_get() be pushed a bit further down?
> > What about `uvmexp.nswget' and `uvmexp.swpgonly' in there?
>
> This has been done as part of another change. This diff uses an atomic
> operation to increase `nswget' in case multiple threads fault on a page
> in swap at the same time.
>
> Updated diff below, ok?
I do not currently have access to a machine where I could really test
this. Since the only code changes regarding my diff are the switch to an
atomic operation and a different idiom for the lock assertions, I am
not worried.
Thank you very much for pushing this over the line.
ok tb
Re: uvm_swap: introduce uvm_swap_data_lock
On 16/01/22(Sun) 15:35, Martin Pieuchot wrote:
> On 30/12/21(Thu) 23:38, Theo Buehler wrote:
> > The diff below does two things: it adds a uvm_swap_data_lock mutex and
> > trades it for the KERNEL_LOCK in uvm_swapisfull() and uvm_swap_markbad()
>
> Why is it enough? Which fields is the lock protecting in these
> function? Is it `uvmexp.swpages', could that be documented?
It is documented in the diff below.
>
> What about `nswapdev'? Why is the rwlock grabbed before reading it in
> sys_swapctl()?i
Because it is always modified with the lock, I added some documentation.
> What about `swpginuse'?
This is still under KERNEL_LOCK(), documented below.
> If the mutex/rwlock are used to protect the global `swap_priority' could
> that be also documented? Once this is documented it should be trivial to
> see that some places are missing some locking. Is it intentional?
>
> > The uvm_swap_data_lock protects all swap data structures, so needs to be
> > grabbed a few times, many of them already documented in the comments.
> >
> > For review, I suggest comparing to what NetBSD did and also going
> > through the consumers (swaplist_insert, swaplist_find, swaplist_trim)
> > and check that they are properly locked when called, or that there is
> > the KERNEL_LOCK() in place when swap data structures are manipulated.
>
> I'd suggest using the KASSERT(rw_write_held()) idiom to further reduce
> the differences with NetBSD.
Done.
> > In swapmount() I introduced locking since that's needed to be able to
> > assert that the proper locks are held in swaplist_{insert,find,trim}.
>
> Could the KERNEL_LOCK() in uvm_swap_get() be pushed a bit further down?
> What about `uvmexp.nswget' and `uvmexp.swpgonly' in there?
This has been done as part of another change. This diff uses an atomic
operation to increase `nswget' in case multiple threads fault on a page
in swap at the same time.
Updated diff below, ok?
Index: uvm/uvm_swap.c
===
RCS file: /cvs/src/sys/uvm/uvm_swap.c,v
retrieving revision 1.163
diff -u -p -r1.163 uvm_swap.c
--- uvm/uvm_swap.c 6 Aug 2022 13:44:04 - 1.163
+++ uvm/uvm_swap.c 17 Aug 2022 11:46:20 -
@@ -45,6 +45,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -84,13 +85,16 @@
* the system maintains a global data structure describing all swap
* partitions/files. there is a sorted LIST of "swappri" structures
* which describe "swapdev"'s at that priority. this LIST is headed
- * by the "swap_priority" global var.each "swappri" contains a
+ * by the "swap_priority" global var.each "swappri" contains a
* TAILQ of "swapdev" structures at that priority.
*
* locking:
* - swap_syscall_lock (sleep lock): this lock serializes the swapctl
*system call and prevents the swap priority list from changing
*while we are in the middle of a system call (e.g. SWAP_STATS).
+ * - uvm_swap_data_lock (mutex): this lock protects all swap data
+ *structures including the priority list, the swapdev structures,
+ *and the swapmap arena.
*
* each swap device has the following info:
* - swap device in use (could be disabled, preventing future use)
@@ -106,7 +110,7 @@
* userland controls and configures swap with the swapctl(2) system call.
* the sys_swapctl performs the following operations:
* [1] SWAP_NSWAP: returns the number of swap devices currently configured
- * [2] SWAP_STATS: given a pointer to an array of swapent structures
+ * [2] SWAP_STATS: given a pointer to an array of swapent structures
* (passed in via "arg") of a size passed in via "misc" ... we load
* the current swap config into the array.
* [3] SWAP_ON: given a pathname in arg (could be device or file) and a
@@ -208,9 +212,10 @@ struct extent *swapmap;/* controls the
/* list of all active swap devices [by priority] */
LIST_HEAD(swap_priority, swappri);
-struct swap_priority swap_priority;
+struct swap_priority swap_priority;/* [S] */
/* locks */
+struct mutex uvm_swap_data_lock = MUTEX_INITIALIZER(IPL_NONE);
struct rwlock swap_syscall_lock = RWLOCK_INITIALIZER("swplk");
struct mutex oommtx = MUTEX_INITIALIZER(IPL_VM);
@@ -224,7 +229,7 @@ void swapdrum_add(struct swapdev *, in
struct swapdev *swapdrum_getsdp(int);
struct swapdev *swaplist_find(struct vnode *, int);
-voidswaplist_insert(struct swapdev *,
+voidswaplist_insert(struct swapdev *,
struct swappri *, int);
voidswaplist_trim(void);
@@ -472,16 +477,19 @@ uvm_swap_finicrypt_all(void)
/*
* swaplist_insert: insert swap device "sdp" into the global list
*
- * => caller must hold both swap_syscall_lock and uvm.swap_data_lock
- * => caller must provide a newly malloc'd swappri structure (we will
- * FREE it if we don't need it... this it to prevent malloc blocking
- *
Re: uvm_swap: introduce uvm_swap_data_lock
Nice!
On 30/12/21(Thu) 23:38, Theo Buehler wrote:
> The diff below does two things: it adds a uvm_swap_data_lock mutex and
> trades it for the KERNEL_LOCK in uvm_swapisfull() and uvm_swap_markbad()
Why is it enough? Which fields is the lock protecting in these
function? Is it `uvmexp.swpages', could that be documented?
What about `nswapdev'? Why is the rwlock grabbed before reading it in
sys_swapctl()?i
What about `swpginuse'?
If the mutex/rwlock are used to protect the global `swap_priority' could
that be also documented? Once this is documented it should be trivial to
see that some places are missing some locking. Is it intentional?
> The uvm_swap_data_lock protects all swap data structures, so needs to be
> grabbed a few times, many of them already documented in the comments.
>
> For review, I suggest comparing to what NetBSD did and also going
> through the consumers (swaplist_insert, swaplist_find, swaplist_trim)
> and check that they are properly locked when called, or that there is
> the KERNEL_LOCK() in place when swap data structures are manipulated.
I'd suggest using the KASSERT(rw_write_held()) idiom to further reduce
the differences with NetBSD.
> In swapmount() I introduced locking since that's needed to be able to
> assert that the proper locks are held in swaplist_{insert,find,trim}.
Could the KERNEL_LOCK() in uvm_swap_get() be pushed a bit further down?
What about `uvmexp.nswget' and `uvmexp.swpgonly' in there?
> Index: uvm/uvm_swap.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_swap.c,v
> retrieving revision 1.152
> diff -u -p -r1.152 uvm_swap.c
> --- uvm/uvm_swap.c12 Dec 2021 09:14:59 - 1.152
> +++ uvm/uvm_swap.c30 Dec 2021 15:47:20 -
> @@ -44,6 +44,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -91,6 +92,9 @@
> * - swap_syscall_lock (sleep lock): this lock serializes the swapctl
> *system call and prevents the swap priority list from changing
> *while we are in the middle of a system call (e.g. SWAP_STATS).
> + * - uvm_swap_data_lock (mutex): this lock protects all swap data
> + *structures including the priority list, the swapdev structures,
> + *and the swapmap arena.
> *
> * each swap device has the following info:
> * - swap device in use (could be disabled, preventing future use)
> @@ -212,6 +216,7 @@ LIST_HEAD(swap_priority, swappri);
> struct swap_priority swap_priority;
>
> /* locks */
> +struct mutex uvm_swap_data_lock = MUTEX_INITIALIZER(IPL_NONE);
> struct rwlock swap_syscall_lock = RWLOCK_INITIALIZER("swplk");
>
> /*
> @@ -442,7 +447,7 @@ uvm_swap_finicrypt_all(void)
> /*
> * swaplist_insert: insert swap device "sdp" into the global list
> *
> - * => caller must hold both swap_syscall_lock and uvm.swap_data_lock
> + * => caller must hold both swap_syscall_lock and uvm_swap_data_lock
> * => caller must provide a newly malloc'd swappri structure (we will
> * FREE it if we don't need it... this it to prevent malloc blocking
> * here while adding swap)
> @@ -452,6 +457,9 @@ swaplist_insert(struct swapdev *sdp, str
> {
> struct swappri *spp, *pspp;
>
> + rw_assert_wrlock(_syscall_lock);
> + MUTEX_ASSERT_LOCKED(_swap_data_lock);
> +
> /*
>* find entry at or after which to insert the new device.
>*/
> @@ -493,7 +501,7 @@ swaplist_insert(struct swapdev *sdp, str
> * swaplist_find: find and optionally remove a swap device from the
> * global list.
> *
> - * => caller must hold both swap_syscall_lock and uvm.swap_data_lock
> + * => caller must hold both swap_syscall_lock and uvm_swap_data_lock
> * => we return the swapdev we found (and removed)
> */
> struct swapdev *
> @@ -502,6 +510,9 @@ swaplist_find(struct vnode *vp, boolean_
> struct swapdev *sdp;
> struct swappri *spp;
>
> + rw_assert_wrlock(_syscall_lock);
> + MUTEX_ASSERT_LOCKED(_swap_data_lock);
> +
> /*
>* search the lists for the requested vp
>*/
> @@ -524,13 +535,16 @@ swaplist_find(struct vnode *vp, boolean_
> * swaplist_trim: scan priority list for empty priority entries and kill
> * them.
> *
> - * => caller must hold both swap_syscall_lock and uvm.swap_data_lock
> + * => caller must hold both swap_syscall_lock and uvm_swap_data_lock
> */
> void
> swaplist_trim(void)
> {
> struct swappri *spp, *nextspp;
>
> + rw_assert_wrlock(_syscall_lock);
> + MUTEX_ASSERT_LOCKED(_swap_data_lock);
> +
> LIST_FOREACH_SAFE(spp, _priority, spi_swappri, nextspp) {
> if (!TAILQ_EMPTY(>spi_swapdev))
> continue;
> @@ -543,7 +557,7 @@ swaplist_trim(void)
> * swapdrum_add: add a "swapdev"'s blocks into /dev/drum's area.
> *
> * => caller must hold swap_syscall_lock
> - * => uvm.swap_data_lock should be unlocked (we may sleep)
> + * => uvm_swap_data_lock should be unlocked
uvm_swap: introduce uvm_swap_data_lock
The diff below does two things: it adds a uvm_swap_data_lock mutex and
trades it for the KERNEL_LOCK in uvm_swapisfull() and uvm_swap_markbad()
The uvm_swap_data_lock protects all swap data structures, so needs to be
grabbed a few times, many of them already documented in the comments.
For review, I suggest comparing to what NetBSD did and also going
through the consumers (swaplist_insert, swaplist_find, swaplist_trim)
and check that they are properly locked when called, or that there is
the KERNEL_LOCK() in place when swap data structures are manipulated.
In swapmount() I introduced locking since that's needed to be able to
assert that the proper locks are held in swaplist_{insert,find,trim}.
Index: uvm/uvm_swap.c
===
RCS file: /cvs/src/sys/uvm/uvm_swap.c,v
retrieving revision 1.152
diff -u -p -r1.152 uvm_swap.c
--- uvm/uvm_swap.c 12 Dec 2021 09:14:59 - 1.152
+++ uvm/uvm_swap.c 30 Dec 2021 15:47:20 -
@@ -44,6 +44,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -91,6 +92,9 @@
* - swap_syscall_lock (sleep lock): this lock serializes the swapctl
*system call and prevents the swap priority list from changing
*while we are in the middle of a system call (e.g. SWAP_STATS).
+ * - uvm_swap_data_lock (mutex): this lock protects all swap data
+ *structures including the priority list, the swapdev structures,
+ *and the swapmap arena.
*
* each swap device has the following info:
* - swap device in use (could be disabled, preventing future use)
@@ -212,6 +216,7 @@ LIST_HEAD(swap_priority, swappri);
struct swap_priority swap_priority;
/* locks */
+struct mutex uvm_swap_data_lock = MUTEX_INITIALIZER(IPL_NONE);
struct rwlock swap_syscall_lock = RWLOCK_INITIALIZER("swplk");
/*
@@ -442,7 +447,7 @@ uvm_swap_finicrypt_all(void)
/*
* swaplist_insert: insert swap device "sdp" into the global list
*
- * => caller must hold both swap_syscall_lock and uvm.swap_data_lock
+ * => caller must hold both swap_syscall_lock and uvm_swap_data_lock
* => caller must provide a newly malloc'd swappri structure (we will
* FREE it if we don't need it... this it to prevent malloc blocking
* here while adding swap)
@@ -452,6 +457,9 @@ swaplist_insert(struct swapdev *sdp, str
{
struct swappri *spp, *pspp;
+ rw_assert_wrlock(_syscall_lock);
+ MUTEX_ASSERT_LOCKED(_swap_data_lock);
+
/*
* find entry at or after which to insert the new device.
*/
@@ -493,7 +501,7 @@ swaplist_insert(struct swapdev *sdp, str
* swaplist_find: find and optionally remove a swap device from the
* global list.
*
- * => caller must hold both swap_syscall_lock and uvm.swap_data_lock
+ * => caller must hold both swap_syscall_lock and uvm_swap_data_lock
* => we return the swapdev we found (and removed)
*/
struct swapdev *
@@ -502,6 +510,9 @@ swaplist_find(struct vnode *vp, boolean_
struct swapdev *sdp;
struct swappri *spp;
+ rw_assert_wrlock(_syscall_lock);
+ MUTEX_ASSERT_LOCKED(_swap_data_lock);
+
/*
* search the lists for the requested vp
*/
@@ -524,13 +535,16 @@ swaplist_find(struct vnode *vp, boolean_
* swaplist_trim: scan priority list for empty priority entries and kill
* them.
*
- * => caller must hold both swap_syscall_lock and uvm.swap_data_lock
+ * => caller must hold both swap_syscall_lock and uvm_swap_data_lock
*/
void
swaplist_trim(void)
{
struct swappri *spp, *nextspp;
+ rw_assert_wrlock(_syscall_lock);
+ MUTEX_ASSERT_LOCKED(_swap_data_lock);
+
LIST_FOREACH_SAFE(spp, _priority, spi_swappri, nextspp) {
if (!TAILQ_EMPTY(>spi_swapdev))
continue;
@@ -543,7 +557,7 @@ swaplist_trim(void)
* swapdrum_add: add a "swapdev"'s blocks into /dev/drum's area.
*
* => caller must hold swap_syscall_lock
- * => uvm.swap_data_lock should be unlocked (we may sleep)
+ * => uvm_swap_data_lock should be unlocked (we may sleep)
*/
void
swapdrum_add(struct swapdev *sdp, int npages)
@@ -563,7 +577,7 @@ swapdrum_add(struct swapdev *sdp, int np
* to the "swapdev" that maps that section of the drum.
*
* => each swapdev takes one big contig chunk of the drum
- * => caller must hold uvm.swap_data_lock
+ * => caller must hold uvm_swap_data_lock
*/
struct swapdev *
swapdrum_getsdp(int pgno)
@@ -571,6 +585,8 @@ swapdrum_getsdp(int pgno)
struct swapdev *sdp;
struct swappri *spp;
+ MUTEX_ASSERT_LOCKED(_swap_data_lock);
+
LIST_FOREACH(spp, _priority, spi_swappri) {
TAILQ_FOREACH(sdp, >spi_swapdev, swd_next) {
if (pgno >= sdp->swd_drumoffset &&
@@ -629,7 +645,7 @@ sys_swapctl(struct proc *p, void *v, reg
*
* note that the swap_priority list can't change as long
* as we are holding the
