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
