[PATCH] uninline exported ata_host_intr

[PATCH] uninline exported ata_host_intr

[Olaf Hering]

ata_host_intr cant be either inline or exported.

Signed-off-by: Olaf Hering <olh@suse.de>

Index: linux-2.6.12/drivers/scsi/libata-core.c
===================================================================
--- linux-2.6.12.orig/drivers/scsi/libata-core.c
+++ linux-2.6.12/drivers/scsi/libata-core.c
@@ -3487,7 +3487,7 @@ void ata_bmdma_stop(struct ata_port *ap)
  *	One if interrupt was handled, zero if not (shared irq).
  */
 
-inline unsigned int ata_host_intr (struct ata_port *ap,
+unsigned int ata_host_intr (struct ata_port *ap,
 				   struct ata_queued_cmd *qc)
 {
 	u8 status, host_stat;

Re: [PATCH] uninline exported ata_host_intr

[Jeff Garzik]

Olaf Hering wrote:
> ata_host_intr cant be either inline or exported.
> 
> Signed-off-by: Olaf Hering <olh@suse.de>
> 
> Index: linux-2.6.12/drivers/scsi/libata-core.c
> ===================================================================
> --- linux-2.6.12.orig/drivers/scsi/libata-core.c
> +++ linux-2.6.12/drivers/scsi/libata-core.c
> @@ -3487,7 +3487,7 @@ void ata_bmdma_stop(struct ata_port *ap)
>   *	One if interrupt was handled, zero if not (shared irq).
>   */
>  
> -inline unsigned int ata_host_intr (struct ata_port *ap,
> +unsigned int ata_host_intr (struct ata_port *ap,

er, huh?

When I wrote this, the compiler did what I expected:  it inlined the 
local copy, and exported a copy for external modules to call.

Have you verified the asm does not do this?  What changed?

	Jeff

Re: [PATCH] uninline exported ata_host_intr

[Olaf Hering]

 On Fri, Aug 05, Jeff Garzik wrote:

> When I wrote this, the compiler did what I expected:  it inlined the 
> local copy, and exported a copy for external modules to call.

so you have the code twice? how clever...

> Have you verified the asm does not do this?  What changed?

I just grepped ^inline below drivers/scsi and found this in qla4xxx and
libata-core.

Re: [PATCH] uninline exported ata_host_intr

[Andrew Vasquez]

On Fri, 05 Aug 2005, Olaf Hering wrote:

> > When I wrote this, the compiler did what I expected:  it inlined the 
> > local copy, and exported a copy for external modules to call.
> 
> so you have the code twice? how clever...
> 
> > Have you verified the asm does not do this?  What changed?
> 
> I just grepped ^inline below drivers/scsi and found this in qla4xxx and
> libata-core.

Confused, you found a reference to ata_host_intr() in qla4xxx
(QLogic's iSCSI driver)?  This is in a SLES kernel correct, and not in
mainline?

--
av

Re: [PATCH] uninline exported ata_host_intr

[Doug Maxey]

On Fri, 05 Aug 2005 13:10:56 EDT, Jeff Garzik wrote:
>Olaf Hering wrote:
>> ata_host_intr cant be either inline or exported.
>> 
>> Signed-off-by: Olaf Hering <olh@suse.de>
>> 
>> Index: linux-2.6.12/drivers/scsi/libata-core.c
>> ===================================================================
>> --- linux-2.6.12.orig/drivers/scsi/libata-core.c
>> +++ linux-2.6.12/drivers/scsi/libata-core.c
>> @@ -3487,7 +3487,7 @@ void ata_bmdma_stop(struct ata_port *ap)
>>   *	One if interrupt was handled, zero if not (shared irq).
>>   */
>>  
>> -inline unsigned int ata_host_intr (struct ata_port *ap,
>> +unsigned int ata_host_intr (struct ata_port *ap,
>
>er, huh?
>
>When I wrote this, the compiler did what I expected:  it inlined the 
>local copy, and exported a copy for external modules to call.
>
>Have you verified the asm does not do this?  What changed?
>
>	Jeff

breaks on ppc64.
 
++doug

Re: [PATCH] uninline exported ata_host_intr

[Olaf Hering]

 On Fri, Aug 05, Andrew Vasquez wrote:

> Confused, you found a reference to ata_host_intr() in qla4xxx
> (QLogic's iSCSI driver)?  This is in a SLES kernel correct, and not in
> mainline?

No, qla4xxx called extern inline function in foo.c from bar.c. gcc4 doesnt like
that because the 'function body is not available'. I found no external users of
ata_host_intr, maybe it happens there as well.
If there is a significant performance boost with these inline constructs,
thats just fine with me as long as it compiles.

Re: [PATCH] uninline exported ata_host_intr

[Jeff Garzik]

Olaf Hering wrote:
>  On Fri, Aug 05, Jeff Garzik wrote:
> 
> 
>>When I wrote this, the compiler did what I expected:  it inlined the 
>>local copy, and exported a copy for external modules to call.
> 
> 
> so you have the code twice? how clever...

This is one of the key hot paths in the driver.

Considering that drivers either call one copy, or the other, the 
tradeoff is minor overall code size increase for performance gains. 
This is a typical tradeoff in hot paths.

The code stays until you prove otherwise...

	Jeff

Re: [PATCH] uninline exported ata_host_intr

[Andrew Morton]

Doug Maxey <dwm@maxeymade.com> wrote:
>
> 
> On Fri, 05 Aug 2005 13:10:56 EDT, Jeff Garzik wrote:
> >Olaf Hering wrote:
> >> ata_host_intr cant be either inline or exported.
> >> 
> >> Signed-off-by: Olaf Hering <olh@suse.de>
> >> 
> >> Index: linux-2.6.12/drivers/scsi/libata-core.c
> >> ===================================================================
> >> --- linux-2.6.12.orig/drivers/scsi/libata-core.c
> >> +++ linux-2.6.12/drivers/scsi/libata-core.c
> >> @@ -3487,7 +3487,7 @@ void ata_bmdma_stop(struct ata_port *ap)
> >>   *	One if interrupt was handled, zero if not (shared irq).
> >>   */
> >>  
> >> -inline unsigned int ata_host_intr (struct ata_port *ap,
> >> +unsigned int ata_host_intr (struct ata_port *ap,
> >
> >er, huh?
> >
> >When I wrote this, the compiler did what I expected:  it inlined the 
> >local copy, and exported a copy for external modules to call.
> >
> >Have you verified the asm does not do this?  What changed?
> >
> >	Jeff
> 
> breaks on ppc64.
>  

I don't see why it should break.  What the code's trying to do there is
bloaty-but-legitimate.

The function is decared `extern unsigned int ata_host_intr(...)' in .h and
`inline unsigned int ata_host_intr(...)' in .c.  Should all work.

Can you share the error messages?

Re: [PATCH] uninline exported ata_host_intr

[Doug Maxey]

On Fri, 05 Aug 2005 13:10:51 PDT, Andrew Morton wrote:
>Doug Maxey <dwm@maxeymade.com> wrote:
>>
>> 
>> On Fri, 05 Aug 2005 13:10:56 EDT, Jeff Garzik wrote:
>> >Olaf Hering wrote:
>> >> ata_host_intr cant be either inline or exported.
>> >> 
>> >> Signed-off-by: Olaf Hering <olh@suse.de>
>> >> 
>> >> Index: linux-2.6.12/drivers/scsi/libata-core.c
>> >> ===================================================================
>> >> --- linux-2.6.12.orig/drivers/scsi/libata-core.c
>> >> +++ linux-2.6.12/drivers/scsi/libata-core.c
>> >> @@ -3487,7 +3487,7 @@ void ata_bmdma_stop(struct ata_port *ap)
>> >>   *	One if interrupt was handled, zero if not (shared irq).
>> >>   */
>> >>  
>> >> -inline unsigned int ata_host_intr (struct ata_port *ap,
>> >> +unsigned int ata_host_intr (struct ata_port *ap,
>> >
>> >er, huh?
>> >
>> >When I wrote this, the compiler did what I expected:  it inlined the 
>> >local copy, and exported a copy for external modules to call.
>> >
>> >Have you verified the asm does not do this?  What changed?
>> >
>> >	Jeff
>> 
>> breaks on ppc64.
>>  
>
>I don't see why it should break.  What the code's trying to do there is
>bloaty-but-legitimate.
>
>The function is decared `extern unsigned int ata_host_intr(...)' in .h and
>`inline unsigned int ata_host_intr(...)' in .c.  Should all work.
>
>Can you share the error messages?
>

Um, er, no.  This was a ways back.  Using 3.3.x would not complain, but 
3.4.x did.  

Not this specific instance, but with another driver that had a similar
construct.  IIRC, the header had a defined "extern inline" and there was
a definition in the .c file without inline.   It should be an error, IMHO.

++doug