uninline exported ata_host_intr

PATCH In review Olaf Hering 5 participants 9 messages Aug 5, 2005 SCSI

lore

Share & tools

lore

Olaf Hering

author

2005-08-05 17:03 UTC #0 lore raw

PATCH 1/1 uninline exported ata_host_intr

ata_host_intr cant be either inline or exported.

Signed-off-by Olaf Hering <[email protected]>

drivers/scsi/libata-core.c +1 -1

Index: 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;

Jeff Garzik

2005-08-05 17:11 UTC #1 lore raw

Quoted 9 lines · Olaf Hering

ata_host_intr cant be either inline or exported.
Signed-off-by Olaf Hering <[email protected]>
Index: linux-2.6.12/drivers/scsi/libata-core.c
===================================================================

drivers/scsi/libata-core.c void ata_bmdma_stop(struct ata_port *ap) · line 3490

Quoted 5 lines

drivers/scsi/libata-core.c +1 -1

@@ -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

Olaf Hering

author

2005-08-05 17:15 UTC #2 lore raw

Quoted 2 lines · Jeff Garzik

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...

Quoted 1 line

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.

Andrew Vasquez

2005-08-05 17:30 UTC #3 lore raw

Quoted 9 lines · Olaf Hering

Quoted 2 lines
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...
Quoted 1 line
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

Olaf Hering

author

2005-08-05 17:39 UTC #4 lore raw

Quoted 3 lines · Andrew Vasquez

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.

Jeff Garzik

2005-08-05 17:44 UTC #5 lore raw

In reply to Olaf Hering

Quoted 7 lines · Olaf Hering

Quoted 1 line · Jeff Garzik
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

Doug Maxey

2005-08-05 17:39 UTC #6 lore raw

In reply to Jeff Garzik

Quoted 18 lines · Jeff Garzik

Quoted 9 lines · Olaf Hering
ata_host_intr cant be either inline or exported.
Signed-off-by Olaf Hering <[email protected]>
Index: linux-2.6.12/drivers/scsi/libata-core.c
===================================================================
drivers/scsi/libata-core.c void ata_bmdma_stop(struct ata_port *ap) · line 3490
Quoted 5 lines
drivers/scsi/libata-core.c +1 -1
@@ -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?

Quoted 6 lines

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

Andrew Morton

2005-08-05 20:15 UTC #7 lore raw

Quoted 21 lines · Doug Maxey

Quoted 18 lines · Jeff Garzik
Quoted 9 lines · Olaf Hering
ata_host_intr cant be either inline or exported.
Signed-off-by Olaf Hering <[email protected]>
Index: linux-2.6.12/drivers/scsi/libata-core.c
===================================================================
drivers/scsi/libata-core.c void ata_bmdma_stop(struct ata_port *ap) · line 3490
Quoted 5 lines
drivers/scsi/libata-core.c +1 -1
@@ -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?

Quoted 9 lines

Quoted 6 lines
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?

Doug Maxey

2005-08-05 20:52 UTC #8 lore raw

Quoted 32 lines · Andrew Morton

Quoted 21 lines · Doug Maxey
Quoted 18 lines · Jeff Garzik
Quoted 9 lines · Olaf Hering
ata_host_intr cant be either inline or exported.
Signed-off-by Olaf Hering <[email protected]>
Index: linux-2.6.12/drivers/scsi/libata-core.c
===================================================================
drivers/scsi/libata-core.c void ata_bmdma_stop(struct ata_port *ap) · line 3490
Quoted 5 lines
drivers/scsi/libata-core.c +1 -1
@@ -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?
Quoted 9 lines
Quoted 6 lines
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.

Quoted 8 lines

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