[PATCH 1/8] ixgbe: Out of line ixgbe_read/write_reg

Subsystems: intel ethernet drivers, networking drivers, the rest

STALE4493d

5 messages, 4 authors, 2014-05-20 · open the first message on its own page

[PATCH 1/8] ixgbe: Out of line ixgbe_read/write_reg

From: Andi Kleen <hidden>
Date: 2014-05-16 21:43:27

From: Andi Kleen <redacted>

ixgbe_read_reg and ixgbe_write_reg are frequently called and are very big
because they have complex error handling code.

Moving them out of line saves ~27k text in the ixgbe driver.

   text	   data	    bss	    dec	    hex	filename
14220873	2008072	1507328	17736273	10ea251	vmlinux-before-ixgbe
14193673	2003976	1507328	17704977	10e2811	vmlinux-ixgbe

Cc: netdev@vger.kernel.org
Cc: Jeff Kirsher <redacted>
Signed-off-by: Andi Kleen <redacted>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_common.h | 22 ++--------------------
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c   | 22 ++++++++++++++++++++++
 2 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
index f12c40f..05f094d 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h

@@ -162,28 +162,10 @@ static inline void writeq(u64 val, void __iomem *addr)
 }
 #endif
 
-static inline void ixgbe_write_reg64(struct ixgbe_hw *hw, u32 reg, u64 value)
-{
-	u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
+void ixgbe_write_reg64(struct ixgbe_hw *hw, u32 reg, u64 value);
+u32 ixgbe_read_reg(struct ixgbe_hw *hw, u32 reg);
 
-	if (ixgbe_removed(reg_addr))
-		return;
-	writeq(value, reg_addr + reg);
-}
 #define IXGBE_WRITE_REG64(a, reg, value) ixgbe_write_reg64((a), (reg), (value))
-
-static inline u32 ixgbe_read_reg(struct ixgbe_hw *hw, u32 reg)
-{
-	u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
-	u32 value;
-
-	if (ixgbe_removed(reg_addr))
-		return IXGBE_FAILED_READ_REG;
-	value = readl(reg_addr + reg);
-	if (unlikely(value == IXGBE_FAILED_READ_REG))
-		ixgbe_check_remove(hw, reg);
-	return value;
-}
 #define IXGBE_READ_REG(a, reg) ixgbe_read_reg((a), (reg))
 
 #define IXGBE_WRITE_REG_ARRAY(a, reg, offset, value) \

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index d62e7a2..5f81f62 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c

@@ -371,6 +371,28 @@ void ixgbe_write_pci_cfg_word(struct ixgbe_hw *hw, u32 reg, u16 value)
 	pci_write_config_word(adapter->pdev, reg, value);
 }
 
+void ixgbe_write_reg64(struct ixgbe_hw *hw, u32 reg, u64 value)
+{
+	u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
+
+	if (ixgbe_removed(reg_addr))
+		return;
+	writeq(value, reg_addr + reg);
+}
+
+u32 ixgbe_read_reg(struct ixgbe_hw *hw, u32 reg)
+{
+	u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
+	u32 value;
+
+	if (ixgbe_removed(reg_addr))
+		return IXGBE_FAILED_READ_REG;
+	value = readl(reg_addr + reg);
+	if (unlikely(value == IXGBE_FAILED_READ_REG))
+		ixgbe_check_remove(hw, reg);
+	return value;
+}
+
 static void ixgbe_service_event_complete(struct ixgbe_adapter *adapter)
 {
 	BUG_ON(!test_bit(__IXGBE_SERVICE_SCHED, &adapter->state));

-- 
1.9.0

RE: [PATCH 1/8] ixgbe: Out of line ixgbe_read/write_reg

From: David Laight <hidden>
Date: 2014-05-19 09:15:27

From: Andi Kleen

ixgbe_read_reg and ixgbe_write_reg are frequently called and are very big
because they have complex error handling code.

Have you measured the performance impact?
I suspect that it might me measurable.

Clearly the calls during initialisation don't need to be inline,
but there will be some in the normal tx and rx paths.

	David

quoted hunk

Moving them out of line saves ~27k text in the ixgbe driver.

   text	   data	    bss	    dec	    hex	filename
14220873	2008072	1507328	17736273	10ea251	vmlinux-before-ixgbe
14193673	2003976	1507328	17704977	10e2811	vmlinux-ixgbe

Cc: netdev@vger.kernel.org
Cc: Jeff Kirsher <redacted>
Signed-off-by: Andi Kleen <redacted>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_common.h | 22 ++--------------------
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c   | 22 ++++++++++++++++++++++
 2 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h

b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
index f12c40f..05f094d 100644

--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h

@@ -162,28 +162,10 @@ static inline void writeq(u64 val, void __iomem *addr)
 }
 #endif

-static inline void ixgbe_write_reg64(struct ixgbe_hw *hw, u32 reg, u64 value)
-{
-	u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
+void ixgbe_write_reg64(struct ixgbe_hw *hw, u32 reg, u64 value);
+u32 ixgbe_read_reg(struct ixgbe_hw *hw, u32 reg);

-	if (ixgbe_removed(reg_addr))
-		return;
-	writeq(value, reg_addr + reg);
-}
 #define IXGBE_WRITE_REG64(a, reg, value) ixgbe_write_reg64((a), (reg), (value))
-
-static inline u32 ixgbe_read_reg(struct ixgbe_hw *hw, u32 reg)
-{
-	u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
-	u32 value;
-
-	if (ixgbe_removed(reg_addr))
-		return IXGBE_FAILED_READ_REG;
-	value = readl(reg_addr + reg);
-	if (unlikely(value == IXGBE_FAILED_READ_REG))
-		ixgbe_check_remove(hw, reg);
-	return value;
-}
 #define IXGBE_READ_REG(a, reg) ixgbe_read_reg((a), (reg))

 #define IXGBE_WRITE_REG_ARRAY(a, reg, offset, value) \

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c

b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index d62e7a2..5f81f62 100644

--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c

@@ -371,6 +371,28 @@ void ixgbe_write_pci_cfg_word(struct ixgbe_hw *hw, u32 reg, u16 value)
 	pci_write_config_word(adapter->pdev, reg, value);
 }

+void ixgbe_write_reg64(struct ixgbe_hw *hw, u32 reg, u64 value)
+{
+	u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
+
+	if (ixgbe_removed(reg_addr))
+		return;
+	writeq(value, reg_addr + reg);
+}
+
+u32 ixgbe_read_reg(struct ixgbe_hw *hw, u32 reg)
+{
+	u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
+	u32 value;
+
+	if (ixgbe_removed(reg_addr))
+		return IXGBE_FAILED_READ_REG;
+	value = readl(reg_addr + reg);
+	if (unlikely(value == IXGBE_FAILED_READ_REG))
+		ixgbe_check_remove(hw, reg);
+	return value;
+}
+
 static void ixgbe_service_event_complete(struct ixgbe_adapter *adapter)
 {
 	BUG_ON(!test_bit(__IXGBE_SERVICE_SCHED, &adapter->state));
--

1.9.0

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/8] ixgbe: Out of line ixgbe_read/write_reg

From: Rustad, Mark D <hidden>
Date: 2014-05-19 22:01:06

On May 16, 2014, at 2:43 PM, Andi Kleen [off-list ref] wrote:

From: Andi Kleen <redacted>

ixgbe_read_reg and ixgbe_write_reg are frequently called and are very big
because they have complex error handling code.

Actually, this patch doesn't do anything to ixgbe_write_reg, which would almost certainly be very bad for performance, but instead changes ixgbe_write_reg64. The latter is not in a performance-sensitive path, but is only called from one site, so there is little reason to take it out-of-line.

I already have a patch in queue to make ixgbe_read_reg out-of-line, because it does have a very costly memory footprint inline, as you have found.

quoted hunk

Moving them out of line saves ~27k text in the ixgbe driver.

  text	   data	    bss	    dec	    hex	filename
14220873	2008072	1507328	17736273	10ea251	vmlinux-before-ixgbe
14193673	2003976	1507328	17704977	10e2811	vmlinux-ixgbe

Cc: netdev@vger.kernel.org
Cc: Jeff Kirsher <redacted>
Signed-off-by: Andi Kleen <redacted>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_common.h | 22 ++--------------------
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c   | 22 ++++++++++++++++++++++
2 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
index f12c40f..05f094d 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h

@@ -162,28 +162,10 @@ static inline void writeq(u64 val, void __iomem *addr)

}
#endif

-static inline void ixgbe_write_reg64(struct ixgbe_hw *hw, u32 reg, u64 value)
-{
-	u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
+void ixgbe_write_reg64(struct ixgbe_hw *hw, u32 reg, u64 value);
+u32 ixgbe_read_reg(struct ixgbe_hw *hw, u32 reg);

-	if (ixgbe_removed(reg_addr))
-		return;
-	writeq(value, reg_addr + reg);
-}
#define IXGBE_WRITE_REG64(a, reg, value) ixgbe_write_reg64((a), (reg), (value))
-
-static inline u32 ixgbe_read_reg(struct ixgbe_hw *hw, u32 reg)
-{
-	u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
-	u32 value;
-
-	if (ixgbe_removed(reg_addr))
-		return IXGBE_FAILED_READ_REG;
-	value = readl(reg_addr + reg);
-	if (unlikely(value == IXGBE_FAILED_READ_REG))
-		ixgbe_check_remove(hw, reg);
-	return value;
-}
#define IXGBE_READ_REG(a, reg) ixgbe_read_reg((a), (reg))

#define IXGBE_WRITE_REG_ARRAY(a, reg, offset, value) \

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index d62e7a2..5f81f62 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c

@@ -371,6 +371,28 @@ void ixgbe_write_pci_cfg_word(struct ixgbe_hw *hw, u32 reg, u16 value)

	pci_write_config_word(adapter->pdev, reg, value);
}

+void ixgbe_write_reg64(struct ixgbe_hw *hw, u32 reg, u64 value)
+{
+	u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
+
+	if (ixgbe_removed(reg_addr))
+		return;
+	writeq(value, reg_addr + reg);
+}
+
+u32 ixgbe_read_reg(struct ixgbe_hw *hw, u32 reg)
+{
+	u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
+	u32 value;
+
+	if (ixgbe_removed(reg_addr))
+		return IXGBE_FAILED_READ_REG;
+	value = readl(reg_addr + reg);
+	if (unlikely(value == IXGBE_FAILED_READ_REG))
+		ixgbe_check_remove(hw, reg);
+	return value;
+}
+
static void ixgbe_service_event_complete(struct ixgbe_adapter *adapter)
{
	BUG_ON(!test_bit(__IXGBE_SERVICE_SCHED, &adapter->state));
-- 
1.9.0

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Mark Rustad, Networking Division, Intel Corporation

Re: [PATCH 1/8] ixgbe: Out of line ixgbe_read/write_reg

From: Andi Kleen <hidden>
Date: 2014-05-19 23:32:16

On Mon, May 19, 2014 at 10:00:52PM +0000, Rustad, Mark D wrote:

On May 16, 2014, at 2:43 PM, Andi Kleen [off-list ref] wrote:

quoted

From: Andi Kleen <redacted>

ixgbe_read_reg and ixgbe_write_reg are frequently called and are very big
because they have complex error handling code.

Actually, this patch doesn't do anything to ixgbe_write_reg, which would almost certainly be very bad for performance, but instead changes ixgbe_write_reg64.

I doubt a few cycles around the write make a lot of difference for MMIO. MMIO is dominated
by other things.

The latter is not in a performance-sensitive path, but is only called from one site, so there is little reason to take it out-of-line.

True I moved the wrong one.

ixgbe_write_reg                                    3305  (0.00%)  8     409

I already have a patch in queue to make ixgbe_read_reg out-of-line, because it does have a very costly memory footprint inline, as you have found.

Please move write_reg too.

-Andi

Re: [PATCH 1/8] ixgbe: Out of line ixgbe_read/write_reg

From: Rustad, Mark D <hidden>
Date: 2014-05-20 17:06:49

On May 19, 2014, at 4:25 PM, Andi Kleen [off-list ref] wrote:

On Mon, May 19, 2014 at 10:00:52PM +0000, Rustad, Mark D wrote:

quoted

On May 16, 2014, at 2:43 PM, Andi Kleen [off-list ref] wrote:

quoted

From: Andi Kleen <redacted>

ixgbe_read_reg and ixgbe_write_reg are frequently called and are very big
because they have complex error handling code.

Actually, this patch doesn't do anything to ixgbe_write_reg, which would almost certainly be very bad for performance, but instead changes ixgbe_write_reg64.

I doubt a few cycles around the write make a lot of difference for MMIO. MMIO is dominated
by other things.

quoted

The latter is not in a performance-sensitive path, but is only called from one site, so there is little reason to take it out-of-line.

True I moved the wrong one.

ixgbe_write_reg                                    3305  (0.00%)  8     409

quoted

I already have a patch in queue to make ixgbe_read_reg out-of-line, because it does have a very costly memory footprint inline, as you have found.

Please move write_reg too.

I will take a look at moving most of them out-of-line. There are just a few in very hot paths that should remain inline.

-- 
Mark Rustad, Networking Division, Intel Corporation

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help