Date: Wed, 2 Aug 2023 11:15:37 +0300 From: Dmitry Chagin <dchagin@freebsd.org> To: Doug Moore <dougm@freebsd.org> Cc: dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org, src-committers@freebsd.org Subject: Re: git: 1a7fcf6d51eb - main - vm_phys_enqueue_contig: handle npages==0 Message-ID: <CAC0jpUCEyN0STBvuqv2kcvV8NY04RXGdszEf2MS6pJKC3bf8Kw@mail.gmail.com> In-Reply-To: <202308020314.3723ETgj027830@gitrepo.freebsd.org> References: <202308020314.3723ETgj027830@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] Hi, With this change, I got a panic: vm_phys_enq_range: page 0xfffffe0000000000 and pages 0 are misaligned Ср, 2 авг. 2023 г. в 06:22, Doug Moore <dougm@freebsd.org>: > The branch main has been updated by dougm: > > URL: > https://cgit.FreeBSD.org/src/commit/?id=1a7fcf6d51eb67ee3e05fdbb806f7e68f9f53c9c > > commit 1a7fcf6d51eb67ee3e05fdbb806f7e68f9f53c9c > Author: Doug Moore <dougm@FreeBSD.org> > AuthorDate: 2023-08-02 03:12:00 +0000 > Commit: Doug Moore <dougm@FreeBSD.org> > CommitDate: 2023-08-02 03:12:00 +0000 > > vm_phys_enqueue_contig: handle npages==0 > > By letting vm_phys_enqueue_contig handle the case when npages == 0, > the callers can stop checking it, and the compiler can stop > zero-checking with every call to ffs(). Letting vm_phys_enqueue_contig > call vm_phys_enqueue_contig for part of its work also saves a few > bytes. > > The amd64 object code shrinks by 80 bytes. > > Reviewed by: kib > Differential Revision: https://reviews.freebsd.org/D41154 > --- > sys/vm/vm_phys.c | 74 > +++++++++++++++++++++++++++----------------------------- > 1 file changed, 35 insertions(+), 39 deletions(-) > > diff --git a/sys/vm/vm_phys.c b/sys/vm/vm_phys.c > index 28f12231e01c..b2084bdef4e1 100644 > --- a/sys/vm/vm_phys.c > +++ b/sys/vm/vm_phys.c > @@ -72,6 +72,8 @@ __FBSDID("$FreeBSD$"); > > _Static_assert(sizeof(long) * NBBY >= VM_PHYSSEG_MAX, > "Too many physsegs."); > +_Static_assert(sizeof(long long) >= sizeof(vm_paddr_t), > + "vm_paddr_t too big for ffsll, flsll."); > > #ifdef NUMA > struct mem_affinity __read_mostly *mem_affinity; > @@ -690,18 +692,16 @@ vm_phys_split_pages(vm_page_t m, int oind, struct > vm_freelist *fl, int order, > * > * The physical page m's buddy must not be free. > */ > -static void > +static vm_page_t > vm_phys_enq_range(vm_page_t m, u_int npages, struct vm_freelist *fl, int > tail) > { > - u_int n; > int order; > > - KASSERT(npages > 0, ("vm_phys_enq_range: npages is 0")); > KASSERT(((VM_PAGE_TO_PHYS(m) + npages * PAGE_SIZE) & > ((PAGE_SIZE << (fls(npages) - 1)) - 1)) == 0, > ("vm_phys_enq_range: page %p and npages %u are misaligned", > m, npages)); > - do { > + while (npages > 0) { > KASSERT(m->order == VM_NFREEORDER, > ("vm_phys_enq_range: page %p has unexpected order %d", > m, m->order)); > @@ -709,10 +709,10 @@ vm_phys_enq_range(vm_page_t m, u_int npages, struct > vm_freelist *fl, int tail) > KASSERT(order < VM_NFREEORDER, > ("vm_phys_enq_range: order %d is out of range", > order)); > vm_freelist_add(fl, m, order, tail); > - n = 1 << order; > - m += n; > - npages -= n; > - } while (npages > 0); > + m += 1 << order; > + npages -= 1 << order; > + } > + return (m); > } > > /* > @@ -744,7 +744,7 @@ vm_phys_alloc_npages(int domain, int pool, int npages, > vm_page_t ma[]) > { > struct vm_freelist *alt, *fl; > vm_page_t m; > - int avail, end, flind, freelist, i, need, oind, pind; > + int avail, end, flind, freelist, i, oind, pind; > > KASSERT(domain >= 0 && domain < vm_ndomains, > ("vm_phys_alloc_npages: domain %d is out of range", domain)); > @@ -762,20 +762,18 @@ vm_phys_alloc_npages(int domain, int pool, int > npages, vm_page_t ma[]) > for (oind = 0; oind < VM_NFREEORDER; oind++) { > while ((m = TAILQ_FIRST(&fl[oind].pl)) != NULL) { > vm_freelist_rem(fl, m, oind); > - avail = 1 << oind; > - need = imin(npages - i, avail); > - for (end = i + need; i < end;) > + avail = i + (1 << oind); > + end = imin(npages, avail); > + while (i < end) > ma[i++] = m++; > - if (need < avail) { > + if (i == npages) { > /* > - * Return excess pages to fl. Its > - * order [0, oind) queues are > empty. > + * Return excess pages to fl. Its > order > + * [0, oind) queues are empty. > */ > - vm_phys_enq_range(m, avail - need, > fl, > - 1); > - return (npages); > - } else if (i == npages) > + vm_phys_enq_range(m, avail - i, > fl, 1); > return (npages); > + } > } > } > for (oind = VM_NFREEORDER - 1; oind >= 0; oind--) { > @@ -785,21 +783,20 @@ vm_phys_alloc_npages(int domain, int pool, int > npages, vm_page_t ma[]) > NULL) { > vm_freelist_rem(alt, m, oind); > vm_phys_set_pool(pool, m, oind); > - avail = 1 << oind; > - need = imin(npages - i, avail); > - for (end = i + need; i < end;) > + avail = i + (1 << oind); > + end = imin(npages, avail); > + while (i < end) > ma[i++] = m++; > - if (need < avail) { > + if (i == npages) { > /* > * Return excess pages to > fl. > * Its order [0, oind) > queues > * are empty. > */ > - vm_phys_enq_range(m, avail > - > - need, fl, 1); > - return (npages); > - } else if (i == npages) > + vm_phys_enq_range(m, avail > - i, > + fl, 1); > return (npages); > + } > } > } > } > @@ -1146,7 +1143,7 @@ max_order(vm_page_t m) > * because the size of a physical address exceeds the size of > * a long. > */ > - return (min(ffsl(VM_PAGE_TO_PHYS(m) >> PAGE_SHIFT) - 1, > + return (min(ffsll(VM_PAGE_TO_PHYS(m) >> PAGE_SHIFT) - 1, > VM_NFREEORDER - 1)); > } > > @@ -1162,6 +1159,7 @@ vm_phys_enqueue_contig(vm_page_t m, u_long npages) > struct vm_freelist *fl; > struct vm_phys_seg *seg; > vm_page_t m_end; > + vm_paddr_t diff, lo; > int order; > > /* > @@ -1173,15 +1171,15 @@ vm_phys_enqueue_contig(vm_page_t m, u_long npages) > fl = (*seg->free_queues)[m->pool]; > m_end = m + npages; > /* Free blocks of increasing size. */ > - while ((order = max_order(m)) < VM_NFREEORDER - 1 && > - m + (1 << order) <= m_end) { > - KASSERT(seg == &vm_phys_segs[m->segind], > - ("%s: page range [%p,%p) spans multiple segments", > - __func__, m_end - npages, m)); > - vm_freelist_add(fl, m, order, 1); > - m += 1 << order; > + lo = VM_PAGE_TO_PHYS(m) >> PAGE_SHIFT; > + if (m < m_end && > + (diff = lo ^ (lo + npages - 1)) != 0) { > + order = min(flsll(diff) - 1, VM_NFREEORDER - 1); > + m = vm_phys_enq_range(m, roundup2(-lo, 1 << order), fl, 1); > } > + > /* Free blocks of maximum size. */ > + order = VM_NFREEORDER - 1; > while (m + (1 << order) <= m_end) { > KASSERT(seg == &vm_phys_segs[m->segind], > ("%s: page range [%p,%p) spans multiple segments", > @@ -1560,10 +1558,8 @@ vm_phys_alloc_contig(int domain, u_long npages, > vm_paddr_t low, vm_paddr_t high, > vm_phys_set_pool(VM_FREEPOOL_DEFAULT, m, oind); > } > /* Return excess pages to the free lists. */ > - if (&m_run[npages] < m) { > - fl = (*queues)[VM_FREEPOOL_DEFAULT]; > - vm_phys_enq_range(&m_run[npages], m - &m_run[npages], fl, > 0); > - } > + fl = (*queues)[VM_FREEPOOL_DEFAULT]; > + vm_phys_enq_range(&m_run[npages], m - &m_run[npages], fl, 0); > return (m_run); > } > > [-- Attachment #2 --] <div><br></div><div dir="auto">Hi,</div><div dir="auto"><br></div><div dir="auto"><div dir="auto">With this change, I got a panic:</div><div dir="auto">vm_phys_enq_range: page 0xfffffe0000000000 and pages 0 are misaligned</div></div><div dir="auto"><br><div class="gmail_quote" dir="auto"><div dir="ltr" class="gmail_attr">Ср, 2 авг. 2023 г. в 06:22, Doug Moore <<a href="mailto:dougm@freebsd.org">dougm@freebsd.org</a>>:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The branch main has been updated by dougm:<br> <br> URL: <a href="https://cgit.FreeBSD.org/src/commit/?id=1a7fcf6d51eb67ee3e05fdbb806f7e68f9f53c9c" rel="noreferrer" target="_blank">https://cgit.FreeBSD.org/src/commit/?id=1a7fcf6d51eb67ee3e05fdbb806f7e68f9f53c9c</a><br>; <br> commit 1a7fcf6d51eb67ee3e05fdbb806f7e68f9f53c9c<br> Author: Doug Moore <dougm@FreeBSD.org><br> AuthorDate: 2023-08-02 03:12:00 +0000<br> Commit: Doug Moore <dougm@FreeBSD.org><br> CommitDate: 2023-08-02 03:12:00 +0000<br> <br> vm_phys_enqueue_contig: handle npages==0<br> <br> By letting vm_phys_enqueue_contig handle the case when npages == 0,<br> the callers can stop checking it, and the compiler can stop<br> zero-checking with every call to ffs(). Letting vm_phys_enqueue_contig<br> call vm_phys_enqueue_contig for part of its work also saves a few<br> bytes.<br> <br> The amd64 object code shrinks by 80 bytes.<br> <br> Reviewed by: kib<br> Differential Revision: <a href="https://reviews.freebsd.org/D41154" rel="noreferrer" target="_blank">https://reviews.freebsd.org/D41154</a><br>; ---<br> sys/vm/vm_phys.c | 74 +++++++++++++++++++++++++++-----------------------------<br> 1 file changed, 35 insertions(+), 39 deletions(-)<br> <br> diff --git a/sys/vm/vm_phys.c b/sys/vm/vm_phys.c<br> index 28f12231e01c..b2084bdef4e1 100644<br> --- a/sys/vm/vm_phys.c<br> +++ b/sys/vm/vm_phys.c<br> @@ -72,6 +72,8 @@ __FBSDID("$FreeBSD$");<br> <br> _Static_assert(sizeof(long) * NBBY >= VM_PHYSSEG_MAX,<br> "Too many physsegs.");<br> +_Static_assert(sizeof(long long) >= sizeof(vm_paddr_t),<br> + "vm_paddr_t too big for ffsll, flsll.");<br> <br> #ifdef NUMA<br> struct mem_affinity __read_mostly *mem_affinity;<br> @@ -690,18 +692,16 @@ vm_phys_split_pages(vm_page_t m, int oind, struct vm_freelist *fl, int order,<br> *<br> * The physical page m's buddy must not be free.<br> */<br> -static void<br> +static vm_page_t<br> vm_phys_enq_range(vm_page_t m, u_int npages, struct vm_freelist *fl, int tail)<br> {<br> - u_int n;<br> int order;<br> <br> - KASSERT(npages > 0, ("vm_phys_enq_range: npages is 0"));<br> KASSERT(((VM_PAGE_TO_PHYS(m) + npages * PAGE_SIZE) &<br> ((PAGE_SIZE << (fls(npages) - 1)) - 1)) == 0,<br> ("vm_phys_enq_range: page %p and npages %u are misaligned",<br> m, npages));<br> - do {<br> + while (npages > 0) {<br> KASSERT(m->order == VM_NFREEORDER,<br> ("vm_phys_enq_range: page %p has unexpected order %d",<br> m, m->order));<br> @@ -709,10 +709,10 @@ vm_phys_enq_range(vm_page_t m, u_int npages, struct vm_freelist *fl, int tail)<br> KASSERT(order < VM_NFREEORDER,<br> ("vm_phys_enq_range: order %d is out of range", order));<br> vm_freelist_add(fl, m, order, tail);<br> - n = 1 << order;<br> - m += n;<br> - npages -= n;<br> - } while (npages > 0);<br> + m += 1 << order;<br> + npages -= 1 << order;<br> + }<br> + return (m);<br> }<br> <br> /*<br> @@ -744,7 +744,7 @@ vm_phys_alloc_npages(int domain, int pool, int npages, vm_page_t ma[])<br> {<br> struct vm_freelist *alt, *fl;<br> vm_page_t m;<br> - int avail, end, flind, freelist, i, need, oind, pind;<br> + int avail, end, flind, freelist, i, oind, pind;<br> <br> KASSERT(domain >= 0 && domain < vm_ndomains,<br> ("vm_phys_alloc_npages: domain %d is out of range", domain));<br> @@ -762,20 +762,18 @@ vm_phys_alloc_npages(int domain, int pool, int npages, vm_page_t ma[])<br> for (oind = 0; oind < VM_NFREEORDER; oind++) {<br> while ((m = TAILQ_FIRST(&fl[oind].pl)) != NULL) {<br> vm_freelist_rem(fl, m, oind);<br> - avail = 1 << oind;<br> - need = imin(npages - i, avail);<br> - for (end = i + need; i < end;)<br> + avail = i + (1 << oind);<br> + end = imin(npages, avail);<br> + while (i < end)<br> ma[i++] = m++;<br> - if (need < avail) {<br> + if (i == npages) {<br> /*<br> - * Return excess pages to fl. Its<br> - * order [0, oind) queues are empty.<br> + * Return excess pages to fl. Its order<br> + * [0, oind) queues are empty.<br> */<br> - vm_phys_enq_range(m, avail - need, fl,<br> - 1);<br> - return (npages);<br> - } else if (i == npages)<br> + vm_phys_enq_range(m, avail - i, fl, 1);<br> return (npages);<br> + }<br> }<br> }<br> for (oind = VM_NFREEORDER - 1; oind >= 0; oind--) {<br> @@ -785,21 +783,20 @@ vm_phys_alloc_npages(int domain, int pool, int npages, vm_page_t ma[])<br> NULL) {<br> vm_freelist_rem(alt, m, oind);<br> vm_phys_set_pool(pool, m, oind);<br> - avail = 1 << oind;<br> - need = imin(npages - i, avail);<br> - for (end = i + need; i < end;)<br> + avail = i + (1 << oind);<br> + end = imin(npages, avail);<br> + while (i < end)<br> ma[i++] = m++;<br> - if (need < avail) {<br> + if (i == npages) {<br> /*<br> * Return excess pages to fl.<br> * Its order [0, oind) queues<br> * are empty.<br> */<br> - vm_phys_enq_range(m, avail -<br> - need, fl, 1);<br> - return (npages);<br> - } else if (i == npages)<br> + vm_phys_enq_range(m, avail - i,<br> + fl, 1);<br> return (npages);<br> + }<br> }<br> }<br> }<br> @@ -1146,7 +1143,7 @@ max_order(vm_page_t m)<br> * because the size of a physical address exceeds the size of<br> * a long.<br> */<br> - return (min(ffsl(VM_PAGE_TO_PHYS(m) >> PAGE_SHIFT) - 1,<br> + return (min(ffsll(VM_PAGE_TO_PHYS(m) >> PAGE_SHIFT) - 1,<br> VM_NFREEORDER - 1));<br> }<br> <br> @@ -1162,6 +1159,7 @@ vm_phys_enqueue_contig(vm_page_t m, u_long npages)<br> struct vm_freelist *fl;<br> struct vm_phys_seg *seg;<br> vm_page_t m_end;<br> + vm_paddr_t diff, lo;<br> int order;<br> <br> /*<br> @@ -1173,15 +1171,15 @@ vm_phys_enqueue_contig(vm_page_t m, u_long npages)<br> fl = (*seg->free_queues)[m->pool];<br> m_end = m + npages;<br> /* Free blocks of increasing size. */<br> - while ((order = max_order(m)) < VM_NFREEORDER - 1 &&<br> - m + (1 << order) <= m_end) {<br> - KASSERT(seg == &vm_phys_segs[m->segind],<br> - ("%s: page range [%p,%p) spans multiple segments",<br> - __func__, m_end - npages, m));<br> - vm_freelist_add(fl, m, order, 1);<br> - m += 1 << order;<br> + lo = VM_PAGE_TO_PHYS(m) >> PAGE_SHIFT;<br> + if (m < m_end &&<br> + (diff = lo ^ (lo + npages - 1)) != 0) {<br> + order = min(flsll(diff) - 1, VM_NFREEORDER - 1);<br> + m = vm_phys_enq_range(m, roundup2(-lo, 1 << order), fl, 1);<br> }<br> +<br> /* Free blocks of maximum size. */<br> + order = VM_NFREEORDER - 1;<br> while (m + (1 << order) <= m_end) {<br> KASSERT(seg == &vm_phys_segs[m->segind],<br> ("%s: page range [%p,%p) spans multiple segments",<br> @@ -1560,10 +1558,8 @@ vm_phys_alloc_contig(int domain, u_long npages, vm_paddr_t low, vm_paddr_t high,<br> vm_phys_set_pool(VM_FREEPOOL_DEFAULT, m, oind);<br> }<br> /* Return excess pages to the free lists. */<br> - if (&m_run[npages] < m) {<br> - fl = (*queues)[VM_FREEPOOL_DEFAULT];<br> - vm_phys_enq_range(&m_run[npages], m - &m_run[npages], fl, 0);<br> - }<br> + fl = (*queues)[VM_FREEPOOL_DEFAULT];<br> + vm_phys_enq_range(&m_run[npages], m - &m_run[npages], fl, 0);<br> return (m_run);<br> }<br> <br> </blockquote></div></div>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAC0jpUCEyN0STBvuqv2kcvV8NY04RXGdszEf2MS6pJKC3bf8Kw>