Collect statistics about recived duplicate and STBC packets.
This information should help see if STBC is actually working.
Tested on ar9285;
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/wireless/ath/ath9k/debug.c | 20 +++++++++++++++++++-
drivers/net/wireless/ath/ath9k/debug.h | 4 ++++
drivers/net/wireless/ath/ath9k/mac.c | 5 +++++
drivers/net/wireless/ath/ath9k/mac.h | 14 +++++++++++---
4 files changed, 39 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
index e6307b8..f6c0288 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -848,7 +848,7 @@ static ssize_t read_file_recv(struct file *file, char __user *user_buf,
struct ath_softc *sc = file->private_data;
char *buf;
- unsigned int len = 0, size = 1600;
+ unsigned int len = 0, size = 2048;
ssize_t retval = 0;
buf = kzalloc(size, GFP_KERNEL);
@@ -900,6 +900,11 @@ static ssize_t read_file_recv(struct file *file, char __user *user_buf,
RXS_ERR("RX-Frags", rx_frags);
RXS_ERR("RX-Spectral", rx_spectral);
+ RXS_ERR("RX-GI", rx_gi);
+ RXS_ERR("RX-HT40", rx_ht40);
+ RXS_ERR("RX-Duplicate", rx_duplicate);
+ RXS_ERR("RX-STBC", rx_stbc);
+
if (len > size)
len = size;
@@ -939,6 +944,14 @@ void ath_debug_stat_rx(struct ath_softc *sc, struct ath_rx_status *rs)
if (rs->rs_phyerr < ATH9K_PHYERR_MAX)
RX_PHY_ERR_INC(rs->rs_phyerr);
}
+ if (rs->rs_flags & ATH9K_RX_GI)
+ RX_STAT_INC(rx_gi);
+ if (rs->rs_flags & ATH9K_RX_2040)
+ RX_STAT_INC(rx_ht40);
+ if (rs->rs_flags_2 & ATH9K_RX_DUP)
+ RX_STAT_INC(rx_duplicate);
+ if (rs->rs_flags_2 & ATH9K_RX_STBC)
+ RX_STAT_INC(rx_stbc);
#ifdef CONFIG_ATH9K_MAC_DEBUG
spin_lock(&sc->debug.samp_lock);
@@ -1993,6 +2006,11 @@ void ath9k_get_et_stats(struct ieee80211_hw *hw,
AWDATA(data_underrun);
AWDATA(delim_underrun);
+ AWDATA_RX(rx_gi);
+ AWDATA_RX(rx_ht40);
+ AWDATA_RX(rx_duplicate);
+ AWDATA_RX(rx_stbc);
+
AWDATA_RX(crc_err);
AWDATA_RX(decrypt_crc_err);
AWDATA_RX(phy_err);
diff --git a/drivers/net/wireless/ath/ath9k/debug.h b/drivers/net/wireless/ath/ath9k/debug.h
index 794a7ec..bdae1fd 100644
--- a/drivers/net/wireless/ath/ath9k/debug.h
+++ b/drivers/net/wireless/ath/ath9k/debug.h
@@ -241,6 +241,10 @@ struct ath_rx_stats {
u32 rx_beacons;
u32 rx_frags;
u32 rx_spectral;
+ u32 rx_gi;
+ u32 rx_ht40;
+ u32 rx_duplicate;
+ u32 rx_stbc;
};
struct ath_stats {
diff --git a/drivers/net/wireless/ath/ath9k/mac.c b/drivers/net/wireless/ath/ath9k/mac.c
index 498fee0..75ea079 100644
--- a/drivers/net/wireless/ath/ath9k/mac.c
+++ b/drivers/net/wireless/ath/ath9k/mac.c
@@ -547,6 +547,7 @@ int ath9k_hw_rxprocdesc(struct ath_hw *ah, struct ath_desc *ds,
rs->rs_status = 0;
rs->rs_flags = 0;
+ rs->rs_flags_2 = 0;
rs->rs_datalen = ads.ds_rxstatus1 & AR_DataLen;
rs->rs_tstamp = ads.AR_RcvTimestamp;
@@ -590,6 +591,10 @@ int ath9k_hw_rxprocdesc(struct ath_hw *ah, struct ath_desc *ds,
(ads.ds_rxstatus3 & AR_GI) ? ATH9K_RX_GI : 0;
rs->rs_flags |=
(ads.ds_rxstatus3 & AR_2040) ? ATH9K_RX_2040 : 0;
+ rs->rs_flags_2 |=
+ (ads.ds_rxstatus3 & AR_RXST_DUP) ? ATH9K_RX_DUP : 0;
+ rs->rs_flags_2 |=
+ (ads.ds_rxstatus3 & AR_RXST_STBC) ? ATH9K_RX_STBC : 0;
if (ads.ds_rxstatus8 & AR_PreDelimCRCErr)
rs->rs_flags |= ATH9K_RX_DELIM_CRC_PRE;
diff --git a/drivers/net/wireless/ath/ath9k/mac.h b/drivers/net/wireless/ath/ath9k/mac.h
index 5865f92..958d72f 100644
--- a/drivers/net/wireless/ath/ath9k/mac.h
+++ b/drivers/net/wireless/ath/ath9k/mac.h
@@ -143,6 +143,7 @@ struct ath_rx_status {
u8 rs_moreaggr;
u8 rs_num_delims;
u8 rs_flags;
+ u8 rs_flags_2;
bool is_mybeacon;
u32 evm0;
u32 evm1;
@@ -185,6 +186,7 @@ struct ath_htc_rx_status {
#define ATH9K_RXERR_KEYMISS 0x20
#define ATH9K_RXERR_CORRUPT_DESC 0x40
+/* rs_flags */
#define ATH9K_RX_MORE 0x01
#define ATH9K_RX_MORE_AGGR 0x02
#define ATH9K_RX_GI 0x04
@@ -193,6 +195,10 @@ struct ath_htc_rx_status {
#define ATH9K_RX_DELIM_CRC_POST 0x20
#define ATH9K_RX_DECRYPT_BUSY 0x40
+/* rs_flags_2 */
+#define ATH9K_RX_DUP 0x01
+#define ATH9K_RX_STBC 0x02
+
#define ATH9K_RXKEYIX_INVALID ((u8)-1)
#define ATH9K_TXKEYIX_INVALID ((u8)-1)
@@ -529,11 +535,13 @@ struct ar5416_desc {
#define AR_RcvTimestamp ds_rxstatus2
+/* rxstatus3 */
#define AR_GI 0x00000001
#define AR_2040 0x00000002
-#define AR_Parallel40 0x00000004
-#define AR_Parallel40_S 2
-#define AR_RxStatusRsvd30 0x000000f8
+/* revision specific */
+#define AR_RXST_DUP 0x00000004 /* duplicate packets */
+#define AR_RXST_STBC 0x00000008
+#define AR_RxStatusRsvd30 0x000000f0
#define AR_RxAntenna 0xffffff00
#define AR_RxAntenna_S 8
--
1.8.1.2
Collect statistics about recived duplicate and STBC packets.
This information should help see if STBC is actually working.
Tested on ar9285; Should work for all chips after ar9280.
Changes:
- v2. test for stbc vield only on ar9280 and later.
reanme rx_gi to rx_short_gi
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/wireless/ath/ath9k/debug.c | 20 +++++++++++++++++++-
drivers/net/wireless/ath/ath9k/debug.h | 4 ++++
drivers/net/wireless/ath/ath9k/mac.c | 7 +++++++
drivers/net/wireless/ath/ath9k/mac.h | 13 ++++++++++---
4 files changed, 40 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
index e6307b8..fec68f3 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -848,7 +848,7 @@ static ssize_t read_file_recv(struct file *file, char __user *user_buf,
struct ath_softc *sc = file->private_data;
char *buf;
- unsigned int len = 0, size = 1600;
+ unsigned int len = 0, size = 2048;
ssize_t retval = 0;
buf = kzalloc(size, GFP_KERNEL);
@@ -900,6 +900,11 @@ static ssize_t read_file_recv(struct file *file, char __user *user_buf,
RXS_ERR("RX-Frags", rx_frags);
RXS_ERR("RX-Spectral", rx_spectral);
+ RXS_ERR("RX-ShortGI", rx_short_gi);
+ RXS_ERR("RX-HT40", rx_ht40);
+ RXS_ERR("RX-Duplicate", rx_duplicate);
+ RXS_ERR("RX-STBC", rx_stbc);
+
if (len > size)
len = size;
@@ -939,6 +944,14 @@ void ath_debug_stat_rx(struct ath_softc *sc, struct ath_rx_status *rs)
if (rs->rs_phyerr < ATH9K_PHYERR_MAX)
RX_PHY_ERR_INC(rs->rs_phyerr);
}
+ if (rs->rs_flags & ATH9K_RX_GI)
+ RX_STAT_INC(rx_short_gi);
+ if (rs->rs_flags & ATH9K_RX_2040)
+ RX_STAT_INC(rx_ht40);
+ if (rs->rs_flags_2 & ATH9K_RX_DUP)
+ RX_STAT_INC(rx_duplicate);
+ if (rs->rs_flags_2 & ATH9K_RX_STBC)
+ RX_STAT_INC(rx_stbc);
#ifdef CONFIG_ATH9K_MAC_DEBUG
spin_lock(&sc->debug.samp_lock);
@@ -1993,6 +2006,11 @@ void ath9k_get_et_stats(struct ieee80211_hw *hw,
AWDATA(data_underrun);
AWDATA(delim_underrun);
+ AWDATA_RX(rx_short_gi);
+ AWDATA_RX(rx_ht40);
+ AWDATA_RX(rx_duplicate);
+ AWDATA_RX(rx_stbc);
+
AWDATA_RX(crc_err);
AWDATA_RX(decrypt_crc_err);
AWDATA_RX(phy_err);
diff --git a/drivers/net/wireless/ath/ath9k/debug.h b/drivers/net/wireless/ath/ath9k/debug.h
index 794a7ec..639dae9 100644
--- a/drivers/net/wireless/ath/ath9k/debug.h
+++ b/drivers/net/wireless/ath/ath9k/debug.h
@@ -241,6 +241,10 @@ struct ath_rx_stats {
u32 rx_beacons;
u32 rx_frags;
u32 rx_spectral;
+ u32 rx_short_gi;
+ u32 rx_ht40;
+ u32 rx_duplicate;
+ u32 rx_stbc;
};
struct ath_stats {
diff --git a/drivers/net/wireless/ath/ath9k/mac.c b/drivers/net/wireless/ath/ath9k/mac.c
index 498fee0..2064d45 100644
--- a/drivers/net/wireless/ath/ath9k/mac.c
+++ b/drivers/net/wireless/ath/ath9k/mac.c
@@ -547,6 +547,7 @@ int ath9k_hw_rxprocdesc(struct ath_hw *ah, struct ath_desc *ds,
rs->rs_status = 0;
rs->rs_flags = 0;
+ rs->rs_flags_2 = 0;
rs->rs_datalen = ads.ds_rxstatus1 & AR_DataLen;
rs->rs_tstamp = ads.AR_RcvTimestamp;
@@ -590,6 +591,12 @@ int ath9k_hw_rxprocdesc(struct ath_hw *ah, struct ath_desc *ds,
(ads.ds_rxstatus3 & AR_GI) ? ATH9K_RX_GI : 0;
rs->rs_flags |=
(ads.ds_rxstatus3 & AR_2040) ? ATH9K_RX_2040 : 0;
+ rs->rs_flags_2 |=
+ (ads.ds_rxstatus3 & AR_RXST_DUP) ? ATH9K_RX_DUP : 0;
+
+ if (AR_SREV_9280_20_OR_LATER(ah))
+ rs->rs_flags_2 |=
+ (ads.ds_rxstatus3 & AR_RXST_STBC) ? ATH9K_RX_STBC : 0;
if (ads.ds_rxstatus8 & AR_PreDelimCRCErr)
rs->rs_flags |= ATH9K_RX_DELIM_CRC_PRE;
diff --git a/drivers/net/wireless/ath/ath9k/mac.h b/drivers/net/wireless/ath/ath9k/mac.h
index 5865f92..5e5a3af 100644
--- a/drivers/net/wireless/ath/ath9k/mac.h
+++ b/drivers/net/wireless/ath/ath9k/mac.h
@@ -143,6 +143,7 @@ struct ath_rx_status {
u8 rs_moreaggr;
u8 rs_num_delims;
u8 rs_flags;
+ u8 rs_flags_2;
bool is_mybeacon;
u32 evm0;
u32 evm1;
@@ -185,6 +186,7 @@ struct ath_htc_rx_status {
#define ATH9K_RXERR_KEYMISS 0x20
#define ATH9K_RXERR_CORRUPT_DESC 0x40
+/* rs_flags */
#define ATH9K_RX_MORE 0x01
#define ATH9K_RX_MORE_AGGR 0x02
#define ATH9K_RX_GI 0x04
@@ -193,6 +195,10 @@ struct ath_htc_rx_status {
#define ATH9K_RX_DELIM_CRC_POST 0x20
#define ATH9K_RX_DECRYPT_BUSY 0x40
+/* rs_flags_2 */
+#define ATH9K_RX_DUP 0x01
+#define ATH9K_RX_STBC 0x02
+
#define ATH9K_RXKEYIX_INVALID ((u8)-1)
#define ATH9K_TXKEYIX_INVALID ((u8)-1)
@@ -529,11 +535,12 @@ struct ar5416_desc {
#define AR_RcvTimestamp ds_rxstatus2
+/* rxstatus3 */
#define AR_GI 0x00000001
#define AR_2040 0x00000002
-#define AR_Parallel40 0x00000004
-#define AR_Parallel40_S 2
-#define AR_RxStatusRsvd30 0x000000f8
+#define AR_RXST_DUP 0x00000004 /* duplicate packets */
+#define AR_RXST_STBC 0x00000008 /* ar9280 and later */
+#define AR_RxStatusRsvd30 0x000000f0
#define AR_RxAntenna 0xffffff00
#define AR_RxAntenna_S 8
--
1.8.1.2
On 2013-04-28 9:19 PM, Oleksij Rempel wrote:
> Am 28.04.2013 17:03, schrieb Oleksij Rempel:
>> Am 28.04.2013 16:13, schrieb Oleksij Rempel:
>>> Am 28.04.2013 14:51, schrieb Felix Fietkau:
>>>> On 2013-04-27 5:25 PM, Oleksij Rempel wrote:
>>>>> Collect statistics about recived duplicate and STBC packets.
>>>>> This information should help see if STBC is actually working.
>>>>>
>>>>> Tested on ar9285;
>>>>>
>>>>> Signed-off-by: Oleksij Rempel <[email protected]>
>>>> I thought about this patch some more, and I'm wondering what's the point
>>>> in doing this? These statistics are going to be completely useless for
>>>> most people and they'll waste some memory/cpu cycles, especially on
>>>> small-cache devices. I think it's much more useful to simply pass the
>>>> information to mac80211 via rx flags and get them added to the radiotap
>>>> header.
>>>
>>> Sure.
>
> Now i need some help.
> Why there is no traces of radiotap in ath9k. Some other drivers have
> them? Do it communicate in some different way or there is just no
> radiotap support in ath9k?
radiotap is generated in mac80211, so there's no need for the driver to
do it.
- Felix
On 2013-04-28 4:54 PM, Ben Greear wrote:
> On 04/28/2013 05:51 AM, Felix Fietkau wrote:
>> On 2013-04-27 5:25 PM, Oleksij Rempel wrote:
>>> Collect statistics about recived duplicate and STBC packets.
>>> This information should help see if STBC is actually working.
>>>
>>> Tested on ar9285;
>>>
>>> Signed-off-by: Oleksij Rempel <[email protected]>
>> I thought about this patch some more, and I'm wondering what's the point
>> in doing this? These statistics are going to be completely useless for
>> most people and they'll waste some memory/cpu cycles, especially on
>> small-cache devices. I think it's much more useful to simply pass the
>> information to mac80211 via rx flags and get them added to the radiotap
>> header.
>> I'd like to keep the number of 'poor man's debug hacks' in the driver to
>> a minimum, and there are some other things that I think should be
>> removed: rx_frags and rx_beacons in struct ath_rx_stats, the tx/rx MAC
>> sampling hack, and pretty much anything else that can be just as easily
>> accessed from mac80211 through regular interfaces.
>
> Does that mean we can just put the stats in mac80211, or do we have
> to be running a sniffer to gather the stats?
Right now you'd have to use a sniffer, but if you really care about
getting specific stats it might make sense to write a kernel module that
attaches to a monitor interface and gathers them (maybe even with
support for gathering arbitrary stats by attaching bpf filters).
The problem I have with the current stats is they're just an arbitrary
collection of random stuff that is probably useless for 99% of all
users. In many cases the way the stats are collected also makes the data
completely meaningless (e.g. because the source/destination address is
not taken into account).
Why care about the number of packets on the air that were sent with a
specific rate flag? Why care about the number of beacons on the air
(with no filter on a set of APs or anything)? Or what about the number
of fragments received? To me it just looks like an incoherent set of
useless facts.
- Felix
Am 28.04.2013 17:03, schrieb Oleksij Rempel:
> Am 28.04.2013 16:13, schrieb Oleksij Rempel:
>> Am 28.04.2013 14:51, schrieb Felix Fietkau:
>>> On 2013-04-27 5:25 PM, Oleksij Rempel wrote:
>>>> Collect statistics about recived duplicate and STBC packets.
>>>> This information should help see if STBC is actually working.
>>>>
>>>> Tested on ar9285;
>>>>
>>>> Signed-off-by: Oleksij Rempel <[email protected]>
>>> I thought about this patch some more, and I'm wondering what's the point
>>> in doing this? These statistics are going to be completely useless for
>>> most people and they'll waste some memory/cpu cycles, especially on
>>> small-cache devices. I think it's much more useful to simply pass the
>>> information to mac80211 via rx flags and get them added to the radiotap
>>> header.
>>
>> Sure.
Now i need some help.
Why there is no traces of radiotap in ath9k. Some other drivers have
them? Do it communicate in some different way or there is just no
radiotap support in ath9k?
--
Regards,
Oleksij
Am 27.04.2013 20:51, schrieb Adrian Chadd:
> Hiya,
>
> Why not just bump rs_flags to be a u16, rather than a u8? then you
> don't need an rs_flags_2.
ok
> (And then go and re-align things inside that struct so you don't waste space.)
hmm.. what do you mean here?
--
Regards,
Oleksij
Am 28.04.2013 14:51, schrieb Felix Fietkau:
> On 2013-04-27 5:25 PM, Oleksij Rempel wrote:
>> Collect statistics about recived duplicate and STBC packets.
>> This information should help see if STBC is actually working.
>>
>> Tested on ar9285;
>>
>> Signed-off-by: Oleksij Rempel <[email protected]>
> I thought about this patch some more, and I'm wondering what's the point
> in doing this? These statistics are going to be completely useless for
> most people and they'll waste some memory/cpu cycles, especially on
> small-cache devices. I think it's much more useful to simply pass the
> information to mac80211 via rx flags and get them added to the radiotap
> header.
Sure.
> I'd like to keep the number of 'poor man's debug hacks' in the driver to
> a minimum,
well, i'll prefer it to call with my name: lazy man's hack ;)
> and there are some other things that I think should be
> removed: rx_frags and rx_beacons in struct ath_rx_stats, the tx/rx MAC
> sampling hack, and pretty much anything else that can be just as easily
> accessed from mac80211 through regular interfaces.
I think there is more things to do in ath9k. Create the list and we can
talk about it. For example one of it: I needed some time to understand
how some parts of code relate to the hardware. Especially Rx and Tx
descriptors. Some comments in header will help... I can add some of them
(may be it will help other beginners), but i afraid that i spend time
but my patches wont be included.
--
Regards,
Oleksij
On 2013-04-28 5:15 PM, Ben Greear wrote:
> On 04/28/2013 08:08 AM, Felix Fietkau wrote:
>> On 2013-04-28 4:54 PM, Ben Greear wrote:
>>> On 04/28/2013 05:51 AM, Felix Fietkau wrote:
>>>> On 2013-04-27 5:25 PM, Oleksij Rempel wrote:
>>>>> Collect statistics about recived duplicate and STBC packets.
>>>>> This information should help see if STBC is actually working.
>>>>>
>>>>> Tested on ar9285;
>>>>>
>>>>> Signed-off-by: Oleksij Rempel <[email protected]>
>>>> I thought about this patch some more, and I'm wondering what's the point
>>>> in doing this? These statistics are going to be completely useless for
>>>> most people and they'll waste some memory/cpu cycles, especially on
>>>> small-cache devices. I think it's much more useful to simply pass the
>>>> information to mac80211 via rx flags and get them added to the radiotap
>>>> header.
>>>> I'd like to keep the number of 'poor man's debug hacks' in the driver to
>>>> a minimum, and there are some other things that I think should be
>>>> removed: rx_frags and rx_beacons in struct ath_rx_stats, the tx/rx MAC
>>>> sampling hack, and pretty much anything else that can be just as easily
>>>> accessed from mac80211 through regular interfaces.
>>>
>>> Does that mean we can just put the stats in mac80211, or do we have
>>> to be running a sniffer to gather the stats?
>> Right now you'd have to use a sniffer, but if you really care about
>> getting specific stats it might make sense to write a kernel module that
>> attaches to a monitor interface and gathers them (maybe even with
>> support for gathering arbitrary stats by attaching bpf filters).
>
> I'd like ongoing stats w/out a monitor interface or sniffer, though
> it would be fine to add it to the sniffer path as well. Maybe we can
> have compile-time flag to enable the extra and mostly useless
> stats so only the 1% that cares pays the overhead.
I'd like to have proper justification for stats added to the code and
kick out stuff not worth keeping. It's not just about runtime overhead,
but keeping the number of lines of code down is important for
maintenance as well. Adding extra compile time flags just makes the
maintenance part worse.
>> The problem I have with the current stats is they're just an arbitrary
>> collection of random stuff that is probably useless for 99% of all
>> users. In many cases the way the stats are collected also makes the data
>> completely meaningless (e.g. because the source/destination address is
>> not taken into account).
>>
>> Why care about the number of packets on the air that were sent with a
>> specific rate flag? Why care about the number of beacons on the air
>> (with no filter on a set of APs or anything)? Or what about the number
>> of fragments received? To me it just looks like an incoherent set of
>> useless facts.
>
> I think having lots of stats allows for the possibility of seeing a pattern
> that might otherwise be missed, and when testing in an isolated environment,
> you don't have to care about who is sending what being reported..you already
> know.
The stats I pointed out seem mostly useless for that purpose. When
testing in an isolated environment you might as well run a capture on a
monitor mode interface and do some more sophisticated analysis afterwards.
> One thing I'd like to do, for instance, is to figure out how to get some
> average MPDU lengths calculated in hopes that would correlate with decreased
> performance in cases were we have lots of stations....
Please don't throw hooks for such stacks all over the ath9k or mac80211
data path. Making a module for analyzing such stuff on a monitor mode
interface might be a bit more work than spraying hacks onto the code,
but at least it doesn't push the maintenance/performance overhead burden
onto everybody else.
Something like a BPF based statistics gathering module would be useful
for more people as well - I'd probably help with the code myself as
well. I made a proof of concept of such a module several years ago. It
was based on madwifi back then, but the code is mostly generic. You can
find it here:
http://git.openwrt.org/?p=packages.git;a=tree;f=net/wprobe/src
When implemented properly, such am module would even make things a lot
easier for you by letting you add new stats at runtime without changing
kernel code or reloading modules.
So let's stop wasting time on keeping lots of tiny useless hacks around
and instead build something more useful. :)
- Felix
On 04/28/2013 05:03 PM, Oleksij Rempel wrote:
> Am 28.04.2013 16:13, schrieb Oleksij Rempel:
>> Am 28.04.2013 14:51, schrieb Felix Fietkau:
>>> On 2013-04-27 5:25 PM, Oleksij Rempel wrote:
>>>> Collect statistics about recived duplicate and STBC packets.
>>>> This information should help see if STBC is actually working.
>>>>
>>>> Tested on ar9285;
>>>>
>>>> Signed-off-by: Oleksij Rempel <[email protected]>
>>> I thought about this patch some more, and I'm wondering what's the
>>> point
>>> in doing this? These statistics are going to be completely useless for
>>> most people and they'll waste some memory/cpu cycles, especially on
>>> small-cache devices. I think it's much more useful to simply pass the
>>> information to mac80211 via rx flags and get them added to the radiotap
>>> header.
>>
>> Sure.
>>
>
> I see Wojciech Dubowik sanded some patches, for at least one year, to
> make exactly what Felix suggested. Are there any reason why this
> patches was not accepted?
> Wojciech if you alive and have some time, can you update them?
Sure. I will try to update them based on suggested radiotap field
structure for STBC and Ness.
On the other hand as Felix already mentioned it's yet another line of
code one needs once in a lifetime.
If one is checking whether stbc is working the easiest way is to create
own debugging namespace
in radiotap and dump all descriptor registers there. It probably doesn't
go mainline but you could apply
the patches when you need and they mostly rebase cleanly. At least
that's what I do.
Wojtek
On 27 April 2013 11:53, Oleksij Rempel <[email protected]> wrote:
>> (And then go and re-align things inside that struct so you don't waste
>> space.)
>
>
> hmm.. what do you mean here?
Structure alignment? Well, you typically want to have everything be
dword aligned (32 bits) or word (16 bits) aligned. Otherwise the
compiler may insert extra padding between fields in order to meet
alignment requirements on platforms that need it (MIPS, older ARM) or
platforms that perform slower (newer ARM.)
Eg:
u32 a;
u16 b;
u8 c;
u8 d;
.. that's fine - the u32 is dword aligned, the u16 is word aligned,
the u8's don't need aligning.
But, considder:
u32 a;
u8 b;
u16 c;
u8 d;
.. u32 is dword aligned, u8 b is fine as it's a a byte and doesn't
need aligning, but 'u16 c' isn't dword aligned! So the compiler will
insert a byte padding between 'b' and 'c'.
same deal with:
u32 a;
u16 b;
u32 c;
.. 'a' is fine; 'b' is fine, but 'c' starts at a word boundary, not a
dword boundary.
Hence why things like IP/TCP headers and such look the way they do. :-)
Now, i don't know what 'bool' is, whether it's a byte, word or dword.
That "is_mybeacon" field should probably be just another flag in
rx_status, then just extend 'rs_flags' to 16 bits and include it. That
way the alignment is easy to see - all the fields in rx_status and the
htc rx_status structs have explicit sizes. :-)
Felix, what do you think?
Adrian
On 2013-04-27 5:25 PM, Oleksij Rempel wrote:
> Collect statistics about recived duplicate and STBC packets.
> This information should help see if STBC is actually working.
>
> Tested on ar9285;
>
> Signed-off-by: Oleksij Rempel <[email protected]>
I thought about this patch some more, and I'm wondering what's the point
in doing this? These statistics are going to be completely useless for
most people and they'll waste some memory/cpu cycles, especially on
small-cache devices. I think it's much more useful to simply pass the
information to mac80211 via rx flags and get them added to the radiotap
header.
I'd like to keep the number of 'poor man's debug hacks' in the driver to
a minimum, and there are some other things that I think should be
removed: rx_frags and rx_beacons in struct ath_rx_stats, the tx/rx MAC
sampling hack, and pretty much anything else that can be just as easily
accessed from mac80211 through regular interfaces.
- Felix
Am 28.04.2013 16:13, schrieb Oleksij Rempel:
> Am 28.04.2013 14:51, schrieb Felix Fietkau:
>> On 2013-04-27 5:25 PM, Oleksij Rempel wrote:
>>> Collect statistics about recived duplicate and STBC packets.
>>> This information should help see if STBC is actually working.
>>>
>>> Tested on ar9285;
>>>
>>> Signed-off-by: Oleksij Rempel <[email protected]>
>> I thought about this patch some more, and I'm wondering what's the point
>> in doing this? These statistics are going to be completely useless for
>> most people and they'll waste some memory/cpu cycles, especially on
>> small-cache devices. I think it's much more useful to simply pass the
>> information to mac80211 via rx flags and get them added to the radiotap
>> header.
>
> Sure.
>
I see Wojciech Dubowik sanded some patches, for at least one year, to
make exactly what Felix suggested. Are there any reason why this patches
was not accepted?
Wojciech if you alive and have some time, can you update them?
--
Regards,
Oleksij
Collect statistics about recived duplicate and STBC packets.
This information should help see if STBC is actually working.
Tested on ar9285; Should work for all chips after ar9280.
Changes:
- v2. test for stbc vield only on ar9280 and later.
reanme rx_gi to rx_short_gi
- v3. use u16 rs_flags instead of two u8
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/wireless/ath/ath9k/debug.c | 20 +++++++++++++++++++-
drivers/net/wireless/ath/ath9k/debug.h | 4 ++++
drivers/net/wireless/ath/ath9k/mac.c | 6 ++++++
drivers/net/wireless/ath/ath9k/mac.h | 26 +++++++++++++++-----------
4 files changed, 44 insertions(+), 12 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
index e6307b8..3cb2ae9 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -848,7 +848,7 @@ static ssize_t read_file_recv(struct file *file, char __user *user_buf,
struct ath_softc *sc = file->private_data;
char *buf;
- unsigned int len = 0, size = 1600;
+ unsigned int len = 0, size = 2048;
ssize_t retval = 0;
buf = kzalloc(size, GFP_KERNEL);
@@ -900,6 +900,11 @@ static ssize_t read_file_recv(struct file *file, char __user *user_buf,
RXS_ERR("RX-Frags", rx_frags);
RXS_ERR("RX-Spectral", rx_spectral);
+ RXS_ERR("RX-ShortGI", rx_short_gi);
+ RXS_ERR("RX-HT40", rx_ht40);
+ RXS_ERR("RX-Duplicate", rx_duplicate);
+ RXS_ERR("RX-STBC", rx_stbc);
+
if (len > size)
len = size;
@@ -939,6 +944,14 @@ void ath_debug_stat_rx(struct ath_softc *sc, struct ath_rx_status *rs)
if (rs->rs_phyerr < ATH9K_PHYERR_MAX)
RX_PHY_ERR_INC(rs->rs_phyerr);
}
+ if (rs->rs_flags & ATH9K_RX_GI)
+ RX_STAT_INC(rx_short_gi);
+ if (rs->rs_flags & ATH9K_RX_2040)
+ RX_STAT_INC(rx_ht40);
+ if (rs->rs_flags & ATH9K_RX_DUP)
+ RX_STAT_INC(rx_duplicate);
+ if (rs->rs_flags & ATH9K_RX_STBC)
+ RX_STAT_INC(rx_stbc);
#ifdef CONFIG_ATH9K_MAC_DEBUG
spin_lock(&sc->debug.samp_lock);
@@ -1993,6 +2006,11 @@ void ath9k_get_et_stats(struct ieee80211_hw *hw,
AWDATA(data_underrun);
AWDATA(delim_underrun);
+ AWDATA_RX(rx_short_gi);
+ AWDATA_RX(rx_ht40);
+ AWDATA_RX(rx_duplicate);
+ AWDATA_RX(rx_stbc);
+
AWDATA_RX(crc_err);
AWDATA_RX(decrypt_crc_err);
AWDATA_RX(phy_err);
diff --git a/drivers/net/wireless/ath/ath9k/debug.h b/drivers/net/wireless/ath/ath9k/debug.h
index 794a7ec..639dae9 100644
--- a/drivers/net/wireless/ath/ath9k/debug.h
+++ b/drivers/net/wireless/ath/ath9k/debug.h
@@ -241,6 +241,10 @@ struct ath_rx_stats {
u32 rx_beacons;
u32 rx_frags;
u32 rx_spectral;
+ u32 rx_short_gi;
+ u32 rx_ht40;
+ u32 rx_duplicate;
+ u32 rx_stbc;
};
struct ath_stats {
diff --git a/drivers/net/wireless/ath/ath9k/mac.c b/drivers/net/wireless/ath/ath9k/mac.c
index 498fee0..853cb09 100644
--- a/drivers/net/wireless/ath/ath9k/mac.c
+++ b/drivers/net/wireless/ath/ath9k/mac.c
@@ -590,6 +590,12 @@ int ath9k_hw_rxprocdesc(struct ath_hw *ah, struct ath_desc *ds,
(ads.ds_rxstatus3 & AR_GI) ? ATH9K_RX_GI : 0;
rs->rs_flags |=
(ads.ds_rxstatus3 & AR_2040) ? ATH9K_RX_2040 : 0;
+ rs->rs_flags |=
+ (ads.ds_rxstatus3 & AR_RXST_DUP) ? ATH9K_RX_DUP : 0;
+
+ if (AR_SREV_9280_20_OR_LATER(ah))
+ rs->rs_flags |=
+ (ads.ds_rxstatus3 & AR_RXST_STBC) ? ATH9K_RX_STBC : 0;
if (ads.ds_rxstatus8 & AR_PreDelimCRCErr)
rs->rs_flags |= ATH9K_RX_DELIM_CRC_PRE;
diff --git a/drivers/net/wireless/ath/ath9k/mac.h b/drivers/net/wireless/ath/ath9k/mac.h
index 5865f92..8ea9566 100644
--- a/drivers/net/wireless/ath/ath9k/mac.h
+++ b/drivers/net/wireless/ath/ath9k/mac.h
@@ -142,7 +142,7 @@ struct ath_rx_status {
u8 rs_isaggr;
u8 rs_moreaggr;
u8 rs_num_delims;
- u8 rs_flags;
+ u16 rs_flags;
bool is_mybeacon;
u32 evm0;
u32 evm1;
@@ -185,13 +185,16 @@ struct ath_htc_rx_status {
#define ATH9K_RXERR_KEYMISS 0x20
#define ATH9K_RXERR_CORRUPT_DESC 0x40
-#define ATH9K_RX_MORE 0x01
-#define ATH9K_RX_MORE_AGGR 0x02
-#define ATH9K_RX_GI 0x04
-#define ATH9K_RX_2040 0x08
-#define ATH9K_RX_DELIM_CRC_PRE 0x10
-#define ATH9K_RX_DELIM_CRC_POST 0x20
-#define ATH9K_RX_DECRYPT_BUSY 0x40
+/* rs_flags */
+#define ATH9K_RX_MORE 0x0001
+#define ATH9K_RX_MORE_AGGR 0x0002
+#define ATH9K_RX_GI 0x0004
+#define ATH9K_RX_2040 0x0008
+#define ATH9K_RX_DELIM_CRC_PRE 0x0010
+#define ATH9K_RX_DELIM_CRC_POST 0x0020
+#define ATH9K_RX_DECRYPT_BUSY 0x0040
+#define ATH9K_RX_DUP 0x0080
+#define ATH9K_RX_STBC 0x0100
#define ATH9K_RXKEYIX_INVALID ((u8)-1)
#define ATH9K_TXKEYIX_INVALID ((u8)-1)
@@ -529,11 +532,12 @@ struct ar5416_desc {
#define AR_RcvTimestamp ds_rxstatus2
+/* rxstatus3 */
#define AR_GI 0x00000001
#define AR_2040 0x00000002
-#define AR_Parallel40 0x00000004
-#define AR_Parallel40_S 2
-#define AR_RxStatusRsvd30 0x000000f8
+#define AR_RXST_DUP 0x00000004 /* duplicate packets */
+#define AR_RXST_STBC 0x00000008 /* ar9280 and later */
+#define AR_RxStatusRsvd30 0x000000f0
#define AR_RxAntenna 0xffffff00
#define AR_RxAntenna_S 8
--
1.8.1.2
On 2013-04-27 9:06 PM, Adrian Chadd wrote:
> On 27 April 2013 11:53, Oleksij Rempel <[email protected]> wrote:
>
>>> (And then go and re-align things inside that struct so you don't waste
>>> space.)
>>
>>
>> hmm.. what do you mean here?
>
> Structure alignment? Well, you typically want to have everything be
> dword aligned (32 bits) or word (16 bits) aligned. Otherwise the
> compiler may insert extra padding between fields in order to meet
> alignment requirements on platforms that need it (MIPS, older ARM) or
> platforms that perform slower (newer ARM.)
I think in struct ath_rx_status alignment does not matter much, it's
only kept on the stack anyway. But yes, in other cases it makes sense to
pay attention to padding to keep structs small. I also agree that making
rs_flags u16 is a good idea.
> Now, i don't know what 'bool' is, whether it's a byte, word or dword.
bool is a byte.
> That "is_mybeacon" field should probably be just another flag in
> rx_status, then just extend 'rs_flags' to 16 bits and include it. That
> way the alignment is easy to see - all the fields in rx_status and the
> htc rx_status structs have explicit sizes. :-)
>
> Felix, what do you think?
Sounds good :)
- Felix
Hiya,
Why not just bump rs_flags to be a u16, rather than a u8? then you
don't need an rs_flags_2.
(And then go and re-align things inside that struct so you don't waste space.)
Adrian
On 04/28/2013 08:08 AM, Felix Fietkau wrote:
> On 2013-04-28 4:54 PM, Ben Greear wrote:
>> On 04/28/2013 05:51 AM, Felix Fietkau wrote:
>>> On 2013-04-27 5:25 PM, Oleksij Rempel wrote:
>>>> Collect statistics about recived duplicate and STBC packets.
>>>> This information should help see if STBC is actually working.
>>>>
>>>> Tested on ar9285;
>>>>
>>>> Signed-off-by: Oleksij Rempel <[email protected]>
>>> I thought about this patch some more, and I'm wondering what's the point
>>> in doing this? These statistics are going to be completely useless for
>>> most people and they'll waste some memory/cpu cycles, especially on
>>> small-cache devices. I think it's much more useful to simply pass the
>>> information to mac80211 via rx flags and get them added to the radiotap
>>> header.
>>> I'd like to keep the number of 'poor man's debug hacks' in the driver to
>>> a minimum, and there are some other things that I think should be
>>> removed: rx_frags and rx_beacons in struct ath_rx_stats, the tx/rx MAC
>>> sampling hack, and pretty much anything else that can be just as easily
>>> accessed from mac80211 through regular interfaces.
>>
>> Does that mean we can just put the stats in mac80211, or do we have
>> to be running a sniffer to gather the stats?
> Right now you'd have to use a sniffer, but if you really care about
> getting specific stats it might make sense to write a kernel module that
> attaches to a monitor interface and gathers them (maybe even with
> support for gathering arbitrary stats by attaching bpf filters).
I'd like ongoing stats w/out a monitor interface or sniffer, though
it would be fine to add it to the sniffer path as well. Maybe we can
have compile-time flag to enable the extra and mostly useless
stats so only the 1% that cares pays the overhead.
> The problem I have with the current stats is they're just an arbitrary
> collection of random stuff that is probably useless for 99% of all
> users. In many cases the way the stats are collected also makes the data
> completely meaningless (e.g. because the source/destination address is
> not taken into account).
>
> Why care about the number of packets on the air that were sent with a
> specific rate flag? Why care about the number of beacons on the air
> (with no filter on a set of APs or anything)? Or what about the number
> of fragments received? To me it just looks like an incoherent set of
> useless facts.
I think having lots of stats allows for the possibility of seeing a pattern
that might otherwise be missed, and when testing in an isolated environment,
you don't have to care about who is sending what being reported..you already
know.
One thing I'd like to do, for instance, is to figure out how to get some
average MPDU lengths calculated in hopes that would correlate with decreased
performance in cases were we have lots of stations....
Thanks,
Ben
>
> - Felix
>
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
Am 27.04.2013 21:06, schrieb Adrian Chadd:
> On 27 April 2013 11:53, Oleksij Rempel <[email protected]> wrote:
>
>>> (And then go and re-align things inside that struct so you don't waste
>>> space.)
>>
>>
>> hmm.. what do you mean here?
>
> Structure alignment? Well, you typically want to have everything be
> dword aligned (32 bits) or word (16 bits) aligned. Otherwise the
> compiler may insert extra padding between fields in order to meet
> alignment requirements on platforms that need it (MIPS, older ARM) or
> platforms that perform slower (newer ARM.)
>
> Eg:
>
> u32 a;
> u16 b;
> u8 c;
> u8 d;
>
> .. that's fine - the u32 is dword aligned, the u16 is word aligned,
> the u8's don't need aligning.
>
> But, considder:
>
> u32 a;
> u8 b;
> u16 c;
> u8 d;
>
> .. u32 is dword aligned, u8 b is fine as it's a a byte and doesn't
> need aligning, but 'u16 c' isn't dword aligned! So the compiler will
> insert a byte padding between 'b' and 'c'.
>
> same deal with:
>
> u32 a;
> u16 b;
> u32 c;
>
> .. 'a' is fine; 'b' is fine, but 'c' starts at a word boundary, not a
> dword boundary.
>
> Hence why things like IP/TCP headers and such look the way they do. :-)
>
> Now, i don't know what 'bool' is, whether it's a byte, word or dword.
> That "is_mybeacon" field should probably be just another flag in
> rx_status, then just extend 'rs_flags' to 16 bits and include it. That
> way the alignment is easy to see - all the fields in rx_status and the
> htc rx_status structs have explicit sizes. :-)
ok, i was not sure if you mean what i think :)
I would prefer to do this in separate patch.
--
Regards,
Oleksij
Am 29.04.2013 08:45, schrieb Wojciech Dubowik:
> On 04/28/2013 05:03 PM, Oleksij Rempel wrote:
>> Am 28.04.2013 16:13, schrieb Oleksij Rempel:
>>> Am 28.04.2013 14:51, schrieb Felix Fietkau:
>>>> On 2013-04-27 5:25 PM, Oleksij Rempel wrote:
>>>>> Collect statistics about recived duplicate and STBC packets.
>>>>> This information should help see if STBC is actually working.
>>>>>
>>>>> Tested on ar9285;
>>>>>
>>>>> Signed-off-by: Oleksij Rempel <[email protected]>
>>>> I thought about this patch some more, and I'm wondering what's the
>>>> point
>>>> in doing this? These statistics are going to be completely useless for
>>>> most people and they'll waste some memory/cpu cycles, especially on
>>>> small-cache devices. I think it's much more useful to simply pass the
>>>> information to mac80211 via rx flags and get them added to the radiotap
>>>> header.
>>>
>>> Sure.
>>>
>>
>> I see Wojciech Dubowik sanded some patches, for at least one year, to
>> make exactly what Felix suggested. Are there any reason why this
>> patches was not accepted?
>> Wojciech if you alive and have some time, can you update them?
> Sure. I will try to update them based on suggested radiotap field
> structure for STBC and Ness.
>
> On the other hand as Felix already mentioned it's yet another line of
> code one needs once in a lifetime.
> If one is checking whether stbc is working the easiest way is to create
> own debugging namespace
> in radiotap and dump all descriptor registers there. It probably doesn't
> go mainline but you could apply
> the patches when you need and they mostly rebase cleanly. At least
> that's what I do.
Felix,
will this patches go upstream if i make it raditap compatible?
If yes, and performance and cache usage is an issue, it think we should
remove this double flags mapping in ath9k. Currently we convert device
specific flags to driver flags, and then convert them to mac80211 flags.
There is lots of useless checks and conversations for each packet.
Should i change it?
--
Regards,
Oleksij
On 04/28/2013 05:51 AM, Felix Fietkau wrote:
> On 2013-04-27 5:25 PM, Oleksij Rempel wrote:
>> Collect statistics about recived duplicate and STBC packets.
>> This information should help see if STBC is actually working.
>>
>> Tested on ar9285;
>>
>> Signed-off-by: Oleksij Rempel <[email protected]>
> I thought about this patch some more, and I'm wondering what's the point
> in doing this? These statistics are going to be completely useless for
> most people and they'll waste some memory/cpu cycles, especially on
> small-cache devices. I think it's much more useful to simply pass the
> information to mac80211 via rx flags and get them added to the radiotap
> header.
> I'd like to keep the number of 'poor man's debug hacks' in the driver to
> a minimum, and there are some other things that I think should be
> removed: rx_frags and rx_beacons in struct ath_rx_stats, the tx/rx MAC
> sampling hack, and pretty much anything else that can be just as easily
> accessed from mac80211 through regular interfaces.
Does that mean we can just put the stats in mac80211, or do we have
to be running a sniffer to gather the stats?
Thanks,
Ben
>
> - Felix
> _______________________________________________
> ath9k-devel mailing list
> [email protected]
> https://lists.ath9k.org/mailman/listinfo/ath9k-devel
>
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On 8 May 2013 09:07, Ben Greear <[email protected]> wrote:
> I think it's good to keep global counters too..otherwise it is going to be
> a lot of work to gather per NIC stats, and since you would have to read for
> each
> station, your stats might not be quite as atomic as they used to be.
>
> But, I'm fine with adding per-station counters on top of the global
> stats.
As a reference point, one of my in-progress hacks in FreeBSD is to add
some macros to the ath(4) driver to keep per-state and global stats,
using the same sample pointers.
So, a macro would look like this:
ATH_STAT_INC(sc, ath_node, stat_name, 1);
* if I've enabled global stat counting, the sc->sc_stats.sc_stat_name
stat would be bumped.
* if I've enabled node stat counting, the
ath_node->an_stats.an_stat_name would be bumped.
I have similar ideas with net80211 - right now there's some per-node
stats, but lots of stats are per-VAP. I'm going to do the same thing -
take the vap and the node as part of the stat macro, and bump the
per-node and per-VAP stats. Again, leaving it as a build-time option.
I've found both global device / VAP stats _and_ per-node stats equally
useful for debugging. They're good for debugging different sets of
issues - I use the global stats for figuring out what is breaking
things, and I use the per-node stats to try and understand how the AP
is "viewing" a particular station, separate from all the other
stations currently chatting.
Stats are hard. Let's go shopping. :-)
Adrian
Adrian
Felix Fietkau wrote:
> The problem I have with the current stats is they're just an arbitrary
> collection of random stuff that is probably useless for 99% of all
> users. In many cases the way the stats are collected also makes the data
> completely meaningless (e.g. because the source/destination address is
> not taken into account).
>
> Why care about the number of packets on the air that were sent with a
> specific rate flag? Why care about the number of beacons on the air
> (with no filter on a set of APs or anything)? Or what about the number
> of fragments received? To me it just looks like an incoherent set of
> useless facts.
Yes, having per-station statistics would be useful, mainly for RX and TX. Right
now, all the counters are global and there is no way to find out how a
particular station is performing, especially in AP mode. Since mac80211 gives us
proper debugfs hooks for station addition/deletion, relevant stuff can be moved
there.
The 'recv' file used to be just for HW errors (DESC, CRC etc.), now it has various
counters that should probably be node-specific.
The 'xmit' file can be trimmed and information can be maintained per-station.
This will be really useful in AP mode - especially for diagnosing aggregation, QoS, PS etc.
Other than these two files, the rest are simple.
Sujith
On 05/07/2013 10:32 PM, Sujith Manoharan wrote:
> Felix Fietkau wrote:
>> The problem I have with the current stats is they're just an arbitrary
>> collection of random stuff that is probably useless for 99% of all
>> users. In many cases the way the stats are collected also makes the data
>> completely meaningless (e.g. because the source/destination address is
>> not taken into account).
>>
>> Why care about the number of packets on the air that were sent with a
>> specific rate flag? Why care about the number of beacons on the air
>> (with no filter on a set of APs or anything)? Or what about the number
>> of fragments received? To me it just looks like an incoherent set of
>> useless facts.
>
> Yes, having per-station statistics would be useful, mainly for RX and TX. Right
> now, all the counters are global and there is no way to find out how a
> particular station is performing, especially in AP mode. Since mac80211 gives us
> proper debugfs hooks for station addition/deletion, relevant stuff can be moved
> there.
>
> The 'recv' file used to be just for HW errors (DESC, CRC etc.), now it has various
> counters that should probably be node-specific.
>
> The 'xmit' file can be trimmed and information can be maintained per-station.
> This will be really useful in AP mode - especially for diagnosing aggregation, QoS, PS etc.
I think it's good to keep global counters too..otherwise it is going to be
a lot of work to gather per NIC stats, and since you would have to read for each
station, your stats might not be quite as atomic as they used to be.
But, I'm fine with adding per-station counters on top of the global
stats.
Thanks,
Ben
>
> Other than these two files, the rest are simple.
>
> Sujith
>
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com