| Message ID | 20250306163124.127473-1-eleanor15x@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] qlcnic: Optimize performance by replacing rw_lock with spinlock | expand |
On Fri, Mar 07, 2025 at 12:31:24AM +0800, Yu-Chun Lin wrote: > The 'crb_lock', an rwlock, is only used by writers, making it functionally > equivalent to a spinlock. > > According to Documentation/locking/spinlocks.rst: > > "Reader-writer locks require more atomic memory operations than simple > spinlocks. Unless the reader critical section is long, you are better > off just using spinlocks." > > Since read_lock() is never called, switching to a spinlock reduces > overhead and improves efficiency. > > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> Hi Yu-Chun Lin, Thanks for your patch. My main question is if you have hardware to test this? And if so, was a benefit observed? If not, my feeling is that although your change looks correct, we'd be better off taking the lower risk option of leaving things be.
On Fri, Mar 07, 2025 at 01:29:29PM +0000, Simon Horman wrote: > On Fri, Mar 07, 2025 at 12:31:24AM +0800, Yu-Chun Lin wrote: > > The 'crb_lock', an rwlock, is only used by writers, making it functionally > > equivalent to a spinlock. > > > > According to Documentation/locking/spinlocks.rst: > > > > "Reader-writer locks require more atomic memory operations than simple > > spinlocks. Unless the reader critical section is long, you are better > > off just using spinlocks." > > > > Since read_lock() is never called, switching to a spinlock reduces > > overhead and improves efficiency. > > > > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> > > Hi Yu-Chun Lin, > > Thanks for your patch. > > My main question is if you have hardware to test this? > And if so, was a benefit observed? > > If not, my feeling is that although your change looks > correct, we'd be better off taking the lower risk option > of leaving things be. Hi Simon I perform a compile test to ensure correctness. But I don't have the hardware to run a full test. Yu-Chun Lin
On Sun, Mar 09, 2025 at 12:35:29AM +0800, Yu-Chun Lin wrote: > On Fri, Mar 07, 2025 at 01:29:29PM +0000, Simon Horman wrote: > > On Fri, Mar 07, 2025 at 12:31:24AM +0800, Yu-Chun Lin wrote: > > > The 'crb_lock', an rwlock, is only used by writers, making it functionally > > > equivalent to a spinlock. > > > > > > According to Documentation/locking/spinlocks.rst: > > > > > > "Reader-writer locks require more atomic memory operations than simple > > > spinlocks. Unless the reader critical section is long, you are better > > > off just using spinlocks." > > > > > > Since read_lock() is never called, switching to a spinlock reduces > > > overhead and improves efficiency. > > > > > > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> > > > > Hi Yu-Chun Lin, > > > > Thanks for your patch. > > > > My main question is if you have hardware to test this? > > And if so, was a benefit observed? > > > > If not, my feeling is that although your change looks > > correct, we'd be better off taking the lower risk option > > of leaving things be. > > Hi Simon > > I perform a compile test to ensure correctness. But I don't have the > hardware to run a full test. Thanks Yu-Chun Lin, Unfortunately I think we need hardware testing to accept this kind of change.
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h b/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h index 3d0b5cd978cb..b8c8bc572042 100644 --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h @@ -470,7 +470,7 @@ struct qlcnic_hardware_context { unsigned long pci_len0; - rwlock_t crb_lock; + spinlock_t crb_lock; struct mutex mem_lock; u8 revision_id; diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hw.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hw.c index ae4ee0326ee1..7b9bd0938229 100644 --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hw.c +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hw.c @@ -1185,13 +1185,13 @@ int qlcnic_82xx_hw_write_wx_2M(struct qlcnic_adapter *adapter, ulong off, if (rv > 0) { /* indirect access */ - write_lock_irqsave(&adapter->ahw->crb_lock, flags); + spin_lock_irqsave(&adapter->ahw->crb_lock, flags); crb_win_lock(adapter); rv = qlcnic_pci_set_crbwindow_2M(adapter, off); if (!rv) writel(data, addr); crb_win_unlock(adapter); - write_unlock_irqrestore(&adapter->ahw->crb_lock, flags); + spin_unlock_irqrestore(&adapter->ahw->crb_lock, flags); return rv; } @@ -1216,12 +1216,12 @@ int qlcnic_82xx_hw_read_wx_2M(struct qlcnic_adapter *adapter, ulong off, if (rv > 0) { /* indirect access */ - write_lock_irqsave(&adapter->ahw->crb_lock, flags); + spin_lock_irqsave(&adapter->ahw->crb_lock, flags); crb_win_lock(adapter); if (!qlcnic_pci_set_crbwindow_2M(adapter, off)) data = readl(addr); crb_win_unlock(adapter); - write_unlock_irqrestore(&adapter->ahw->crb_lock, flags); + spin_unlock_irqrestore(&adapter->ahw->crb_lock, flags); return data; } diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c index eb69121df726..5389e441fdae 100644 --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c @@ -2508,7 +2508,7 @@ qlcnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent) else if (qlcnic_mac_learn == DRV_MAC_LEARN) adapter->drv_mac_learn = true; - rwlock_init(&adapter->ahw->crb_lock); + spin_lock_init(&adapter->ahw->crb_lock); mutex_init(&adapter->ahw->mem_lock); INIT_LIST_HEAD(&adapter->mac_list);
The 'crb_lock', an rwlock, is only used by writers, making it functionally equivalent to a spinlock. According to Documentation/locking/spinlocks.rst: "Reader-writer locks require more atomic memory operations than simple spinlocks. Unless the reader critical section is long, you are better off just using spinlocks." Since read_lock() is never called, switching to a spinlock reduces overhead and improves efficiency. Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> --- drivers/net/ethernet/qlogic/qlcnic/qlcnic.h | 2 +- drivers/net/ethernet/qlogic/qlcnic/qlcnic_hw.c | 8 ++++---- drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-)