Re: [PATCH 1/3] ath11k: Enable threaded NAPI on WCN6750

[Manikanta Pubbisetty <quic_mpubbise@quicinc.com>]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k