[haiku-commits] Re: haiku: hrev44773 - src/system/kernel/vm
- From: Jérôme Duval <korli@xxxxxxxxxxxxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Tue, 6 Nov 2012 13:34:51 +0100
2012/11/6 Ingo Weinhold <ingo_weinhold@xxxxxx>
> korli@xxxxxxxxxxxxxxxx wrote:
> > // enforce alignment
> > - if ((offsetStart & alignmentMask) != 0) {
> > + if (alignmentMask != 0 && (offsetStart & alignmentMask) != 0) {
>
> The additional check is superfluous, since the & would yield 0 in this
> case anyway.
>
Agreed, will remove.
<http://cgit.haiku-os.org/haiku/tree/src/system/kernel/vm/vm_page.cpp#n3867>
>
> > offsetStart = ((offsetStart + alignmentMask) & ~alignmentMask)
> > - sPhysicalPageOffset;
> > }
>
I just noticed sPhysicalPageOffset shouldn't be substracted here. You
confirm?
> >
> > // enforce boundary
> > - if (offsetStart << boundaryShift
> > + if (boundaryShift != 0 && offsetStart << boundaryShift
> > != (offsetStart + length - 1) << boundaryShift) {
> > offsetStart = (offsetStart + length - 1) << boundaryShift
> > >> boundaryShift;
>
> Here the check is necessary indeed. On the other hand all the shifts are
> the wrong way around. I wonder why I didn't use a mask for the boundary,
> anyway. The checks would be similiar, but a mask doesn't need a loop to be
> computed.
>
Right. You think to something like ((offsetStart ^ (offsetStart + length -
1)) & ~(boundary - 1)) == 0 ?
>
> > @@ -3887,7 +3887,10 @@ vm_page_allocate_page_run(uint32 flags,
> page_num_t length,
> > }
> >
> > dprintf("vm_page_allocate_page_run(): Failed to allocate run of "
> > - "length %" B_PRIuPHYSADDR " in second iteration!", length);
> > + "length %" B_PRIuPHYSADDR " (%" B_PRIuPHYSADDR " %"
> > + B_PRIuPHYSADDR ") in second iteration (align: %" B_PRIuPHYSADDR
> > + " boundary: %" B_PRIuPHYSADDR ") !", length, requestedStart,
> > + end, restrictions->alignment, restrictions->boundary);
> >
> > freeClearQueueLocker.Unlock();
> > vm_page_unreserve_pages(&reservation);
> > @@ -3916,7 +3919,7 @@ vm_page_allocate_page_run(uint32 flags, page_num_t
> length,
> > freeClearQueueLocker.Lock();
> > }
> >
> > - start += i + 1;
> > + start += max_c(i, alignmentMask) + 1;
>
> Superfluous. At the beginning of the next iteration start is aligned
> anyway.
>
Right, will remove. I think the infinite loop was maybe triggered by the
double sPhysicalPageOffset substraction.
Thanks,
Jérôme
Other related posts:
