Re: [PATCH] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ
Tony Chuang 於 2019年8月16日 週五 下午4:07寫道：
>
> Hi,
>
> A few more questions below
>
> > > From: Jian-Hong Pan [mailto:jian-h...@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
> > > ---
> > > 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(>irq_lock);
> > > + spin_lock_irqsave(>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(>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(>irq_lock);
> > > + /* all of the jobs for this interrupt have been done */
> > > + spin_lock_irqsave(>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
RE: [PATCH] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ
Hi,
A few more questions below
> > From: Jian-Hong Pan [mailto:jian-h...@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
> > ---
> > 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(>irq_lock);
> > + spin_lock_irqsave(>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(>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(>irq_lock);
> > + /* all of the jobs for this interrupt have been done */
> > + spin_lock_irqsave(>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(>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, _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
RE: [PATCH] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ
> From: Jian-Hong Pan [mailto:jian-h...@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
> ---
> 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(>irq_lock);
> + spin_lock_irqsave(>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(>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(>irq_lock);
> + /* all of the jobs for this interrupt have been done */
> + spin_lock_irqsave(>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(>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, _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
[PATCH] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ
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
---
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(>irq_lock);
+ spin_lock_irqsave(>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(>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(>irq_lock);
+ /* all of the jobs for this interrupt have been done */
+ spin_lock_irqsave(>irq_lock, flags);
+ if (rtw_flag_check(rtwdev, RTW_FLAG_RUNNING))
+ rtw_pci_enable_interrupt(rtwdev, rtwpci);
+ spin_unlock_irqrestore(>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, _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
