Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220720135113.15755-1-quic_mpubbise@quicinc.com>
 <20220720135113.15755-2-quic_mpubbise@quicinc.com> <87czceaqng.fsf@kernel.org>
 <dcc14883-51c9-c1bb-bbea-1b309eccedf2@quicinc.com>
In-Reply-To: <dcc14883-51c9-c1bb-bbea-1b309eccedf2@quicinc.com>
From:   Robert Marko <robert.marko@sartura.hr>
Date:   Fri, 2 Sep 2022 15:20:11 +0200
Message-ID: <CA+HBbNGEoSyfbs=FHtWyvBnG_jh9m=Q9KW=Pcfq+pDdHWEV4mw@mail.gmail.com>
Subject: Re: [PATCH 1/3] ath11k: Enable threaded NAPI on WCN6750
To:     Manikanta Pubbisetty <quic_mpubbise@quicinc.com>
Cc:     Kalle Valo <kvalo@kernel.org>, ath11k@lists.infradead.org,
        linux-wireless@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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


-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr