On 27/11/2019 6:24 pm, Nicolas Saenz Julienne wrote:

On Wed, 2019-11-27 at 18:06 +0000, Robin Murphy wrote:

quoted

On 26/11/2019 12:51 pm, Leon Romanovsky wrote:

quoted

On Tue, Nov 26, 2019 at 10:19:39AM +0100, Nicolas Saenz Julienne wrote:

quoted

Some users need to make sure their rounding function accepts and returns
64bit long variables regardless of the architecture. Sadly
roundup/rounddown_pow_two() takes and returns unsigned longs. Create a
new generic 64bit variant of the function and cleanup rougue custom
implementations.

Is it possible to create general roundup/rounddown_pow_two() which will
work correctly for any type of variables, instead of creating special
variant for every type?

In fact, that is sort of the case already - roundup_pow_of_two() itself
wraps ilog2() such that the constant case *is* type-independent. And
since ilog2() handles non-constant values anyway, might it be reasonable
to just take the strongly-typed __roundup_pow_of_two() helper out of the
loop as below?

Robin

That looks way better that's for sure. Some questions.

quoted

----->8-----

diff --git a/include/linux/log2.h b/include/linux/log2.h
index 83a4a3ca3e8a..e825f8a6e8b5 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h

@@ -172,11 +172,8 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
    */
   #define roundup_pow_of_two(n)			\
   (						\
-	__builtin_constant_p(n) ? (		\
-		(n == 1) ? 1 :			\
-		(1UL << (ilog2((n) - 1) + 1))	\
-				   ) :		\
-	__roundup_pow_of_two(n)			\
+	(__builtin_constant_p(n) && (n == 1)) ?	\
+	1 : (1UL << (ilog2((n) - 1) + 1))	\

Then here you'd have to use ULL instead of UL, right? I want my 64bit value
everywhere regardless of the CPU arch. The downside is that would affect
performance to some extent (i.e. returning a 64bit value where you used to have
a 32bit one)?

True, although it's possible that 1ULL might result in the same codegen 
if the compiler can see that the result is immediately truncated back to 
long anyway. Or at worst, I suppose "(typeof(n))1" could suffice, 
however ugly. Either way, this diff was only an illustration rather than 
a concrete proposal, but it might be an interesting diversion to 
investigate.

On that note, though, you should probably be using ULL in your current 
patch too.

Also, what about callers to this function on platforms with 32bit 'unsigned
longs' that happen to input a 64bit value into this. IIUC we'd have a change of
behaviour.

Indeed, although the change in such a case would be "start getting the 
expected value instead of nonsense", so it might very well be welcome ;)

Robin.