[RFC-UGLYPATCH] ata: small optimization in linux/libata.h

[RFC-UGLYPATCH] ata: small optimization in linux/libata.h

[Harvey Harrison]

This patch may be too ugly to live, it suppresses a lot of
sparse warnings in the libata build and produces slightly
tighter code. (4 instructions vs 5 and a few bytes saved).

include/linux/libata.h:1214:13: warning: potentially expensive pointer subtraction

Original:
	if (++link - ap->pmp_link < ap->nr_pmp_links)
		return link;

     52b:	89 d8                	mov    %ebx,%eax
     52d:	2b 82 60 26 00 00    	sub    0x2660(%edx),%eax
     533:	c1 f8 02             	sar    $0x2,%eax
     536:	69 c0 dd 3d c8 44    	imul   $0x44c83ddd,%eax,%eax
     53c:	3b 82 5c 26 00 00    	cmp    0x265c(%edx),%eax
     542:	7d 04                	jge    548 <sata_pmp_detach+0xbe>

Next:
	if ((char*)++link - (char *)ap->pmp_link < ap->nr_pmp_links * sizeof(*link))
		return link;

     52b:	69 81 5c 26 00 00 d4 	imul   $0x9d4,0x265c(%ecx),%eax
     532:	09 00 00
     535:	89 da                	mov    %ebx,%edx
     537:	2b 91 60 26 00 00    	sub    0x2660(%ecx),%edx
     53d:	39 c2                	cmp    %eax,%edx
     53f:	73 04                	jae    545 <sata_pmp_detach+0xbb>

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
I know it's ugly, but I had it done anyways.  The one real problem I have
with it is that if link and ap->pmp_link ever get changed to different types
the compiler will not even warn as we cast away to (char *).  To make it
a bit more robust, a BUILD_BUG_ON checking the pointer types may be a
good idea.

 include/linux/libata.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/libata.h b/include/linux/libata.h
index 2845983..f0e1178 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1211,7 +1211,7 @@ static inline struct ata_link *ata_port_next_link(struct ata_link *link)
 		return ap->pmp_link;
 	}
 
-	if (++link - ap->pmp_link < ap->nr_pmp_links)
+	if ((char*)++link - (char *)ap->pmp_link < ap->nr_pmp_links * sizeof(*link))
 		return link;
 	return NULL;
 }
-- 
1.5.4.1.1278.gc75be

Re: [RFC-UGLYPATCH] ata: small optimization in linux/libata.h

[Tejun Heo]

Harvey Harrison wrote:
> This patch may be too ugly to live, it suppresses a lot of
> sparse warnings in the libata build and produces slightly
> tighter code. (4 instructions vs 5 and a few bytes saved).
> 
> include/linux/libata.h:1214:13: warning: potentially expensive pointer subtraction
> 
> Original:
> 	if (++link - ap->pmp_link < ap->nr_pmp_links)
> 		return link;
> 
>      52b:	89 d8                	mov    %ebx,%eax
>      52d:	2b 82 60 26 00 00    	sub    0x2660(%edx),%eax
>      533:	c1 f8 02             	sar    $0x2,%eax
>      536:	69 c0 dd 3d c8 44    	imul   $0x44c83ddd,%eax,%eax
>      53c:	3b 82 5c 26 00 00    	cmp    0x265c(%edx),%eax
>      542:	7d 04                	jge    548 <sata_pmp_detach+0xbe>
> 
> Next:
> 	if ((char*)++link - (char *)ap->pmp_link < ap->nr_pmp_links * sizeof(*link))
> 		return link;
> 
>      52b:	69 81 5c 26 00 00 d4 	imul   $0x9d4,0x265c(%ecx),%eax
>      532:	09 00 00
>      535:	89 da                	mov    %ebx,%edx
>      537:	2b 91 60 26 00 00    	sub    0x2660(%ecx),%edx
>      53d:	39 c2                	cmp    %eax,%edx
>      53f:	73 04                	jae    545 <sata_pmp_detach+0xbb>
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> ---
> I know it's ugly, but I had it done anyways.  The one real problem I have
> with it is that if link and ap->pmp_link ever get changed to different types
> the compiler will not even warn as we cast away to (char *).  To make it
> a bit more robust, a BUILD_BUG_ON checking the pointer types may be a
> good idea.

Sorry, but Nacked-by: Tejun Heo <htejun@gmail.com>

-- 
tejun

Re: [RFC-UGLYPATCH] ata: small optimization in linux/libata.h

[Harvey Harrison]

On Fri, 2008-02-15 at 08:39 +0900, Tejun Heo wrote:
> Harvey Harrison wrote:
> > This patch may be too ugly to live, it suppresses a lot of
> > sparse warnings in the libata build and produces slightly
> > tighter code. (4 instructions vs 5 and a few bytes saved).
> > 
> > include/linux/libata.h:1214:13: warning: potentially expensive pointer subtraction
> > ---

> > I know it's ugly, but I had it done anyways.  The one real problem I have
> > with it is that if link and ap->pmp_link ever get changed to different types
> > the compiler will not even warn as we cast away to (char *).  To make it
> > a bit more robust, a BUILD_BUG_ON checking the pointer types may be a
> > good idea.
> 
> Sorry, but Nacked-by: Tejun Heo <htejun@gmail.com>
> 

Can't say I really blame you, other than this one error, drivers/ata
builds almost sparse-clean, and I had it done anyways.  I wonder if
a helper similar in spirit would be any better.  This doesn't have
any typechecking, but perhaps that could be dealt with too.

Ran into a similar problem with mmzone.h, akpm has the patch, but
maybe a helper (kernel.h?) would be cleaner.  Or maybe it's just
better to live with the sparse warnings....

/*
 * return the offset of the ptr from the base, in bytes.
 */
#define PTR_OFF(base, ptr) \
((char *)ptr - (char *)base)

if (PTR_OFF(ap->pmp_link, ++link) < ap->nr_pmp_links * sizeof(*link))
	return link;

Cheers,

Harvey

Re: [RFC-UGLYPATCH] ata: small optimization in linux/libata.h

[Tejun Heo]

Harvey Harrison wrote:
>>> I know it's ugly, but I had it done anyways.  The one real problem I have
>>> with it is that if link and ap->pmp_link ever get changed to different types
>>> the compiler will not even warn as we cast away to (char *).  To make it
>>> a bit more robust, a BUILD_BUG_ON checking the pointer types may be a
>>> good idea.
>> Sorry, but Nacked-by: Tejun Heo <htejun@gmail.com>
>>
> 
> Can't say I really blame you, other than this one error, drivers/ata
> builds almost sparse-clean, and I had it done anyways.  I wonder if
> a helper similar in spirit would be any better.  This doesn't have
> any typechecking, but perhaps that could be dealt with too.
> 
> Ran into a similar problem with mmzone.h, akpm has the patch, but
> maybe a helper (kernel.h?) would be cleaner.  Or maybe it's just
> better to live with the sparse warnings....
> 
> /*
>  * return the offset of the ptr from the base, in bytes.
>  */
> #define PTR_OFF(base, ptr) \
> ((char *)ptr - (char *)base)
> 
> if (PTR_OFF(ap->pmp_link, ++link) < ap->nr_pmp_links * sizeof(*link))
> 	return link;

Can you please explain why it warns against pointer substraction?  Is it
because it can generate division?

Thanks.

-- 
tejun

Re: [RFC-UGLYPATCH] ata: small optimization in linux/libata.h

[Mikael Pettersson]

Tejun Heo writes:
 > Harvey Harrison wrote:
 > >>> I know it's ugly, but I had it done anyways.  The one real problem I have
 > >>> with it is that if link and ap->pmp_link ever get changed to different types
 > >>> the compiler will not even warn as we cast away to (char *).  To make it
 > >>> a bit more robust, a BUILD_BUG_ON checking the pointer types may be a
 > >>> good idea.
 > >> Sorry, but Nacked-by: Tejun Heo <htejun@gmail.com>
 > >>
 > > 
 > > Can't say I really blame you, other than this one error, drivers/ata
 > > builds almost sparse-clean, and I had it done anyways.  I wonder if
 > > a helper similar in spirit would be any better.  This doesn't have
 > > any typechecking, but perhaps that could be dealt with too.
 > > 
 > > Ran into a similar problem with mmzone.h, akpm has the patch, but
 > > maybe a helper (kernel.h?) would be cleaner.  Or maybe it's just
 > > better to live with the sparse warnings....
 > > 
 > > /*
 > >  * return the offset of the ptr from the base, in bytes.
 > >  */
 > > #define PTR_OFF(base, ptr) \
 > > ((char *)ptr - (char *)base)
 > > 
 > > if (PTR_OFF(ap->pmp_link, ++link) < ap->nr_pmp_links * sizeof(*link))
 > > 	return link;
 > 
 > Can you please explain why it warns against pointer substraction?  Is it
 > because it can generate division?

Pointer subtraction as commonly implemented implies a signed
integer division, because it may need to convert a negative
byte pointer difference to a negative array index difference.
Seasoned C programmers avoid it like the plague...

Re: [RFC-UGLYPATCH] ata: small optimization in linux/libata.h

[Jeff Garzik]

Harvey Harrison wrote:
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 2845983..f0e1178 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1211,7 +1211,7 @@ static inline struct ata_link *ata_port_next_link(struct ata_link *link)
>  		return ap->pmp_link;
>  	}
>  
> -	if (++link - ap->pmp_link < ap->nr_pmp_links)
> +	if ((char*)++link - (char *)ap->pmp_link < ap->nr_pmp_links * sizeof(*link))
>  		return link;


I prefer the less ugly version :)

Re: [RFC-UGLYPATCH] ata: small optimization in linux/libata.h

[Christer Weinigel]

On Thu, 14 Feb 2008 11:16:18 -0800
Harvey Harrison <harvey.harrison@gmail.com> wrote:

> Original:
> 	if (++link - ap->pmp_link < ap->nr_pmp_links)
> Next:
> 	if ((char*)++link - (char *)ap->pmp_link < ap->nr_pmp_links * sizeof(*link)) return link;

Ungh. 

Why not rewrite it as (untested):

    if (++link < ap->pmp_link + ap->nr_pmp_links)

That should be just as efficient and more readable since it
is a simple if (ptr < start + size).

  /Christer

[PATCH] ata: fix sparse warning in libata.h

[Harvey Harrison]

Avoids lots of these, also is more readable.
include/linux/libata.h:1210:13: warning: potentially expensive pointer subtraction

Change the subtraction to addition on the other side of the comparison.

Thanks to Christer Weinigel for the suggestion.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 include/linux/libata.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/libata.h b/include/linux/libata.h
index bc5a8d0..a6243bb 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1207,7 +1207,7 @@ static inline struct ata_link *ata_port_next_link(struct ata_link *link)
 		return ap->pmp_link;
 	}
 
-	if (++link - ap->pmp_link < ap->nr_pmp_links)
+	if (++link < ap->nr_pmp_links + ap->pmp_link)
 		return link;
 	return NULL;
 }
-- 
1.5.4.1.1278.gc75be

Re: [PATCH] ata: fix sparse warning in libata.h

[Tejun Heo]

Harvey Harrison wrote:
> Avoids lots of these, also is more readable.
> include/linux/libata.h:1210:13: warning: potentially expensive pointer subtraction
> 
> Change the subtraction to addition on the other side of the comparison.
> 
> Thanks to Christer Weinigel for the suggestion.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>

Acked-by: Tejun Heo <htejun@gmail.com>

-- 
tejun

Re: [PATCH] ata: fix sparse warning in libata.h

[Jeff Garzik]

Harvey Harrison wrote:
> Avoids lots of these, also is more readable.
> include/linux/libata.h:1210:13: warning: potentially expensive pointer subtraction
> 
> Change the subtraction to addition on the other side of the comparison.
> 
> Thanks to Christer Weinigel for the suggestion.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> ---
>  include/linux/libata.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index bc5a8d0..a6243bb 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1207,7 +1207,7 @@ static inline struct ata_link *ata_port_next_link(struct ata_link *link)
>  		return ap->pmp_link;
>  	}
>  
> -	if (++link - ap->pmp_link < ap->nr_pmp_links)
> +	if (++link < ap->nr_pmp_links + ap->pmp_link)
>  		return link;

applied