svn commit: r218966 - head/sys/vm

[Alan Cox]

Bruce Evans wrote:
> On Wed, 23 Feb 2011, Bruce Cran wrote:
>
>> On Thu, 2011-02-24 at 08:23 +1100, Bruce Evans wrote:
>>
>>> The bug seems to have been overflow in this calculation.
>>> [swap_bcount * SWAP_META_PAGES * n / <non-overflowing division>]
>>
>> I've attached a patch which changes 'n' to be of type vm_ooffset_t. I
>> think this should fix the overflow bug?
>
> I don't like using vm_ooffset_t either.  There are no offsets here, and
> it's bad technique to depend on having a large type to avoid overflows
> in expressions when the result type is different.
>
> I would cast operand(s) in the expression as necessary to prevent 
> overflow
> of subexpressions.  vm_pindex_t would work, but I prefer to use a type
> related to the subexpressions.  Not sure what that is.  Maybe just
> uintmax_t for safety (even that is not safe if the subexpressions have
> large values).  So:
>
>     (uintmax_t)swap_bcount * SWAP_META_PAGES * n / mumble.
>
> I like to cast only the leftmost term if possible, and depend on the
> larger type propagating to all subexpressions via left-to-right
> evaluation.  This saves a lot of casts.  Here this may be sub-optimal
> and we could probably delay the cast to the final multiplication, which
> reduces to the same safeness as using uintmax_t for n.
>
> Next, there is the return type to consider.  I don't see why it needs
> to be changed from int.  The patch in the PR actually changed it to
> long, while changing n to vm_offset_t.  But on 32-bit machines, long
> is essentially the same as int, and vm_offset_t is not much larger.
> Even 32-bit machine might actually need a type larger than 32 bits to
> prevent overflow in expressions like the above.
>

With one exception, I would agree with what you have suggested above.  I 
would argue for using a long.  HP is already shipping amd64 architecture 
machines that support 2TB of RAM.  In fact, we have already made changes 
to HEAD so that FreeBSD boots on these machines, albeit with a more 
modest amount of RAM.  So, we are not that far away from the number of 
4KB pages overflowing an int.  In fact, it might plausibly happen before 
the End of Life for 9-STABLE.  As you point out, most if not all of our 
page counters are still ints, but I see no reason for new or modified 
code like this not to begin the transition to a larger type.

Alan