Date: Mon, 5 May 2014 09:27:43 -0700 (PDT) From: Don Lewis <truckman@FreeBSD.org> To: jhb@FreeBSD.org Cc: stable@FreeBSD.org Subject: Re: Thinkpad R60 hangs when booting recent 8.4-STABLE Message-ID: <201405051627.s45GRhbK052786@gw.catspoiler.org> In-Reply-To: <201405021405.32570.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201405051627.s45GRhbK052786>