Eliminate duplicate barriers on weakly-ordered archs
Subsystems (16)
v3 cover Eliminate duplicate barriers on weakly-ordered archs
Code includes wmb() followed by writel() in multiple places. writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
I did a regex search for wmb() followed by writel() in each drivers directory.
I scrubbed the ones I care about in this series.
I considered "ease of change", "popular usage" and "performance critical path" as the determining criteria for my filtering.
We used relaxed API heavily on ARM for a long time but
it did not exist on other architectures. For this reason, relaxed architectures have been paying double penalty in order to use the common drivers.
Now that relaxed API is present on all architectures, we can go and scrub all drivers to see what needs to change and what can remain.
We start with mostly used ones and hope to increase the coverage over time. It will take a while to cover all drivers.
Feel free to apply patches individually.
Changes since v2:
- https://www.spinics.net/lists/arm-kernel/msg641383.html
- compilation fix for ixgbevf
- add patches 7..18
Sinan Kaya (18):
i40e/i40evf: Eliminate duplicate barriers on weakly-ordered archs
ixgbe: eliminate duplicate barriers on weakly-ordered archs
igbvf: eliminate duplicate barriers on weakly-ordered archs
igb: eliminate duplicate barriers on weakly-ordered archs
ixgbevf: keep writel() closer to wmb()
ixgbevf: eliminate duplicate barriers on weakly-ordered archs
drivers: net: cxgb: Eliminate duplicate barriers on weakly-ordered
archs
scsi: hpsa: Eliminate duplicate barriers on weakly-ordered archs
fm10k: Eliminate duplicate barriers on weakly-ordered archs
net: qla3xxx: Eliminate duplicate barriers on weakly-ordered archs
qlcnic: Eliminate duplicate barriers on weakly-ordered archs
bnx2x: Eliminate duplicate barriers on weakly-ordered archs
net: cxgb4/cxgb4vf: Eliminate duplicate barriers on weakly-ordered
archs
net: cxgb3: Eliminate duplicate barriers on weakly-ordered archs RDMA/bnxt_re: Eliminate duplicate barriers on weakly-ordered archs IB/mlx4: Eliminate duplicate barriers on weakly-ordered archs RDMA/i40iw: Eliminate duplicate barriers on weakly-ordered archs
infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered
archs
drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 8 ++++---- drivers/infiniband/hw/cxgb4/t4.h | 12 ++++++------ drivers/infiniband/hw/i40iw/i40iw_ctrl.c | 6 ++++-- drivers/infiniband/hw/i40iw/i40iw_osdep.h | 1 + drivers/infiniband/hw/i40iw/i40iw_uk.c | 2 +- drivers/infiniband/hw/i40iw/i40iw_utils.c | 11 +++++++++++ drivers/infiniband/hw/mlx4/qp.c | 4 ++-- drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c | 2 +- drivers/net/ethernet/chelsio/cxgb/sge.c | 2 +- drivers/net/ethernet/chelsio/cxgb3/adapter.h | 7 +++++++ drivers/net/ethernet/chelsio/cxgb3/sge.c | 19 ++++++++++--------- drivers/net/ethernet/chelsio/cxgb4/cxgb4.h | 6 ++++++ drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 13 +++++++------ drivers/net/ethernet/chelsio/cxgb4/sge.c | 8 ++++---- drivers/net/ethernet/chelsio/cxgb4/t4_hw.c | 2 +- drivers/net/ethernet/chelsio/cxgb4vf/adapter.h | 14 ++++++++++++++ drivers/net/ethernet/chelsio/cxgb4vf/sge.c | 16 +++++++++------- drivers/net/ethernet/intel/fm10k/fm10k_main.c | 2 +- drivers/net/ethernet/intel/i40e/i40e_txrx.c | 8 ++++---- drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 4 ++-- drivers/net/ethernet/intel/igb/igb_main.c | 4 ++-- drivers/net/ethernet/intel/igbvf/netdev.c | 4 ++-- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 8 ++++---- drivers/net/ethernet/intel/ixgbevf/ixgbevf.h | 5 ----- drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 4 ++-- drivers/net/ethernet/qlogic/qla3xxx.c | 4 ++-- drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_hw.c | 2 +- drivers/scsi/hpsa.h | 2 +- 28 files changed, 110 insertions(+), 70 deletions(-)
Signature · 1 line
2.7.4
PATCH v3 1/18 i40e/i40evf: Eliminate duplicate barriers on weakly-ordered archs
Code includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/net/ethernet/intel/i40e/i40e_txrx.c | 8 ++++---- drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-)
drivers/net/ethernet/intel/i40e/i40e_txrx.c +4 -4
@@ -185,7 +185,7 @@ static int i40e_program_fdir_filter(struct i40e_fdir_filter *fdir_data,/* Mark the data descriptor to be watched */ first->next_to_watch = tx_desc; -writel (tx_ring->next_to_use, tx_ring->tail); +writel_relaxed (tx_ring->next_to_use, tx_ring->tail);return 0 ; dma_fail:
@@ -1375,7 +1375,7 @@ static inline void i40e_release_rx_desc(struct i40e_ring *rx_ring, u32 val)* such as IA-64). */ wmb (); -writel (val, rx_ring->tail); +writel_relaxed (val, rx_ring->tail); }/**
@@ -2258,7 +2258,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)*/ wmb (); -writel (xdp_ring->next_to_use, xdp_ring->tail); +writel_relaxed (xdp_ring->next_to_use, xdp_ring->tail); } rx_ring->skb = skb;
@@ -3286,7 +3286,7 @@ static inline int i40e_tx_map(struct i40e_ring *tx_ring, struct sk_buff *skb,/* notify HW of packet */ if (netif_xmit_stopped (txring_txq (tx_ring)) || !skb->xmit_more) { -writel (i, tx_ring->tail); +writel_relaxed (i, tx_ring->tail);/* we need this if more than one processor can write to our tail * at a time, it synchronizes IO on IA64/Altix systems
drivers/net/ethernet/intel/i40evf/i40e_txrx.c +2 -2
@@ -667,7 +667,7 @@ static inline void i40e_release_rx_desc(struct i40e_ring *rx_ring, u32 val)* such as IA-64). */ wmb (); -writel (val, rx_ring->tail); +writel_relaxed (val, rx_ring->tail); }/**
@@ -2243,7 +2243,7 @@ static inline void i40evf_tx_map(struct i40e_ring *tx_ring, struct sk_buff *skb,/* notify HW of packet */ if (netif_xmit_stopped (txring_txq (tx_ring)) || !skb->xmit_more) { -writel (i, tx_ring->tail); +writel_relaxed (i, tx_ring->tail);/* we need this if more than one processor can write to our tail * at a time, it synchronizes IO on IA64/Altix systems
Signature · 1 line
2.7.4
PATCH v3 2/18 ixgbe: eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel() in multiple places. writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +4 -4
@@ -1692,7 +1692,7 @@ void ixgbe_alloc_rx_buffers(struct ixgbe_ring *rx_ring, u16 cleaned_count)* such as IA-64). */ wmb (); -writel (i, rx_ring->tail); +writel_relaxed (i, rx_ring->tail); } }
@@ -2453,7 +2453,7 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,* know there are new descriptors to fetch. */ wmb (); -writel (ring->next_to_use, ring->tail); +writel_relaxed (ring->next_to_use, ring->tail);xdp_do_flush_map (); }
@@ -8078,7 +8078,7 @@ static int ixgbe_tx_map(struct ixgbe_ring *tx_ring,ixgbe_maybe_stop_tx (tx_ring,DESC_NEEDED );if (netif_xmit_stopped (txring_txq (tx_ring)) || !skb->xmit_more) { -writel (i, tx_ring->tail); +writel_relaxed (i, tx_ring->tail);/* we need this if more than one processor can write to our tail * at a time, it synchronizes IO on IA64/Altix systems
@@ -10014,7 +10014,7 @@ static void ixgbe_xdp_flush(struct net_device *dev)* are new descriptors to fetch. */ wmb (); -writel (ring->next_to_use, ring->tail); +writel_relaxed (ring->next_to_use, ring->tail);return ; }
Signature · 1 line
2.7.4
PATCH v3 3/18 igbvf: eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/net/ethernet/intel/igbvf/netdev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/intel/igbvf/netdev.c +2 -2
@@ -251,7 +251,7 @@ static void igbvf_alloc_rx_buffers(struct igbvf_ring *rx_ring,* such as IA-64). */ wmb (); -writel (i, adapter->hw.hw_addr + rx_ring->tail); +writel_relaxed (i, adapter->hw.hw_addr + rx_ring->tail); } }
@@ -2297,7 +2297,7 @@ static inline void igbvf_tx_queue_adv(struct igbvf_adapter *adapter, tx_ring->buffer_info[first].next_to_watch = tx_desc; tx_ring->next_to_use = i; -writel (i, adapter->hw.hw_addr + tx_ring->tail); +writel_relaxed (i, adapter->hw.hw_addr + tx_ring->tail);/* we need this if more than one processor can write to our tail * at a time, it synchronizes IO on IA64/Altix systems */
Signature · 1 line
2.7.4
PATCH v3 4/18 igb: eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/net/ethernet/intel/igb/igb_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/intel/igb/igb_main.c +2 -2
@@ -5671,7 +5671,7 @@ static int igb_tx_map(struct igb_ring *tx_ring,igb_maybe_stop_tx (tx_ring,DESC_NEEDED );if (netif_xmit_stopped (txring_txq (tx_ring)) || !skb->xmit_more) { -writel (i, tx_ring->tail); +writel_relaxed (i, tx_ring->tail);/* we need this if more than one processor can write to our tail * at a time, it synchronizes IO on IA64/Altix systems
@@ -8072,7 +8072,7 @@ void igb_alloc_rx_buffers(struct igb_ring *rx_ring, u16 cleaned_count)* such as IA-64). */ wmb (); -writel (i, rx_ring->tail); +writel_relaxed (i, rx_ring->tail); } }
Signature · 1 line
2.7.4
PATCH v3 5/18 ixgbevf: keep writel() closer to wmb()
In reply to Sinan KayaRemove ixgbevf_write_tail() in favor of moving writel() close to wmb().
--- drivers/net/ethernet/intel/ixgbevf/ixgbevf.h | 5 ----- drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 4 ++-- 2 files changed, 2 insertions(+), 7 deletions(-)
drivers/net/ethernet/intel/ixgbevf/ixgbevf.h +0 -5
@@ -244,11 +244,6 @@ static inline u16 ixgbevf_desc_unused(struct ixgbevf_ring *ring)return ((ntc > ntu) ?0 : ring->count) + ntc - ntu -1 ; } -static inline void ixgbevf_write_tail (struct ixgbevf_ring *ring,u32 value) -{ -writel (value, ring->tail); -} -#define IXGBEVF_RX_DESC (R , i) \ (&(((union ixgbe_adv_rx_desc *)((R)->desc))[i]))#define IXGBEVF_TX_DESC (R , i) \
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c +2 -2
@@ -659,7 +659,7 @@ static void ixgbevf_alloc_rx_buffers(struct ixgbevf_ring *rx_ring,* such as IA-64). */ wmb (); -ixgbevf_write_tail (rx_ring, i); +writel (i, rx_ring->tail); } }
@@ -3644,7 +3644,7 @@ static void ixgbevf_tx_map(struct ixgbevf_ring *tx_ring, tx_ring->next_to_use = i;/* notify HW of packet */ -ixgbevf_write_tail (tx_ring, i); +writel (i, tx_ring->tail);return ; dma_error:
Signature · 1 line
2.7.4
PATCH v3 6/18 ixgbevf: eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel() in multiple places. writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c +2 -2
@@ -659,7 +659,7 @@ static void ixgbevf_alloc_rx_buffers(struct ixgbevf_ring *rx_ring,* such as IA-64). */ wmb (); -writel (i, rx_ring->tail); +writel_relaxed (i, rx_ring->tail); } }
@@ -3644,7 +3644,7 @@ static void ixgbevf_tx_map(struct ixgbevf_ring *tx_ring, tx_ring->next_to_use = i;/* notify HW of packet */ -writel (i, tx_ring->tail); +writel_relaxed (i, tx_ring->tail);return ; dma_error:
Signature · 1 line
2.7.4
PATCH v3 7/18 drivers: net: cxgb: Eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/net/ethernet/chelsio/cxgb/sge.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/chelsio/cxgb/sge.c +1 -1
@@ -495,7 +495,7 @@ static struct sk_buff *sched_skb(struct sge *sge, struct sk_buff *skb,static inline void doorbell_pio (struct adapter *adapter,u32 val) {wmb (); -writel (val, adapter->regs +A_SG_DOORBELL ); +writel_relaxed (val, adapter->regs +A_SG_DOORBELL ); }/*
Signature · 1 line
2.7.4
PATCH v3 8/18 scsi: hpsa: Eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/scsi/hpsa.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/hpsa.h +1 -1
@@ -599,7 +599,7 @@ static unsigned long SA5_ioaccel_mode1_completed(struct ctlr_info *h, u8 q)* but with current driver design this is easiest. */ wmb (); -writel ((q <<24 ) | rq->current_entry, h->vaddr + +writel_relaxed ((q <<24 ) | rq->current_entry, h->vaddr +IOACCEL_MODE1_CONSUMER_INDEX );atomic_dec (&h->commands_outstanding); }
Signature · 1 line
2.7.4
PATCH v3 9/18 fm10k: Eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/net/ethernet/intel/fm10k/fm10k_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/intel/fm10k/fm10k_main.c +1 -1
@@ -179,7 +179,7 @@ void fm10k_alloc_rx_buffers(struct fm10k_ring *rx_ring, u16 cleaned_count)wmb ();/* notify hardware of new descriptors */ -writel (i, rx_ring->tail); +writel_relaxed (i, rx_ring->tail); } }
Signature · 1 line
2.7.4
Re: [Intel-wired-lan] [PATCH v3 09/18] fm10k: Eliminate duplicate barriers on weakly-ordered archs
Quoted 10 lines · Sinan Kaya
Code includes
wmb()followed bywritel().writel()already has a barrier on some architectures like arm64.This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing
writel()towritel_relaxed().Signed-off-by Sinan Kaya <[email protected]>
You can update the writel call in fm10k_tx_map as well.
Of the drivers updated in drivers/net/ethernet/intel/* it looks like this is the only one that still requires any additional changes.
Thanks.
- Alex
Re: [Intel-wired-lan] [PATCH v3 09/18] fm10k: Eliminate duplicate barriers on weakly-ordered archs
Quoted 10 lines · Alexander Duyck
Quoted 9 lines · Sinan Kaya
Code includes
wmb()followed bywritel().writel()already has a barrier on some architectures like arm64.This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing
writel()towritel_relaxed().
Quoted 6 lines
Quoted 1 line
Signed-off-by Sinan Kaya <[email protected]>You can update the writel call in
fm10k_tx_mapas well.Of the drivers updated in
drivers/net/ethernet/intel/*it looks like this is the only one that still requires any additional changes.
will do. thanks for the feedback.
Quoted 5 lines
Thanks.
- Alex
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v3 10/18 net: qla3xxx: Eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/net/ethernet/qlogic/qla3xxx.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/qlogic/qla3xxx.c +2 -2
@@ -1858,8 +1858,8 @@ static void ql_update_small_bufq_prod_index(struct ql3_adapter *qdev) qdev->small_buf_release_cnt -=8 ; }wmb (); -writel (qdev->small_buf_q_producer_index, - &port_regs->CommonRegs.rxSmallQProducerIndex); +writel_relaxed (qdev->small_buf_q_producer_index, + &port_regs->CommonRegs.rxSmallQProducerIndex); } }
Signature · 1 line
2.7.4
PATCH v3 11/18 qlcnic: Eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_hw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_hw.c +1 -1
@@ -478,7 +478,7 @@ irqreturn_t qlcnic_83xx_clear_legacy_intr(struct qlcnic_adapter *adapter)wmb ();/* clear the interrupt trigger control register */ -writel (0 , adapter->isr_int_vec); +writel_relaxed (0 , adapter->isr_int_vec); intr_val =readl (adapter->isr_int_vec); do { intr_val = readl(adapter->tgt_status_reg);
Signature · 1 line
2.7.4
RE: [PATCH v3 11/18] qlcnic: Eliminate duplicate barriers on weakly-ordered archs
Quoted 39 lines
-----Original Message-----
From: Sinan Kaya [mailto:okaya at codeaurora.org]
Sent: Friday, March 16, 2018 9:46 PM
To: netdev at vger.kernel.org; timur at codeaurora.org; sulrich at codeaurora.org
Cc: linux-arm-msm at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Sinan Kaya <[email protected]>; Patil, Harish <[email protected]>; Chopra, Manish <[email protected]>; Dept-GE Linux NIC Dev <Dept- GELinuxNICDev at cavium.com>; linux-kernel at vger.kernel.org
Subject: [PATCH v3 11/18] qlcnic: Eliminate duplicate barriers on weakly-ordered archsCode includes
wmb()followed bywritel().writel()already has a barrier on some architectures like arm64.This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing
writel()towritel_relaxed().Signed-off-by Sinan Kaya <[email protected]>--- drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_hw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_hw.c +1
-1@@ -478,7 +478,7 @@ irqreturn_t qlcnic_83xx_clear_legacy_intr(struct qlcnic_adapter *adapter)wmb ();/* clear the interrupt trigger control register */ -writel (0 , adapter->isr_int_vec); +writel_relaxed (0 , adapter->isr_int_vec); intr_val =readl (adapter->isr_int_vec); do {
Quoted 3 lines
intr_val=readl(adapter->tgt_status_reg);
--
2.7.4
Thanks.
PATCH v3 12/18 bnx2x: Eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c +1 -1
@@ -170,7 +170,7 @@ static int bnx2x_send_msg2pf(struct bnx2x *bp, u8 *done, dma_addr_t msg_mapping)wmb ();/* Trigger the PF FW */ -writeb (1 , &zone_data->trigger.vf_pf_channel.addr_valid); +writeb_relaxed (1 , &zone_data->trigger.vf_pf_channel.addr_valid);/* Wait for PF to complete */ while ((tout >=0 ) && (!*done)) {
Signature · 1 line
2.7.4
PATCH v3 13/18 net: cxgb4/cxgb4vf: Eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Create a new wrapper function with relaxed write operator. Use the new wrapper when a write is following a wmb().
--- drivers/net/ethernet/chelsio/cxgb4/cxgb4.h | 6 ++++++ drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 13 +++++++------ drivers/net/ethernet/chelsio/cxgb4/sge.c | 8 ++++---- drivers/net/ethernet/chelsio/cxgb4/t4_hw.c | 2 +- drivers/net/ethernet/chelsio/cxgb4vf/adapter.h | 14 ++++++++++++++ drivers/net/ethernet/chelsio/cxgb4vf/sge.c | 16 +++++++++------- 6 files changed, 41 insertions(+), 18 deletions(-)
drivers/net/ethernet/chelsio/cxgb4/cxgb4.h +6 -0
@@ -1202,6 +1202,12 @@ static inline void t4_write_reg(struct adapter *adap, u32 reg_addr, u32 val)writel (val, adap->regs + reg_addr); } +static inline void t4_write_reg_relaxed (struct adapter *adap,u32 reg_addr, +u32 val) +{ +writel_relaxed (val, adap->regs + reg_addr); +} +#ifndef readqstatic inline u64 readq (const volatile void __iomem *addr) {
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c +7 -6
@@ -1723,8 +1723,8 @@ int cxgb4_sync_txq_pidx(struct net_device *dev, u16 qid, u16 pidx,else val =PIDX_T5_V (delta);wmb (); -t4_write_reg (adap,MYPF_REG (SGE_PF_KDOORBELL_A ), -QID_V (qid) | val); +t4_write_reg_relaxed (adap,MYPF_REG (SGE_PF_KDOORBELL_A ), +QID_V (qid) | val); } out:return ret;
@@ -1902,8 +1902,9 @@ static void enable_txq_db(struct adapter *adap, struct sge_txq *q)* are committed before we tell HW about them. */ wmb (); -t4_write_reg (adap,MYPF_REG (SGE_PF_KDOORBELL_A ), -QID_V (q->cntxt_id) |PIDX_V (q->db_pidx_inc)); +t4_write_reg_relaxed (adap,MYPF_REG (SGE_PF_KDOORBELL_A ), +QID_V (q->cntxt_id) | +PIDX_V (q->db_pidx_inc)); q->db_pidx_inc =0 ; } q->db_disabled =0 ;
@@ -2003,8 +2004,8 @@ static void sync_txq_pidx(struct adapter *adap, struct sge_txq *q)else val =PIDX_T5_V (delta);wmb (); -t4_write_reg (adap,MYPF_REG (SGE_PF_KDOORBELL_A ), -QID_V (q->cntxt_id) | val); +t4_write_reg_relaxed (adap,MYPF_REG (SGE_PF_KDOORBELL_A ), +QID_V (q->cntxt_id) | val); } out: q->db_disabled =0 ;
drivers/net/ethernet/chelsio/cxgb4/sge.c +4 -4
@@ -530,11 +530,11 @@ static inline void ring_fl_db(struct adapter *adap, struct sge_fl *q)* mechanism. */ if (unlikely (q->bar2_addr ==NULL )) { -t4_write_reg (adap,MYPF_REG (SGE_PF_KDOORBELL_A ), - val |QID_V (q->cntxt_id)); +t4_write_reg_relaxed (adap,MYPF_REG (SGE_PF_KDOORBELL_A ), + val |QID_V (q->cntxt_id)); }else { -writel (val |QID_V (q->bar2_qid), - q->bar2_addr +SGE_UDB_KDOORBELL ); +writel_relaxed (val |QID_V (q->bar2_qid), + q->bar2_addr +SGE_UDB_KDOORBELL );/* This Write memory Barrier will force the write to * the User Doorbell area to be flushed.
drivers/net/ethernet/chelsio/cxgb4/t4_hw.c +1 -1
@@ -139,7 +139,7 @@ void t4_write_indirect(struct adapter *adap, unsigned int addr_reg, {while (nregs--) {t4_write_reg (adap, addr_reg, start_idx++); -t4_write_reg (adap, data_reg, *vals++); +t4_write_reg_relaxed (adap, data_reg, *vals++); } }
drivers/net/ethernet/chelsio/cxgb4vf/adapter.h +14 -0
@@ -442,6 +442,20 @@ static inline void t4_write_reg(struct adapter *adapter, u32 reg_addr, u32 val)writel (val, adapter->regs + reg_addr); } +/** +* t4_write_reg_relaxed - write a HW register without ordering guarantees +* @adapter: the adapter +* @reg_addr: the register address +* @val: the value to write +* +* Write a 32-bit value into the given HW register. +*/ +static inline void t4_write_reg_relaxed (struct adapter *adapter,u32 reg_addr, +u32 val) +{ +writel_relaxed (val, adapter->regs + reg_addr); +} +#ifndef readqstatic inline u64 readq (const volatile void __iomem *addr) {
drivers/net/ethernet/chelsio/cxgb4vf/sge.c +9 -7
@@ -546,12 +546,13 @@ static inline void ring_fl_db(struct adapter *adapter, struct sge_fl *fl)* mechanism. */ if (unlikely (fl->bar2_addr ==NULL )) { -t4_write_reg (adapter, -T4VF_SGE_BASE_ADDR +SGE_VF_KDOORBELL , -QID_V (fl->cntxt_id) | val); +t4_write_reg_relaxed (adapter, +T4VF_SGE_BASE_ADDR + +SGE_VF_KDOORBELL , +QID_V (fl->cntxt_id) | val); }else { -writel (val |QID_V (fl->bar2_qid), - fl->bar2_addr +SGE_UDB_KDOORBELL ); +writel_relaxed (val |QID_V (fl->bar2_qid), + fl->bar2_addr +SGE_UDB_KDOORBELL );/* This Write memory Barrier will force the write to * the User Doorbell area to be flushed.
@@ -980,8 +981,9 @@ static inline void ring_tx_db(struct adapter *adapter, struct sge_txq *tq,if (unlikely (tq->bar2_addr ==NULL )) {u32 val =PIDX_V (n); -t4_write_reg (adapter,T4VF_SGE_BASE_ADDR +SGE_VF_KDOORBELL , -QID_V (tq->cntxt_id) | val); +t4_write_reg_relaxed (adapter, +T4VF_SGE_BASE_ADDR +SGE_VF_KDOORBELL , +QID_V (tq->cntxt_id) | val); }else {u32 val =PIDX_T5_V (n);
Signature · 1 line
2.7.4
PATCH v3 14/18 net: cxgb3: Eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Create a new wrapper function with relaxed write operator. Use the new wrapper when a write is following a wmb().
--- drivers/net/ethernet/chelsio/cxgb3/adapter.h | 7 +++++++ drivers/net/ethernet/chelsio/cxgb3/sge.c | 19 ++++++++++--------- 2 files changed, 17 insertions(+), 9 deletions(-)
drivers/net/ethernet/chelsio/cxgb3/adapter.h +7 -0
@@ -281,6 +281,13 @@ static inline void t3_write_reg(struct adapter *adapter, u32 reg_addr, u32 val)writel (val, adapter->regs + reg_addr); } +static inline void t3_write_reg_relaxed (struct adapter *adapter,u32 reg_addr, +u32 val) +{ +CH_DBG (adapter,MMIO ,"setting register 0x%x to 0x%x\n" , reg_addr, val); +writel_relaxed (val, adapter->regs + reg_addr); +} +static inline struct port_info *adap2pinfo (struct adapter *adap,int idx) {return netdev_priv (adap->port[idx]);
drivers/net/ethernet/chelsio/cxgb3/sge.c +10 -9
@@ -487,7 +487,8 @@ static inline void ring_fl_db(struct adapter *adap, struct sge_fl *q)if (q->pend_cred >= q->credits /4 ) { q->pend_cred =0 ;wmb (); -t3_write_reg (adap,A_SG_KDOORBELL ,V_EGRCNTX (q->cntxt_id)); +t3_write_reg_relaxed (adap,A_SG_KDOORBELL , +V_EGRCNTX (q->cntxt_id)); } }
@@ -1058,8 +1059,8 @@ static inline void check_ring_tx_db(struct adapter *adap, struct sge_txq *q) }#else wmb ();/* write descriptors before telling HW */ -t3_write_reg (adap,A_SG_KDOORBELL , -F_SELEGRCNTX |V_EGRCNTX (q->cntxt_id)); +t3_write_reg_relaxed (adap,A_SG_KDOORBELL , +F_SELEGRCNTX |V_EGRCNTX (q->cntxt_id));#endif }
@@ -1510,8 +1511,8 @@ static int ctrl_xmit(struct adapter *adap, struct sge_txq *q, }spin_unlock (&q->lock);wmb (); -t3_write_reg (adap,A_SG_KDOORBELL , -F_SELEGRCNTX |V_EGRCNTX (q->cntxt_id)); +t3_write_reg_relaxed (adap,A_SG_KDOORBELL , +F_SELEGRCNTX |V_EGRCNTX (q->cntxt_id));return NET_XMIT_SUCCESS ; }
@@ -1554,8 +1555,8 @@ static void restart_ctrlq(unsigned long data)spin_unlock (&q->lock);wmb (); -t3_write_reg (qs->adap,A_SG_KDOORBELL , -F_SELEGRCNTX |V_EGRCNTX (q->cntxt_id)); +t3_write_reg_relaxed (qs->adap,A_SG_KDOORBELL , +F_SELEGRCNTX |V_EGRCNTX (q->cntxt_id)); }/*
@@ -1793,8 +1794,8 @@ again: reclaim_completed_tx(adap, q, TX_RECLAIM_CHUNK);#endif wmb ();if (likely (written)) -t3_write_reg (adap,A_SG_KDOORBELL , -F_SELEGRCNTX |V_EGRCNTX (q->cntxt_id)); +t3_write_reg_relaxed (adap,A_SG_KDOORBELL , +F_SELEGRCNTX |V_EGRCNTX (q->cntxt_id)); }/**
Signature · 1 line
2.7.4
PATCH v3 15/18 RDMA/bnxt_re: Eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/infiniband/hw/bnxt_re/qplib_rcfw.c +4 -4
@@ -181,10 +181,10 @@ static int __send_message(struct bnxt_qplib_rcfw *rcfw, struct cmdq_base *req,/* ring CMDQ DB */ wmb (); -writel (cmdq_prod, rcfw->cmdq_bar_reg_iomem + - rcfw->cmdq_bar_reg_prod_off); -writel (RCFW_CMDQ_TRIG_VAL , rcfw->cmdq_bar_reg_iomem + - rcfw->cmdq_bar_reg_trig_off); +writel_relaxed (cmdq_prod, rcfw->cmdq_bar_reg_iomem + + rcfw->cmdq_bar_reg_prod_off); +writel_relaxed (RCFW_CMDQ_TRIG_VAL , rcfw->cmdq_bar_reg_iomem + + rcfw->cmdq_bar_reg_trig_off); done:spin_unlock_irqrestore (&cmdq->lock, flags);/* Return the CREQ response pointer */
Signature · 1 line
2.7.4
PATCH v3 16/18 IB/mlx4: Eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/infiniband/hw/mlx4/qp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/infiniband/hw/mlx4/qp.c +2 -2
@@ -3880,8 +3880,8 @@ int mlx4_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,*/ wmb (); -writel (qp->doorbell_qpn, -to_mdev (ibqp->device)->uar_map +MLX4_SEND_DOORBELL ); +writel_relaxed (qp->doorbell_qpn, +to_mdev (ibqp->device)->uar_map +MLX4_SEND_DOORBELL );/* * Make sure doorbells don't leak out of SQ spinlock
Signature · 1 line
2.7.4
PATCH v3 17/18 RDMA/i40iw: Eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Create a new wrapper function with relaxed write operator. Use the new wrapper when a write is following a wmb().
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/infiniband/hw/i40iw/i40iw_ctrl.c | 6 ++++-- drivers/infiniband/hw/i40iw/i40iw_osdep.h | 1 + drivers/infiniband/hw/i40iw/i40iw_uk.c | 2 +- drivers/infiniband/hw/i40iw/i40iw_utils.c | 11 +++++++++++ 4 files changed, 17 insertions(+), 3 deletions(-)
drivers/infiniband/hw/i40iw/i40iw_ctrl.c +4 -2
@@ -706,9 +706,11 @@ static void i40iw_sc_ccq_arm(struct i40iw_sc_cq *ccq)wmb ();/* make sure shadow area is updated before arming */ if (ccq->dev->is_pf) -i40iw_wr32 (ccq->dev->hw,I40E_PFPE_CQARM , ccq->cq_uk.cq_id); +i40iw_wr32_relaxed (ccq->dev->hw,I40E_PFPE_CQARM , + ccq->cq_uk.cq_id);else -i40iw_wr32 (ccq->dev->hw,I40E_VFPE_CQARM1 , ccq->cq_uk.cq_id); +i40iw_wr32_relaxed (ccq->dev->hw,I40E_VFPE_CQARM1 , + ccq->cq_uk.cq_id); }/**
drivers/infiniband/hw/i40iw/i40iw_osdep.h +1 -0
@@ -213,5 +213,6 @@ void i40iw_hw_stats_start_timer(struct i40iw_sc_vsi *vsi);void i40iw_hw_stats_stop_timer (struct i40iw_sc_vsi *vsi);#define i40iw_mmiowb () mmiowb()void i40iw_wr32 (struct i40iw_hw *hw,u32 reg,u32 value); +void i40iw_wr32_relaxed (struct i40iw_hw *hw,u32 reg,u32 value);u32 i40iw_rd32 (struct i40iw_hw *hw,u32 reg);#endif /* _I40IW_OSDEP_H_ */
drivers/infiniband/hw/i40iw/i40iw_uk.c +1 -1
@@ -723,7 +723,7 @@ static void i40iw_cq_request_notification(struct i40iw_cq_uk *cq,wmb ();/* make sure WQE is populated before valid bit is set */ -writel (cq->cq_id, cq->cqe_alloc_reg); +writel_relaxed (cq->cq_id, cq->cqe_alloc_reg); }/**
drivers/infiniband/hw/i40iw/i40iw_utils.c +11 -0
@@ -125,6 +125,17 @@ inline void i40iw_wr32(struct i40iw_hw *hw, u32 reg, u32 value) }/** +* i40iw_wr32_relaxed - write 32 bits to hw register without ordering +* @hw: hardware information including registers +* @reg: register offset +* @value: vvalue to write to register +*/ +inline void i40iw_wr32_relaxed (struct i40iw_hw *hw,u32 reg,u32 value) +{ +writel_relaxed (value, hw->hw_addr + reg); +} + +/** * i40iw_rd32 - read a 32 bit hw register * @hw: hardware information including registers * @reg: register offset
Signature · 1 line
2.7.4
PATCH v3 18/18 infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaCode includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing writel() to writel_relaxed().
--- drivers/infiniband/hw/cxgb4/t4.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/infiniband/hw/cxgb4/t4.h +6 -6
@@ -477,15 +477,15 @@ static inline void t4_ring_sq_db(struct t4_wq *wq, u16 inc, union t4_wr *wqe) (u64 *)wqe); }else {pr_debug ("DB wq->sq.pidx = %d\n" , wq->sq.pidx); -writel (PIDX_T5_V (inc) |QID_V (wq->sq.bar2_qid), - wq->sq.bar2_va +SGE_UDB_KDOORBELL ); +writel_relaxed (PIDX_T5_V (inc) |QID_V (wq->sq.bar2_qid), + wq->sq.bar2_va +SGE_UDB_KDOORBELL ); }/* Flush user doorbell area writes. */ wmb ();return ; } -writel (QID_V (wq->sq.qid) |PIDX_V (inc), wq->db); +writel_relaxed (QID_V (wq->sq.qid) |PIDX_V (inc), wq->db); } static inline void t4_ring_rq_db(struct t4_wq *wq, u16 inc,
@@ -502,15 +502,15 @@ static inline void t4_ring_rq_db(struct t4_wq *wq, u16 inc, (void *)wqe); }else {pr_debug ("DB wq->rq.pidx = %d\n" , wq->rq.pidx); -writel (PIDX_T5_V (inc) |QID_V (wq->rq.bar2_qid), - wq->rq.bar2_va +SGE_UDB_KDOORBELL ); +writel_relaxed (PIDX_T5_V (inc) |QID_V (wq->rq.bar2_qid), + wq->rq.bar2_va +SGE_UDB_KDOORBELL ); }/* Flush user doorbell area writes. */ wmb ();return ; } -writel (QID_V (wq->rq.qid) |PIDX_V (inc), wq->db); +writel_relaxed (QID_V (wq->rq.qid) |PIDX_V (inc), wq->db); }static inline int t4_wq_in_error (struct t4_wq *wq)
Signature · 1 line
2.7.4
Steve Wise
NAKRE: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
Quoted 1 line
Code includes
wmb()followed bywritel().writel()already has a barrier
on
Quoted 9 lines
some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing
writel()towritel_relaxed().Signed-off-by Sinan Kaya <[email protected]>
NAK - This isn't correct for PowerPC. For PowerPC, writeX_relaxed() is just writeX().
I was just looking at this with Chelsio developers, and they said the writeX() should be replaced with __raw_writeX(), not writeX_relaxed(), to get rid of the extra barrier for all architectures.
Also, t4.h:pio_copy() needs to use __raw_writeq() to enable the write combining fastpath for ARM and PowerPC. The code as it stands doesn't achieve any write combining on PowerPC at least.
And the writel()s at the end of the ring functions (the non bar2 udb path) needs a mmiowb() afterwards if you're going to use __raw_writeX() there. However that path is only used for very old hardware (T4), so I wouldn't worry about them.
Steve.
Quoted 55 lines
--- drivers/infiniband/hw/cxgb4/t4.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)drivers/infiniband/hw/cxgb4/t4.h +3
-3@@ -477,15 +477,15 @@ static inline void t4_ring_sq_db(struct t4_wq *wq, u16 inc, union t4_wr *wqe) (u64 *)wqe); }else {pr_debug ("DB wq->sq.pidx = %d\n" , wq->sq.pidx); -writel (PIDX_T5_V (inc) |QID_V (wq->sq.bar2_qid), - wq->sq.bar2_va +SGE_UDB_KDOORBELL ); +writel_relaxed (PIDX_T5_V (inc) |QID_V (wq- >sq.bar2_qid), + wq->sq.bar2_va +SGE_UDB_KDOORBELL ); }/* Flush user doorbell area writes. */ wmb ();return ; } -writel (QID_V (wq->sq.qid) |PIDX_V (inc), wq->db); +writel_relaxed (QID_V (wq->sq.qid) |PIDX_V (inc), wq->db);}
static inline void t4_ring_rq_db(struct
t4_wq*wq, u16 inc,drivers/infiniband/hw/cxgb4/t4.h +3
-3@@ -502,15 +502,15 @@ static inline void t4_ring_rq_db(struct t4_wq *wq,u16 inc, (void *)wqe); }else {pr_debug ("DB wq->rq.pidx = %d\n" , wq->rq.pidx); -writel (PIDX_T5_V (inc) |QID_V (wq->rq.bar2_qid), - wq->rq.bar2_va +SGE_UDB_KDOORBELL ); +writel_relaxed (PIDX_T5_V (inc) |QID_V (wq- >rq.bar2_qid), + wq->rq.bar2_va +SGE_UDB_KDOORBELL ); }/* Flush user doorbell area writes. */ wmb ();return ; } -writel (QID_V (wq->rq.qid) |PIDX_V (inc), wq->db); +writel_relaxed (QID_V (wq->rq.qid) |PIDX_V (inc), wq->db);}
static inline int
t4_wq_in_error(struct t4_wq *wq)
--
2.7.4
Re: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
Quoted 10 lines · Steve Wise
Quoted 1 line
Code includes
wmb()followed bywritel().writel()already has a barrieron
Quoted 8 lines
some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Since code already has an explicit barrier call, changing
writel()towritel_relaxed().
Quoted 8 lines
Quoted 1 line
Signed-off-by Sinan Kaya <[email protected]>NAK - This isn't correct for PowerPC. For PowerPC,
writeX_relaxed()is justwriteX().I was just looking at this with Chelsio developers, and they said the
writeX()should be replaced with__raw_writeX(), notwriteX_relaxed(), to get rid of the extra barrier for all architectures.
OK. I can do that but isn't the problem at PowerPC adaptation?
/*
* We don't do relaxed operations yet, at least not with this semantic */
#define readb_relaxed(addr) readb(addr)
#define readw_relaxed(addr) readw(addr)
#define readl_relaxed(addr) readl(addr)
#define readq_relaxed(addr) readq(addr)
#define writeb_relaxed(v, addr) writeb(v, addr)
#define writew_relaxed(v, addr) writew(v, addr)
#define writel_relaxed(v, addr) writel(v, addr)
#define writeq_relaxed(v, addr) writeq(v, addr)
Why don't we fix the PowerPC's relaxed operators? Is that a bigger task?
Quoted 1 line
From API perspective both
__raw_writeX()andwriteX_relaxed()are correct.
It is just PowerPC doesn't seem the follow the definition yet.
RE: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
Quoted 36 lines · Steve Wise
Quoted 5 lines
Quoted 1 line
Code includes
wmb()followed bywritel().writel()already has a barrieron
Quoted 3 lines
some architectures like arm64.
This ends up CPU observing two barriers back to back before executing
the
Quoted 5 lines
Quoted 5 lines
register write.
Since code already has an explicit barrier call, changing
writel()towritel_relaxed().Quoted 8 lines
Quoted 1 line
Signed-off-by Sinan Kaya <[email protected]>NAK - This isn't correct for PowerPC. For PowerPC,
writeX_relaxed()is justwriteX().I was just looking at this with Chelsio developers, and they said the
writeX()should be replaced with__raw_writeX(), notwriteX_relaxed(), to get rid of the extra barrier for all architectures.OK. I can do that but isn't the problem at PowerPC adaptation?
/*
* We don't do relaxed operations yet, at least not with this semantic */
#definereadb_relaxed(addr)readb(addr)
#definereadw_relaxed(addr)readw(addr)
#definereadl_relaxed(addr)readl(addr)
#definereadq_relaxed(addr)readq(addr)
#definewriteb_relaxed(v, addr)writeb(v, addr)
#definewritew_relaxed(v, addr)writew(v, addr)
#definewritel_relaxed(v, addr)writel(v, addr)
#definewriteq_relaxed(v, addr)writeq(v, addr)
Quoted 1 line
Why don't we fix the PowerPC's relaxed operators? Is that a bigger task?
I don't know the answer, but perhaps the proper fix is to correctly implement these for PPC?
Quoted 4 lines
Quoted 2 lines
From API perspective both
__raw_writeX()andwriteX_relaxed()arecorrect.
It is just PowerPC doesn't seem the follow the definition yet.
Re: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
Quoted 36 lines · Steve Wise
Quoted 36 lines · Steve Wise
Quoted 5 lines
Quoted 1 line
Code includes
wmb()followed bywritel().writel()already has a barrieron
Quoted 3 lines
some architectures like arm64.
This ends up CPU observing two barriers back to back before executing
the
Quoted 5 lines
Quoted 5 lines
register write.
Since code already has an explicit barrier call, changing
writel()towritel_relaxed().Quoted 8 lines
Quoted 1 line
Signed-off-by Sinan Kaya <[email protected]>NAK - This isn't correct for PowerPC. For PowerPC,
writeX_relaxed()is justwriteX().I was just looking at this with Chelsio developers, and they said the
writeX()should be replaced with__raw_writeX(), notwriteX_relaxed(), to get rid of the extra barrier for all architectures.OK. I can do that but isn't the problem at PowerPC adaptation?
/*
* We don't do relaxed operations yet, at least not with this semantic */
#definereadb_relaxed(addr)readb(addr)
#definereadw_relaxed(addr)readw(addr)
#definereadl_relaxed(addr)readl(addr)
#definereadq_relaxed(addr)readq(addr)
#definewriteb_relaxed(v, addr)writeb(v, addr)
#definewritew_relaxed(v, addr)writew(v, addr)
#definewritel_relaxed(v, addr)writel(v, addr)
#definewriteq_relaxed(v, addr)writeq(v, addr)
Quoted 4 lines
Quoted 1 line
Why don't we fix the PowerPC's relaxed operators? Is that a bigger task?
I don't know the answer, but perhaps the proper fix is to correctly implement these for PPC?
I found this article: https://lwn.net/Articles/698014/#PowerPC%20Implementation
Apparently, this issue was discussed at a conference in 2015.
Based on how I read this article, writel() and writel_relaxed() behave exactly the same on PowerPC because the barrier is enforced by the time code is leaving a critical section not during MMIO execution.
I also see that __raw_writel() is being heavily used in drivers directory.
https://elixir.bootlin.com/linux/latest/ident/__raw_writel
I'll change writel_relaxed() with __raw_writel() in the series like you suggested and also look at your other comments.
This seems to be the current norm.
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines
From API perspective both
__raw_writeX()andwriteX_relaxed()arecorrect.
It is just PowerPC doesn't seem the follow the definition yet.
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
Quoted 2 lines · Sinan Kaya
I'll change
writel_relaxed()with__raw_writel()in the series like you suggested and also look at your other comments.
I spoke too soon.
Now that I realized, code needs to follow one of the following patterns for correctness
1)wmb()
writel()/writel_relaxed()
or
2)wmb()__raw_wrltel()mmiowb()
but definitely not
wmb()__raw_wrltel()
Since #1 == #2, I'll stick to my current implementation of writel_relaxed()
Changing writel() to writel_relaxed() or __raw_writel() isn't enough. PowerPC needs mmiowb() for correctness. ARM's mmiowb() implementation is empty.
So, there is no one size fits all solution with the current state of affairs.
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
Quoted 24 lines · Sinan Kaya
Quoted 2 lines · Sinan Kaya
I'll change
writel_relaxed()with__raw_writel()in the series like you suggested and also look at your other comments.I spoke too soon.
Now that I realized, code needs to follow one of the following patterns for correctness
1)
wmb()
writel()/writel_relaxed()or
2)
wmb()__raw_wrltel()mmiowb()but definitely not
wmb()__raw_wrltel()
Quoted 8 lines
Since #1 == #2, I'll stick to my current implementation of
writel_relaxed()Changing
writel()towritel_relaxed()or__raw_writel()isn't enough. PowerPC needsmmiowb()for correctness. ARM'smmiowb()implementation is empty.So, there is no one size fits all solution with the current state of affairs.
I think I finally got what you mean.
Code seems to have
wmb()
writel()/writeq()wmb()
this can be safely replaced with
wmb()
__raw_writel()/__raw_writeq()wmb()
This will work on all arches. Below is the new version. Let me know if this is OK.
+++ b/drivers/infiniband/hw/cxgb4/t4.h
@@ -457,7 +457,7 @@ static inline void pio_copy(u64 __iomem *dst, u64 *src)
int count = 8;
while (count) {
- writeq(*src, dst);
+ __raw_writeq(*src, dst);
src++;
dst++;
count--;
@@ -477,15 +477,16 @@ static inline void t4_ring_sq_db(struct t4_wq *wq, u16 inc, union t4_wr *wqe)
(u64 *)wqe);
} else {
pr_debug("DB wq->sq.pidx = %d\n", wq->sq.pidx);
- writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid),
- wq->sq.bar2_va + SGE_UDB_KDOORBELL);
+ __raw_writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid),
+ wq->sq.bar2_va + SGE_UDB_KDOORBELL);
}
/* Flush user doorbell area writes. */
wmb();
return;
}
- writel(QID_V(wq->sq.qid) | PIDX_V(inc), wq->db);
+ __raw_writel(QID_V(wq->sq.qid) | PIDX_V(inc), wq->db);
+ mmiowmb()}
static inline void t4_ring_rq_db(struct t4_wq *wq, u16 inc,
@@ -502,15 +503,16 @@ static inline void t4_ring_rq_db(struct t4_wq *wq, u16 inc,
(void *)wqe);
} else {
pr_debug("DB wq->rq.pidx = %d\n", wq->rq.pidx);
- writel(PIDX_T5_V(inc) | QID_V(wq->rq.bar2_qid),
- wq->rq.bar2_va + SGE_UDB_KDOORBELL);
+ __raw_writel(PIDX_T5_V(inc) | QID_V(wq->rq.bar2_qid),
+ wq->rq.bar2_va + SGE_UDB_KDOORBELL);
}
/* Flush user doorbell area writes. */
wmb();
return;
}
- writel(QID_V(wq->rq.qid) | PIDX_V(inc), wq->db);
+ __raw_writel(QID_V(wq->rq.qid) | PIDX_V(inc), wq->db);
+ mmiowmb();}
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
Review static inline void t4_ring_sq_db(struct t4_wq *wq, u16 inc, union t4_wr *wqe) · line 480
Quoted 7 lines · Sinan Kaya
(quoted diff) +1
-2@@ -477,15 +477,16 @@ static inline void t4_ring_sq_db(struct t4_wq *wq, u16 inc, union t4_wr *wqe) (u64 *)wqe); } else { pr_debug("DB wq->sq.pidx = %d\n", wq->sq.pidx); - writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid), - wq->sq.bar2_va + SGE_UDB_KDOORBELL); + __raw_writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid),
I think Jason might be right that drivers shouldn't be calling the __raw_write functions. You definitely need to run this by the PPC developers, specifically Ben Herrenschmidt.
Signature · 3 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Steve Wise
Reviewed-byRE: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaQuoted 98 lines · Sinan Kaya
Quoted 2 lines · Sinan Kaya
Quoted 1 line
I'll change
writel_relaxed()with__raw_writel()in the series like yousuggested
Quoted 5 lines
Quoted 1 line
and also look at your other comments.
I spoke too soon.
Now that I realized, code needs to follow one of the following patterns
for correctness
Quoted 12 lines
1)
wmb()
writel()/writel_relaxed()or
2)
wmb()__raw_wrltel()mmiowb()Quoted 8 lines
but definitely not
wmb()__raw_wrltel()Since #1 == #2, I'll stick to my current implementation of
writel_relaxed()Changing
writel()towritel_relaxed()or__raw_writel()isn't enough.PowerPC needs
mmiowb()Quoted 5 lines
for correctness. ARM's
mmiowb()implementation is empty.So, there is no one size fits all solution with the current state of affairs.
I think I finally got what you mean.
Code seems to have
wmb()
writel()/writeq()wmb()this can be safely replaced with
wmb()
__raw_writel()/__raw_writeq()wmb()This will work on all arches. Below is the new version. Let me know if this is OK.
+++
b/drivers/infiniband/hw/cxgb4/t4.h(quoted diff) +1
-1@@ -457,7 +457,7 @@ static inline void pio_copy(u64 __iomem *dst, u64 *src) int count = 8; while (count) { - writeq(*src, dst); + __raw_writeq(*src, dst); src++; dst++;count--;(quoted diff) +4
-3@@ -477,15 +477,16 @@ static inline void t4_ring_sq_db(struct t4_wq *wq, u16 inc, union t4_wr *wqe) (u64 *)wqe); } else { pr_debug("DB wq->sq.pidx = %d\n", wq->sq.pidx); - writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid), - wq->sq.bar2_va + SGE_UDB_KDOORBELL); + __raw_writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid), + wq->sq.bar2_va + SGE_UDB_KDOORBELL); } /* Flush user doorbell area writes. */ wmb(); return; } - writel(QID_V(wq->sq.qid) | PIDX_V(inc), wq->db); + __raw_writel(QID_V(wq->sq.qid) | PIDX_V(inc), wq->db); + mmiowmb() }static inline void t4_ring_rq_db(struct
t4_wq*wq, u16 inc,(quoted diff) +2
-2@@ -502,15 +503,16 @@ static inline void t4_ring_rq_db(struct t4_wq *wq, u16 inc, (void *)wqe); } else { pr_debug("DB wq->rq.pidx = %d\n", wq->rq.pidx); - writel(PIDX_T5_V(inc) | QID_V(wq->rq.bar2_qid), - wq->rq.bar2_va + SGE_UDB_KDOORBELL); + __raw_writel(PIDX_T5_V(inc) | QID_V(wq->rq.bar2_qid), + wq->rq.bar2_va + SGE_UDB_KDOORBELL); }
Review static inline void t4_ring_rq_db(struct t4_wq *wq, · line 518
Quoted 10 lines
(quoted diff) +2
-1/* Flush user doorbell area writes. */ wmb(); return; } - writel(QID_V(wq->rq.qid) | PIDX_V(inc), wq->db); + __raw_writel(QID_V(wq->rq.qid) | PIDX_V(inc), wq->db); + mmiowmb(); }
Yes, this is what chelsio recommended to me.
David Miller
maintainerRe: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaFrom: Sinan Kaya <[email protected]>
Date: Sat, 17 Mar 2018 00:25:14 -0400
Quoted 8 lines
I think I finally got what you mean.
Code seems to have
wmb()
writel()/writeq()wmb()
Quoted 7 lines
this can be safely replaced with
wmb()
__raw_writel()/__raw_writeq()wmb()This will work on all arches. Below is the new version. Let me know if this is OK.
Unfortunately, I think this won't work.
At least on sparc, the __raw_*() variants also change the endianness to native endianness.
PowerPC does this as well, even documented in a comment :-)
/*
* Non ordered and non-swapping "raw" accessors
*/
Only the non-__raw_ variants do the endianness swap to/from little-endian.
Re: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
In reply to Sinan KayaQuoted 87 lines · Sinan Kaya
Quoted 24 lines · Sinan Kaya
Quoted 2 lines · Sinan Kaya
I'll change
writel_relaxed()with__raw_writel()in the series like you suggested and also look at your other comments.I spoke too soon.
Now that I realized, code needs to follow one of the following patterns for correctness
1)
wmb()
writel()/writel_relaxed()or
2)
wmb()__raw_wrltel()mmiowb()but definitely not
wmb()__raw_wrltel()Quoted 8 lines
Since #1 == #2, I'll stick to my current implementation of
writel_relaxed()Changing
writel()towritel_relaxed()or__raw_writel()isn't enough. PowerPC needsmmiowb()for correctness. ARM'smmiowb()implementation is empty.So, there is no one size fits all solution with the current state of affairs.
I think I finally got what you mean.
Code seems to have
wmb()
writel()/writeq()wmb()this can be safely replaced with
wmb()
__raw_writel()/__raw_writeq()wmb()This will work on all arches. Below is the new version. Let me know if this is OK.
+++
b/drivers/infiniband/hw/cxgb4/t4.h(quoted diff) +6
-6@@ -457,7 +457,7 @@ static inline void pio_copy(u64 __iomem *dst, u64 *src) int count = 8; while (count) { - writeq(*src, dst); + __raw_writeq(*src, dst); src++; dst++; count--;@@ -477,15 +477,16 @@ static inline void t4_ring_sq_db(struct t4_wq *wq, u16 inc, union t4_wr *wqe) (u64 *)wqe); } else { pr_debug("DB wq->sq.pidx = %d\n", wq->sq.pidx); - writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid), - wq->sq.bar2_va + SGE_UDB_KDOORBELL); + __raw_writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid), + wq->sq.bar2_va + SGE_UDB_KDOORBELL); } /* Flush user doorbell area writes. */ wmb(); return; } - writel(QID_V(wq->sq.qid) | PIDX_V(inc), wq->db); + __raw_writel(QID_V(wq->sq.qid) | PIDX_V(inc), wq->db); + mmiowmb() } static inline void t4_ring_rq_db(struct t4_wq *wq, u16 inc,@@ -502,15 +503,16 @@ static inline void t4_ring_rq_db(struct t4_wq *wq, u16 inc, (void *)wqe); } else { pr_debug("DB wq->rq.pidx = %d\n", wq->rq.pidx); - writel(PIDX_T5_V(inc) | QID_V(wq->rq.bar2_qid), - wq->rq.bar2_va + SGE_UDB_KDOORBELL); + __raw_writel(PIDX_T5_V(inc) | QID_V(wq->rq.bar2_qid),
Review static inline void t4_ring_rq_db(struct t4_wq *wq, u16 inc, · line 515
Quoted 10 lines
(quoted diff) +3
-1+ wq->rq.bar2_va + SGE_UDB_KDOORBELL); } /* Flush user doorbell area writes. */ wmb(); return; } - writel(QID_V(wq->rq.qid) | PIDX_V(inc), wq->db); + __raw_writel(QID_V(wq->rq.qid) | PIDX_V(inc), wq->db); + mmiowmb();
No! NAK! Adding *new* barriers to use the *wrong* accessor is crazy!
Your first patch was right, replacing
wmb()
writel()
With wmb(); writel_relaxed() is fine, and helps ARM.
The problem with PPC has nothing to do with the writel, it is with the spinlock, and we can't improve it without adding some new infrastructure. Certainly don't hack around the problem by using __raw_writel in multi-arch drivers!
In userspace we settled on something like this as a pattern:
mmio_wc_spin_lock()
writel_wc_mmio()
mmio_wc_spin_unlock()
Where mmio_wc_spin_unlock incorporates the mmiowmb and is defined to fully contain all MMIO access to WC and UC memory.
Due to our userspace the pattern is specifically used with MMIO writes to WC BAR memory, but it could be generalized..
This allows all known arches to use the best code in all cases. x86 can even optimize a lot and combine the mmiowmb() into the spin_unlock, apparently.
Jason
Re: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
+linuxppc-dev at lists.ozlabs.org
Quoted 122 lines · Jason Gunthorpe
Quoted 87 lines · Sinan Kaya
Quoted 24 lines · Sinan Kaya
Quoted 2 lines · Sinan Kaya
I'll change
writel_relaxed()with__raw_writel()in the series like you suggested and also look at your other comments.I spoke too soon.
Now that I realized, code needs to follow one of the following patterns for correctness
1)
wmb()
writel()/writel_relaxed()or
2)
wmb()__raw_wrltel()mmiowb()but definitely not
wmb()__raw_wrltel()Quoted 8 lines
Since #1 == #2, I'll stick to my current implementation of
writel_relaxed()Changing
writel()towritel_relaxed()or__raw_writel()isn't enough. PowerPC needsmmiowb()for correctness. ARM'smmiowb()implementation is empty.So, there is no one size fits all solution with the current state of affairs.
I think I finally got what you mean.
Code seems to have
wmb()
writel()/writeq()wmb()this can be safely replaced with
wmb()
__raw_writel()/__raw_writeq()wmb()This will work on all arches. Below is the new version. Let me know if this is OK.
+++
b/drivers/infiniband/hw/cxgb4/t4.h(quoted diff) +6
-6@@ -457,7 +457,7 @@ static inline void pio_copy(u64 __iomem *dst, u64 *src) int count = 8; while (count) { - writeq(*src, dst); + __raw_writeq(*src, dst); src++; dst++; count--;@@ -477,15 +477,16 @@ static inline void t4_ring_sq_db(struct t4_wq *wq, u16 inc, union t4_wr *wqe) (u64 *)wqe); } else { pr_debug("DB wq->sq.pidx = %d\n", wq->sq.pidx); - writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid), - wq->sq.bar2_va + SGE_UDB_KDOORBELL); + __raw_writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid), + wq->sq.bar2_va + SGE_UDB_KDOORBELL); } /* Flush user doorbell area writes. */ wmb(); return; } - writel(QID_V(wq->sq.qid) | PIDX_V(inc), wq->db); + __raw_writel(QID_V(wq->sq.qid) | PIDX_V(inc), wq->db); + mmiowmb() } static inline void t4_ring_rq_db(struct t4_wq *wq, u16 inc,@@ -502,15 +503,16 @@ static inline void t4_ring_rq_db(struct t4_wq *wq, u16 inc, (void *)wqe); } else { pr_debug("DB wq->rq.pidx = %d\n", wq->rq.pidx); - writel(PIDX_T5_V(inc) | QID_V(wq->rq.bar2_qid), - wq->rq.bar2_va + SGE_UDB_KDOORBELL); + __raw_writel(PIDX_T5_V(inc) | QID_V(wq->rq.bar2_qid),Review static inline void t4_ring_rq_db(struct t4_wq *wq, u16 inc, · line 515
Quoted 10 lines
(quoted diff) +3
-1+ wq->rq.bar2_va + SGE_UDB_KDOORBELL); } /* Flush user doorbell area writes. */ wmb(); return; } - writel(QID_V(wq->rq.qid) | PIDX_V(inc), wq->db); + __raw_writel(QID_V(wq->rq.qid) | PIDX_V(inc), wq->db); + mmiowmb();No! NAK! Adding *new* barriers to use the *wrong* accessor is crazy!
Your first patch was right, replacing
wmb()
writel()With
wmb();writel_relaxed()is fine, and helps ARM.The problem with PPC has nothing to do with the writel, it is with the spinlock, and we can't improve it without adding some new infrastructure. Certainly don't hack around the problem by using
__raw_writelin multi-arch drivers!In userspace we settled on something like this as a pattern:
mmio_wc_spin_lock()
writel_wc_mmio()
mmio_wc_spin_unlock()Where
mmio_wc_spin_unlockincorporates the mmiowmb and is defined to fully contain all MMIO access to WC and UC memory.
Quoted 6 lines
Due to our userspace the pattern is specifically used with MMIO writes to WC BAR memory, but it could be generalized..
This allows all known arches to use the best code in all cases. x86 can even optimize a lot and combine the
mmiowmb()into thespin_unlock, apparently.
Given both Dave [1] and Jason's feedback, we have to ask PowerPC developers to fix writel_relaxed().
Otherwise, PowerPC won't be able to take advantage of the optimizations introduced in this series.
I don't think we need writel_really_relaxed() API.
Somebody also has to take a task and work very hard to get rid of __raw_writeX() APIs in drivers/net directory. It looked like a very common practice though it clearly violates multiarch portability concerns Jason and Deve highlighted.
This will be the next version:
iff --git a/drivers/infiniband/hw/cxgb4/t4.h b/drivers/infiniband/hw/cxgb4/t4.h index 8369c7c..6e5658a 100644
drivers/infiniband/hw/cxgb4/t4.h +7 -7
@@ -457,7 +457,7 @@ static inline void pio_copy(u64 __iomem *dst, u64 *src)int count =8 ;while (count) { -writeq (*src, dst); +writeq_relaxed (*src, dst); src++; dst++; count--;
@@ -477,15 +477,15 @@ static inline void t4_ring_sq_db(struct t4_wq *wq, u16 inc, union t4_wr *wqe) (u64 *)wqe); }else {pr_debug ("DB wq->sq.pidx = %d\n" , wq->sq.pidx); -writel (PIDX_T5_V (inc) |QID_V (wq->sq.bar2_qid), - wq->sq.bar2_va +SGE_UDB_KDOORBELL ); +writel_relaxed (PIDX_T5_V (inc) |QID_V (wq->sq.bar2_qid), + wq->sq.bar2_va +SGE_UDB_KDOORBELL ); }/* Flush user doorbell area writes. */ wmb ();return ; } -writel (QID_V (wq->sq.qid) |PIDX_V (inc), wq->db); +writel_relaxed (QID_V (wq->sq.qid) |PIDX_V (inc), wq->db); } static inline void t4_ring_rq_db(struct t4_wq *wq, u16 inc,
@@ -502,15 +502,15 @@ static inline void t4_ring_rq_db(struct t4_wq *wq, u16 inc, (void *)wqe); }else {pr_debug ("DB wq->rq.pidx = %d\n" , wq->rq.pidx); -writel (PIDX_T5_V (inc) |QID_V (wq->rq.bar2_qid), - wq->rq.bar2_va +SGE_UDB_KDOORBELL ); +writel_relaxed (PIDX_T5_V (inc) |QID_V (wq->rq.bar2_qid), + wq->rq.bar2_va +SGE_UDB_KDOORBELL ); }/* Flush user doorbell area writes. */ wmb ();return ; } -writel (QID_V (wq->rq.qid) |PIDX_V (inc), wq->db); +writel_relaxed (QID_V (wq->rq.qid) |PIDX_V (inc), wq->db); }
[1] https://lkml.org/lkml/2018/3/17/100
Quoted 3 lines
Jason
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
Quoted 3 lines · Sinan Kaya
Somebody also has to take a task and work very hard to get rid of
__raw_writeX()APIs indrivers/netdirectory. It looked like a very common practice though it clearly violates multiarch portability concerns Jason and Deve highlighted.
When you posted your list I thought most of the hits were in what I'd think of 'one-arch drivers', eg an IRQ controller or clock driver or something.. Some might have a reason for it (eg avoiding the swap, for instance), maybe it is a hold over from before writel_relaxed, or maybe it is just a cargo-cult behavior..
It is the obviously multi-arch drivers that probably need some attention..
Jason
Re: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
In reply to Steve WiseQuoted 7 lines · Steve Wise
Quoted 1 line
Code includes
wmb()followed bywritel().writel()already has a barrieron
Quoted 5 lines
some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the register write.
Quoted 7 lines
Quoted 4 lines
Since code already has an explicit barrier call, changing
writel()towritel_relaxed().Signed-off-by Sinan Kaya <[email protected]>NAK - This isn't correct for PowerPC. For PowerPC,
writeX_relaxed()is justwriteX().
?? Why is changing writex() to writeX() a NAK then?
Quoted 3 lines
I was just looking at this with Chelsio developers, and they said the
writeX()should be replaced with__raw_writeX(), notwriteX_relaxed(), to get rid of the extra barrier for all architectures.
That doesn't seem semanticaly sane.
__raw_writeX() should not appear in driver code, IMHO. Only the arch code can know what the exact semantics of that accessor are..
If ppc can't use writel_relaxed to optimize then we probably need yet another io accessor semantic defined :(
Jason
RE: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
Quoted 3 lines · Steve Wise
Quoted 1 line
Quoted 1 line
Code includes
wmb()followed bywritel().writel()already has a
barrier
Quoted 13 lines
Quoted 4 lines
on
Quoted 3 lines
some architectures like arm64.
This ends up CPU observing two barriers back to back before executing
the
Quoted 8 lines
Quoted 6 lines
register write.
Since code already has an explicit barrier call, changing
writel()towritel_relaxed().Signed-off-by Sinan Kaya <[email protected]>NAK - This isn't correct for PowerPC. For PowerPC,
writeX_relaxed()is
just
Quoted 3 lines
Quoted 1 line
writeX().?? Why is changing
writex()towriteX()a NAK then?
Because I want it correct for PPC as well.
Quoted 3 lines
Quoted 3 lines
I was just looking at this with Chelsio developers, and they said the
writeX()should be replaced with__raw_writeX(), notwriteX_relaxed(),
to
Quoted 9 lines
Quoted 1 line
get rid of the extra barrier for all architectures.
That doesn't seem semanticaly sane.
__raw_writeX()should not appear in driver code, IMHO. Only the arch code can know what the exact semantics of that accessor are..If ppc can't use
writel_relaxedto optimize then we probably need yet another io accessor semantic defined :(
Anybody understand why the PPC implementation of writeX_relaxed() isn't relaxed?
Steve.
Re: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
Quoted 2 lines · Steve Wise
Anybody understand why the PPC implementation of
writeX_relaxed()isn't relaxed?
You probably should ask that on the linuxppc-dev at lists.ozlabs.org mailing list.
I've always wondered why PowerPC has non-standard I/O accessors.
Signature · 3 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.