From owner-freebsd-stable@FreeBSD.ORG Mon May 5 16:27:57 2014 Return-Path: Delivered-To: stable@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 9BDEBAA3; Mon, 5 May 2014 16:27:57 +0000 (UTC) Received: from gw.catspoiler.org (gw.catspoiler.org [75.1.14.242]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 6067A147; Mon, 5 May 2014 16:27:56 +0000 (UTC) Received: from FreeBSD.org (mousie.catspoiler.org [192.168.101.2]) by gw.catspoiler.org (8.13.3/8.13.3) with ESMTP id s45GRhbK052786; Mon, 5 May 2014 09:27:47 -0700 (PDT) (envelope-from truckman@FreeBSD.org) Message-Id: <201405051627.s45GRhbK052786@gw.catspoiler.org> Date: Mon, 5 May 2014 09:27:43 -0700 (PDT) From: Don Lewis Subject: Re: Thinkpad R60 hangs when booting recent 8.4-STABLE To: jhb@FreeBSD.org In-Reply-To: <201405021405.32570.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/plain; charset=us-ascii Cc: stable@FreeBSD.org X-BeenThere: freebsd-stable@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Production branch of FreeBSD source code List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 05 May 2014 16:27:57 -0000 On 2 May, John Baldwin wrote: > On Friday, May 02, 2014 5:39:19 am Don Lewis wrote: >> This expression will overflow: >> if (s->r_start + count - 1 > end) { >> I don't understand the point of this test. Is it an optimization to >> exit the loop early based on an assumption about the ordering of >> elements in the list? > > Yes. The elements in an rman list are supposed to be sorted. I missed this comment earlier. >> This expression will also overflow: >> rstart = (rstart + amask) & ~amask; >> which is where I think the start address is getting set to 0. >> >> >> After reverting subr_rman.c and applying the following patch, I see: >> considering [0xfec00000, 0xffffffff] >> no unshared regions found >> Then nexus_alloc_resource() gets called and I see >> considering [0x3ff60000, 0xfebfffff] >> and then all the right stuff happens. >> >> >> Index: sys/kern/subr_rman.c >> =================================================================== >> --- sys/kern/subr_rman.c (revision 262226) >> +++ sys/kern/subr_rman.c (working copy) >> @@ -468,11 +468,9 @@ >> */ >> for (s = r; s; s = TAILQ_NEXT(s, r_link)) { >> DPRINTF(("considering [%#lx, %#lx]\n", s->r_start, s->r_end)); >> - if (s->r_start + count - 1 > end) { >> - DPRINTF(("s->r_start (%#lx) + count - 1> end (%#lx)\n", >> - s->r_start, end)); >> - break; >> - } >> + if (s->r_end < start + count - 1 || >> + s->r_start > end - count + 1) >> + continue; >> if (s->r_flags & RF_ALLOCATED) { >> DPRINTF(("region is allocated\n")); >> continue; >> >> > > I think this looks good. I just committed a variation of this fix which preserves the early exit optimization to HEAD in r265363. It also tweaks the intial start of search test so that the s->r_start > end - count + 1 test above is not needed. I'm also considering some changes to the second for loop, but I've got some questions about the (s->r_flags & flags) != flags test: Are there any cases where we get a false match because a bit is set in s->r_flags that is not set in flags? Should there be a KASSERT to check that the bits passed in flags are valid? Should the RF_ALIGNMENT_MASK bits participate in this comparision? If so, I think that the comparison should either be strict equality or should be a magnitude comparison and not a bitwise comparison. If the RF_ALIGNMENT_MASK bits should be compared, then the fact that they are not preserved could be a problem. Just a few lines below: rv->r_flags = s->r_flags & (RF_ALLOCATED | RF_SHAREABLE | RF_TIMESHARE); If the RF_ALIGNMENT_MASK bits are equal, then the (s->r_start & amask) == 0 test would be redundant.