From: Thiraviyam Mariyappan <tmariyap@codeaurora.org>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: ath11k@lists.infradead.org, linux-wireless@vger.kernel.org
Subject: Re: [PATCHv2] mac80211: increment rx stats according to USES_RSS flag
Date: Wed, 21 Apr 2021 22:18:46 +0530 [thread overview]
Message-ID: <1ee8d562986128767c037d20aedb96a5@codeaurora.org> (raw)
In-Reply-To: <c0aef41d2ecf09188de372fe4f7d6b1954e54e07.camel@sipsolutions.net>
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
next prev parent reply other threads:[~2021-04-21 16:48 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-17 11:56 [PATCHv2] mac80211: increment rx stats according to USES_RSS flag Thiraviyam Mariyappan
2021-04-08 9:39 ` Johannes Berg
2021-04-08 10:01 ` Johannes Berg
2021-04-21 16:48 ` Thiraviyam Mariyappan [this message]
2021-04-23 7:58 ` Johannes Berg
2021-05-06 17:49 ` Thiraviyam Mariyappan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1ee8d562986128767c037d20aedb96a5@codeaurora.org \
--to=tmariyap@codeaurora.org \
--cc=ath11k@lists.infradead.org \
--cc=johannes@sipsolutions.net \
--cc=linux-wireless@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).