Re: [PATCHv2] mac80211: increment rx stats according to USES_RSS flag

[Thiraviyam Mariyappan <tmariyap@codeaurora.org>]

On 2021-04-08 15:31, Johannes Berg wrote:
> On Wed, 2021-02-17 at 17:26 +0530, Thiraviyam Mariyappan wrote:
>> Currently, rx_stats were updated regardless of USES_RSS flag is
>> enabled/disabled. So, updating the rx_stats from percpu pointers
>> according to the USES_RSS flag.
The issue is rx packets not incremented in mesh link and this change 
made to
overcome the issue.
> 
> OK actually, I'm not going to fix the commit log, you'll probably have
> to resend it anyway.
> 
> 
>> @@ -425,7 +426,8 @@ static void mesh_sta_info_init(struct 
>> ieee80211_sub_if_data *sdata,
>>  					&basic_rates);
> 
>>  	spin_lock_bh(&sta->mesh->plink_lock);
>> -	sta->rx_stats.last_rx = jiffies;
>> +	stats = ieee80211_get_rx_stats(&local->hw, sta);
>> +	stats->last_rx = jiffies;
> 
> This doesn't really make much sense? Not sure why that is even updating
> anything at all, it doesn't update anything else?
> 
> Or at least you didn't change anything else, maybe you should have?
>> 
>> @@ -1734,49 +1745,49 @@ ieee80211_rx_h_sta_process(struct 
>> ieee80211_rx_data *rx)
>>  	 * something went wrong the first time.
>>  	 */
>>  	if (rx->sdata->vif.type == NL80211_IFTYPE_ADHOC) {
>> -		u8 *bssid = ieee80211_get_bssid(hdr, rx->skb->len,
>> +		u8 *bssid = ieee80211_get_bssid(hdr, skb->len,
> 
> That seems unrelated.
> 
>> @@ -3625,8 +3648,10 @@ ieee80211_rx_h_mgmt(struct ieee80211_rx_data 
>> *rx)
>>  	/* queue up frame and kick off work to process it */
>>  	skb_queue_tail(&sdata->skb_queue, rx->skb);
>>  	ieee80211_queue_work(&rx->local->hw, &sdata->work);
>> -	if (rx->sta)
>> -		rx->sta->rx_stats.packets++;
>> +	if (rx->sta) {
>> +		stats = ieee80211_get_rx_stats(&rx->sdata->local->hw, rx->sta);
>> +		stats->packets++;
>> +	}
>> 
> 
> Picking this for no particular reason - everything else in this patch 
> is
> unnecessary since we have rx_path_lock held afaict, so it doesn't
> matter. The whole per-cpu status stuff only matters once you get into
> fast-rx path.
In case of Mesh fast_rx is not applicable, but still USES_RSS can be 
enabled from driver
when parallel RX is supported by HW/Driver, right? Hence checked for 
USES_RSS support to
update per cpu stats.Please correct me if the meaning of USES_RSS is 
misunderstood and
it applies only when fast_rx for a STA is enabled.
> 
> 
> I'd argue that had you written a proper commit log that actually says
> why you need to change things, you'd probably even have noticed these
> issues yourself.
> 
> johannes

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