diff mbox

[RFC/RFT,5/5] ath9k: Count RX airtime in airtime deficit

Message ID 20160603165144.17356-6-toke@toke.dk (mailing list archive)
State RFC
Delegated to: Johannes Berg
Headers show

Commit Message

Toke Høiland-Jørgensen June 3, 2016, 4:51 p.m. UTC
This adds RX airtime to the airtime deficit used in the scheduler. This
is not a definite win, but I have only done very limited testing where
it has been included. Feel free to skip this patch when testing.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 drivers/net/wireless/ath/ath9k/recv.c | 51 ++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

Comments

Adrian Chadd June 4, 2016, 5:06 p.m. UTC | #1
hi,

I've been working on something similar in freebsd, so cool to see this
happening here!

The only thing missing atm is STBC and LDPC. My RX airtime code looks
basically like this one too; but I have TODO items for ensuring
LDPC/STBC calculations are sane.



-adrian


On 3 June 2016 at 09:51, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> This adds RX airtime to the airtime deficit used in the scheduler. This
> is not a definite win, but I have only done very limited testing where
> it has been included. Feel free to skip this patch when testing.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
> ---
>  drivers/net/wireless/ath/ath9k/recv.c | 51 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
> index 7eb8980..23d6ebe 100644
> --- a/drivers/net/wireless/ath/ath9k/recv.c
> +++ b/drivers/net/wireless/ath/ath9k/recv.c
> @@ -991,6 +991,55 @@ static void ath9k_apply_ampdu_details(struct ath_softc *sc,
>         }
>  }
>
> +static void ath_rx_count_airtime(struct ath_softc *sc,
> +                                struct ath_rx_status *rs,
> +                                struct sk_buff *skb)
> +{
> +       struct ath_node *an;
> +       struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
> +       struct ath_hw *ah = sc->sc_ah;
> +       struct ath_common *common = ath9k_hw_common(ah);
> +       struct ieee80211_sta *sta;
> +       struct ieee80211_rx_status *rxs;
> +       const struct ieee80211_rate *rate;
> +       bool is_sgi, is_40, is_sp;
> +       int phy;
> +       u32 airtime = 0;
> +
> +       if (!ieee80211_is_data(hdr->frame_control))
> +               return;
> +
> +       rcu_read_lock();
> +
> +       sta = ieee80211_find_sta_by_ifaddr(sc->hw, hdr->addr2, NULL);
> +       if (!sta)
> +               goto exit;
> +       an = (struct ath_node *) sta->drv_priv;
> +       rxs = IEEE80211_SKB_RXCB(skb);
> +
> +       is_sgi = !!(rxs->flag & RX_FLAG_SHORT_GI);
> +       is_40 = !!(rxs->flag & RX_FLAG_40MHZ);
> +       is_sp = !!(rxs->flag & RX_FLAG_SHORTPRE);
> +
> +       if (!!(rxs->flag & RX_FLAG_HT)) {
> +               /* MCS rates */
> +
> +               airtime += ath_pkt_duration(sc, rxs->rate_idx, rs->rs_datalen,
> +                                       is_40, is_sgi, is_sp);
> +       } else {
> +
> +               phy = IS_CCK_RATE(rs->rs_rate) ? WLAN_RC_PHY_CCK : WLAN_RC_PHY_OFDM;
> +               rate = &common->sbands[rxs->band].bitrates[rxs->rate_idx];
> +               airtime += ath9k_hw_computetxtime(ah, phy, rate->bitrate * 100,
> +                                               rs->rs_datalen, rxs->rate_idx, is_sp);
> +       }
> +
> +       an->airtime_deficit -= airtime;
> +       ath_debug_airtime(sc, an, airtime, 0);
> +exit:
> +       rcu_read_unlock();
> +}
> +
>  int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
>  {
>         struct ath_rxbuf *bf;
> @@ -1137,7 +1186,7 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
>                 ath9k_antenna_check(sc, &rs);
>                 ath9k_apply_ampdu_details(sc, &rs, rxs);
>                 ath_debug_rate_stats(sc, &rs, skb);
> -               ath_debug_rx_airtime(sc, &rs, skb);
> +               ath_rx_count_airtime(sc, &rs, skb);
>
>                 hdr = (struct ieee80211_hdr *)skb->data;
>                 if (ieee80211_is_ack(hdr->frame_control))
> --
> 2.7.4
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Toke Høiland-Jørgensen June 5, 2016, 10:55 a.m. UTC | #2
Adrian Chadd <adrian@freebsd.org> writes:

> I've been working on something similar in freebsd, so cool to see this
> happening here!

Cool. Do you have code available somewhere?

> The only thing missing atm is STBC and LDPC. My RX airtime code looks
> basically like this one too; but I have TODO items for ensuring
> LDPC/STBC calculations are sane.

So basically this means taking into account whether there was MIMO in
use (which may lower the transmit time)? My understanding is that (at
least in Linux) this is encoded in the rate tables (i.e. a MIMO rate is
a separate entry). Am I wrong in thinking so? Or is this something else
entirely?

-Toke
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adrian Chadd June 5, 2016, 5:23 p.m. UTC | #3
On 5 June 2016 at 03:55, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> Adrian Chadd <adrian@freebsd.org> writes:
>
>> I've been working on something similar in freebsd, so cool to see this
>> happening here!
>
> Cool. Do you have code available somewhere?

not yet. It was mostly what you just did in ath9k anyway :)

The sample rate control module and net80211 already had the duration
calculation bits, and I was just trying to figure out how to track it
per-node in a useful way without blowing CPU time.

>
>> The only thing missing atm is STBC and LDPC. My RX airtime code looks
>> basically like this one too; but I have TODO items for ensuring
>> LDPC/STBC calculations are sane.
>
> So basically this means taking into account whether there was MIMO in
> use (which may lower the transmit time)? My understanding is that (at
> least in Linux) this is encoded in the rate tables (i.e. a MIMO rate is
> a separate entry). Am I wrong in thinking so? Or is this something else
> entirely?

The existing routines take GI, MCS rate, channel width; figure out
MIMO, and do the math. The assumption is the symbol count is the same.
For STBC I think there's an additional symbol at the end (because it's
mixed current symbol plus previous symbol.) I forget the details for
LDPC.

I'll er, go read the standard again. :)


-adrian
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adrian Chadd June 7, 2016, 12:01 a.m. UTC | #4
[snip]

I also found one of my notes in my version of this - how can we
estimate the duration of an A-MPDU, when we only get hardware
de-encapsulated frames?



-adrian
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jonathan Morton June 7, 2016, 1:31 a.m. UTC | #5
> On 7 Jun, 2016, at 03:01, Adrian Chadd <adrian@freebsd.org> wrote:
> 
> I also found one of my notes in my version of this - how can we
> estimate the duration of an A-MPDU, when we only get hardware
> de-encapsulated frames?

If my understanding is correct:

The A-MPDU itself is sent at the “data” rate (selected by Minstrel), so its total size can be used to calculate this portion of its duration in the same way as any other packet.

It is also surrounded by the same framing/preamble material, sent at the fixed “control” rate, as any normal frame.

The sum of these two major components should be the total airtime, no?

Or is the difficulty in identifying which de-encapsulated frames belonged to a particular A-MPDU?

 - Jonathan Morton

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Toke Høiland-Jørgensen June 7, 2016, 8:58 a.m. UTC | #6
Adrian Chadd <adrian@freebsd.org> writes:

> [snip]
>
> I also found one of my notes in my version of this - how can we
> estimate the duration of an A-MPDU, when we only get hardware
> de-encapsulated frames?

Well in my case I'm sidestepping this by getting the TX duration from a
register in the hardware. There seems to be registers containing the
duration spent on each step in the retry chain; I simply sum these.

-Toke
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Toke Høiland-Jørgensen June 7, 2016, 11:12 a.m. UTC | #7
Toke Høiland-Jørgensen <toke@toke.dk> writes:

>> [snip]
>>
>> I also found one of my notes in my version of this - how can we
>> estimate the duration of an A-MPDU, when we only get hardware
>> de-encapsulated frames?
>
> Well in my case I'm sidestepping this by getting the TX duration from
> a register in the hardware. There seems to be registers containing the
> duration spent on each step in the retry chain; I simply sum these.

Ah, but you're still talking RX? Hmm, I'm using ath_pkt_duration() to
compute the RX time, which does take into account MIMO (I think) but
expects the size to include padding. Which is probably not included in
the rs_datalen field of struct ath_rx_status that I'm using.

So yeah, how to account for that?

I initially thought that using the timestamp put into the frame by the
hardware could be a way to get timing. But there's only a timestamp of
the first symbol in rs_tstamp, and getting a time to compare it with is
difficult; by the time the frame is handled in the rx tasklet, way too
much time has been spent on interrupt handling etc for the current time
to be worth comparing with.

-Toke
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adrian Chadd June 8, 2016, 1:41 a.m. UTC | #8
On 7 June 2016 at 04:12, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> Toke Høiland-Jørgensen <toke@toke.dk> writes:
>
>>> [snip]
>>>
>>> I also found one of my notes in my version of this - how can we
>>> estimate the duration of an A-MPDU, when we only get hardware
>>> de-encapsulated frames?
>>
>> Well in my case I'm sidestepping this by getting the TX duration from
>> a register in the hardware. There seems to be registers containing the
>> duration spent on each step in the retry chain; I simply sum these.
>
> Ah, but you're still talking RX? Hmm, I'm using ath_pkt_duration() to
> compute the RX time, which does take into account MIMO (I think) but
> expects the size to include padding. Which is probably not included in
> the rs_datalen field of struct ath_rx_status that I'm using.
>
> So yeah, how to account for that?
>
> I initially thought that using the timestamp put into the frame by the
> hardware could be a way to get timing. But there's only a timestamp of
> the first symbol in rs_tstamp, and getting a time to compare it with is
> difficult; by the time the frame is handled in the rx tasklet, way too
> much time has been spent on interrupt handling etc for the current time
> to be worth comparing with.

Right. In the case of RX'ing an A-MPDU, we only get told about the
A-MPDU boundaries (isaggr/lastaggr or something in the RX descriptor)
but nothing telling us how long the original RX'ed PPDU is.

So if we get say 16 frames and we are missing the middle one, we can
reconstruct things okay. But if we miss the first 8 frames, we don't
know when it started - we only get the RX aggr boundary flags set on
the 9th and the 16th and we don't even know about the missed frames.

I think that's going to be a shortcoming right now. I couldn't think
of a clever way to figure it out except to detect holes in the BAW and
determine the client is missing frames and take actions there.


-adrian
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Toke Høiland-Jørgensen June 8, 2016, 1:06 p.m. UTC | #9
Adrian Chadd <adrian@freebsd.org> writes:

> Right. In the case of RX'ing an A-MPDU, we only get told about the
> A-MPDU boundaries (isaggr/lastaggr or something in the RX descriptor)
> but nothing telling us how long the original RX'ed PPDU is.
>
> So if we get say 16 frames and we are missing the middle one, we can
> reconstruct things okay. But if we miss the first 8 frames, we don't
> know when it started - we only get the RX aggr boundary flags set on
> the 9th and the 16th and we don't even know about the missed frames.

Yes, discovering retransmissions is difficult at the RX side by nature.
Another approach to catching (some of) these is trying to parse the MAC
address even for corrupted frames, and if it fits to a station, account
the airtime. Of course this will only catch cases where the frame header
does make it through, and it won't work if the hardware discards the
frame as corrupt before we get to see it.

> I think that's going to be a shortcoming right now. I couldn't think
> of a clever way to figure it out except to detect holes in the BAW and
> determine the client is missing frames and take actions there.

Yes, I'm fine with having the RX side airtime accounting just ignore the
retransmission issue, at least for now.

-Toke
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Kazior June 10, 2016, 8:40 a.m. UTC | #10
On 7 June 2016 at 13:12, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> Toke Høiland-Jørgensen <toke@toke.dk> writes:
>
>>> [snip]
>>>
>>> I also found one of my notes in my version of this - how can we
>>> estimate the duration of an A-MPDU, when we only get hardware
>>> de-encapsulated frames?
>>
>> Well in my case I'm sidestepping this by getting the TX duration from
>> a register in the hardware. There seems to be registers containing the
>> duration spent on each step in the retry chain; I simply sum these.
>
> Ah, but you're still talking RX? Hmm, I'm using ath_pkt_duration() to
> compute the RX time, which does take into account MIMO (I think) but
> expects the size to include padding. Which is probably not included in
> the rs_datalen field of struct ath_rx_status that I'm using.
>
> So yeah, how to account for that?
>
> I initially thought that using the timestamp put into the frame by the
> hardware could be a way to get timing. But there's only a timestamp of
> the first symbol in rs_tstamp, and getting a time to compare it with is
> difficult; by the time the frame is handled in the rx tasklet, way too
> much time has been spent on interrupt handling etc for the current time
> to be worth comparing with.

I think rs_tstamp in rx-status is different for first MPDU and last
MPDU in an A-MPDU meaning you should be able to compute the entire
duration (if you track it, and this should be fairly straightforward
as you can't really rx interleaved MPDUs from different
A-MPDUs/stations). I'm not sure if the last MPDU defines the tstamp of
first symbol or last one.


Michał
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Toke Høiland-Jørgensen June 10, 2016, 8:53 a.m. UTC | #11
>> I initially thought that using the timestamp put into the frame by the
>> hardware could be a way to get timing. But there's only a timestamp of
>> the first symbol in rs_tstamp, and getting a time to compare it with is
>> difficult; by the time the frame is handled in the rx tasklet, way too
>> much time has been spent on interrupt handling etc for the current time
>> to be worth comparing with.

As an aside, I'm no longer sure this explanation for why I got wrong
timings for this way of measuring RX time is correct. It may simply be
that I was counting the same time interval more than once because I
didn't realise that the handler would be called for each frame. See
below.

> I think rs_tstamp in rx-status is different for first MPDU and last
> MPDU in an A-MPDU meaning you should be able to compute the entire
> duration (if you track it, and this should be fairly straightforward
> as you can't really rx interleaved MPDUs from different
> A-MPDUs/stations). I'm not sure if the last MPDU defines the tstamp of
> first symbol or last one.

I actually went down this path again last night, but haven't had a
chance to test it yet.

So what I have now is this:

       /* Only count airtime for last frame in an aggregate. FIXME: Should
        * this be only the first frame? */
       if (!rs->rs_isaggr || !rs->rs_moreaggr)
               airtime = (tsf & 0xffffffff) - rs->rs_tstamp;

Which was under the assumption that rs_tstamp will be the time of the
first symbol *of the whole ampdu* for all the frames in that aggregate.
However, if rs_tstamp differs between frames, tracking it at the first
frame might be the right things to do?

Is the entire A-MPDU received before the RX handler is called for the
first frame?

-Toke
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Kazior June 10, 2016, 9:02 a.m. UTC | #12
On 10 June 2016 at 10:53, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
>>> I initially thought that using the timestamp put into the frame by the
>>> hardware could be a way to get timing. But there's only a timestamp of
>>> the first symbol in rs_tstamp, and getting a time to compare it with is
>>> difficult; by the time the frame is handled in the rx tasklet, way too
>>> much time has been spent on interrupt handling etc for the current time
>>> to be worth comparing with.
>
> As an aside, I'm no longer sure this explanation for why I got wrong
> timings for this way of measuring RX time is correct. It may simply be
> that I was counting the same time interval more than once because I
> didn't realise that the handler would be called for each frame. See
> below.
>
>> I think rs_tstamp in rx-status is different for first MPDU and last
>> MPDU in an A-MPDU meaning you should be able to compute the entire
>> duration (if you track it, and this should be fairly straightforward
>> as you can't really rx interleaved MPDUs from different
>> A-MPDUs/stations). I'm not sure if the last MPDU defines the tstamp of
>> first symbol or last one.
>
> I actually went down this path again last night, but haven't had a
> chance to test it yet.
>
> So what I have now is this:
>
>        /* Only count airtime for last frame in an aggregate. FIXME: Should
>         * this be only the first frame? */
>        if (!rs->rs_isaggr || !rs->rs_moreaggr)
>                airtime = (tsf & 0xffffffff) - rs->rs_tstamp;
>
> Which was under the assumption that rs_tstamp will be the time of the
> first symbol *of the whole ampdu* for all the frames in that aggregate.
> However, if rs_tstamp differs between frames, tracking it at the first
> frame might be the right things to do?

For A-MPDU all MPDU rx status (except last one) should share the same
timestamp. Last one has a different one so all you need is to
distinguish first and last MPDU. Non A-MPDU obviously are special case
(status bits are pricky).


> Is the entire A-MPDU received before the RX handler is called for the
> first frame?

No idea. Maybe it is as there's distinction between "more" and "moreaggr".


Michał
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Toke Høiland-Jørgensen June 10, 2016, 9:08 a.m. UTC | #13
Michal Kazior <michal.kazior@tieto.com> writes:

> For A-MPDU all MPDU rx status (except last one) should share the same
> timestamp. Last one has a different one so all you need is to
> distinguish first and last MPDU. Non A-MPDU obviously are special case
> (status bits are pricky).

Right. So comparing the rs_stamp between first and last MPDU should give
the duration of the entire thing? This would require keeping state
between subsequent calls to the RX handler. Also, what happens if the
last MPDU is lost?

>> Is the entire A-MPDU received before the RX handler is called for the
>> first frame?
>
> No idea. Maybe it is as there's distinction between "more" and
> "moreaggr".

Hmm. If it is, comparing the stamp of the first MPDU to the current time
(when handling it) should give the needed duration? Will try doing that
and see what the result is.

-Toke
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Kazior June 10, 2016, 9:20 a.m. UTC | #14
On 10 June 2016 at 11:08, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> For A-MPDU all MPDU rx status (except last one) should share the same
>> timestamp. Last one has a different one so all you need is to
>> distinguish first and last MPDU. Non A-MPDU obviously are special case
>> (status bits are pricky).
>
> Right. So comparing the rs_stamp between first and last MPDU should give
> the duration of the entire thing?

Depends on how you define your "thing" :) I no, I don't know what
you'll actually measure. It should be reasonable either way.


> This would require keeping state
> between subsequent calls to the RX handler. Also, what happens if the
> last MPDU is lost?

No idea. If that's possible, then track last MPDU within PPDU, so you
can at least fallback to _something_ when you detect a new first
(A-)MPDU?

Or maybe it's impossible (i.e. not worth worrying) and HW always
reports last MPDU as far as status bits are concerned (regardless of
it being _actual_ last MPDU, i.e. it just says "ok, I'm done with this
PPDU").


>>> Is the entire A-MPDU received before the RX handler is called for the
>>> first frame?
>>
>> No idea. Maybe it is as there's distinction between "more" and
>> "moreaggr".
>
> Hmm. If it is, comparing the stamp of the first MPDU to the current time
> (when handling it) should give the needed duration? Will try doing that
> and see what the result is.

I'd say it's a little racy/inaccurate (and perhaps unreliable) to
compare any kind of global timer and compare it against your rx-status
descriptors.


Michał
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Toke Høiland-Jørgensen June 10, 2016, 9:49 a.m. UTC | #15
Michal Kazior <michal.kazior@tieto.com> writes:

> On 10 June 2016 at 11:08, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> Michal Kazior <michal.kazior@tieto.com> writes:
>>
>>> For A-MPDU all MPDU rx status (except last one) should share the same
>>> timestamp. Last one has a different one so all you need is to
>>> distinguish first and last MPDU. Non A-MPDU obviously are special case
>>> (status bits are pricky).
>>
>> Right. So comparing the rs_stamp between first and last MPDU should give
>> the duration of the entire thing?
>
> Depends on how you define your "thing" :) I no, I don't know what
> you'll actually measure. It should be reasonable either way.

"Time spent receiving from this station" is the overall metric I want.
In this case that then becomes "Time spend receiving this aggregate"
which I can then account to that stations' total RX airtime.

>> This would require keeping state between subsequent calls to the RX
>> handler. Also, what happens if the last MPDU is lost?
>
> No idea. If that's possible, then track last MPDU within PPDU, so you
> can at least fallback to _something_ when you detect a new first
> (A-)MPDU?
>
> Or maybe it's impossible (i.e. not worth worrying) and HW always
> reports last MPDU as far as status bits are concerned (regardless of
> it being _actual_ last MPDU, i.e. it just says "ok, I'm done with this
> PPDU").

I'll try a couple of different permutations of this and see what works.
Thanks for the ideas!

>>> No idea. Maybe it is as there's distinction between "more" and
>>> "moreaggr".
>>
>> Hmm. If it is, comparing the stamp of the first MPDU to the current time
>> (when handling it) should give the needed duration? Will try doing that
>> and see what the result is.
>
> I'd say it's a little racy/inaccurate (and perhaps unreliable) to
> compare any kind of global timer and compare it against your rx-status
> descriptors.

Well, the timer would be the one coming from the card (tsf in the code
snippet I posted comes from ath9k_hw_gettsf64()). But yeah, it is going
to account some things that are not *strictly* air time (interrupt
latency for one). However, the question is how much this really
contributes (at least in cases where we are not running behind because
of lack of CPU). And if there's a constant overhead it shouldn't matter
so much as long as it is the same for all stations. Certainly just
timestamping packets with the current time works well enough in the
qdisc layer...

-Toke
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Toke Høiland-Jørgensen June 10, 2016, 3:33 p.m. UTC | #16
Toke Høiland-Jørgensen <toke@toke.dk> writes:

>> No idea. If that's possible, then track last MPDU within PPDU, so you
>> can at least fallback to _something_ when you detect a new first
>> (A-)MPDU?
>>
>> Or maybe it's impossible (i.e. not worth worrying) and HW always
>> reports last MPDU as far as status bits are concerned (regardless of
>> it being _actual_ last MPDU, i.e. it just says "ok, I'm done with this
>> PPDU").
>
> I'll try a couple of different permutations of this and see what works.
> Thanks for the ideas!

OK, so having tried all the different approaches, this seems to be the
best one. Basically, this:

	if (rs->rs_isaggr && rs->rs_firstaggr) {
		an->airtime_rx_start = rs->rs_tstamp;
	} else if (rs->rs_isaggr && !rs->rs_moreaggr && an->airtime_rx_start) {
		airtime = rs->rs_tstamp - an->airtime_rx_start;
	} else if (!rs->rs_isaggr) {
		an->airtime_rx_start = 0;
                /* keeping the previous length-based calculation here */
        }


This seems to give me RX airtime figures that correspond fairly well to
the TX airtime for running the same test in the opposite direction (I'm
running a ten-second UDP flood test and looking at the total airtime
accounted after the end of the run).

-Toke
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adrian Chadd June 10, 2016, 3:52 p.m. UTC | #17
[picking a random email to reply to]

I /think/ the TSF is only valid for the last frame of an aggregate.

'more' means "this RX frame is split across multiple RX descriptors"
"moreaggr" means "there's more subframes in this RX'ed A-MPDU"

If you get a TSF in the first frame, it's whatever was left over at
that point in the RX FIFO when it was last updated; it isn't an actual
timestamp timestamp. The logic for populating the descriptor contents
just takes stuff out of certain offsets in the RX FIFO, and sometimes
that contains stale/wrong data.

I'll go and double/triple check this (as well as where the TSF is
actually stamped - I think it's actually stamped at the leading edge
of a packet, but stamped at the /end/ of it.)


-adrian
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 7eb8980..23d6ebe 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -991,6 +991,55 @@  static void ath9k_apply_ampdu_details(struct ath_softc *sc,
 	}
 }
 
+static void ath_rx_count_airtime(struct ath_softc *sc,
+				 struct ath_rx_status *rs,
+				 struct sk_buff *skb)
+{
+	struct ath_node *an;
+	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
+	struct ath_hw *ah = sc->sc_ah;
+	struct ath_common *common = ath9k_hw_common(ah);
+	struct ieee80211_sta *sta;
+	struct ieee80211_rx_status *rxs;
+	const struct ieee80211_rate *rate;
+	bool is_sgi, is_40, is_sp;
+	int phy;
+	u32 airtime = 0;
+
+	if (!ieee80211_is_data(hdr->frame_control))
+		return;
+
+	rcu_read_lock();
+
+	sta = ieee80211_find_sta_by_ifaddr(sc->hw, hdr->addr2, NULL);
+	if (!sta)
+		goto exit;
+	an = (struct ath_node *) sta->drv_priv;
+	rxs = IEEE80211_SKB_RXCB(skb);
+
+	is_sgi = !!(rxs->flag & RX_FLAG_SHORT_GI);
+	is_40 = !!(rxs->flag & RX_FLAG_40MHZ);
+	is_sp = !!(rxs->flag & RX_FLAG_SHORTPRE);
+
+	if (!!(rxs->flag & RX_FLAG_HT)) {
+		/* MCS rates */
+
+		airtime += ath_pkt_duration(sc, rxs->rate_idx, rs->rs_datalen,
+					is_40, is_sgi, is_sp);
+	} else {
+
+		phy = IS_CCK_RATE(rs->rs_rate) ? WLAN_RC_PHY_CCK : WLAN_RC_PHY_OFDM;
+		rate = &common->sbands[rxs->band].bitrates[rxs->rate_idx];
+		airtime += ath9k_hw_computetxtime(ah, phy, rate->bitrate * 100,
+						rs->rs_datalen, rxs->rate_idx, is_sp);
+	}
+
+	an->airtime_deficit -= airtime;
+	ath_debug_airtime(sc, an, airtime, 0);
+exit:
+	rcu_read_unlock();
+}
+
 int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
 {
 	struct ath_rxbuf *bf;
@@ -1137,7 +1186,7 @@  int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
 		ath9k_antenna_check(sc, &rs);
 		ath9k_apply_ampdu_details(sc, &rs, rxs);
 		ath_debug_rate_stats(sc, &rs, skb);
-		ath_debug_rx_airtime(sc, &rs, skb);
+		ath_rx_count_airtime(sc, &rs, skb);
 
 		hdr = (struct ieee80211_hdr *)skb->data;
 		if (ieee80211_is_ack(hdr->frame_control))