Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200427145435.13151-1-greearb@candelatech.com>
 <CALLGbR+fY9w1q=6HuU56OZLD6BeP_0KkU2xeoAA0ZZXxns+i3g@mail.gmail.com>
 <1e1664b6-1998-5a4b-67ba-09113ec8d3a7@candelatech.com> <87k11zv1ux.fsf@toke.dk>
In-Reply-To: <87k11zv1ux.fsf@toke.dk>
From:   Dave Taht <dave.taht@gmail.com>
Date:   Tue, 28 Apr 2020 12:39:50 -0700
Message-ID: <CAA93jw7exafEx3YkvR5uaaBm5kxzYp3nw14zMfgT=2SwUjaQFg@mail.gmail.com>
Subject: Re: [PATCH] ath10k: Restart xmit queues below low-water mark.
To:     =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= <toke@toke.dk>
Cc:     Ben Greear <greearb@candelatech.com>,
        Steve deRosier <derosier@gmail.com>,
        linux-wireless <linux-wireless@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Tue, Apr 28, 2020 at 12:37 PM Toke H=C3=B8iland-J=C3=B8rgensen <toke@tok=
e.dk> wrote:
>
> Ben Greear <greearb@candelatech.com> writes:
>
> > On 04/28/2020 07:56 AM, Steve deRosier wrote:
> >> On Mon, Apr 27, 2020 at 7:54 AM <greearb@candelatech.com> wrote:
> >>>
> >>> From: Ben Greear <greearb@candelatech.com>
> >>>
> >>> While running tcp upload + download tests with ~200
> >>> concurrent TCP streams, 1-2 processes, and 30 station
> >>> vdevs, I noticed that the __ieee80211_stop_queue was taking
> >>> around 20% of the CPU according to perf-top, which other locking
> >>> taking an additional ~15%.
> >>>
> >>> I believe the issue is that the ath10k driver would unlock the
> >>> txqueue when a single frame could be transmitted, instead of
> >>> waiting for a low water mark.
> >>>
> >>> So, this patch adds a low-water mark that is 1/4 of the total
> >>> tx buffers allowed.
> >>>
> >>> This appears to resolve the performance problem that I saw.
> >>>
> >>> Tested with recent wave-1 ath10k-ct firmware.
> >>>
> >>
> >> Hey Ben,
> >>
> >> Did you do any testing with this patch around latency?  The nature of
> >> the thing that you fixed makes me wonder if it was intentional with
> >> respect to making WiFi fast - ie getting rid of buffers as much as
> >> possible.  Obviously the CPU impact is likely to be an unintended
> >> consequence. In any case, I don't know anything for sure, it was just
> >> a thought that went through my head when reading this.
> >
> > I did not, but on average my patch should make the queues be less full,
> > so I doubt it will hurt latency.
>
> I would tend to agree with that.

Well, I don't, as it's dependent on right sizing the ring in the first plac=
e.

But testing is in order! :)

> -Toke
>
> >>> Signed-off-by: Ben Greear <greearb@candelatech.com>
> >>> ---
> >>>  drivers/net/wireless/ath/ath10k/htt.h    | 1 +
> >>>  drivers/net/wireless/ath/ath10k/htt_tx.c | 8 ++++++--
> >>>  2 files changed, 7 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/wireless/ath/ath10k/htt.h b/drivers/net/wire=
less/ath/ath10k/htt.h
> >>> index 31c4ddbf45cb..b5634781c0dc 100644
> >>> --- a/drivers/net/wireless/ath/ath10k/htt.h
> >>> +++ b/drivers/net/wireless/ath/ath10k/htt.h
> >>> @@ -1941,6 +1941,7 @@ struct ath10k_htt {
> >>>
> >>>         u8 target_version_major;
> >>>         u8 target_version_minor;
> >>> +       bool needs_unlock;
> >>>         struct completion target_version_received;
> >>>         u8 max_num_amsdu;
> >>>         u8 max_num_ampdu;
> >>> diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/w=
ireless/ath/ath10k/htt_tx.c
> >>> index 9b3c3b080e92..44795d9a7c0c 100644
> >>> --- a/drivers/net/wireless/ath/ath10k/htt_tx.c
> >>> +++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
> >>> @@ -145,8 +145,10 @@ void ath10k_htt_tx_dec_pending(struct ath10k_htt=
 *htt)
> >>>         lockdep_assert_held(&htt->tx_lock);
> >>>
> >>>         htt->num_pending_tx--;
> >>> -       if (htt->num_pending_tx =3D=3D htt->max_num_pending_tx - 1)
> >>> +       if ((htt->num_pending_tx <=3D (htt->max_num_pending_tx / 4)) =
&& htt->needs_unlock) {
> >>> +               htt->needs_unlock =3D false;
> >>>                 ath10k_mac_tx_unlock(htt->ar, ATH10K_TX_PAUSE_Q_FULL)=
;
> >>> +       }
> >>>  }
> >>>
> >>>  int ath10k_htt_tx_inc_pending(struct ath10k_htt *htt)
> >>> @@ -157,8 +159,10 @@ int ath10k_htt_tx_inc_pending(struct ath10k_htt =
*htt)
> >>>                 return -EBUSY;
> >>>
> >>>         htt->num_pending_tx++;
> >>> -       if (htt->num_pending_tx =3D=3D htt->max_num_pending_tx)
> >>> +       if (htt->num_pending_tx =3D=3D htt->max_num_pending_tx) {
> >>> +               htt->needs_unlock =3D true;
> >>>                 ath10k_mac_tx_lock(htt->ar, ATH10K_TX_PAUSE_Q_FULL);
> >>> +       }
> >>>
> >>>         return 0;
> >>>  }
> >>> --
> >>> 2.20.1
> >>>
> >>
> >
> > --
> > Ben Greear <greearb@candelatech.com>
> > Candela Technologies Inc  http://www.candelatech.com


--=20
Make Music, Not War

Dave T=C3=A4ht
CTO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-831-435-0729