On Fri, Aug 23, 2024 at 7:23 AM Martin Pieuchot <mpi@openbsd.org> wrote:
>
> On a 24 CPUs amd64 machine building a kernel with -j24 always result in
> a corrupted "RES" `pmap_kernel()->pm_stats.resident_count' view in top(1).
>
> Modifications to this field are (mostly) serialized by the pmap's mutex.
> Sadly pmap_enter() do not grab the mutex for the kernel's pmap.  Diff
> below changes that and fix the issue for me.  I believe it is easier and
> cheaper than using atomics everywhere.
>
> ok?

I think I'm okay with this, but I think pmap_growkernel() needs to
grab the kernel pmap lock around the pmap_alloc_level() call, as that
calls pmap_get_physpage() which updates the resident_count.  Grabbing
the (kernel) pmap lock should probably replace the "s = splhigh(); /*
to be safe */" and corresponding splx(s) in pmap_growkernel(), which
are presumably assuming kernel-lock currently.  Or maybe the lock
should be held for the entire function?  Hmm


> Index: arch/amd64/amd64/pmap.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/amd64/pmap.c,v
> diff -u -p -r1.171 pmap.c
> --- arch/amd64/amd64/pmap.c     8 Aug 2024 15:57:22 -0000       1.171
> +++ arch/amd64/amd64/pmap.c     23 Aug 2024 14:13:46 -0000
> @@ -416,10 +416,6 @@ pmap_map_ptes(struct pmap *pmap)
>
>         KASSERT(pmap->pm_type != PMAP_TYPE_EPT);
>
> -       /* the kernel's pmap is always accessible */
> -       if (pmap == pmap_kernel())
> -               return 0;
> -
>         /*
>          * Lock the target map before switching to its page tables to
>          * guarantee other CPUs have finished changing the tables before
> @@ -427,6 +423,10 @@ pmap_map_ptes(struct pmap *pmap)
>          */
>         mtx_enter(&pmap->pm_mtx);
>
> +       /* the kernel's pmap is always accessible */
> +       if (pmap == pmap_kernel())
> +               return 0;
> +
>         cr3 = rcr3();
>         KASSERT((cr3 & CR3_PCID) == PCID_KERN ||
>                 (cr3 & CR3_PCID) == PCID_PROC);
> @@ -443,8 +443,7 @@ pmap_map_ptes(struct pmap *pmap)
>  void
>  pmap_unmap_ptes(struct pmap *pmap, paddr_t save_cr3)
>  {
> -       if (pmap != pmap_kernel())
> -               mtx_leave(&pmap->pm_mtx);
> +       mtx_leave(&pmap->pm_mtx);
>
>         if (save_cr3 != 0)
>                 lcr3(save_cr3);
>