Re: [RFC][PATCH] introduce ptr_diff()

[Bernd Petrovitsch <bernd@sysprog.at>]

On Don, 2010-08-19 at 20:37 +0900, Namhyung Kim wrote:
> When I compiled allyesconfig'ed kernel with C=1, I got 1519 lines of
> following message:
> 
>  include/linux/mm.h:599:16: warning: potentially expensive pointer subtraction
> 
> which is around 10% of total warnings. this was caused by page_to_pfn() macro
> so I think it's worth to remove it by calculating pointer subtraction
> manually.
> 
> ptr_diff() does subtraction of two pointers as if they were integer values
> and divides it by the size of their base type. Currently it does not deal
> with a difference alignment requirement than the size of base type, it
> would be a trivial job.
> 
> Impact: remove a _lot_ of sparse warnings
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> ---
> 
> I have no idea where the right place is.
> Please kindly point me to the place if here is not the one.
> And any comments are greatly welcomed also.
> 
> Thanks.
> 
>  include/asm-generic/memory_model.h |   23 +++++++++++++++++++----
>  1 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/include/asm-generic/memory_model.h b/include/asm-generic/memory_model.h
> index fb2d63f..ffec625 100644
> --- a/include/asm-generic/memory_model.h
> +++ b/include/asm-generic/memory_model.h
> @@ -22,13 +22,28 @@
>  
>  #endif /* CONFIG_DISCONTIGMEM */
>  
> +#include <linux/log2.h>
> +
> +#define ptr_diff(p1, p2)					\
> +({	long __diff;						\
> +	unsigned long  __addr1 = (unsigned long) (p1);		\
> +	unsigned long  __addr2 = (unsigned long) (p2);		\
> +	unsigned __size = sizeof(typeof(*p1));			\
The typeof() is actually redundant here.
> +								\
> +	if (is_power_of_2(__size))				\
> +		__diff = (__addr1 - __addr2) >> ilog2(__size);	\
> +	else							\
> +		__diff = (__addr1 - __addr2) / __size;		\
> +	__diff;							\
> +})
> +

IMHO this kills gcc's type compatibility checks on p1 and p2 (even if
they are suboptimal).
There is the "__same_type" macro in compiler.h for this or the "else"
branch uses plain C
	__diff = p1 - p2;
to keep them (which probably also keeps sparse's warnings).

Actually one would assume that gcc internally just does the above as the
size of the type is statically known (and sparse could be improved to
not warn where sizeof(*p1) is a power of 2).

	Bernd
-- 
mobile: +43 664 4416156              http://www.sysprog.at/
    Linux Software Development, Consulting and Services