Received-SPF: pass (google.com: domain of linux-wireless+bounces-8511-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <4fdfdb34-7892-4bf9-8da1-e09cf6ff7731@quicinc.com>
Date: Wed, 5 Jun 2024 14:31:31 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 1/3] wifi: ath12k: report station mode transmit rate
To: Kalle Valo <kvalo@kernel.org>
CC: <ath12k@lists.infradead.org>, <linux-wireless@vger.kernel.org>
References: <20240419032122.7009-1-quic_lingbok@quicinc.com>
 <20240419032122.7009-2-quic_lingbok@quicinc.com> <87bk5xs7qj.fsf@kernel.org>
 <050ae0d4-c879-40c2-b2ac-1766aaa2c789@quicinc.com>
 <87v844qsih.fsf@kernel.org>
Content-Language: en-US
From: Lingbo Kong <quic_lingbok@quicinc.com>
In-Reply-To: <87v844qsih.fsf@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 2024/4/26 19:21, Kalle Valo wrote:
> Lingbo Kong <quic_lingbok@quicinc.com> writes:
> 
>> On 2024/4/26 0:54, Kalle Valo wrote:
>>> Lingbo Kong <quic_lingbok@quicinc.com> writes:
>>>
>>>> +static void ath12k_dp_tx_update_txcompl(struct ath12k *ar, struct
>>>> hal_tx_status *ts)
>>>> +{
>>>> +	struct ath12k_base *ab = ar->ab;
>>>> +	struct ath12k_peer *peer;
>>>> +	struct ath12k_sta *arsta;
>>>> +	struct ieee80211_sta *sta;
>>>> +	u16 rate;
>>>> +	u8 rate_idx = 0;
>>>> +	int ret;
>>>> +
>>>> +	spin_lock_bh(&ab->base_lock);
>>>
>>> Did you analyse how this function, and especially taking the
>>> base_lock,
>>> affects performance?
>>
>> The base_lock is used here because of the need to look for peers based
>> on the ts->peer_id when calling ath12k_peer_find_by_id() function,
>> which i think might affect performance.
>>
>> Do i need to run a throughput test?
> 
> Ok, so to answer my question: no, you didn't do any performance
> analysis. Throughput test might not be enough, for example the driver
> can be used on slower systems and running the test on a fast CPU might
> not reveal any problem. A proper analysis would be much better.
> 

Hi, kalle,
I did a simple performance analysis of the ath12k_dp_tx_update_txcompl() 
function on slower systems.

Firstly, i use perf tool to set dynamic tracepoints in 
ath12k_dp_tx_complete_msdu() function, and then used the command of 
"iperf -c ip address -w 4M -n 1G -i 1" to do traffic test.

During this process, use ./perf record -a -g to detect the performace of 
the system.

Finally, compare the results with and without this patch.

without this patch
./perf report output
children    self	command		symbol
7.28%	   0.08%      ksoftirqd/0 ath12k_dp_tx_complete_msdu
5.96%      0.03%      swapper     ath12k_dp_tx_complete_msdu   		

iperf output
[  1] 0.0000-62.6712 sec  1.00 GBytes   137 Mbits/sec

with this patch
children    self       command         symbol
7.42%	   0.08%      ksoftirqd/0  ath12k_dp_tx_complete_msdu
6.32%      0.03%      swapper      ath12k_dp_tx_complete_msdu

iperf output
[  1] 0.0000-62.6732 sec  1.00 GBytes   137 Mbits/sec

As can be seen from the table above, with this patch, the CPU time 
percentage will increase by 0.5%.

So, i think applying this patch will definitely have an impact on system 
performance, but the impact is not that big and i think it can be ignored:)

Best regards
Lingbo Kong