svn commit: r267364 - head/sys/vm

[Adrian Chadd]

Hi!

Wouldn't this also be fixed if popmap_t was a unsigned char (to match
what setbit/clrbit/etc do) and then use the existing macros?

Is there any reason popmap_t is not a uchar ? Is it ever accessed as
anything other than an array of chars?



-a


On 11 June 2014 11:11, Alan Cox <alc at freebsd.org> wrote:
> Author: alc
> Date: Wed Jun 11 16:11:12 2014
> New Revision: 267364
> URL: http://svnweb.freebsd.org/changeset/base/267364
>
> Log:
>   Correct a bug in the management of the population map on big-endian
>   machines.  Specifically, there was a mismatch between how the routine
>   allocation and deallocation operations accessed the population map
>   and how the aggressively optimized reservation-breaking operation
>   accessed it.  So, problems only occurred when reservations were broken.
>   This change makes the routine operations access the population map in
>   the same way as the reservation breaking operation.
>
>   This bug was introduced in r259999.
>
>   PR:           187080
>   Tested by:    jmg (on an "armeb" machine)
>   Sponsored by: EMC / Isilon Storage Division
>
> Modified:
>   head/sys/vm/vm_reserv.c
>
> Modified: head/sys/vm/vm_reserv.c
> ==============================================================================
> --- head/sys/vm/vm_reserv.c     Wed Jun 11 14:53:58 2014        (r267363)
> +++ head/sys/vm/vm_reserv.c     Wed Jun 11 16:11:12 2014        (r267364)
> @@ -108,6 +108,46 @@ typedef    u_long          popmap_t;
>  #define        NPOPMAP         howmany(VM_LEVEL_0_NPAGES, NBPOPMAP)
>
>  /*
> + * Clear a bit in the population map.
> + */
> +static __inline void
> +popmap_clear(popmap_t popmap[], int i)
> +{
> +
> +       popmap[i / NBPOPMAP] &= ~(1UL << (i % NBPOPMAP));
> +}
> +
> +/*
> + * Set a bit in the population map.
> + */
> +static __inline void
> +popmap_set(popmap_t popmap[], int i)
> +{
> +
> +       popmap[i / NBPOPMAP] |= 1UL << (i % NBPOPMAP);
> +}
> +
> +/*
> + * Is a bit in the population map clear?
> + */
> +static __inline boolean_t
> +popmap_is_clear(popmap_t popmap[], int i)
> +{
> +
> +       return ((popmap[i / NBPOPMAP] & (1UL << (i % NBPOPMAP))) == 0);
> +}
> +
> +/*
> + * Is a bit in the population map set?
> + */
> +static __inline boolean_t
> +popmap_is_set(popmap_t popmap[], int i)
> +{
> +
> +       return ((popmap[i / NBPOPMAP] & (1UL << (i % NBPOPMAP))) != 0);
> +}
> +
> +/*
>   * The reservation structure
>   *
>   * A reservation structure is constructed whenever a large physical page is
> @@ -241,7 +281,7 @@ vm_reserv_depopulate(vm_reserv_t rv, int
>         mtx_assert(&vm_page_queue_free_mtx, MA_OWNED);
>         KASSERT(rv->object != NULL,
>             ("vm_reserv_depopulate: reserv %p is free", rv));
> -       KASSERT(isset(rv->popmap, index),
> +       KASSERT(popmap_is_set(rv->popmap, index),
>             ("vm_reserv_depopulate: reserv %p's popmap[%d] is clear", rv,
>             index));
>         KASSERT(rv->popcnt > 0,
> @@ -255,7 +295,7 @@ vm_reserv_depopulate(vm_reserv_t rv, int
>                     rv));
>                 rv->pages->psind = 0;
>         }
> -       clrbit(rv->popmap, index);
> +       popmap_clear(rv->popmap, index);
>         rv->popcnt--;
>         if (rv->popcnt == 0) {
>                 LIST_REMOVE(rv, objq);
> @@ -302,7 +342,7 @@ vm_reserv_populate(vm_reserv_t rv, int i
>         mtx_assert(&vm_page_queue_free_mtx, MA_OWNED);
>         KASSERT(rv->object != NULL,
>             ("vm_reserv_populate: reserv %p is free", rv));
> -       KASSERT(isclr(rv->popmap, index),
> +       KASSERT(popmap_is_clear(rv->popmap, index),
>             ("vm_reserv_populate: reserv %p's popmap[%d] is set", rv,
>             index));
>         KASSERT(rv->popcnt < VM_LEVEL_0_NPAGES,
> @@ -313,7 +353,7 @@ vm_reserv_populate(vm_reserv_t rv, int i
>                 TAILQ_REMOVE(&vm_rvq_partpop, rv, partpopq);
>                 rv->inpartpopq = FALSE;
>         }
> -       setbit(rv->popmap, index);
> +       popmap_set(rv->popmap, index);
>         rv->popcnt++;
>         if (rv->popcnt < VM_LEVEL_0_NPAGES) {
>                 rv->inpartpopq = TRUE;
> @@ -503,7 +543,7 @@ found:
>                 return (NULL);
>         /* Handle vm_page_rename(m, new_object, ...). */
>         for (i = 0; i < npages; i++)
> -               if (isset(rv->popmap, index + i))
> +               if (popmap_is_set(rv->popmap, index + i))
>                         return (NULL);
>         for (i = 0; i < npages; i++)
>                 vm_reserv_populate(rv, index + i);
> @@ -628,7 +668,7 @@ found:
>         index = VM_RESERV_INDEX(object, pindex);
>         m = &rv->pages[index];
>         /* Handle vm_page_rename(m, new_object, ...). */
> -       if (isset(rv->popmap, index))
> +       if (popmap_is_set(rv->popmap, index))
>                 return (NULL);
>         vm_reserv_populate(rv, index);
>         return (m);
> @@ -662,9 +702,9 @@ vm_reserv_break(vm_reserv_t rv, vm_page_
>                  * to the physical memory allocator.
>                  */
>                 i = m - rv->pages;
> -               KASSERT(isclr(rv->popmap, i),
> +               KASSERT(popmap_is_clear(rv->popmap, i),
>                     ("vm_reserv_break: reserv %p's popmap is corrupted", rv));
> -               setbit(rv->popmap, i);
> +               popmap_set(rv->popmap, i);
>                 rv->popcnt++;
>         }
>         i = hi = 0;
>