Re: [PATCH 3/5] lib: lockless generic and arch independent page table (gpt) v2.

[Linus Torvalds]

On Mon, Nov 10, 2014 at 7:16 PM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> There's no reason for a "u64 cast". The value of "1 << pd_shift" is
> going to be an "int" regardless of what type pd_shift is. The type of
> a shift expression is the type of the left-hand side (with the C
> promotion rules forcing it to at least "int"), the right-hand
> expression type has absolutely no relevance.

Btw, for that exact reason, code like this:

+                  (uint64_t)(pdp->index +
+                  (1UL << (gpt_pdp_shift(gpt, pdp) + gpt->pd_shift)) - 1UL));

is likely buggy if you actually care about the uint64_t part.

On 32-bit, 1ul will be 32-bit. And so will "(1ul << .. ) -1UL",
regardless of the type of the right hand of the shift. So the fact
that gpt->pd_shift and gpt_pdp_shift() are both u64, the actual end
result is u32 (page->index is a 32-bit entity on 32-bit architectures,
since pgoff_t is an "unsigned long" too). So you're doing the shifts
in 32-bit, the addition in 32-bit, and then just casting the resulting
32-bit thing to a 64-bit entity.  The high 32 bits are guaranteed to
be zero, in other words.

This just highlights how wrong it is to make those shifts be u64. That
gpt_pdp_shift() helper similarly should at no point be returning u64.
It doesn't help, it only hurts. It makes the structure bigger for no
gain, and apparently it confuses people into thinking those shifts are
done in 64 bit.

When you do "a+b" or similar operations, the end result is the biggest
type size of 'a' and 'b' respectively (with the normal promotion to at
least 'int'). But that's not true of shifts, the type of the shift
expression is the (integer-promoted) left-hand side. The right-hand
side just gives the amount that value is shifted by, it doesn't affect
the type of the result.

                       Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/