Date: Thu, 12 Jun 2014 18:46:18 -0500 From: Adrian Chadd <adrian@freebsd.org> To: Alan Cox <alc@freebsd.org> Cc: "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org> Subject: Re: svn commit: r267364 - head/sys/vm Message-ID: <CAJ-Vmona1MpXeiBNiLkUjRt%2BTpQ-sd2yWKOfsb8wELGrq_-dBA@mail.gmail.com> In-Reply-To: <201406111611.s5BGBCHU058618@svn.freebsd.org> References: <201406111611.s5BGBCHU058618@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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; >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-Vmona1MpXeiBNiLkUjRt%2BTpQ-sd2yWKOfsb8wELGrq_-dBA>