Re: uvm_pseg_get & uvm_pseg_lck fix
On Mon, Feb 8, 2010 at 11:11 AM, Owain Ainsworth wrote: > On Mon, Feb 01, 2010 at 11:30:06PM -0500, Ted Unangst wrote: >> I think this fixes the problem with sleeping and holding pseg_lck. > > This won't build on sparc64 (the only caller of valloc_align). If you > fix that, then I am ok with this diff. I was wondering about where it was used, thanks.
Re: uvm_pseg_get & uvm_pseg_lck fix
On Mon, Feb 01, 2010 at 11:30:06PM -0500, Ted Unangst wrote:
> I think this fixes the problem with sleeping and holding pseg_lck.
>
> Index: uvm_extern.h
> ===
> RCS file: /home/tedu/cvs/src/sys/uvm/uvm_extern.h,v
> retrieving revision 1.82
> diff -u -r1.82 uvm_extern.h
> --- uvm_extern.h 11 Aug 2009 18:43:33 - 1.82
> +++ uvm_extern.h 2 Feb 2010 04:28:09 -
> @@ -517,8 +517,9 @@
> vaddr_t *, vsize_t, int,
> boolean_t, vm_map_t);
> vaddr_t uvm_km_valloc(vm_map_t, vsize_t);
> -vaddr_t uvm_km_valloc_align(vm_map_t, vsize_t, vsize_t);
> +vaddr_t uvm_km_valloc_try(vm_map_t, vsize_t);
> vaddr_t uvm_km_valloc_wait(vm_map_t, vsize_t);
> +vaddr_t uvm_km_valloc_align(struct vm_map *, vsize_t,
> vsize_t, int);
> vaddr_t uvm_km_valloc_prefer_wait(vm_map_t, vsize_t,
> voff_t);
> void *uvm_km_getpage(boolean_t, int *);
> Index: uvm_km.c
> ===
> RCS file: /home/tedu/cvs/src/sys/uvm/uvm_km.c,v
> retrieving revision 1.75
> diff -u -r1.75 uvm_km.c
> --- uvm_km.c 25 Jul 2009 12:55:40 - 1.75
> +++ uvm_km.c 2 Feb 2010 04:27:35 -
> @@ -571,11 +571,17 @@
> vaddr_t
> uvm_km_valloc(struct vm_map *map, vsize_t size)
> {
> - return(uvm_km_valloc_align(map, size, 0));
> + return(uvm_km_valloc_align(map, size, 0, 0));
> }
>
> vaddr_t
> -uvm_km_valloc_align(struct vm_map *map, vsize_t size, vsize_t align)
> +uvm_km_valloc_try(struct vm_map *map, vsize_t size)
> +{
> + return(uvm_km_valloc_align(map, size, 0, UVM_FLAG_TRYLOCK));
> +}
> +
> +vaddr_t
> +uvm_km_valloc_align(struct vm_map *map, vsize_t size, vsize_t align, int
> flags)
> {
> vaddr_t kva;
> UVMHIST_FUNC("uvm_km_valloc"); UVMHIST_CALLED(maphist);
> @@ -592,7 +598,7 @@
>
> if (__predict_false(uvm_map(map, &kva, size, uvm.kernel_object,
> UVM_UNKNOWN_OFFSET, align, UVM_MAPFLAG(UVM_PROT_ALL, UVM_PROT_ALL,
> - UVM_INH_NONE, UVM_ADV_RANDOM, 0)) != 0)) {
> + UVM_INH_NONE, UVM_ADV_RANDOM, flags)) != 0)) {
> UVMHIST_LOG(maphist, "<- done (no VM)", 0,0,0,0);
> return(0);
> }
> Index: uvm_pager.c
> ===
> RCS file: /home/tedu/cvs/src/sys/uvm/uvm_pager.c,v
> retrieving revision 1.54
> diff -u -r1.54 uvm_pager.c
> --- uvm_pager.c 22 Jul 2009 21:05:37 - 1.54
> +++ uvm_pager.c 2 Feb 2010 04:23:57 -
> @@ -138,7 +138,7 @@
> {
> KASSERT(pseg->start == 0);
> KASSERT(pseg->use == 0);
> - pseg->start = uvm_km_valloc(kernel_map, MAX_PAGER_SEGS * MAXBSIZE);
> + pseg->start = uvm_km_valloc_try(kernel_map, MAX_PAGER_SEGS * MAXBSIZE);
> }
This won't build on sparc64 (the only caller of valloc_align). If you
fix that, then I am ok with this diff.
Though to be honest, most of the valloc functions could mostly just be
replaced with uvm_km_kmemalloc (valloc_wait and valloc_align can't
without changes). The code called is equivalent in action to:
valloc() -> kmemalloc( VALLOC)
valloc_try() -> kmemalloc(TRYLOCK | VALLOC)
One thing at a time though. uvm_km.c has 3 or 4 allocators that are all
almost identical (some differ in flags, but not much else). I'll look
into clearing it up.
Reviewing that last paragraph, I notice that kmemalloc doesn't lock the
object that it is called with. I'll fix that soon...
Cheers,
-0-
--
If you keep anything long enough, you can throw it away.
Re: uvm_pseg_get & uvm_pseg_lck fix
On Wed, Feb 03, 2010 at 04:08:39PM +0100, Mark Kettenis wrote:
> > Date: Wed, 3 Feb 2010 14:41:04 +
> > From: Owain Ainsworth
> >
> > On Mon, Feb 01, 2010 at 11:30:06PM -0500, Ted Unangst wrote:
> > > I think this fixes the problem with sleeping and holding pseg_lck.
> > >
> > > Index: uvm_extern.h
> > > ===
> > > RCS file: /home/tedu/cvs/src/sys/uvm/uvm_extern.h,v
> > > retrieving revision 1.82
> > > diff -u -r1.82 uvm_extern.h
> > > --- uvm_extern.h 11 Aug 2009 18:43:33 - 1.82
> > > +++ uvm_extern.h 2 Feb 2010 04:28:09 -
> > > @@ -517,8 +517,9 @@
> > > vaddr_t *, vsize_t, int,
> > > boolean_t, vm_map_t);
> > > vaddr_t uvm_km_valloc(vm_map_t, vsize_t);
> > > -vaddr_t uvm_km_valloc_align(vm_map_t, vsize_t, vsize_t);
> > > +vaddr_t uvm_km_valloc_try(vm_map_t, vsize_t);
> > > vaddr_t uvm_km_valloc_wait(vm_map_t, vsize_t);
> > > +vaddr_t uvm_km_valloc_align(struct vm_map *, vsize_t,
> > > vsize_t, int);
> > > vaddr_t uvm_km_valloc_prefer_wait(vm_map_t, vsize_t,
> > > voff_t);
> > > void *uvm_km_getpage(boolean_t, int *);
> > > Index: uvm_km.c
> > > ===
> > > RCS file: /home/tedu/cvs/src/sys/uvm/uvm_km.c,v
> > > retrieving revision 1.75
> > > diff -u -r1.75 uvm_km.c
> > > --- uvm_km.c 25 Jul 2009 12:55:40 - 1.75
> > > +++ uvm_km.c 2 Feb 2010 04:27:35 -
> > > @@ -571,11 +571,17 @@
> > > vaddr_t
> > > uvm_km_valloc(struct vm_map *map, vsize_t size)
> > > {
> > > - return(uvm_km_valloc_align(map, size, 0));
> > > + return(uvm_km_valloc_align(map, size, 0, 0));
> > > }
> > >
> > > vaddr_t
> > > -uvm_km_valloc_align(struct vm_map *map, vsize_t size, vsize_t align)
> > > +uvm_km_valloc_try(struct vm_map *map, vsize_t size)
> > > +{
> > > + return(uvm_km_valloc_align(map, size, 0, UVM_FLAG_TRYLOCK));
> > > +}
> > > +
> > > +vaddr_t
> > > +uvm_km_valloc_align(struct vm_map *map, vsize_t size, vsize_t align, int
> > > flags)
> > > {
> > > vaddr_t kva;
> > > UVMHIST_FUNC("uvm_km_valloc"); UVMHIST_CALLED(maphist);
> > > @@ -592,7 +598,7 @@
> > >
> > > if (__predict_false(uvm_map(map, &kva, size, uvm.kernel_object,
> > > UVM_UNKNOWN_OFFSET, align, UVM_MAPFLAG(UVM_PROT_ALL, UVM_PROT_ALL,
> > > - UVM_INH_NONE, UVM_ADV_RANDOM, 0)) != 0)) {
> > > + UVM_INH_NONE, UVM_ADV_RANDOM, flags)) != 0)) {
> > > UVMHIST_LOG(maphist, "<- done (no VM)", 0,0,0,0);
> > > return(0);
> > > }
> > > Index: uvm_pager.c
> > > ===
> > > RCS file: /home/tedu/cvs/src/sys/uvm/uvm_pager.c,v
> > > retrieving revision 1.54
> > > diff -u -r1.54 uvm_pager.c
> > > --- uvm_pager.c 22 Jul 2009 21:05:37 - 1.54
> > > +++ uvm_pager.c 2 Feb 2010 04:23:57 -
> > > @@ -138,7 +138,7 @@
> > > {
> > > KASSERT(pseg->start == 0);
> > > KASSERT(pseg->use == 0);
> > > - pseg->start = uvm_km_valloc(kernel_map, MAX_PAGER_SEGS * MAXBSIZE);
> > > + pseg->start = uvm_km_valloc_try(kernel_map, MAX_PAGER_SEGS * MAXBSIZE);
> >
> >
> > this doesn't take into account the fact that valloc_try may fail and
> > return 0.
>
> That case is handled just fine. In uvm_pseg_get() we have a check
> for pseg->start == 0 immediately after the uvm_pseg_init() call.
Ah yes, I missed that (also pointed out by ted). In which case this does
look correct. Will review more carefully when I get home from work.
-0-
--
I had to censor everything my sons watched ... even on the Mary Tyler
Moore show I heard the word "damn"!
-- Mary Lou Bax
Re: uvm_pseg_get & uvm_pseg_lck fix
> Date: Wed, 3 Feb 2010 06:10:09 +0100 > From: Ariane van der Steldt > > I think it'll only fix the problem partially though: uvm_km_free* > doesn't have a trylock variant and may sleep just as well on the lock. That's fairly easy to fix though. You can simply save pseg->start in a local variable, relase the lock and call uvm_km_free().
Re: uvm_pseg_get & uvm_pseg_lck fix
> Date: Wed, 3 Feb 2010 14:41:04 +
> From: Owain Ainsworth
>
> On Mon, Feb 01, 2010 at 11:30:06PM -0500, Ted Unangst wrote:
> > I think this fixes the problem with sleeping and holding pseg_lck.
> >
> > Index: uvm_extern.h
> > ===
> > RCS file: /home/tedu/cvs/src/sys/uvm/uvm_extern.h,v
> > retrieving revision 1.82
> > diff -u -r1.82 uvm_extern.h
> > --- uvm_extern.h11 Aug 2009 18:43:33 - 1.82
> > +++ uvm_extern.h2 Feb 2010 04:28:09 -
> > @@ -517,8 +517,9 @@
> > vaddr_t *, vsize_t, int,
> > boolean_t, vm_map_t);
> > vaddr_tuvm_km_valloc(vm_map_t, vsize_t);
> > -vaddr_tuvm_km_valloc_align(vm_map_t, vsize_t, vsize_t);
> > +vaddr_tuvm_km_valloc_try(vm_map_t, vsize_t);
> > vaddr_tuvm_km_valloc_wait(vm_map_t, vsize_t);
> > +vaddr_tuvm_km_valloc_align(struct vm_map *, vsize_t,
> > vsize_t, int);
> > vaddr_tuvm_km_valloc_prefer_wait(vm_map_t, vsize_t,
> > voff_t);
> > void *uvm_km_getpage(boolean_t, int *);
> > Index: uvm_km.c
> > ===
> > RCS file: /home/tedu/cvs/src/sys/uvm/uvm_km.c,v
> > retrieving revision 1.75
> > diff -u -r1.75 uvm_km.c
> > --- uvm_km.c25 Jul 2009 12:55:40 - 1.75
> > +++ uvm_km.c2 Feb 2010 04:27:35 -
> > @@ -571,11 +571,17 @@
> > vaddr_t
> > uvm_km_valloc(struct vm_map *map, vsize_t size)
> > {
> > - return(uvm_km_valloc_align(map, size, 0));
> > + return(uvm_km_valloc_align(map, size, 0, 0));
> > }
> >
> > vaddr_t
> > -uvm_km_valloc_align(struct vm_map *map, vsize_t size, vsize_t align)
> > +uvm_km_valloc_try(struct vm_map *map, vsize_t size)
> > +{
> > + return(uvm_km_valloc_align(map, size, 0, UVM_FLAG_TRYLOCK));
> > +}
> > +
> > +vaddr_t
> > +uvm_km_valloc_align(struct vm_map *map, vsize_t size, vsize_t align, int
> > flags)
> > {
> > vaddr_t kva;
> > UVMHIST_FUNC("uvm_km_valloc"); UVMHIST_CALLED(maphist);
> > @@ -592,7 +598,7 @@
> >
> > if (__predict_false(uvm_map(map, &kva, size, uvm.kernel_object,
> > UVM_UNKNOWN_OFFSET, align, UVM_MAPFLAG(UVM_PROT_ALL, UVM_PROT_ALL,
> > - UVM_INH_NONE, UVM_ADV_RANDOM, 0)) != 0)) {
> > + UVM_INH_NONE, UVM_ADV_RANDOM, flags)) != 0)) {
> > UVMHIST_LOG(maphist, "<- done (no VM)", 0,0,0,0);
> > return(0);
> > }
> > Index: uvm_pager.c
> > ===
> > RCS file: /home/tedu/cvs/src/sys/uvm/uvm_pager.c,v
> > retrieving revision 1.54
> > diff -u -r1.54 uvm_pager.c
> > --- uvm_pager.c 22 Jul 2009 21:05:37 - 1.54
> > +++ uvm_pager.c 2 Feb 2010 04:23:57 -
> > @@ -138,7 +138,7 @@
> > {
> > KASSERT(pseg->start == 0);
> > KASSERT(pseg->use == 0);
> > - pseg->start = uvm_km_valloc(kernel_map, MAX_PAGER_SEGS * MAXBSIZE);
> > + pseg->start = uvm_km_valloc_try(kernel_map, MAX_PAGER_SEGS * MAXBSIZE);
>
>
> this doesn't take into account the fact that valloc_try may fail and
> return 0.
That case is handled just fine. In uvm_pseg_get() we have a check
for pseg->start == 0 immediately after the uvm_pseg_init() call.
Re: uvm_pseg_get & uvm_pseg_lck fix
On Wed, Feb 3, 2010 at 9:41 AM, Owain Ainsworth
wrote:
>> Index: uvm_pager.c
>> ===
>> RCS file: /home/tedu/cvs/src/sys/uvm/uvm_pager.c,v
>> retrieving revision 1.54
>> diff -u -r1.54 uvm_pager.c
>> --- uvm_pager.c 22 Jul 2009 21:05:37 - 1.54
>> +++ uvm_pager.c 2 Feb 2010 04:23:57 -
>> @@ -138,7 +138,7 @@
>> {
>> KASSERT(pseg->start == 0);
>> KASSERT(pseg->use == 0);
>> - pseg->start = uvm_km_valloc(kernel_map, MAX_PAGER_SEGS * MAXBSIZE);
>> + pseg->start = uvm_km_valloc_try(kernel_map, MAX_PAGER_SEGS *
MAXBSIZE);
>
>
> this doesn't take into account the fact that valloc_try may fail and
> return 0.
That appears to be the caller's concern. There's no difference in
that regard between try and not try. (only do :)).
Re: uvm_pseg_get & uvm_pseg_lck fix
On Mon, Feb 01, 2010 at 11:30:06PM -0500, Ted Unangst wrote:
> I think this fixes the problem with sleeping and holding pseg_lck.
>
> Index: uvm_extern.h
> ===
> RCS file: /home/tedu/cvs/src/sys/uvm/uvm_extern.h,v
> retrieving revision 1.82
> diff -u -r1.82 uvm_extern.h
> --- uvm_extern.h 11 Aug 2009 18:43:33 - 1.82
> +++ uvm_extern.h 2 Feb 2010 04:28:09 -
> @@ -517,8 +517,9 @@
> vaddr_t *, vsize_t, int,
> boolean_t, vm_map_t);
> vaddr_t uvm_km_valloc(vm_map_t, vsize_t);
> -vaddr_t uvm_km_valloc_align(vm_map_t, vsize_t, vsize_t);
> +vaddr_t uvm_km_valloc_try(vm_map_t, vsize_t);
> vaddr_t uvm_km_valloc_wait(vm_map_t, vsize_t);
> +vaddr_t uvm_km_valloc_align(struct vm_map *, vsize_t,
> vsize_t, int);
> vaddr_t uvm_km_valloc_prefer_wait(vm_map_t, vsize_t,
> voff_t);
> void *uvm_km_getpage(boolean_t, int *);
> Index: uvm_km.c
> ===
> RCS file: /home/tedu/cvs/src/sys/uvm/uvm_km.c,v
> retrieving revision 1.75
> diff -u -r1.75 uvm_km.c
> --- uvm_km.c 25 Jul 2009 12:55:40 - 1.75
> +++ uvm_km.c 2 Feb 2010 04:27:35 -
> @@ -571,11 +571,17 @@
> vaddr_t
> uvm_km_valloc(struct vm_map *map, vsize_t size)
> {
> - return(uvm_km_valloc_align(map, size, 0));
> + return(uvm_km_valloc_align(map, size, 0, 0));
> }
>
> vaddr_t
> -uvm_km_valloc_align(struct vm_map *map, vsize_t size, vsize_t align)
> +uvm_km_valloc_try(struct vm_map *map, vsize_t size)
> +{
> + return(uvm_km_valloc_align(map, size, 0, UVM_FLAG_TRYLOCK));
> +}
> +
> +vaddr_t
> +uvm_km_valloc_align(struct vm_map *map, vsize_t size, vsize_t align, int
> flags)
> {
> vaddr_t kva;
> UVMHIST_FUNC("uvm_km_valloc"); UVMHIST_CALLED(maphist);
> @@ -592,7 +598,7 @@
>
> if (__predict_false(uvm_map(map, &kva, size, uvm.kernel_object,
> UVM_UNKNOWN_OFFSET, align, UVM_MAPFLAG(UVM_PROT_ALL, UVM_PROT_ALL,
> - UVM_INH_NONE, UVM_ADV_RANDOM, 0)) != 0)) {
> + UVM_INH_NONE, UVM_ADV_RANDOM, flags)) != 0)) {
> UVMHIST_LOG(maphist, "<- done (no VM)", 0,0,0,0);
> return(0);
> }
> Index: uvm_pager.c
> ===
> RCS file: /home/tedu/cvs/src/sys/uvm/uvm_pager.c,v
> retrieving revision 1.54
> diff -u -r1.54 uvm_pager.c
> --- uvm_pager.c 22 Jul 2009 21:05:37 - 1.54
> +++ uvm_pager.c 2 Feb 2010 04:23:57 -
> @@ -138,7 +138,7 @@
> {
> KASSERT(pseg->start == 0);
> KASSERT(pseg->use == 0);
> - pseg->start = uvm_km_valloc(kernel_map, MAX_PAGER_SEGS * MAXBSIZE);
> + pseg->start = uvm_km_valloc_try(kernel_map, MAX_PAGER_SEGS * MAXBSIZE);
this doesn't take into account the fact that valloc_try may fail and
return 0.
The principle is good, but more is needed. Perhaps up to unlocking
everything and calling where we can sleep.
-0-
--
Fortune's Office Door Sign of the Week:
Incorrigible punster -- Do not incorrige.
Re: uvm_pseg_get & uvm_pseg_lck fix
On Mon, Feb 01, 2010 at 11:30:06PM -0500, Ted Unangst wrote:
> I think this fixes the problem with sleeping and holding pseg_lck.
>
> Index: uvm_extern.h
> ===
> RCS file: /home/tedu/cvs/src/sys/uvm/uvm_extern.h,v
> retrieving revision 1.82
> diff -u -r1.82 uvm_extern.h
> --- uvm_extern.h 11 Aug 2009 18:43:33 - 1.82
> +++ uvm_extern.h 2 Feb 2010 04:28:09 -
> @@ -517,8 +517,9 @@
> vaddr_t *, vsize_t, int,
> boolean_t, vm_map_t);
> vaddr_t uvm_km_valloc(vm_map_t, vsize_t);
> -vaddr_t uvm_km_valloc_align(vm_map_t, vsize_t, vsize_t);
> +vaddr_t uvm_km_valloc_try(vm_map_t, vsize_t);
> vaddr_t uvm_km_valloc_wait(vm_map_t, vsize_t);
> +vaddr_t uvm_km_valloc_align(struct vm_map *, vsize_t,
> vsize_t, int);
> vaddr_t uvm_km_valloc_prefer_wait(vm_map_t, vsize_t,
> voff_t);
> void *uvm_km_getpage(boolean_t, int *);
> Index: uvm_km.c
> ===
> RCS file: /home/tedu/cvs/src/sys/uvm/uvm_km.c,v
> retrieving revision 1.75
> diff -u -r1.75 uvm_km.c
> --- uvm_km.c 25 Jul 2009 12:55:40 - 1.75
> +++ uvm_km.c 2 Feb 2010 04:27:35 -
> @@ -571,11 +571,17 @@
> vaddr_t
> uvm_km_valloc(struct vm_map *map, vsize_t size)
> {
> - return(uvm_km_valloc_align(map, size, 0));
> + return(uvm_km_valloc_align(map, size, 0, 0));
> }
>
> vaddr_t
> -uvm_km_valloc_align(struct vm_map *map, vsize_t size, vsize_t align)
> +uvm_km_valloc_try(struct vm_map *map, vsize_t size)
> +{
> + return(uvm_km_valloc_align(map, size, 0, UVM_FLAG_TRYLOCK));
> +}
> +
> +vaddr_t
> +uvm_km_valloc_align(struct vm_map *map, vsize_t size, vsize_t align, int
> flags)
> {
> vaddr_t kva;
> UVMHIST_FUNC("uvm_km_valloc"); UVMHIST_CALLED(maphist);
> @@ -592,7 +598,7 @@
>
> if (__predict_false(uvm_map(map, &kva, size, uvm.kernel_object,
> UVM_UNKNOWN_OFFSET, align, UVM_MAPFLAG(UVM_PROT_ALL, UVM_PROT_ALL,
> - UVM_INH_NONE, UVM_ADV_RANDOM, 0)) != 0)) {
> + UVM_INH_NONE, UVM_ADV_RANDOM, flags)) != 0)) {
> UVMHIST_LOG(maphist, "<- done (no VM)", 0,0,0,0);
> return(0);
> }
> Index: uvm_pager.c
> ===
> RCS file: /home/tedu/cvs/src/sys/uvm/uvm_pager.c,v
> retrieving revision 1.54
> diff -u -r1.54 uvm_pager.c
> --- uvm_pager.c 22 Jul 2009 21:05:37 - 1.54
> +++ uvm_pager.c 2 Feb 2010 04:23:57 -
> @@ -138,7 +138,7 @@
> {
> KASSERT(pseg->start == 0);
> KASSERT(pseg->use == 0);
> - pseg->start = uvm_km_valloc(kernel_map, MAX_PAGER_SEGS * MAXBSIZE);
> + pseg->start = uvm_km_valloc_try(kernel_map, MAX_PAGER_SEGS * MAXBSIZE);
I'd prefer
uvm_km_kmemalloc(kernel_map, NULL, MAX_PAGER_SEGS * MAXBSIZE,
UVM_KMF_TRYLOCK|UVM_KMF_VALLOC)
instead of introducing a new uvm_km_valloc_* function. There are plenty
of those already and most of them can be rewritten to uvm_km_kmemalloc
with suitable flags.
Apart from that, the diff looks fine to me.
> }
>
> /*
>
I think it'll only fix the problem partially though: uvm_km_free*
doesn't have a trylock variant and may sleep just as well on the lock.
I think what happens is that another uvm_km_pgremove is sleeping on
the wanted flag, holding the rwlock. I.e. another uvm_km_free is in
progress somewhere else, waiting for a busy kernelpage (which hopefully
is busy because of a 3rd kernel thread, possibly the pagedaemon).
To summarize:
[1] aiodone needs pseg mutex
[2] mutex is held by pagedaemon thread which illegally sleeps
[3] because the pagedaemon wants access to the uvm_map
[4] which is held by another kernel thread sleeping while locking uvm_map
[5] because another thread has its page marked busy
The thread in [5] could be the pagedaemon, in which case we have a
deadlock. However, making the allocation in [2] try-lock instead may
indeed remove the deadlock (assuming no other process would try to mark
pages busy that don't belong to it).
But I think the deadlock would still resurface, simply because this is a
kernel page and some other kernel thread may try to free its memory
between the pagedaemon starting a page-out and the aiodone daemon
cleaning up.
To make matters worse, it could be that a uvm object decided it needed
to be synced to disk: if memory serves me right any uvm_object io goes
through this code.
The only fix I can think of at the moment is not to use uvm_km_alloc and
uvm_km_free at all for the pseg code.
Ciao,
--
Ariane
Re: uvm_pseg_get & uvm_pseg_lck fix
On Mon, Feb 01, 2010 at 23:30 -0500, Ted Unangst wrote:
> I think this fixes the problem with sleeping and holding pseg_lck.
>
perhaps it also makes sense to honour UVMPAGER_MAPIN_WAITOK flag.
what do you think?
> Index: uvm_extern.h
> ===
> RCS file: /home/tedu/cvs/src/sys/uvm/uvm_extern.h,v
> retrieving revision 1.82
> diff -u -r1.82 uvm_extern.h
> --- uvm_extern.h 11 Aug 2009 18:43:33 - 1.82
> +++ uvm_extern.h 2 Feb 2010 04:28:09 -
> @@ -517,8 +517,9 @@
> vaddr_t *, vsize_t, int,
> boolean_t, vm_map_t);
> vaddr_t uvm_km_valloc(vm_map_t, vsize_t);
> -vaddr_t uvm_km_valloc_align(vm_map_t, vsize_t, vsize_t);
> +vaddr_t uvm_km_valloc_try(vm_map_t, vsize_t);
> vaddr_t uvm_km_valloc_wait(vm_map_t, vsize_t);
> +vaddr_t uvm_km_valloc_align(struct vm_map *, vsize_t,
> vsize_t, int);
> vaddr_t uvm_km_valloc_prefer_wait(vm_map_t, vsize_t,
> voff_t);
> void *uvm_km_getpage(boolean_t, int *);
> Index: uvm_km.c
> ===
> RCS file: /home/tedu/cvs/src/sys/uvm/uvm_km.c,v
> retrieving revision 1.75
> diff -u -r1.75 uvm_km.c
> --- uvm_km.c 25 Jul 2009 12:55:40 - 1.75
> +++ uvm_km.c 2 Feb 2010 04:27:35 -
> @@ -571,11 +571,17 @@
> vaddr_t
> uvm_km_valloc(struct vm_map *map, vsize_t size)
> {
> - return(uvm_km_valloc_align(map, size, 0));
> + return(uvm_km_valloc_align(map, size, 0, 0));
> }
>
> vaddr_t
> -uvm_km_valloc_align(struct vm_map *map, vsize_t size, vsize_t align)
> +uvm_km_valloc_try(struct vm_map *map, vsize_t size)
> +{
> + return(uvm_km_valloc_align(map, size, 0, UVM_FLAG_TRYLOCK));
> +}
> +
> +vaddr_t
> +uvm_km_valloc_align(struct vm_map *map, vsize_t size, vsize_t align, int
> flags)
> {
> vaddr_t kva;
> UVMHIST_FUNC("uvm_km_valloc"); UVMHIST_CALLED(maphist);
> @@ -592,7 +598,7 @@
>
> if (__predict_false(uvm_map(map, &kva, size, uvm.kernel_object,
> UVM_UNKNOWN_OFFSET, align, UVM_MAPFLAG(UVM_PROT_ALL, UVM_PROT_ALL,
> - UVM_INH_NONE, UVM_ADV_RANDOM, 0)) != 0)) {
> + UVM_INH_NONE, UVM_ADV_RANDOM, flags)) != 0)) {
> UVMHIST_LOG(maphist, "<- done (no VM)", 0,0,0,0);
> return(0);
> }
> Index: uvm_pager.c
> ===
> RCS file: /home/tedu/cvs/src/sys/uvm/uvm_pager.c,v
> retrieving revision 1.54
> diff -u -r1.54 uvm_pager.c
> --- uvm_pager.c 22 Jul 2009 21:05:37 - 1.54
> +++ uvm_pager.c 2 Feb 2010 04:23:57 -
> @@ -138,7 +138,7 @@
> {
> KASSERT(pseg->start == 0);
> KASSERT(pseg->use == 0);
> - pseg->start = uvm_km_valloc(kernel_map, MAX_PAGER_SEGS * MAXBSIZE);
> + pseg->start = uvm_km_valloc_try(kernel_map, MAX_PAGER_SEGS * MAXBSIZE);
> }
>
> /*
Re: uvm_pseg_get & uvm_pseg_lck fix
On Tue, Feb 02, 2010 at 15:21 +0300, Mike Belopuhov wrote: > On Mon, Feb 01, 2010 at 23:30 -0500, Ted Unangst wrote: > > I think this fixes the problem with sleeping and holding pseg_lck. > > > > perhaps it also makes sense to honour UVMPAGER_MAPIN_WAITOK flag. > what do you think? > no, i'm sorry, this is wrong.
uvm_pseg_get & uvm_pseg_lck fix
I think this fixes the problem with sleeping and holding pseg_lck.
Index: uvm_extern.h
===
RCS file: /home/tedu/cvs/src/sys/uvm/uvm_extern.h,v
retrieving revision 1.82
diff -u -r1.82 uvm_extern.h
--- uvm_extern.h11 Aug 2009 18:43:33 - 1.82
+++ uvm_extern.h2 Feb 2010 04:28:09 -
@@ -517,8 +517,9 @@
vaddr_t *, vsize_t, int,
boolean_t, vm_map_t);
vaddr_tuvm_km_valloc(vm_map_t, vsize_t);
-vaddr_tuvm_km_valloc_align(vm_map_t, vsize_t, vsize_t);
+vaddr_tuvm_km_valloc_try(vm_map_t, vsize_t);
vaddr_tuvm_km_valloc_wait(vm_map_t, vsize_t);
+vaddr_tuvm_km_valloc_align(struct vm_map *, vsize_t,
vsize_t, int);
vaddr_tuvm_km_valloc_prefer_wait(vm_map_t, vsize_t,
voff_t);
void *uvm_km_getpage(boolean_t, int *);
Index: uvm_km.c
===
RCS file: /home/tedu/cvs/src/sys/uvm/uvm_km.c,v
retrieving revision 1.75
diff -u -r1.75 uvm_km.c
--- uvm_km.c25 Jul 2009 12:55:40 - 1.75
+++ uvm_km.c2 Feb 2010 04:27:35 -
@@ -571,11 +571,17 @@
vaddr_t
uvm_km_valloc(struct vm_map *map, vsize_t size)
{
- return(uvm_km_valloc_align(map, size, 0));
+ return(uvm_km_valloc_align(map, size, 0, 0));
}
vaddr_t
-uvm_km_valloc_align(struct vm_map *map, vsize_t size, vsize_t align)
+uvm_km_valloc_try(struct vm_map *map, vsize_t size)
+{
+ return(uvm_km_valloc_align(map, size, 0, UVM_FLAG_TRYLOCK));
+}
+
+vaddr_t
+uvm_km_valloc_align(struct vm_map *map, vsize_t size, vsize_t align, int flags)
{
vaddr_t kva;
UVMHIST_FUNC("uvm_km_valloc"); UVMHIST_CALLED(maphist);
@@ -592,7 +598,7 @@
if (__predict_false(uvm_map(map, &kva, size, uvm.kernel_object,
UVM_UNKNOWN_OFFSET, align, UVM_MAPFLAG(UVM_PROT_ALL, UVM_PROT_ALL,
- UVM_INH_NONE, UVM_ADV_RANDOM, 0)) != 0)) {
+ UVM_INH_NONE, UVM_ADV_RANDOM, flags)) != 0)) {
UVMHIST_LOG(maphist, "<- done (no VM)", 0,0,0,0);
return(0);
}
Index: uvm_pager.c
===
RCS file: /home/tedu/cvs/src/sys/uvm/uvm_pager.c,v
retrieving revision 1.54
diff -u -r1.54 uvm_pager.c
--- uvm_pager.c 22 Jul 2009 21:05:37 - 1.54
+++ uvm_pager.c 2 Feb 2010 04:23:57 -
@@ -138,7 +138,7 @@
{
KASSERT(pseg->start == 0);
KASSERT(pseg->use == 0);
- pseg->start = uvm_km_valloc(kernel_map, MAX_PAGER_SEGS * MAXBSIZE);
+ pseg->start = uvm_km_valloc_try(kernel_map, MAX_PAGER_SEGS * MAXBSIZE);
}
/*
Re: uvm_pseg_get & uvm_pseg_lck
On Sun, Jan 31, 2010 at 02:05:35PM +0100, Mark Kettenis wrote: > > Date: Thu, 28 Jan 2010 18:55:55 +0300 > > From: Mike Belopuhov > > > > Hi all, > > > > could someone please enlighten me how this uvm_pseg_get is supposed > > to work? > > > > uvm_pseg_get > > mtx_enter(&uvm_pseg_lck); > > `-> uvm_pseg_init > > `-> uvm_km_valloc > > `-> uvm_km_valloc_align > > `-> uvm_map > > `-> vm_map_lock <-- sleeping lock > > > > Debugger(10,d73eeeac,d03e0ee2,d06ab9a0,0) at Debugger+0x4 > > panic(d020319c,d73eeecc,d03ad948,d06ab980,d51c8000) at panic+0x61 > > mtx_enter(d06ab980,d51c8000,d51d8000,d02f70c5,1) at mtx_enter+0x5c > > uvm_pseg_release(d51c8000,10,d73eeefc,d02f41d4,50) at uvm_pseg_release+0x64 > > uvm_aio_aiodone(d0f422e4,0,d11c1c3c,d73eef90,d02e763d) at > > uvm_aio_aiodone+0xba > > uvm_aiodone_daemon(d11c1c3c) at uvm_aiodone_daemon+0x97 > > Bad frame pointer: 0xd079ceb8 > > > > OpenBSD 4.6. > > > > Or am I missing something? > > No, I think you're right. I think uvm_pseg_init() should use a memory > allocation function that can't sleep. > > I think I brought this up before questioning whether uvm_km_valloc() > was allowed to sleep (since we also have uvm_km_valloc_wait()). To > that question art@ answered that uvm_km_valloc() still was allowed to > sleep. Yes, correct. valloc_wait is special and only used for the limited maps (phys and exec iirc). uvm_km_valloc_try or something would be trivial to write which is a valloc that only does a trylock on the map. -0- -- In order to make an apple pie from scratch, you must first create the universe. -- Carl Sagan, Cosmos
Re: uvm_pseg_get & uvm_pseg_lck
> Date: Thu, 28 Jan 2010 18:55:55 +0300 > From: Mike Belopuhov > > Hi all, > > could someone please enlighten me how this uvm_pseg_get is supposed > to work? > > uvm_pseg_get > mtx_enter(&uvm_pseg_lck); > `-> uvm_pseg_init > `-> uvm_km_valloc > `-> uvm_km_valloc_align > `-> uvm_map > `-> vm_map_lock <-- sleeping lock > > Debugger(10,d73eeeac,d03e0ee2,d06ab9a0,0) at Debugger+0x4 > panic(d020319c,d73eeecc,d03ad948,d06ab980,d51c8000) at panic+0x61 > mtx_enter(d06ab980,d51c8000,d51d8000,d02f70c5,1) at mtx_enter+0x5c > uvm_pseg_release(d51c8000,10,d73eeefc,d02f41d4,50) at uvm_pseg_release+0x64 > uvm_aio_aiodone(d0f422e4,0,d11c1c3c,d73eef90,d02e763d) at uvm_aio_aiodone+0xba > uvm_aiodone_daemon(d11c1c3c) at uvm_aiodone_daemon+0x97 > Bad frame pointer: 0xd079ceb8 > > OpenBSD 4.6. > > Or am I missing something? No, I think you're right. I think uvm_pseg_init() should use a memory allocation function that can't sleep. I think I brought this up before questioning whether uvm_km_valloc() was allowed to sleep (since we also have uvm_km_valloc_wait()). To that question art@ answered that uvm_km_valloc() still was allowed to sleep.
Re: uvm_pseg_get & uvm_pseg_lck
On Thu, Jan 28, 2010 at 06:55:55PM +0300, Mike Belopuhov wrote:
> could someone please enlighten me how this uvm_pseg_get is supposed
> to work?
>
> uvm_pseg_get
> mtx_enter(&uvm_pseg_lck);
> `-> uvm_pseg_init
> `-> uvm_km_valloc
> `-> uvm_km_valloc_align
> `-> uvm_map
> `-> vm_map_lock <-- sleeping lock
It's supposed to work like that, except for the sleeping on the lock (if
the lock is busy, it should fail instead).
pseg was written to remove a deadlock during (heavy) paging. The system
works by holding on to a number of virtual addresses, each containing an
area of MAXBSIZE size. The first of these areas is always populated, so
that paging never fails (this was an issue when the diff was created).
The next are allocated when needed and freed when no longer needed, so
that the pager can speed up io (by queueing more) without holding on to
that memory when not doing anything.
uvm_km_valloc probably needs to be replaced by uvm_km_kmemalloc using
flags = UVM_KMF_TRYLOCK|UVM_KMF_VALLOC.
It is important that these are only virtual addresses, with no physical
memory backing them, so that pages from userland processes can be mapped
in to perform the IO for paging out.
> Debugger(10,d73eeeac,d03e0ee2,d06ab9a0,0) at Debugger+0x4
> panic(d020319c,d73eeecc,d03ad948,d06ab980,d51c8000) at panic+0x61
> mtx_enter(d06ab980,d51c8000,d51d8000,d02f70c5,1) at mtx_enter+0x5c
> uvm_pseg_release(d51c8000,10,d73eeefc,d02f41d4,50) at uvm_pseg_release+0x64
> uvm_aio_aiodone(d0f422e4,0,d11c1c3c,d73eef90,d02e763d) at uvm_aio_aiodone+0xba
> uvm_aiodone_daemon(d11c1c3c) at uvm_aiodone_daemon+0x97
> Bad frame pointer: 0xd079ceb8
>
> OpenBSD 4.6.
>
> Or am I missing something?
You're not missing anything. My fault for misinterpreting the
distinction between uvm_km_valloc{,_wait}.
I don't have a reproducable case. How did you hit the panic?
Ciao,
--
Ariane
uvm_pseg_get & uvm_pseg_lck
Hi all, could someone please enlighten me how this uvm_pseg_get is supposed to work? uvm_pseg_get mtx_enter(&uvm_pseg_lck); `-> uvm_pseg_init `-> uvm_km_valloc `-> uvm_km_valloc_align `-> uvm_map `-> vm_map_lock <-- sleeping lock Debugger(10,d73eeeac,d03e0ee2,d06ab9a0,0) at Debugger+0x4 panic(d020319c,d73eeecc,d03ad948,d06ab980,d51c8000) at panic+0x61 mtx_enter(d06ab980,d51c8000,d51d8000,d02f70c5,1) at mtx_enter+0x5c uvm_pseg_release(d51c8000,10,d73eeefc,d02f41d4,50) at uvm_pseg_release+0x64 uvm_aio_aiodone(d0f422e4,0,d11c1c3c,d73eef90,d02e763d) at uvm_aio_aiodone+0xba uvm_aiodone_daemon(d11c1c3c) at uvm_aiodone_daemon+0x97 Bad frame pointer: 0xd079ceb8 OpenBSD 4.6. Or am I missing something?
