From:   Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@toke.dk>
To:     Kalle Valo <kvalo@codeaurora.org>
Cc:     Grant Grundler <grundler@google.com>, Kan Yan <kyan@google.com>,
        linux-wireless@vger.kernel.org,
        Johannes Berg <johannes@sipsolutions.net>,
        wgong@qti.qualcomm.com, ath10k@lists.infradead.org,
        wgong@codeaurora.org
Subject: Re: [PATCH v2 2/2] ath10k: Set sk_pacing_shift to 6 for 11AC WiFi chips
In-Reply-To: <871s41nmvx.fsf@kamboji.qca.qualcomm.com>
References: <1533724802-30944-1-git-send-email-wgong@codeaurora.org> <1533724802-30944-3-git-send-email-wgong@codeaurora.org> <87sh3pdtpg.fsf@toke.dk> <f185ab8722e84c959b375e606d25a29b@aptaiexm02f.ap.qualcomm.com> <87mutue4y8.fsf@toke.dk> <ef7b317006964c0fa72763bbfc5149e5@aptaiexm02f.ap.qualcomm.com> <CANEJEGvcj9gPT8yy++qvi3hz3t9pAXyeUves06gr+ADfn9Ouhg@mail.gmail.com> <1535967508.3437.31.camel@sipsolutions.net> <87in3m25uu.fsf@toke.dk> <1535975240.3437.61.camel@sipsolutions.net> <878t4i1z74.fsf@toke.dk> <CANEJEGsm_BKM6g2x0+a7_5dBy5jkROs+jEjHmrcneGrMdDK41g@mail.gmail.com> <871sa7ylmi.fsf@toke.dk> <CANEJEGuBoRMBJBOrFOYyPtiJSMfTahrVgTfxZ2ift6Q5uGazFA@mail.gmail.com> <87r2c1i1vj.fsf@toke.dk> <871s41nmvx.fsf@kamboji.qca.qualcomm.com>
Date:   Thu, 21 Feb 2019 17:28:13 +0100
Message-ID: <87h8cxhzs2.fsf@toke.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Kalle Valo <kvalo@codeaurora.org> writes:

> Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk> writes:
>
>> Grant Grundler <grundler@google.com> writes:
>>
>>> On Thu, Sep 6, 2018 at 3:18 AM Toke H=C3=B8iland-J=C3=B8rgensen <toke@t=
oke.dk> wrote:
>>>>
>>>> Grant Grundler <grundler@google.com> writes:
>>>>
>>>> >> And, well, Grant's data is from a single test in a noisy
>>>> >> environment where the time series graph shows that throughput is al=
l over
>>>> >> the place for the duration of the test; so it's hard to draw solid
>>>> >> conclusions from (for instance, for the 5-stream test, the average
>>>> >> throughput for 6 is 331 and 379 Mbps for the two repetitions, and f=
or 7
>>>> >> it's 326 and 371 Mbps) . Unfortunately I don't have the same hardwa=
re
>>>> >> used in this test, so I can't go verify it myself; so the only thin=
g I
>>>> >> can do is grumble about it here... :)
>>>> >
>>>> > It's a fair complaint and I agree with it. My counter argument is the
>>>> > opposite is true too: most ideal benchmarks don't measure what most
>>>> > users see. While the data wgong provided are way more noisy than I
>>>> > like, my overall "confidence" in the "conclusion" I offered is still
>>>> > positive.
>>>>
>>>> Right. I guess I would just prefer a slightly more comprehensive
>>>> evaluation to base a 4x increase in buffer size on...
>>>
>>> Kalle, is this why you didn't accept this patch? Other reasons?
>>>
>>> Toke, what else would you like to see evaluated?
>>>
>>> I generally want to see three things measured when "benchmarking"
>>> technologies: throughput, latency, cpu utilization
>>> We've covered those three I think "reasonably".
>>
>> Hmm, going back and looking at this (I'd completely forgotten about this
>> patch), I think I had two main concerns:
>>
>> 1. What happens in a degraded signal situation, where the throughput is
>>    limited by the signal conditions, or by contention with other devices.
>>    Both of these happen regularly, and I worry that latency will be
>>    badly affected under those conditions.
>>
>> 2. What happens with old hardware that has worse buffer management in
>>    the driver->firmware path (especially drivers without push/pull mode
>>    support)? For these, the lower-level queueing structure is less
>>    effective at controlling queueing latency.
>
> Do note that this patch changes behaviour _only_ for QCA6174 and QCA9377
> PCI devices, which IIRC do not even support push/pull mode. All the
> rest, including QCA988X and QCA9984 are unaffected.

Ah, right; I did not go all the way back and look at the actual patch,
so missed that :)

But in that case, why are the latency results that low? Were these tests
done with the ChromeOS queue limit patches?

-Toke