| Message ID | 20220720135113.15755-2-quic_mpubbise@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Throughput fixes for WCN6750 | expand |
Manikanta Pubbisetty <quic_mpubbise@quicinc.com> wrote: > Enable threaded NAPI on WCN6750. Unlike traditional NAPI poll which > runs in softirq context and on the core which scheduled the NAPI, > threaded NAPI makes use of kernel threads which are under direct > control of the scheduler and helps in balancing the NAPI processing > load across multiple CPUs thereby improving throughput. > > In the case of WCN6750, enabling threaded NAPI has improved > 160 MHz RX throughput by nearly 400 Mbps. This should give similar > gains for other ath11k devices as well, therefore enable threaded > NAPI on all other devices. > > Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1 > > Signed-off-by: Manikanta Pubbisetty <quic_mpubbise@quicinc.com> Fails to apply and no base commit provided, so please rebase. error: patch failed: drivers/net/wireless/ath/ath11k/core.c:111 error: drivers/net/wireless/ath/ath11k/core.c: patch does not apply error: patch failed: drivers/net/wireless/ath/ath11k/hw.h:205 error: drivers/net/wireless/ath/ath11k/hw.h: patch does not apply stg import: Diff does not apply cleanly 3 patches set to Changes Requested. 12924009 [1/3] ath11k: Enable threaded NAPI on WCN6750 12924011 [2/3] ath11k: Add multi TX ring support for WCN6750 12924010 [3/3] ath11k: Increase TCL data ring size for WCN6750
Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes: > Enable threaded NAPI on WCN6750. Unlike traditional NAPI poll which > runs in softirq context and on the core which scheduled the NAPI, > threaded NAPI makes use of kernel threads which are under direct > control of the scheduler and helps in balancing the NAPI processing > load across multiple CPUs thereby improving throughput. > > In the case of WCN6750, enabling threaded NAPI has improved > 160 MHz RX throughput by nearly 400 Mbps. This should give similar > gains for other ath11k devices as well, therefore enable threaded > NAPI on all other devices. > > Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1 > > Signed-off-by: Manikanta Pubbisetty <quic_mpubbise@quicinc.com> > --- > drivers/net/wireless/ath/ath11k/ahb.c | 1 + > drivers/net/wireless/ath/ath11k/pcic.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c > index d7d33d5cdfc5..e44e2f29a88f 100644 > --- a/drivers/net/wireless/ath/ath11k/ahb.c > +++ b/drivers/net/wireless/ath/ath11k/ahb.c > @@ -314,6 +314,7 @@ static void ath11k_ahb_ext_irq_enable(struct ath11k_base *ab) > struct ath11k_ext_irq_grp *irq_grp = &ab->ext_irq_grp[i]; > > if (!irq_grp->napi_enabled) { > + dev_set_threaded(&irq_grp->napi_ndev, true); > napi_enable(&irq_grp->napi); > irq_grp->napi_enabled = true; > } > diff --git a/drivers/net/wireless/ath/ath11k/pcic.c b/drivers/net/wireless/ath/ath11k/pcic.c > index cf12b98c480d..c703db19de51 100644 > --- a/drivers/net/wireless/ath/ath11k/pcic.c > +++ b/drivers/net/wireless/ath/ath11k/pcic.c > @@ -440,6 +440,7 @@ void ath11k_pcic_ext_irq_enable(struct ath11k_base *ab) > struct ath11k_ext_irq_grp *irq_grp = &ab->ext_irq_grp[i]; > > if (!irq_grp->napi_enabled) { > + dev_set_threaded(&irq_grp->napi_ndev, true); > napi_enable(&irq_grp->napi); > irq_grp->napi_enabled = true; > } The commit log claims that this enabled _only_ on WCN6750 but aren't we enabling it on all ath11k hardware, or am I missing something? I admit I didn't check this very carefully. (reads the commit log one more time) Ah, in the last sentence you mention that it's enabled on all hardware. That's quite easy to miss and the commit log is quite misleading, please emphasise already in the title and the first sentence that this is for all hardware. Also more testing would be nice. Enabling something like this with testing only on one hardware family (WCN7850) can be risky. I always get warm fuzzy feelings if a patch is tested with all three hardware families we currently support: * IPQ8074 etc * QCA6390 etc * WCN7850
On 9/2/2022 6:07 PM, Kalle Valo wrote: > Manikanta Pubbisetty <quic_mpubbise@quicinc.com> wrote: > >> Enable threaded NAPI on WCN6750. Unlike traditional NAPI poll which >> runs in softirq context and on the core which scheduled the NAPI, >> threaded NAPI makes use of kernel threads which are under direct >> control of the scheduler and helps in balancing the NAPI processing >> load across multiple CPUs thereby improving throughput. >> >> In the case of WCN6750, enabling threaded NAPI has improved >> 160 MHz RX throughput by nearly 400 Mbps. This should give similar >> gains for other ath11k devices as well, therefore enable threaded >> NAPI on all other devices. >> >> Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1 >> >> Signed-off-by: Manikanta Pubbisetty <quic_mpubbise@quicinc.com> > > Fails to apply and no base commit provided, so please rebase. > > error: patch failed: drivers/net/wireless/ath/ath11k/core.c:111 > error: drivers/net/wireless/ath/ath11k/core.c: patch does not apply > error: patch failed: drivers/net/wireless/ath/ath11k/hw.h:205 > error: drivers/net/wireless/ath/ath11k/hw.h: patch does not apply > stg import: Diff does not apply cleanly > > 3 patches set to Changes Requested. > > 12924009 [1/3] ath11k: Enable threaded NAPI on WCN6750 > 12924011 [2/3] ath11k: Add multi TX ring support for WCN6750 > 12924010 [3/3] ath11k: Increase TCL data ring size for WCN6750 > Sure, thanks!
On 9/2/2022 6:18 PM, Kalle Valo wrote: > Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes: > >> Enable threaded NAPI on WCN6750. Unlike traditional NAPI poll which >> runs in softirq context and on the core which scheduled the NAPI, >> threaded NAPI makes use of kernel threads which are under direct >> control of the scheduler and helps in balancing the NAPI processing >> load across multiple CPUs thereby improving throughput. >> >> In the case of WCN6750, enabling threaded NAPI has improved >> 160 MHz RX throughput by nearly 400 Mbps. This should give similar >> gains for other ath11k devices as well, therefore enable threaded >> NAPI on all other devices. >> >> Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1 >> >> Signed-off-by: Manikanta Pubbisetty <quic_mpubbise@quicinc.com> >> --- >> drivers/net/wireless/ath/ath11k/ahb.c | 1 + >> drivers/net/wireless/ath/ath11k/pcic.c | 1 + >> 2 files changed, 2 insertions(+) >> >> diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c >> index d7d33d5cdfc5..e44e2f29a88f 100644 >> --- a/drivers/net/wireless/ath/ath11k/ahb.c >> +++ b/drivers/net/wireless/ath/ath11k/ahb.c >> @@ -314,6 +314,7 @@ static void ath11k_ahb_ext_irq_enable(struct ath11k_base *ab) >> struct ath11k_ext_irq_grp *irq_grp = &ab->ext_irq_grp[i]; >> >> if (!irq_grp->napi_enabled) { >> + dev_set_threaded(&irq_grp->napi_ndev, true); >> napi_enable(&irq_grp->napi); >> irq_grp->napi_enabled = true; >> } >> diff --git a/drivers/net/wireless/ath/ath11k/pcic.c b/drivers/net/wireless/ath/ath11k/pcic.c >> index cf12b98c480d..c703db19de51 100644 >> --- a/drivers/net/wireless/ath/ath11k/pcic.c >> +++ b/drivers/net/wireless/ath/ath11k/pcic.c >> @@ -440,6 +440,7 @@ void ath11k_pcic_ext_irq_enable(struct ath11k_base *ab) >> struct ath11k_ext_irq_grp *irq_grp = &ab->ext_irq_grp[i]; >> >> if (!irq_grp->napi_enabled) { >> + dev_set_threaded(&irq_grp->napi_ndev, true); >> napi_enable(&irq_grp->napi); >> irq_grp->napi_enabled = true; >> } > > The commit log claims that this enabled _only_ on WCN6750 but aren't we > enabling it on all ath11k hardware, or am I missing something? I admit I > didn't check this very carefully. > > (reads the commit log one more time) > > Ah, in the last sentence you mention that it's enabled on all hardware. > That's quite easy to miss and the commit log is quite misleading, please > emphasise already in the title and the first sentence that this is for > all hardware. My Bad, yes you right. The patch was made initially only for WCN6750 and was enabled later for all devices. > > Also more testing would be nice. Enabling something like this with > testing only on one hardware family (WCN7850) can be risky. I always get > warm fuzzy feelings if a patch is tested with all three hardware > families we currently support: > > * IPQ8074 etc > * QCA6390 etc > * WCN7850 > WCN7850 should be an ath12k device If I'm correct. Regardless of the chip family, even I feel that the tput changes like these should be tested on all the chipsets. Availability of the hardware and time are something which are stopping me in testing the changes on all supported targets. As I said, I had made the changes only to WCN6750 initially (using a hw_param). Can we take that approach for now and enable this for other targets only if required & upon thorough testing? Thanks, Manikanta
On Fri, Sep 2, 2022 at 3:18 PM Manikanta Pubbisetty <quic_mpubbise@quicinc.com> wrote: > > On 9/2/2022 6:18 PM, Kalle Valo wrote: > > Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes: > > > >> Enable threaded NAPI on WCN6750. Unlike traditional NAPI poll which > >> runs in softirq context and on the core which scheduled the NAPI, > >> threaded NAPI makes use of kernel threads which are under direct > >> control of the scheduler and helps in balancing the NAPI processing > >> load across multiple CPUs thereby improving throughput. > >> > >> In the case of WCN6750, enabling threaded NAPI has improved > >> 160 MHz RX throughput by nearly 400 Mbps. This should give similar > >> gains for other ath11k devices as well, therefore enable threaded > >> NAPI on all other devices. > >> > >> Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1 > >> > >> Signed-off-by: Manikanta Pubbisetty <quic_mpubbise@quicinc.com> > >> --- > >> drivers/net/wireless/ath/ath11k/ahb.c | 1 + > >> drivers/net/wireless/ath/ath11k/pcic.c | 1 + > >> 2 files changed, 2 insertions(+) > >> > >> diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c > >> index d7d33d5cdfc5..e44e2f29a88f 100644 > >> --- a/drivers/net/wireless/ath/ath11k/ahb.c > >> +++ b/drivers/net/wireless/ath/ath11k/ahb.c > >> @@ -314,6 +314,7 @@ static void ath11k_ahb_ext_irq_enable(struct ath11k_base *ab) > >> struct ath11k_ext_irq_grp *irq_grp = &ab->ext_irq_grp[i]; > >> > >> if (!irq_grp->napi_enabled) { > >> + dev_set_threaded(&irq_grp->napi_ndev, true); > >> napi_enable(&irq_grp->napi); > >> irq_grp->napi_enabled = true; > >> } > >> diff --git a/drivers/net/wireless/ath/ath11k/pcic.c b/drivers/net/wireless/ath/ath11k/pcic.c > >> index cf12b98c480d..c703db19de51 100644 > >> --- a/drivers/net/wireless/ath/ath11k/pcic.c > >> +++ b/drivers/net/wireless/ath/ath11k/pcic.c > >> @@ -440,6 +440,7 @@ void ath11k_pcic_ext_irq_enable(struct ath11k_base *ab) > >> struct ath11k_ext_irq_grp *irq_grp = &ab->ext_irq_grp[i]; > >> > >> if (!irq_grp->napi_enabled) { > >> + dev_set_threaded(&irq_grp->napi_ndev, true); > >> napi_enable(&irq_grp->napi); > >> irq_grp->napi_enabled = true; > >> } > > > > The commit log claims that this enabled _only_ on WCN6750 but aren't we > > enabling it on all ath11k hardware, or am I missing something? I admit I > > didn't check this very carefully. > > > > (reads the commit log one more time) > > > > Ah, in the last sentence you mention that it's enabled on all hardware. > > That's quite easy to miss and the commit log is quite misleading, please > > emphasise already in the title and the first sentence that this is for > > all hardware. > > My Bad, yes you right. The patch was made initially only for WCN6750 and > was enabled later for all devices. > > > > > Also more testing would be nice. Enabling something like this with > > testing only on one hardware family (WCN7850) can be risky. I always get > > warm fuzzy feelings if a patch is tested with all three hardware > > families we currently support: > > > > * IPQ8074 etc > > * QCA6390 etc > > * WCN7850 > > > > WCN7850 should be an ath12k device If I'm correct. > > Regardless of the chip family, even I feel that the tput changes like > these should be tested on all the chipsets. Availability of the hardware > and time are something which are stopping me in testing the changes on > all supported targets. > > As I said, I had made the changes only to WCN6750 initially (using a > hw_param). Can we take that approach for now and enable this for other > targets only if required & upon thorough testing? I can tell you that on IPQ8074 threaded NAPI really improves perfromance. Regards, Robert > > Thanks, > Manikanta > > -- > ath11k mailing list > ath11k@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/ath11k
On 9/2/2022 6:50 PM, Robert Marko wrote: > On Fri, Sep 2, 2022 at 3:18 PM Manikanta Pubbisetty > <quic_mpubbise@quicinc.com> wrote: >> >> On 9/2/2022 6:18 PM, Kalle Valo wrote: >>> Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes: >>> >>>> Enable threaded NAPI on WCN6750. Unlike traditional NAPI poll which >>>> runs in softirq context and on the core which scheduled the NAPI, >>>> threaded NAPI makes use of kernel threads which are under direct >>>> control of the scheduler and helps in balancing the NAPI processing >>>> load across multiple CPUs thereby improving throughput. >>>> >>>> In the case of WCN6750, enabling threaded NAPI has improved >>>> 160 MHz RX throughput by nearly 400 Mbps. This should give similar >>>> gains for other ath11k devices as well, therefore enable threaded >>>> NAPI on all other devices. >>>> >>>> Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1 >>>> >>>> Signed-off-by: Manikanta Pubbisetty <quic_mpubbise@quicinc.com> >>>> --- >>>> drivers/net/wireless/ath/ath11k/ahb.c | 1 + >>>> drivers/net/wireless/ath/ath11k/pcic.c | 1 + >>>> 2 files changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c >>>> index d7d33d5cdfc5..e44e2f29a88f 100644 >>>> --- a/drivers/net/wireless/ath/ath11k/ahb.c >>>> +++ b/drivers/net/wireless/ath/ath11k/ahb.c >>>> @@ -314,6 +314,7 @@ static void ath11k_ahb_ext_irq_enable(struct ath11k_base *ab) >>>> struct ath11k_ext_irq_grp *irq_grp = &ab->ext_irq_grp[i]; >>>> >>>> if (!irq_grp->napi_enabled) { >>>> + dev_set_threaded(&irq_grp->napi_ndev, true); >>>> napi_enable(&irq_grp->napi); >>>> irq_grp->napi_enabled = true; >>>> } >>>> diff --git a/drivers/net/wireless/ath/ath11k/pcic.c b/drivers/net/wireless/ath/ath11k/pcic.c >>>> index cf12b98c480d..c703db19de51 100644 >>>> --- a/drivers/net/wireless/ath/ath11k/pcic.c >>>> +++ b/drivers/net/wireless/ath/ath11k/pcic.c >>>> @@ -440,6 +440,7 @@ void ath11k_pcic_ext_irq_enable(struct ath11k_base *ab) >>>> struct ath11k_ext_irq_grp *irq_grp = &ab->ext_irq_grp[i]; >>>> >>>> if (!irq_grp->napi_enabled) { >>>> + dev_set_threaded(&irq_grp->napi_ndev, true); >>>> napi_enable(&irq_grp->napi); >>>> irq_grp->napi_enabled = true; >>>> } >>> >>> The commit log claims that this enabled _only_ on WCN6750 but aren't we >>> enabling it on all ath11k hardware, or am I missing something? I admit I >>> didn't check this very carefully. >>> >>> (reads the commit log one more time) >>> >>> Ah, in the last sentence you mention that it's enabled on all hardware. >>> That's quite easy to miss and the commit log is quite misleading, please >>> emphasise already in the title and the first sentence that this is for >>> all hardware. >> >> My Bad, yes you right. The patch was made initially only for WCN6750 and >> was enabled later for all devices. >> >>> >>> Also more testing would be nice. Enabling something like this with >>> testing only on one hardware family (WCN7850) can be risky. I always get >>> warm fuzzy feelings if a patch is tested with all three hardware >>> families we currently support: >>> >>> * IPQ8074 etc >>> * QCA6390 etc >>> * WCN7850 >>> >> >> WCN7850 should be an ath12k device If I'm correct. >> >> Regardless of the chip family, even I feel that the tput changes like >> these should be tested on all the chipsets. Availability of the hardware >> and time are something which are stopping me in testing the changes on >> all supported targets. >> >> As I said, I had made the changes only to WCN6750 initially (using a >> hw_param). Can we take that approach for now and enable this for other >> targets only if required & upon thorough testing? > > I can tell you that on IPQ8074 threaded NAPI really improves perfromance. > Great. Do you have any test results on IPQ8074? Thanks, Manikanta
On Fri, Sep 2, 2022 at 3:42 PM Manikanta Pubbisetty <quic_mpubbise@quicinc.com> wrote: > > On 9/2/2022 6:50 PM, Robert Marko wrote: > > On Fri, Sep 2, 2022 at 3:18 PM Manikanta Pubbisetty > > <quic_mpubbise@quicinc.com> wrote: > >> > >> On 9/2/2022 6:18 PM, Kalle Valo wrote: > >>> Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes: > >>> > >>>> Enable threaded NAPI on WCN6750. Unlike traditional NAPI poll which > >>>> runs in softirq context and on the core which scheduled the NAPI, > >>>> threaded NAPI makes use of kernel threads which are under direct > >>>> control of the scheduler and helps in balancing the NAPI processing > >>>> load across multiple CPUs thereby improving throughput. > >>>> > >>>> In the case of WCN6750, enabling threaded NAPI has improved > >>>> 160 MHz RX throughput by nearly 400 Mbps. This should give similar > >>>> gains for other ath11k devices as well, therefore enable threaded > >>>> NAPI on all other devices. > >>>> > >>>> Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1 > >>>> > >>>> Signed-off-by: Manikanta Pubbisetty <quic_mpubbise@quicinc.com> > >>>> --- > >>>> drivers/net/wireless/ath/ath11k/ahb.c | 1 + > >>>> drivers/net/wireless/ath/ath11k/pcic.c | 1 + > >>>> 2 files changed, 2 insertions(+) > >>>> > >>>> diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c > >>>> index d7d33d5cdfc5..e44e2f29a88f 100644 > >>>> --- a/drivers/net/wireless/ath/ath11k/ahb.c > >>>> +++ b/drivers/net/wireless/ath/ath11k/ahb.c > >>>> @@ -314,6 +314,7 @@ static void ath11k_ahb_ext_irq_enable(struct ath11k_base *ab) > >>>> struct ath11k_ext_irq_grp *irq_grp = &ab->ext_irq_grp[i]; > >>>> > >>>> if (!irq_grp->napi_enabled) { > >>>> + dev_set_threaded(&irq_grp->napi_ndev, true); > >>>> napi_enable(&irq_grp->napi); > >>>> irq_grp->napi_enabled = true; > >>>> } > >>>> diff --git a/drivers/net/wireless/ath/ath11k/pcic.c b/drivers/net/wireless/ath/ath11k/pcic.c > >>>> index cf12b98c480d..c703db19de51 100644 > >>>> --- a/drivers/net/wireless/ath/ath11k/pcic.c > >>>> +++ b/drivers/net/wireless/ath/ath11k/pcic.c > >>>> @@ -440,6 +440,7 @@ void ath11k_pcic_ext_irq_enable(struct ath11k_base *ab) > >>>> struct ath11k_ext_irq_grp *irq_grp = &ab->ext_irq_grp[i]; > >>>> > >>>> if (!irq_grp->napi_enabled) { > >>>> + dev_set_threaded(&irq_grp->napi_ndev, true); > >>>> napi_enable(&irq_grp->napi); > >>>> irq_grp->napi_enabled = true; > >>>> } > >>> > >>> The commit log claims that this enabled _only_ on WCN6750 but aren't we > >>> enabling it on all ath11k hardware, or am I missing something? I admit I > >>> didn't check this very carefully. > >>> > >>> (reads the commit log one more time) > >>> > >>> Ah, in the last sentence you mention that it's enabled on all hardware. > >>> That's quite easy to miss and the commit log is quite misleading, please > >>> emphasise already in the title and the first sentence that this is for > >>> all hardware. > >> > >> My Bad, yes you right. The patch was made initially only for WCN6750 and > >> was enabled later for all devices. > >> > >>> > >>> Also more testing would be nice. Enabling something like this with > >>> testing only on one hardware family (WCN7850) can be risky. I always get > >>> warm fuzzy feelings if a patch is tested with all three hardware > >>> families we currently support: > >>> > >>> * IPQ8074 etc > >>> * QCA6390 etc > >>> * WCN7850 > >>> > >> > >> WCN7850 should be an ath12k device If I'm correct. > >> > >> Regardless of the chip family, even I feel that the tput changes like > >> these should be tested on all the chipsets. Availability of the hardware > >> and time are something which are stopping me in testing the changes on > >> all supported targets. > >> > >> As I said, I had made the changes only to WCN6750 initially (using a > >> hw_param). Can we take that approach for now and enable this for other > >> targets only if required & upon thorough testing? > > > > I can tell you that on IPQ8074 threaded NAPI really improves perfromance. > > > > Great. Do you have any test results on IPQ8074? I dont have full test results, but on Poco F2 Pro as the client @80MHz AX I got ~720Mbps compared to ~550Mbps before. I can tell you that in OpenWrt, we have had it enabled for 6+ months at this point and its been really good. Regards, Robert > > Thanks, > Manikanta
On 9/2/2022 7:16 PM, Robert Marko wrote: > On Fri, Sep 2, 2022 at 3:42 PM Manikanta Pubbisetty > <quic_mpubbise@quicinc.com> wrote: >> >> On 9/2/2022 6:50 PM, Robert Marko wrote: >>> On Fri, Sep 2, 2022 at 3:18 PM Manikanta Pubbisetty >>> <quic_mpubbise@quicinc.com> wrote: >>>> >>>> On 9/2/2022 6:18 PM, Kalle Valo wrote: >>>>> Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes: >>>>> >>>>>> Enable threaded NAPI on WCN6750. Unlike traditional NAPI poll which >>>>>> runs in softirq context and on the core which scheduled the NAPI, >>>>>> threaded NAPI makes use of kernel threads which are under direct >>>>>> control of the scheduler and helps in balancing the NAPI processing >>>>>> load across multiple CPUs thereby improving throughput. >>>>>> >>>>>> In the case of WCN6750, enabling threaded NAPI has improved >>>>>> 160 MHz RX throughput by nearly 400 Mbps. This should give similar >>>>>> gains for other ath11k devices as well, therefore enable threaded >>>>>> NAPI on all other devices. >>>>>> >>>>>> Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1 >>>>>> >>>>>> Signed-off-by: Manikanta Pubbisetty <quic_mpubbise@quicinc.com> >>>>>> --- >>>>>> drivers/net/wireless/ath/ath11k/ahb.c | 1 + >>>>>> drivers/net/wireless/ath/ath11k/pcic.c | 1 + >>>>>> 2 files changed, 2 insertions(+) >>>>>> >>>>>> diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c >>>>>> index d7d33d5cdfc5..e44e2f29a88f 100644 >>>>>> --- a/drivers/net/wireless/ath/ath11k/ahb.c >>>>>> +++ b/drivers/net/wireless/ath/ath11k/ahb.c >>>>>> @@ -314,6 +314,7 @@ static void ath11k_ahb_ext_irq_enable(struct ath11k_base *ab) >>>>>> struct ath11k_ext_irq_grp *irq_grp = &ab->ext_irq_grp[i]; >>>>>> >>>>>> if (!irq_grp->napi_enabled) { >>>>>> + dev_set_threaded(&irq_grp->napi_ndev, true); >>>>>> napi_enable(&irq_grp->napi); >>>>>> irq_grp->napi_enabled = true; >>>>>> } >>>>>> diff --git a/drivers/net/wireless/ath/ath11k/pcic.c b/drivers/net/wireless/ath/ath11k/pcic.c >>>>>> index cf12b98c480d..c703db19de51 100644 >>>>>> --- a/drivers/net/wireless/ath/ath11k/pcic.c >>>>>> +++ b/drivers/net/wireless/ath/ath11k/pcic.c >>>>>> @@ -440,6 +440,7 @@ void ath11k_pcic_ext_irq_enable(struct ath11k_base *ab) >>>>>> struct ath11k_ext_irq_grp *irq_grp = &ab->ext_irq_grp[i]; >>>>>> >>>>>> if (!irq_grp->napi_enabled) { >>>>>> + dev_set_threaded(&irq_grp->napi_ndev, true); >>>>>> napi_enable(&irq_grp->napi); >>>>>> irq_grp->napi_enabled = true; >>>>>> } >>>>> >>>>> The commit log claims that this enabled _only_ on WCN6750 but aren't we >>>>> enabling it on all ath11k hardware, or am I missing something? I admit I >>>>> didn't check this very carefully. >>>>> >>>>> (reads the commit log one more time) >>>>> >>>>> Ah, in the last sentence you mention that it's enabled on all hardware. >>>>> That's quite easy to miss and the commit log is quite misleading, please >>>>> emphasise already in the title and the first sentence that this is for >>>>> all hardware. >>>> >>>> My Bad, yes you right. The patch was made initially only for WCN6750 and >>>> was enabled later for all devices. >>>> >>>>> >>>>> Also more testing would be nice. Enabling something like this with >>>>> testing only on one hardware family (WCN7850) can be risky. I always get >>>>> warm fuzzy feelings if a patch is tested with all three hardware >>>>> families we currently support: >>>>> >>>>> * IPQ8074 etc >>>>> * QCA6390 etc >>>>> * WCN7850 >>>>> >>>> >>>> WCN7850 should be an ath12k device If I'm correct. >>>> >>>> Regardless of the chip family, even I feel that the tput changes like >>>> these should be tested on all the chipsets. Availability of the hardware >>>> and time are something which are stopping me in testing the changes on >>>> all supported targets. >>>> >>>> As I said, I had made the changes only to WCN6750 initially (using a >>>> hw_param). Can we take that approach for now and enable this for other >>>> targets only if required & upon thorough testing? >>> >>> I can tell you that on IPQ8074 threaded NAPI really improves perfromance. >>> >> >> Great. Do you have any test results on IPQ8074? > > I dont have full test results, but on Poco F2 Pro as the client @80MHz AX > I got ~720Mbps compared to ~550Mbps before. > > I can tell you that in OpenWrt, we have had it enabled for 6+ months > at this point > and its been really good. > That's a significant improvement, great to hear that. We have another strong reason to have this change in upstream driver. Thanks, Manikanta
Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes: > On 9/2/2022 7:16 PM, Robert Marko wrote: >> On Fri, Sep 2, 2022 at 3:42 PM Manikanta Pubbisetty >> <quic_mpubbise@quicinc.com> wrote: >>> >>> On 9/2/2022 6:50 PM, Robert Marko wrote: >>>> On Fri, Sep 2, 2022 at 3:18 PM Manikanta Pubbisetty >>>> <quic_mpubbise@quicinc.com> wrote: >>>>> >>>>> Regardless of the chip family, even I feel that the tput changes like >>>>> these should be tested on all the chipsets. Availability of the hardware >>>>> and time are something which are stopping me in testing the changes on >>>>> all supported targets. >>>>> >>>>> As I said, I had made the changes only to WCN6750 initially (using a >>>>> hw_param). Can we take that approach for now and enable this for other >>>>> targets only if required & upon thorough testing? >>>> >>>> I can tell you that on IPQ8074 threaded NAPI really improves perfromance. >>>> >>> >>> Great. Do you have any test results on IPQ8074? >> >> I dont have full test results, but on Poco F2 Pro as the client @80MHz AX >> I got ~720Mbps compared to ~550Mbps before. >> >> I can tell you that in OpenWrt, we have had it enabled for 6+ months >> at this point >> and its been really good. >> > > That's a significant improvement, great to hear that. We have another > strong reason to have this change in upstream driver. Indeed, thanks Robert. Is the code in openwrt similar as what Manikanta's patch does?
Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes: >> Also more testing would be nice. Enabling something like this with >> testing only on one hardware family (WCN7850) can be risky. I always get >> warm fuzzy feelings if a patch is tested with all three hardware >> families we currently support: >> >> * IPQ8074 etc >> * QCA6390 etc >> * WCN7850 > > WCN7850 should be an ath12k device If I'm correct. Sorry, I meant WCN6855. Too many drivers and too many hardware... > Regardless of the chip family, even I feel that the tput changes like > these should be tested on all the chipsets. Availability of the > hardware and time are something which are stopping me in testing the > changes on all supported targets. I have the same problem, I cannot test all possible hardware. That's why I was requsting help here.
On Thu, Sep 8, 2022 at 8:46 AM Kalle Valo <kvalo@kernel.org> wrote: > > Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes: > > > On 9/2/2022 7:16 PM, Robert Marko wrote: > >> On Fri, Sep 2, 2022 at 3:42 PM Manikanta Pubbisetty > >> <quic_mpubbise@quicinc.com> wrote: > >>> > >>> On 9/2/2022 6:50 PM, Robert Marko wrote: > >>>> On Fri, Sep 2, 2022 at 3:18 PM Manikanta Pubbisetty > >>>> <quic_mpubbise@quicinc.com> wrote: > >>>>> > >>>>> Regardless of the chip family, even I feel that the tput changes like > >>>>> these should be tested on all the chipsets. Availability of the hardware > >>>>> and time are something which are stopping me in testing the changes on > >>>>> all supported targets. > >>>>> > >>>>> As I said, I had made the changes only to WCN6750 initially (using a > >>>>> hw_param). Can we take that approach for now and enable this for other > >>>>> targets only if required & upon thorough testing? > >>>> > >>>> I can tell you that on IPQ8074 threaded NAPI really improves perfromance. > >>>> > >>> > >>> Great. Do you have any test results on IPQ8074? > >> > >> I dont have full test results, but on Poco F2 Pro as the client @80MHz AX > >> I got ~720Mbps compared to ~550Mbps before. > >> > >> I can tell you that in OpenWrt, we have had it enabled for 6+ months > >> at this point > >> and its been really good. > >> > > > > That's a significant improvement, great to hear that. We have another > > strong reason to have this change in upstream driver. > > Indeed, thanks Robert. Is the code in openwrt similar as what > Manikanta's patch does? Hi, It's actually identical, we are also just calling dev_set_threaded before napi_enable. Regards, Robert > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Robert Marko <robert.marko@sartura.hr> writes: > On Thu, Sep 8, 2022 at 8:46 AM Kalle Valo <kvalo@kernel.org> wrote: > >> >> Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes: >> >> > On 9/2/2022 7:16 PM, Robert Marko wrote: >> >> On Fri, Sep 2, 2022 at 3:42 PM Manikanta Pubbisetty >> >> <quic_mpubbise@quicinc.com> wrote: >> >>> >> >>> On 9/2/2022 6:50 PM, Robert Marko wrote: >> >>>> On Fri, Sep 2, 2022 at 3:18 PM Manikanta Pubbisetty >> >>>> <quic_mpubbise@quicinc.com> wrote: >> >>>>> >> >>>>> Regardless of the chip family, even I feel that the tput changes like >> >>>>> these should be tested on all the chipsets. Availability of the hardware >> >>>>> and time are something which are stopping me in testing the changes on >> >>>>> all supported targets. >> >>>>> >> >>>>> As I said, I had made the changes only to WCN6750 initially (using a >> >>>>> hw_param). Can we take that approach for now and enable this for other >> >>>>> targets only if required & upon thorough testing? >> >>>> >> >>>> I can tell you that on IPQ8074 threaded NAPI really improves perfromance. >> >>>> >> >>> >> >>> Great. Do you have any test results on IPQ8074? >> >> >> >> I dont have full test results, but on Poco F2 Pro as the client @80MHz AX >> >> I got ~720Mbps compared to ~550Mbps before. >> >> >> >> I can tell you that in OpenWrt, we have had it enabled for 6+ months >> >> at this point >> >> and its been really good. >> >> >> > >> > That's a significant improvement, great to hear that. We have another >> > strong reason to have this change in upstream driver. >> >> Indeed, thanks Robert. Is the code in openwrt similar as what >> Manikanta's patch does? > > It's actually identical, we are also just calling dev_set_threaded > before napi_enable. Nice, even better :)
diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c index d7d33d5cdfc5..e44e2f29a88f 100644 --- a/drivers/net/wireless/ath/ath11k/ahb.c +++ b/drivers/net/wireless/ath/ath11k/ahb.c @@ -314,6 +314,7 @@ static void ath11k_ahb_ext_irq_enable(struct ath11k_base *ab) struct ath11k_ext_irq_grp *irq_grp = &ab->ext_irq_grp[i]; if (!irq_grp->napi_enabled) { + dev_set_threaded(&irq_grp->napi_ndev, true); napi_enable(&irq_grp->napi); irq_grp->napi_enabled = true; } diff --git a/drivers/net/wireless/ath/ath11k/pcic.c b/drivers/net/wireless/ath/ath11k/pcic.c index cf12b98c480d..c703db19de51 100644 --- a/drivers/net/wireless/ath/ath11k/pcic.c +++ b/drivers/net/wireless/ath/ath11k/pcic.c @@ -440,6 +440,7 @@ void ath11k_pcic_ext_irq_enable(struct ath11k_base *ab) struct ath11k_ext_irq_grp *irq_grp = &ab->ext_irq_grp[i]; if (!irq_grp->napi_enabled) { + dev_set_threaded(&irq_grp->napi_ndev, true); napi_enable(&irq_grp->napi); irq_grp->napi_enabled = true; }
Enable threaded NAPI on WCN6750. Unlike traditional NAPI poll which runs in softirq context and on the core which scheduled the NAPI, threaded NAPI makes use of kernel threads which are under direct control of the scheduler and helps in balancing the NAPI processing load across multiple CPUs thereby improving throughput. In the case of WCN6750, enabling threaded NAPI has improved 160 MHz RX throughput by nearly 400 Mbps. This should give similar gains for other ath11k devices as well, therefore enable threaded NAPI on all other devices. Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1 Signed-off-by: Manikanta Pubbisetty <quic_mpubbise@quicinc.com> --- drivers/net/wireless/ath/ath11k/ahb.c | 1 + drivers/net/wireless/ath/ath11k/pcic.c | 1 + 2 files changed, 2 insertions(+)