> From: David Gwynne <[email protected]>
> Date: Mon, 19 Jan 2015 11:53:09 +1000
>
> > On 30 Dec 2014, at 22:49, Mark Kettenis <[email protected]> wrote:
> >
> >> From: David Gwynne <[email protected]>
> >> Date: Tue, 30 Dec 2014 21:53:46 +1000
> >>
> >>> On 29 Dec 2014, at 7:55 am, Mark Kettenis <[email protected]> wrote:
> >>>
> >>>> Date: Sun, 28 Dec 2014 20:56:33 +0100 (CET)
> >>>> From: Mark Kettenis <[email protected]>
> >>>>
> >>>> P.S. I would not be surprised if this would "fix" the problems landisk
> >>>> has with the "* 8" pool diff. Currently updating my landisk to
> >>>> verify.
> >>>
> >>> And it seems this does fix the landisk problem. I strongly suspect
> >>> there is some cache nastyness going on.
> >>
> >> i just got this with your diff + my pool garbage collector diff:
> >>
> >> uvm_fault(0xffffffff819042c0, 0x0, 0, 2) -> e
> >> kernel: page fault trap, code=0
> >> Stopped at km_free+0x72: movq $0,0(%rax)
> >> ddb{4}> tr
> >> km_free() at km_free+0x72
> >> pool_large_free() at pool_large_free+0x7e
> >> pool_allocator_free() at pool_allocator_free+0x30
> >> pool_p_free() at pool_p_free+0xd3
> >> pool_gc_pages() at pool_gc_pages+0xc9
> >> taskq_thread() at taskq_thread+0x3e
> >> end trace frame: 0x0, count: -6
> >>
> >> ive been running the gc in production for a week so far and havent
> >> hit this, so i think its new with your diff. however, i havent
> >> looked into it yet so i could be wrong.
> >
> > Hmm, I guess you have a pool that has dma constraints and uses in-page
> > headers? That would actually prevent the direct map from being used,
> > but km_free would not realize this, and try to unmap the direct
> > mapping.
> >
> > Arguably that bug is already present in the current code. We have two
> > options:
> >
> > 1. Make km_free(9) respect the kv_align attribute and skip the direct
> > mapping code path. Should work, since the pool allocator code is
> > careful enough to set the alignment for the km_free() calls. Would
> > make things slightly fragile since people might forget though. But
> > perhaps it is a good to have things crash in that case.
> >
> > 2. Align the physical pages such that mapping them directly produces
> > the desired alignment. Increases the risk of allocations failing.
> > Probably not an issue for smallish alignment values. This may be
> > our only option if we don't find the root cause of the landisk
> > issue. Obviously this has the benefit of using the direct map in
> > more cases than we do now.
> >
> > The diff below implements #2.
>
> despite (at least one) landisk not liking the *8 change in pools,
> wouldnt there still be value in this diff?
Indeed, I think this diff is generally useful, and should go in
regardless. So ok's (and a careful review) are more than welcome.
> > Index: uvm_km.c
> > ===================================================================
> > RCS file: /home/cvs/src/sys/uvm/uvm_km.c,v
> > retrieving revision 1.123
> > diff -u -p -r1.123 uvm_km.c
> > --- uvm_km.c 17 Dec 2014 06:58:11 -0000 1.123
> > +++ uvm_km.c 30 Dec 2014 12:18:54 -0000
> > @@ -809,11 +809,9 @@ km_alloc(size_t sz, const struct kmem_va
> > struct pglist pgl;
> > int mapflags = 0;
> > vm_prot_t prot;
> > + paddr_t pla_align;
> > int pla_flags;
> > int pla_maxseg;
> > -#ifdef __HAVE_PMAP_DIRECT
> > - paddr_t pa;
> > -#endif
> > vaddr_t va, sva;
> >
> > KASSERT(sz == round_page(sz));
> > @@ -828,46 +826,33 @@ km_alloc(size_t sz, const struct kmem_va
> > if (kp->kp_zero)
> > pla_flags |= UVM_PLA_ZERO;
> >
> > + pla_align = kp->kp_align;
> > +#ifdef __HAVE_PMAP_DIRECT
> > + if (pla_align < kv->kv_align)
> > + pla_align = kv->kv_align;
> > +#endif
> > pla_maxseg = kp->kp_maxseg;
> > if (pla_maxseg == 0)
> > pla_maxseg = sz / PAGE_SIZE;
> >
> > if (uvm_pglistalloc(sz, kp->kp_constraint->ucr_low,
> > - kp->kp_constraint->ucr_high, kp->kp_align, kp->kp_boundary,
> > + kp->kp_constraint->ucr_high, pla_align, kp->kp_boundary,
> > &pgl, pla_maxseg, pla_flags)) {
> > return (NULL);
> > }
> >
> > #ifdef __HAVE_PMAP_DIRECT
> > - if (kv->kv_align)
> > - goto alloc_va;
> > -#if 1
> > /*
> > - * For now, only do DIRECT mappings for single page
> > - * allocations, until we figure out a good way to deal
> > - * with contig allocations in km_free.
> > + * Only use direct mappings for single page or single segment
> > + * allocations.
> > */
> > - if (!kv->kv_singlepage)
> > - goto alloc_va;
> > -#endif
> > - /*
> > - * Dubious optimization. If we got a contig segment, just map it
> > - * through the direct map.
> > - */
> > - TAILQ_FOREACH(pg, &pgl, pageq) {
> > - if (pg != TAILQ_FIRST(&pgl) &&
> > - VM_PAGE_TO_PHYS(pg) != pa + PAGE_SIZE)
> > - break;
> > - pa = VM_PAGE_TO_PHYS(pg);
> > - }
> > - if (pg == NULL) {
> > + if (kv->kv_singlepage || kp->kp_maxseg == 1) {
> > TAILQ_FOREACH(pg, &pgl, pageq) {
> > - vaddr_t v;
> > - v = pmap_map_direct(pg);
> > + va = pmap_map_direct(pg);
> > if (pg == TAILQ_FIRST(&pgl))
> > - va = v;
> > + sva = va;
> > }
> > - return ((void *)va);
> > + return ((void *)sva);
> > }
> > #endif
> > alloc_va:
> > @@ -947,28 +932,35 @@ km_free(void *v, size_t sz, const struct
> > struct vm_page *pg;
> > struct pglist pgl;
> >
> > - sva = va = (vaddr_t)v;
> > - eva = va + sz;
> > + sva = (vaddr_t)v;
> > + eva = sva + sz;
> >
> > - if (kp->kp_nomem) {
> > + if (kp->kp_nomem)
> > goto free_va;
> > - }
> >
> > - if (kv->kv_singlepage) {
> > #ifdef __HAVE_PMAP_DIRECT
> > - pg = pmap_unmap_direct(va);
> > - uvm_pagefree(pg);
> > + if (kv->kv_singlepage || kp->kp_maxseg == 1) {
> > + TAILQ_INIT(&pgl);
> > + for (va = sva; va < eva; va += PAGE_SIZE) {
> > + pg = pmap_unmap_direct(va);
> > + TAILQ_INSERT_TAIL(&pgl, pg, pageq);
> > + }
> > + uvm_pglistfree(&pgl);
> > + return;
> > + }
> > #else
> > + if (kv->kv_singlepage) {
> > struct uvm_km_free_page *fp = v;
> > +
> > mtx_enter(&uvm_km_pages.mtx);
> > fp->next = uvm_km_pages.freelist;
> > uvm_km_pages.freelist = fp;
> > if (uvm_km_pages.freelistlen++ > 16)
> > wakeup(&uvm_km_pages.km_proc);
> > mtx_leave(&uvm_km_pages.mtx);
> > -#endif
> > return;
> > }
> > +#endif
> >
> > if (kp->kp_pageable) {
> > pmap_remove(pmap_kernel(), sva, eva);
>
>
>
