| Message ID | 20190816063109.4699-1-jian-hong@endlessm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ | expand |
> From: Jian-Hong Pan [mailto:jian-hong@endlessm.com] > > There is a mass of jobs between spin lock and unlock in the hardware > IRQ which will occupy much time originally. To make system work more > efficiently, this patch moves the jobs to the soft IRQ (bottom half) to > reduce the time in hardware IRQ. > > Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com> > --- > drivers/net/wireless/realtek/rtw88/pci.c | 36 +++++++++++++++++++----- > 1 file changed, 29 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c > b/drivers/net/wireless/realtek/rtw88/pci.c > index 00ef229552d5..355606b167c6 100644 > --- a/drivers/net/wireless/realtek/rtw88/pci.c > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > @@ -866,12 +866,29 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq, > void *dev) > { > struct rtw_dev *rtwdev = dev; > struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv; > - u32 irq_status[4]; > + unsigned long flags; > > - spin_lock(&rtwpci->irq_lock); > + spin_lock_irqsave(&rtwpci->irq_lock, flags); > if (!rtwpci->irq_enabled) > goto out; > > + /* disable RTW PCI interrupt to avoid more interrupts before the end of > + * thread function > + */ > + rtw_pci_disable_interrupt(rtwdev, rtwpci); > +out: > + spin_unlock_irqrestore(&rtwpci->irq_lock, flags); > + > + return IRQ_WAKE_THREAD; > +} > + > +static irqreturn_t rtw_pci_interrupt_threadfn(int irq, void *dev) > +{ > + struct rtw_dev *rtwdev = dev; > + struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv; > + unsigned long flags; > + u32 irq_status[4]; > + > rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); > > if (irq_status[0] & IMR_MGNTDOK) > @@ -891,8 +908,11 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq, > void *dev) > if (irq_status[0] & IMR_ROK) > rtw_pci_rx_isr(rtwdev, rtwpci, RTW_RX_QUEUE_MPDU); > > -out: > - spin_unlock(&rtwpci->irq_lock); > + /* all of the jobs for this interrupt have been done */ > + spin_lock_irqsave(&rtwpci->irq_lock, flags); Shouldn't we protect the ISRs above? This patch could actually reduce the time of IRQ. But I think I need to further test it with PCI MSI interrupt. https://patchwork.kernel.org/patch/11081539/ Maybe we could drop the "rtw_pci_[enable/disable]_interrupt" when MSI Is enabled with this patch. > + if (rtw_flag_check(rtwdev, RTW_FLAG_RUNNING)) > + rtw_pci_enable_interrupt(rtwdev, rtwpci); > + spin_unlock_irqrestore(&rtwpci->irq_lock, flags); > > return IRQ_HANDLED; > } > @@ -1152,8 +1172,10 @@ static int rtw_pci_probe(struct pci_dev *pdev, > goto err_destroy_pci; > } > > - ret = request_irq(pdev->irq, &rtw_pci_interrupt_handler, > - IRQF_SHARED, KBUILD_MODNAME, rtwdev); > + ret = devm_request_threaded_irq(rtwdev->dev, pdev->irq, > + rtw_pci_interrupt_handler, > + rtw_pci_interrupt_threadfn, > + IRQF_SHARED, KBUILD_MODNAME, rtwdev); > if (ret) { > ieee80211_unregister_hw(hw); > goto err_destroy_pci; > @@ -1192,7 +1214,7 @@ static void rtw_pci_remove(struct pci_dev *pdev) > rtw_pci_disable_interrupt(rtwdev, rtwpci); > rtw_pci_destroy(rtwdev, pdev); > rtw_pci_declaim(rtwdev, pdev); > - free_irq(rtwpci->pdev->irq, rtwdev); > + devm_free_irq(rtwdev->dev, rtwpci->pdev->irq, rtwdev); > rtw_core_deinit(rtwdev); > ieee80211_free_hw(hw); > } > -- > 2.20.1 Yan-Hsuan
Hi, A few more questions below > > From: Jian-Hong Pan [mailto:jian-hong@endlessm.com] > > > > There is a mass of jobs between spin lock and unlock in the hardware > > IRQ which will occupy much time originally. To make system work more > > efficiently, this patch moves the jobs to the soft IRQ (bottom half) to > > reduce the time in hardware IRQ. > > > > Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com> > > --- > > drivers/net/wireless/realtek/rtw88/pci.c | 36 +++++++++++++++++++----- > > 1 file changed, 29 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c > > b/drivers/net/wireless/realtek/rtw88/pci.c > > index 00ef229552d5..355606b167c6 100644 > > --- a/drivers/net/wireless/realtek/rtw88/pci.c > > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > > @@ -866,12 +866,29 @@ static irqreturn_t rtw_pci_interrupt_handler(int > irq, > > void *dev) > > { > > struct rtw_dev *rtwdev = dev; > > struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv; > > - u32 irq_status[4]; > > + unsigned long flags; > > > > - spin_lock(&rtwpci->irq_lock); > > + spin_lock_irqsave(&rtwpci->irq_lock, flags); I think you can use 'spin_lock()' here as it's in IRQ context? > > if (!rtwpci->irq_enabled) > > goto out; > > > > + /* disable RTW PCI interrupt to avoid more interrupts before the end of > > + * thread function > > + */ > > + rtw_pci_disable_interrupt(rtwdev, rtwpci); Why do we need rtw_pci_disable_interrupt() here. Have you done any experiment and decided to add this. If you have can you share your results to me? > > +out: > > + spin_unlock_irqrestore(&rtwpci->irq_lock, flags); spin_unlock() > > + > > + return IRQ_WAKE_THREAD; > > +} > > + > > +static irqreturn_t rtw_pci_interrupt_threadfn(int irq, void *dev) > > +{ > > + struct rtw_dev *rtwdev = dev; > > + struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv; > > + unsigned long flags; > > + u32 irq_status[4]; > > + > > rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); > > > > if (irq_status[0] & IMR_MGNTDOK) > > @@ -891,8 +908,11 @@ static irqreturn_t rtw_pci_interrupt_handler(int > irq, > > void *dev) > > if (irq_status[0] & IMR_ROK) > > rtw_pci_rx_isr(rtwdev, rtwpci, RTW_RX_QUEUE_MPDU); > > > > -out: > > - spin_unlock(&rtwpci->irq_lock); > > + /* all of the jobs for this interrupt have been done */ > > + spin_lock_irqsave(&rtwpci->irq_lock, flags); > > Shouldn't we protect the ISRs above? > > This patch could actually reduce the time of IRQ. > But I think I need to further test it with PCI MSI interrupt. > https://patchwork.kernel.org/patch/11081539/ > > Maybe we could drop the "rtw_pci_[enable/disable]_interrupt" when MSI > Is enabled with this patch. > > > + if (rtw_flag_check(rtwdev, RTW_FLAG_RUNNING)) > > + rtw_pci_enable_interrupt(rtwdev, rtwpci); > > + spin_unlock_irqrestore(&rtwpci->irq_lock, flags); > > > > return IRQ_HANDLED; > > } > > @@ -1152,8 +1172,10 @@ static int rtw_pci_probe(struct pci_dev *pdev, > > goto err_destroy_pci; > > } > > > > - ret = request_irq(pdev->irq, &rtw_pci_interrupt_handler, > > - IRQF_SHARED, KBUILD_MODNAME, rtwdev); > > + ret = devm_request_threaded_irq(rtwdev->dev, pdev->irq, > > + rtw_pci_interrupt_handler, > > + rtw_pci_interrupt_threadfn, > > + IRQF_SHARED, KBUILD_MODNAME, rtwdev); > > if (ret) { > > ieee80211_unregister_hw(hw); > > goto err_destroy_pci; > > @@ -1192,7 +1214,7 @@ static void rtw_pci_remove(struct pci_dev > *pdev) > > rtw_pci_disable_interrupt(rtwdev, rtwpci); > > rtw_pci_destroy(rtwdev, pdev); > > rtw_pci_declaim(rtwdev, pdev); > > - free_irq(rtwpci->pdev->irq, rtwdev); > > + devm_free_irq(rtwdev->dev, rtwpci->pdev->irq, rtwdev); > > rtw_core_deinit(rtwdev); > > ieee80211_free_hw(hw); > > } > > -- > > 2.20.1 > > Yan-Hsuan > Thanks Yan-Hsuan
Tony Chuang <yhchuang@realtek.com> 於 2019年8月16日 週五 下午4:07寫道： > > Hi, > > A few more questions below > > > > From: Jian-Hong Pan [mailto:jian-hong@endlessm.com] > > > > > > There is a mass of jobs between spin lock and unlock in the hardware > > > IRQ which will occupy much time originally. To make system work more > > > efficiently, this patch moves the jobs to the soft IRQ (bottom half) to > > > reduce the time in hardware IRQ. > > > > > > Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com> > > > --- > > > drivers/net/wireless/realtek/rtw88/pci.c | 36 +++++++++++++++++++----- > > > 1 file changed, 29 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c > > > b/drivers/net/wireless/realtek/rtw88/pci.c > > > index 00ef229552d5..355606b167c6 100644 > > > --- a/drivers/net/wireless/realtek/rtw88/pci.c > > > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > > > @@ -866,12 +866,29 @@ static irqreturn_t rtw_pci_interrupt_handler(int > > irq, > > > void *dev) > > > { > > > struct rtw_dev *rtwdev = dev; > > > struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv; > > > - u32 irq_status[4]; > > > + unsigned long flags; > > > > > > - spin_lock(&rtwpci->irq_lock); > > > + spin_lock_irqsave(&rtwpci->irq_lock, flags); > > I think you can use 'spin_lock()' here as it's in IRQ context? Ah! You are right! The interrupts are already disabled in the interrupt handler. So, there is no need to disable more once. I can tweak it. > > > if (!rtwpci->irq_enabled) > > > goto out; > > > > > > + /* disable RTW PCI interrupt to avoid more interrupts before the end of > > > + * thread function > > > + */ > > > + rtw_pci_disable_interrupt(rtwdev, rtwpci); > > > Why do we need rtw_pci_disable_interrupt() here. > Have you done any experiment and decided to add this. > If you have can you share your results to me? I got this idea "Avoid back to back interrupt" from Intel WiFi's driver. https://elixir.bootlin.com/linux/v5.3-rc4/source/drivers/net/wireless/intel/iwlwifi/pcie/rx.c#L2071 So, I disable rtw_pci interrupt here in first half IRQ. (Re-enable in bottom half) > > > > +out: > > > + spin_unlock_irqrestore(&rtwpci->irq_lock, flags); > > spin_unlock() > > > > + > > > + return IRQ_WAKE_THREAD; > > > +} > > > + > > > +static irqreturn_t rtw_pci_interrupt_threadfn(int irq, void *dev) > > > +{ > > > + struct rtw_dev *rtwdev = dev; > > > + struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv; > > > + unsigned long flags; > > > + u32 irq_status[4]; > > > + > > > rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); > > > > > > if (irq_status[0] & IMR_MGNTDOK) > > > @@ -891,8 +908,11 @@ static irqreturn_t rtw_pci_interrupt_handler(int > > irq, > > > void *dev) > > > if (irq_status[0] & IMR_ROK) > > > rtw_pci_rx_isr(rtwdev, rtwpci, RTW_RX_QUEUE_MPDU); > > > > > > -out: > > > - spin_unlock(&rtwpci->irq_lock); > > > + /* all of the jobs for this interrupt have been done */ > > > + spin_lock_irqsave(&rtwpci->irq_lock, flags); > > > > Shouldn't we protect the ISRs above? > > > > This patch could actually reduce the time of IRQ. > > But I think I need to further test it with PCI MSI interrupt. > > https://patchwork.kernel.org/patch/11081539/ > > > > Maybe we could drop the "rtw_pci_[enable/disable]_interrupt" when MSI > > Is enabled with this patch. > > > > > + if (rtw_flag_check(rtwdev, RTW_FLAG_RUNNING)) > > > + rtw_pci_enable_interrupt(rtwdev, rtwpci); Then, re-enable rtw_pci interrupt here in bottom half of the IRQ. Here is the place where Intel WiFi re-enable interrupts. https://elixir.bootlin.com/linux/v5.3-rc4/source/drivers/net/wireless/intel/iwlwifi/pcie/rx.c#L1959 Now, we can go back to the question "Shouldn't we protect the ISRs above?" 1. What does the lock: rtwpci->irq_lock protect for? According to the code, seems only rtw_pci interrupt's state which is enabled or not. 2. How about the ISRs you mentioned? This part will only be executed if there is a fresh rtw_pci interrupt. The first half already disabled rtw_pci interrupt, so there is no more fresh rtw_pci interrupt until rtw_pci interrupt is enabled again. Therefor, the rtwpci->irq_lock only wraps the rtw_pci interrupt enablement. If there is any more entry that I missed and will interfere, please let me know. Thank you Jian-Hong Pan
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c index 00ef229552d5..355606b167c6 100644 --- a/drivers/net/wireless/realtek/rtw88/pci.c +++ b/drivers/net/wireless/realtek/rtw88/pci.c @@ -866,12 +866,29 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq, void *dev) { struct rtw_dev *rtwdev = dev; struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv; - u32 irq_status[4]; + unsigned long flags; - spin_lock(&rtwpci->irq_lock); + spin_lock_irqsave(&rtwpci->irq_lock, flags); if (!rtwpci->irq_enabled) goto out; + /* disable RTW PCI interrupt to avoid more interrupts before the end of + * thread function + */ + rtw_pci_disable_interrupt(rtwdev, rtwpci); +out: + spin_unlock_irqrestore(&rtwpci->irq_lock, flags); + + return IRQ_WAKE_THREAD; +} + +static irqreturn_t rtw_pci_interrupt_threadfn(int irq, void *dev) +{ + struct rtw_dev *rtwdev = dev; + struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv; + unsigned long flags; + u32 irq_status[4]; + rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); if (irq_status[0] & IMR_MGNTDOK) @@ -891,8 +908,11 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq, void *dev) if (irq_status[0] & IMR_ROK) rtw_pci_rx_isr(rtwdev, rtwpci, RTW_RX_QUEUE_MPDU); -out: - spin_unlock(&rtwpci->irq_lock); + /* all of the jobs for this interrupt have been done */ + spin_lock_irqsave(&rtwpci->irq_lock, flags); + if (rtw_flag_check(rtwdev, RTW_FLAG_RUNNING)) + rtw_pci_enable_interrupt(rtwdev, rtwpci); + spin_unlock_irqrestore(&rtwpci->irq_lock, flags); return IRQ_HANDLED; } @@ -1152,8 +1172,10 @@ static int rtw_pci_probe(struct pci_dev *pdev, goto err_destroy_pci; } - ret = request_irq(pdev->irq, &rtw_pci_interrupt_handler, - IRQF_SHARED, KBUILD_MODNAME, rtwdev); + ret = devm_request_threaded_irq(rtwdev->dev, pdev->irq, + rtw_pci_interrupt_handler, + rtw_pci_interrupt_threadfn, + IRQF_SHARED, KBUILD_MODNAME, rtwdev); if (ret) { ieee80211_unregister_hw(hw); goto err_destroy_pci; @@ -1192,7 +1214,7 @@ static void rtw_pci_remove(struct pci_dev *pdev) rtw_pci_disable_interrupt(rtwdev, rtwpci); rtw_pci_destroy(rtwdev, pdev); rtw_pci_declaim(rtwdev, pdev); - free_irq(rtwpci->pdev->irq, rtwdev); + devm_free_irq(rtwdev->dev, rtwpci->pdev->irq, rtwdev); rtw_core_deinit(rtwdev); ieee80211_free_hw(hw); }
There is a mass of jobs between spin lock and unlock in the hardware IRQ which will occupy much time originally. To make system work more efficiently, this patch moves the jobs to the soft IRQ (bottom half) to reduce the time in hardware IRQ. Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com> --- drivers/net/wireless/realtek/rtw88/pci.c | 36 +++++++++++++++++++----- 1 file changed, 29 insertions(+), 7 deletions(-)