Date: Wed, 11 Aug 2010 10:36:52 +0530 From: "Jayachandran C." <c.jayachandran@gmail.com> To: Alan Cox <alc@cs.rice.edu>, mips@freebsd.org Cc: "Jayachandran C." <jchandra@freebsd.org> Subject: Re: svn commit: r210846 - in head/sys/mips: include mips Message-ID: <AANLkTi=FowMNMy87aA2K=120a4L_Fd5GPDH%2BdEPKOsek@mail.gmail.com> In-Reply-To: <4C5C3A08.500@cs.rice.edu> References: <201008041412.o74ECAix092415@svn.freebsd.org> <4C5A569B.9090401@cs.rice.edu> <AANLkTinP7eMNm4yp6T2TTteSvthdgLJOj-ihHrQJ4T49@mail.gmail.com> <AANLkTi=vkG-cntJYYEdhO4AzOO91LB6n%2B45dUSxCMTp3@mail.gmail.com> <4C5BA088.7060105@cs.rice.edu> <AANLkTinxAkRTK8pLRkQ7JwesNkuwmuiRevOZMDpj_aj7@mail.gmail.com> <4C5C3A08.500@cs.rice.edu>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --]
On Fri, Aug 6, 2010 at 10:06 PM, Alan Cox <alc@cs.rice.edu> wrote:
> The patch looks good.
>
> While we're talking about software dirty bit emulation, I would encourage
> you to look at two things:
>
> 1. trap.c contains two copies of the same code for emulation. I would
> encourage you to eliminate this duplication by creating a
> pmap_emulate_modified().
>
> 2. Software dirty bit emulation is using pmap_update_page() to invalidate
> the TLB entry on which the modified bit is being set. On a multiprocessor,
> this is going to make dirty bit emulation very costly because every
> processor will be interrupted. In principle, it should be possible and
> faster to only flush the TLB entry from the current processor. The other
> processors can handle this lazily. They either do not have that mapping in
> their TLB, in which case interrupting them was wasted effort, or they do
> have it in their TLB and when they fault on it they'll discover the dirty
> bit is already set. In fact, the emulation code already handles this case,
> on account of the fact that two processors could simultaneously write to the
> same clean page and only one will get the pmap lock first.
I've made the changes suggested, the changes are attached.
The first set of changes just re-arranges the pmap calls that use
smp_rendezvous() on SMP, so that their per-cpu variants are also
available to be called. The first patch also has an optimization from
Juli's branch, to call pmap_update_page in pmap_kenter only if the pte
is valid.
The second patch makes the changes suggested above. My testing shows
no issues so far, but please let me know if you have any comments.
Thanks,
JC.
[-- Attachment #2 --]
Index: sys/mips/mips/pmap.c
===================================================================
--- sys/mips/mips/pmap.c (revision 211068)
+++ sys/mips/mips/pmap.c (working copy)
@@ -595,58 +595,97 @@
#endif
+static __inline void
+pmap_invalidate_all_local(pmap_t pmap)
+{
+ if (pmap == kernel_pmap) {
+ tlb_invalidate_all();
+ return;
+ }
+
+ if (pmap->pm_active & PCPU_GET(cpumask))
+ tlb_invalidate_all_user(pmap);
+ else
+ pmap->pm_asid[PCPU_GET(cpuid)].gen = 0;
+}
+
+#ifdef SMP
static void
pmap_invalidate_all(pmap_t pmap)
{
-#ifdef SMP
- smp_rendezvous(0, pmap_invalidate_all_action, 0, (void *)pmap);
+ smp_rendezvous(0, pmap_invalidate_all_action, 0, pmap);
}
static void
pmap_invalidate_all_action(void *arg)
{
- pmap_t pmap = (pmap_t)arg;
+ pmap_invalidate_all_local((pmap_t)arg);
+}
+#else
+static void
+pmap_invalidate_all(pmap_t pmap)
+{
+
+ pmap_invalidate_all_local(pmap);
+}
#endif
- if (pmap == kernel_pmap) {
- tlb_invalidate_all();
+static __inline void
+pmap_invalidate_page_local(pmap_t pmap, vm_offset_t va)
+{
+
+ if (is_kernel_pmap(pmap)) {
+ tlb_invalidate_address(pmap, va);
return;
}
-
- if (pmap->pm_active & PCPU_GET(cpumask))
- tlb_invalidate_all_user(pmap);
- else
+ if (pmap->pm_asid[PCPU_GET(cpuid)].gen != PCPU_GET(asid_generation))
+ return;
+ else if (!(pmap->pm_active & PCPU_GET(cpumask))) {
pmap->pm_asid[PCPU_GET(cpuid)].gen = 0;
+ return;
+ }
+ tlb_invalidate_address(pmap, va);
}
+#ifdef SMP
struct pmap_invalidate_page_arg {
pmap_t pmap;
vm_offset_t va;
};
-static __inline void
+static void
pmap_invalidate_page(pmap_t pmap, vm_offset_t va)
{
-#ifdef SMP
struct pmap_invalidate_page_arg arg;
arg.pmap = pmap;
arg.va = va;
-
- smp_rendezvous(0, pmap_invalidate_page_action, 0, (void *)&arg);
+ smp_rendezvous(0, pmap_invalidate_page_action, 0, &arg);
}
static void
pmap_invalidate_page_action(void *arg)
{
- pmap_t pmap = ((struct pmap_invalidate_page_arg *)arg)->pmap;
- vm_offset_t va = ((struct pmap_invalidate_page_arg *)arg)->va;
+ struct pmap_invalidate_page_arg *p = arg;
+ pmap_invalidate_page_local(p->pmap, p->va);
+}
+#else
+static void
+pmap_invalidate_page(pmap_t pmap, vm_offset_t va)
+{
+
+ pmap_invalidate_page_local(pmap, va);
+}
#endif
+static __inline void
+pmap_update_page_local(pmap_t pmap, vm_offset_t va, pt_entry_t pte)
+{
+
if (is_kernel_pmap(pmap)) {
- tlb_invalidate_address(pmap, va);
+ tlb_update(pmap, va, pte);
return;
}
if (pmap->pm_asid[PCPU_GET(cpuid)].gen != PCPU_GET(asid_generation))
@@ -655,9 +694,10 @@
pmap->pm_asid[PCPU_GET(cpuid)].gen = 0;
return;
}
- tlb_invalidate_address(pmap, va);
+ tlb_update(pmap, va, pte);
}
+#ifdef SMP
struct pmap_update_page_arg {
pmap_t pmap;
vm_offset_t va;
@@ -667,37 +707,31 @@
void
pmap_update_page(pmap_t pmap, vm_offset_t va, pt_entry_t pte)
{
-#ifdef SMP
struct pmap_update_page_arg arg;
arg.pmap = pmap;
arg.va = va;
arg.pte = pte;
- smp_rendezvous(0, pmap_update_page_action, 0, (void *)&arg);
+ smp_rendezvous(0, pmap_update_page_action, 0, &arg);
}
static void
pmap_update_page_action(void *arg)
{
- pmap_t pmap = ((struct pmap_update_page_arg *)arg)->pmap;
- vm_offset_t va = ((struct pmap_update_page_arg *)arg)->va;
- pt_entry_t pte = ((struct pmap_update_page_arg *)arg)->pte;
+ struct pmap_update_page_arg *p = arg;
-#endif
- if (is_kernel_pmap(pmap)) {
- tlb_update(pmap, va, pte);
- return;
- }
- if (pmap->pm_asid[PCPU_GET(cpuid)].gen != PCPU_GET(asid_generation))
- return;
- else if (!(pmap->pm_active & PCPU_GET(cpumask))) {
- pmap->pm_asid[PCPU_GET(cpuid)].gen = 0;
- return;
- }
- tlb_update(pmap, va, pte);
+ pmap_update_page_local(p->pmap, p->va, p->pte);
}
+#else
+void
+pmap_update_page(pmap_t pmap, vm_offset_t va, pt_entry_t pte)
+{
+ pmap_update_page_local(pmap, va, pte);
+}
+#endif
+
/*
* Routine: pmap_extract
* Function:
@@ -777,7 +811,8 @@
pte = pmap_pte(kernel_pmap, va);
opte = *pte;
*pte = npte;
- pmap_update_page(kernel_pmap, va, npte);
+ if (pte_test(&opte, PTE_V) && opte != npte)
+ pmap_update_page(kernel_pmap, va, npte);
}
/*
[-- Attachment #3 --]
Index: sys/mips/include/pmap.h
===================================================================
--- sys/mips/include/pmap.h (revision 211066)
+++ sys/mips/include/pmap.h (working copy)
@@ -166,7 +166,7 @@
int pmap_compute_pages_to_dump(void);
void pmap_update_page(pmap_t pmap, vm_offset_t va, pt_entry_t pte);
void pmap_flush_pvcache(vm_page_t m);
-
+int pmap_emulate_modified(pmap_t pmap, vm_offset_t va);
#endif /* _KERNEL */
#endif /* !LOCORE */
Index: sys/mips/mips/pmap.c
===================================================================
--- sys/mips/mips/pmap.c (revision 211068)
+++ sys/mips/mips/pmap.c (working copy)
@@ -3230,16 +3265,42 @@
return (rw);
}
-/*
- * pmap_set_modified:
- *
- * Sets the page modified and reference bits for the specified page.
- */
-void
-pmap_set_modified(vm_offset_t pa)
+int
+pmap_emulate_modified(pmap_t pmap, vm_offset_t va)
{
+ vm_page_t m;
+ pt_entry_t *pte;
+ vm_offset_t pa;
- PHYS_TO_VM_PAGE(pa)->md.pv_flags |= (PV_TABLE_REF | PV_TABLE_MOD);
+ PMAP_LOCK(pmap);
+ pte = pmap_pte(pmap, va);
+ if (pte == NULL)
+ panic("pmap_emulate_modified: can't find PTE");
+#ifdef SMP
+ /* It is possible that some other CPU changed m-bit */
+ if (!pte_test(pte, PTE_V) || pte_test(pte, PTE_D)) {
+ pmap_update_page_local(pmap, va, *pte);
+ PMAP_UNLOCK(kernel_pmap);
+ return (0);
+ }
+#else
+ if (!pte_test(pte, PTE_V) || pte_test(pte, PTE_D))
+ panic("pmap_emulate_modified: invalid pte");
+#endif
+ if (pte_test(pte, PTE_RO)) {
+ /* write to read only page in the kernel */
+ PMAP_UNLOCK(pmap);
+ return (1);
+ }
+ pte_set(pte, PTE_D);
+ pmap_update_page_local(pmap, va, *pte);
+ pa = TLBLO_PTE_TO_PA(*pte);
+ if (!page_is_managed(pa))
+ panic("pmap_emulate_modified: unmanaged page");
+ m = PHYS_TO_VM_PAGE(pa);
+ m->md.pv_flags |= (PV_TABLE_REF | PV_TABLE_MOD);
+ PMAP_UNLOCK(pmap);
+ return (0);
}
/*
Index: sys/mips/mips/trap.c
===================================================================
--- sys/mips/mips/trap.c (revision 211066)
+++ sys/mips/mips/trap.c (working copy)
@@ -281,7 +281,6 @@
struct thread *td = curthread;
struct proc *p = curproc;
vm_prot_t ftype;
- pt_entry_t *pte;
pmap_t pmap;
int access_type;
ksiginfo_t ksi;
@@ -372,82 +371,24 @@
case T_TLB_MOD:
/* check for kernel address */
if (KERNLAND(trapframe->badvaddr)) {
- vm_offset_t pa;
-
- PMAP_LOCK(kernel_pmap);
- pte = pmap_pte(kernel_pmap, trapframe->badvaddr);
- if (pte == NULL)
- panic("trap: ktlbmod: can't find PTE");
-#ifdef SMP
- /* It is possible that some other CPU changed m-bit */
- if (!pte_test(pte, PTE_V) || pte_test(pte, PTE_D)) {
- pmap_update_page(kernel_pmap,
- trapframe->badvaddr, *pte);
- PMAP_UNLOCK(kernel_pmap);
- return (trapframe->pc);
- }
-#else
- if (!pte_test(pte, PTE_V) || pte_test(pte, PTE_D))
- panic("trap: ktlbmod: invalid pte");
-#endif
- if (pte_test(pte, PTE_RO)) {
- /* write to read only page in the kernel */
+ if (pmap_emulate_modified(kernel_pmap,
+ trapframe->badvaddr) != 0) {
ftype = VM_PROT_WRITE;
- PMAP_UNLOCK(kernel_pmap);
goto kernel_fault;
}
- pte_set(pte, PTE_D);
- pmap_update_page(kernel_pmap, trapframe->badvaddr, *pte);
- pa = TLBLO_PTE_TO_PA(*pte);
- if (!page_is_managed(pa))
- panic("trap: ktlbmod: unmanaged page");
- pmap_set_modified(pa);
- PMAP_UNLOCK(kernel_pmap);
return (trapframe->pc);
}
/* FALLTHROUGH */
case T_TLB_MOD + T_USER:
- {
- vm_offset_t pa;
-
- pmap = &p->p_vmspace->vm_pmap;
-
- PMAP_LOCK(pmap);
- pte = pmap_pte(pmap, trapframe->badvaddr);
- if (pte == NULL)
- panic("trap: utlbmod: can't find PTE");
-#ifdef SMP
- /* It is possible that some other CPU changed m-bit */
- if (!pte_test(pte, PTE_V) || pte_test(pte, PTE_D)) {
- pmap_update_page(pmap, trapframe->badvaddr, *pte);
- PMAP_UNLOCK(pmap);
- goto out;
- }
-#else
- if (!pte_test(pte, PTE_V) || pte_test(pte, PTE_D))
- panic("trap: utlbmod: invalid pte");
-#endif
-
- if (pte_test(pte, PTE_RO)) {
- /* write to read only page */
- ftype = VM_PROT_WRITE;
- PMAP_UNLOCK(pmap);
- goto dofault;
- }
- pte_set(pte, PTE_D);
- pmap_update_page(pmap, trapframe->badvaddr, *pte);
- pa = TLBLO_PTE_TO_PA(*pte);
- if (!page_is_managed(pa))
- panic("trap: utlbmod: unmanaged page");
- pmap_set_modified(pa);
-
- PMAP_UNLOCK(pmap);
- if (!usermode) {
- return (trapframe->pc);
- }
- goto out;
+ pmap = &p->p_vmspace->vm_pmap;
+ if (pmap_emulate_modified(pmap, trapframe->badvaddr) != 0) {
+ ftype = VM_PROT_WRITE;
+ goto dofault;
}
+ if (!usermode)
+ return (trapframe->pc);
+ goto out;
case T_TLB_LD_MISS:
case T_TLB_ST_MISS:
home |
help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTi=FowMNMy87aA2K=120a4L_Fd5GPDH%2BdEPKOsek>