> 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.
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);
