Hello wireless folks,
Mathias Kretschmer and me would like to bring another new feature to the kernel:
Collecting information for (non-peer) stations. As far as I know, at least
Antonio and Thomas are interested in a similar feature as well, and it seems
Antonio has already prepared some code. I'd like to present our ideas and
requirements to this feature and would like to ask Antonio and Thomas, and everyone
else interested in this feature, to do the same. Then we can hopefully come up
with a design which is acceptable for upstream kernels and useful for everyone.
We would like to gather statistics for all peers, both connected and not connected
to our current IBSS. We do the same in userspace currently using a monitor interface,
but reading every packet from this monitor interface has a huge thoughput limitation
on our routers, and is therefore not acceptable for productive use. The statistics are
then used to evaluate link quality and make some higher level decisions.
The statistics we are interested in are, per station (identified by sender MAC address), for
both peer and non-peer stations:
* RSSI (*)
* sequence number gap size (*) - monitors the sequence number of a 80211 packet, per queue
* bad frame count (badFCS and such)
* total frame count
* bytes received
* last seen - timestamp
Fields marked with (*) should capture min, max, avg, count, sum, sum_square. Other
statistics (stddev, rms) can then be calculated in userspace using floating point numbers.
The other fields can be normal counters. Some of the fields are already accesible for connected
peers (like RSSI, last seen -> iw station dump), some of them are new, and none of them are currently
available for non-peer stations. We probably don't need all of them integrated
in mainline as they might be only interesting for us, but this is just to give you an idea.
Regarding the kernel interface, we don't have any requirements, from nl80211 to debugfs, everything
would be fine. We will access the data with a custom program.
Commands we would like to propose are:
* start collecting - this feature should not run by default to avoid bloating memory for users who
don't even need this
* stop collecting
* read - dumps the data for all stations
* read + reset - dump the data and reset information for all stations. This should also clean up stations,
at least those which are not connected to the BSS, to not bloat the station table.
I guess the right position to implement this is mac80211 receive path. Our intended platform
is ath9k/ath5k, but that feature should work with any mac80211 driver. We don't care if sta_info
structs are allocated or custom structures are used, as long as we can receive a list of stations
which includes peer and non-peer stations, along with their statistics.
We are looking forward to your thoughts. :)
Cheers,
Simon
On 2013-02-22 5:36 PM, Antonio Quartulli wrote:
> On Fri, Feb 22, 2013 at 08:21:14AM -0800, Felix Fietkau wrote:
>> Hi,
>>
>> Since this is a rare special case, I think it really does not belong
>> into the mac80211 data path. How about creating a monitor mode device
>> and claiming it from within the kernel in your own module via rx_handler
>> (the same mechanism that the bridge code uses to hook into the data path
>> of a normal net_device). You can extract relevant information from the
>> radiotap headers.
>
> I think this would increase the complexity given the fact that each and every
> packet needs to be encapsulated into the radiotap header..
I think it's only a slight increase in complexity.
> Then, most of the information that people wants to grab does not appear in
> such header.
What's missing?
> Other than that, by hooking into the mac80211 rx_path, in an early point, packets can
> then be dropped as usual (all those packets not going to this device), while
> with this monitor-like solution all of them have to be carried up to the virtual
> interface.
If the data path overhead here is really that important, then I suggest
adding a new minimalistic data path hook that allows you to inspect the
packets with the rx data in the cb from an external module. The actual
node tracking and reporting should be outside of mac80211 as long as the
reasons for putting it in have not been properly justified.
> Imho this introduces a not negligible complexity that it is better to avoid.
>
> And what about 802.11s? I don't think this is a good solution for them..
I have yet to see an explanation how this gathering of semi-related data
points even helps 802.11s. I only noticed this bit here by Thomas:
> I can't think of anything that would be specifically useful for
> 802.11s right now, and we can always extend the statistics in the
> future. One case where this might be useful is if the driver has a
> limited number of station slots, the MPM could monitor neighbor
> stations for a more "suitable" peer candidate, but your existing stats
> should cover that.
Is 'limited number of station slots' even a real issue or just an
abstract hypothetical? What are other things in 802.11s that this would
be useful for?
I think before non-peer tracking like this gets shoved into mac80211, a
lot more work needs to be done on justifying the extra code by
describing *specific* use cases that aren't just random data-gathering
for some user space based monitoring thingie.
- Felix
On 02/15/2013 06:19 PM, Simon Wunderlich wrote:
> Hello wireless folks,
>
> Mathias Kretschmer and me would like to bring another new feature to the kernel:
> Collecting information for (non-peer) stations. [...]
>
> Cheers,
> Simon
>
Hi Simon,
our areas of operations obviously highly overlap, since I did the same
consideration path only recently ;)
For the frequency planning feature we are currently working on, I need those
extended statistics you are proposing with additional filtering rules (OUI or mask
based) to classify peers.
My initial idea was to do a sliding time window aggregation of those statistics in
the kernel with almost the same interface to the userspace you are proposing.
But with your integration of the spectral feature I realized how efficient relay
based continuous data transfer is (as compared to sending samples over netlink).
Consequently I revised the initial plan and consider realizing the extended
statistics collection in userspace.
The modifications required in the kernel are minimal:
* create relay file in debugfs
* provide enable flag in debugfs
* based on this flag, extract relevant information from frames and write to relay
This way, you move complexity out of the kernel and maximize flexibility for
statistical evaluation in userspace. Also, the overhead for moving data between
kernel and userspace is minimized, since you do them in bulk.
This is all in the planning phase, so I have no proof-of-concept patches yet.
Would just like to hear what your thoughts are and whether this could be the way
to go.
Thanks for your initiative
Zefir
On 02/18/2013 05:51 PM, Johannes Berg wrote:
> On Mon, 2013-02-18 at 17:07 +0100, Antonio Quartulli wrote:
>> On Mon, Feb 18, 2013 at 07:58:18 -0800, Johannes Berg wrote:
>>> On Mon, 2013-02-18 at 16:49 +0100, Antonio Quartulli wrote:
>>>> On Mon, Feb 18, 2013 at 07:43:26 -0800, Johannes Berg wrote:
>>>>>> I did not like this approach because the sta_info struct is so big that
>>>>>> when we want to fill the stats substruct only we will waste a lot of bytes.
>>>>>
>>>>> I don't understand your point.
>>>>>
>>>>> struct sta_info {
>>>>> ...
>>>>> struct stats stats;
>>>>> };
>>>>
>>>> My concern is about those "..." that we are allocating within the sta_info struct
>>>> that we will never use for every non-peer station.
>>>>
>>>> While if we used the struct below (with its own hash table), we would allocate
>>>> only the space needed for the stats.
>>>>
>>>>>
>>>>> struct stats_entry {
>>>>> struct hash/list/whatever;
>>>>> struct stats stats;
>>>>> };
>>>>>
>>>>
>>>>
>>>> no?
>>>> Maybe I misunderstood your idea?
>>>
>>> But I'm not saying that these are mutually exclusive, I'm saying both
>>> should exist.
>>
>> Ah ok..Sorry, but I did not take this as an option :)
>>
>> So, if I understood correctly, this means one table lookup for peer stations,
>> while two table lookups for non peers (first in sta_hash, which will fail). Right?
>>
>> This would save one look up for each peer, since we have to do perform one of
>> them anyway (now I fully understood your previous statement!).
>
> Right... But the failing sta lookup has to happen anyway, so it really
> adds practically no cost in the peer case, and a singe lookup in the
> "non-peer already exists" case.
IMHO, this is the most efficient implementation for most practical use
cases, where 'peer' traffic accounts more the majority of the traffic.
Hence it should be in the 'fast path', while the extra lookup for 'non
peer' traffic should be tolerable.
My five cents,
Mathias
> johannes
>
--
Fraunhofer FOKUS - RESourCe Optimised Networks
Dr. Mathias Kretschmer
Schloss Birlinghoven; D-53754 Sankt Augustin
T +49-2241-14-3466, F +49-2241-14-1050,
E [email protected]
http://www.fokus.fraunhofer.de/en/rescon
On 02/22/2013 12:43 PM, Simon Wunderlich wrote:
> Hello Zefir,
> [...]
>
> it is nice to hear that the relayfs approach works well. :)
>
It does. Before the system was limited to 15k samples/second (with netlink), now
it can handle 50 kS/s.
> Our biggest performance issue is probably the syscall for every packet, so minimizing
> data to be sent to userspace and aggregate it to would benefit to our performance
> problem.
>
Did not check what happens under the hood when you write to a relay buffer, but
since it is tagged as 'efficient' channel to userspace, I don't expect there is
much of an overhead issue. Needs to be verified.
> I would also agree that this approach is simpler as we don't have to manage lists
> of clients and purge them. And we can evaluation in userspace, just as everyone
> needs for its application (maybe some of these statistics are not of general interest).
>
Right. Take the MAC address based filtering we need to have for our application.
It is not useful for most, while any statistics without that feature are worthless
to us. And we are not going to bloat kernel based statistics collection to fit
everyone's needs, I guess.
> Issues I see are:
> * the relayfs has to be regularly polled for new data (maybe multiple times a second),
> so it can't overflow. I'm just looking at a dump from a running iperf test, it shows
> ~12000 packets per second (at 70Mbit/s). Assuming we are sending 64 byte of metainfo
> per packet, this would sum up to 768k every second. Doing the statistics computation
> in userspace makes no difference to doing it in kernelspace, imho.
See above, 50k spectral samples per second (76 bytes each) on an embedded platform
(600MHz PPC) works fine. Agree, computational overhead remains the same - if you
do it on the device itself. Think of a large installation with low profile APs
that provide this data raw over the backbone.
> * in-kernel drivers can't use the gathered statistics (that would affect 802.11s)
Agree, if it needs to be evaluated by drivers, that's a different use-case.
Maybe the best take is to start on top of the statistics generated for in kernel
usage and forward it to userspace over the proposed relay channel.
> * I wonder how debugfs use in mac80211 for this kind of feature is acceptable. This
> would be a question for Johannes. :)
>
Sounds already quite application specific to me, so I'd expect debugfs would not
be the worst location to place it ;)
> Thanks for sharing your thoughts!
>
> Cheers,
> Simon
>
On Fri, Feb 22, 2013 at 08:21:14AM -0800, Felix Fietkau wrote:
> Hi,
>
> Since this is a rare special case, I think it really does not belong
> into the mac80211 data path. How about creating a monitor mode device
> and claiming it from within the kernel in your own module via rx_handler
> (the same mechanism that the bridge code uses to hook into the data path
> of a normal net_device). You can extract relevant information from the
> radiotap headers.
I think this would increase the complexity given the fact that each and every
packet needs to be encapsulated into the radiotap header..
Then, most of the information that people wants to grab does not appear in
such header.
Other than that, by hooking into the mac80211 rx_path, in an early point, packets can
then be dropped as usual (all those packets not going to this device), while
with this monitor-like solution all of them have to be carried up to the virtual
interface.
Imho this introduces a not negligible complexity that it is better to avoid.
And what about 802.11s? I don't think this is a good solution for them..
Cheers,
--
Antonio Quartulli
..each of us alone is worth nothing..
Ernesto "Che" Guevara
Hello Zefir,
On Fri, Feb 22, 2013 at 11:07:46AM +0100, Zefir Kurtisi wrote:
> On 02/15/2013 06:19 PM, Simon Wunderlich wrote:
> > Hello wireless folks,
> >
> > Mathias Kretschmer and me would like to bring another new feature to the kernel:
> > Collecting information for (non-peer) stations. [...]
> >
> > Cheers,
> > Simon
> >
> Hi Simon,
>
> our areas of operations obviously highly overlap, since I did the same
> consideration path only recently ;)
>
> For the frequency planning feature we are currently working on, I need those
> extended statistics you are proposing with additional filtering rules (OUI or mask
> based) to classify peers.
>
> My initial idea was to do a sliding time window aggregation of those statistics in
> the kernel with almost the same interface to the userspace you are proposing.
>
> But with your integration of the spectral feature I realized how efficient relay
> based continuous data transfer is (as compared to sending samples over netlink).
> Consequently I revised the initial plan and consider realizing the extended
> statistics collection in userspace.
>
> The modifications required in the kernel are minimal:
> * create relay file in debugfs
> * provide enable flag in debugfs
> * based on this flag, extract relevant information from frames and write to relay
>
> This way, you move complexity out of the kernel and maximize flexibility for
> statistical evaluation in userspace. Also, the overhead for moving data between
> kernel and userspace is minimized, since you do them in bulk.
>
> This is all in the planning phase, so I have no proof-of-concept patches yet.
> Would just like to hear what your thoughts are and whether this could be the way
> to go.
it is nice to hear that the relayfs approach works well. :)
Our biggest performance issue is probably the syscall for every packet, so minimizing
data to be sent to userspace and aggregate it to would benefit to our performance
problem.
I would also agree that this approach is simpler as we don't have to manage lists
of clients and purge them. And we can evaluation in userspace, just as everyone
needs for its application (maybe some of these statistics are not of general interest).
Issues I see are:
* the relayfs has to be regularly polled for new data (maybe multiple times a second),
so it can't overflow. I'm just looking at a dump from a running iperf test, it shows
~12000 packets per second (at 70Mbit/s). Assuming we are sending 64 byte of metainfo
per packet, this would sum up to 768k every second. Doing the statistics computation
in userspace makes no difference to doing it in kernelspace, imho.
* in-kernel drivers can't use the gathered statistics (that would affect 802.11s)
* I wonder how debugfs use in mac80211 for this kind of feature is acceptable. This
would be a question for Johannes. :)
Thanks for sharing your thoughts!
Cheers,
Simon
Hi all,
You motivate your proposal of collecting several statistics from non-peering stations in IBBS networks with:
(1) "The statistics are then used to evaluate link quality and make some higher level decisions"
&
(2)"reading every packet from this monitor interface has a huge thoughput limitation"
Could you go into more details about what "higher level" decisions you have in mind ?
Until now you just listed a bunch of values you like to monitor on a per packet level without any concrete usage idea or algorithms that could make use of it.
I could guess that a common goal would be to increase performance in wireless networks, but lets get concrete about the approach you have in mind.
A lot of recent theoretical research in wireless goes towards interference management, multi-user and so the gains of using channel state information as feedback that is worth its overhead. But this direction would question the packet level granularity you described? so what about aggregated statistics ?
Cheers
Thomas
On 15.02.2013, at 18:19, Simon Wunderlich <[email protected]> wrote:
> Hello wireless folks,
>
> Mathias Kretschmer and me would like to bring another new feature to the kernel:
> Collecting information for (non-peer) stations. As far as I know, at least
> Antonio and Thomas are interested in a similar feature as well, and it seems
> Antonio has already prepared some code. I'd like to present our ideas and
> requirements to this feature and would like to ask Antonio and Thomas, and everyone
> else interested in this feature, to do the same. Then we can hopefully come up
> with a design which is acceptable for upstream kernels and useful for everyone.
>
> We would like to gather statistics for all peers, both connected and not connected
> to our current IBSS. We do the same in userspace currently using a monitor interface,
> but reading every packet from this monitor interface has a huge thoughput limitation
> on our routers, and is therefore not acceptable for productive use. The statistics are
> then used to evaluate link quality and make some higher level decisions.
>
> The statistics we are interested in are, per station (identified by sender MAC address), for
> both peer and non-peer stations:
> * RSSI (*)
> * sequence number gap size (*) - monitors the sequence number of a 80211 packet, per queue
> * bad frame count (badFCS and such)
> * total frame count
> * bytes received
> * last seen - timestamp
>
> Fields marked with (*) should capture min, max, avg, count, sum, sum_square. Other
> statistics (stddev, rms) can then be calculated in userspace using floating point numbers.
> The other fields can be normal counters. Some of the fields are already accesible for connected
> peers (like RSSI, last seen -> iw station dump), some of them are new, and none of them are currently
> available for non-peer stations. We probably don't need all of them integrated
> in mainline as they might be only interesting for us, but this is just to give you an idea.
>
> Regarding the kernel interface, we don't have any requirements, from nl80211 to debugfs, everything
> would be fine. We will access the data with a custom program.
>
> Commands we would like to propose are:
> * start collecting - this feature should not run by default to avoid bloating memory for users who
> don't even need this
> * stop collecting
> * read - dumps the data for all stations
> * read + reset - dump the data and reset information for all stations. This should also clean up stations,
> at least those which are not connected to the BSS, to not bloat the station table.
>
> I guess the right position to implement this is mac80211 receive path. Our intended platform
> is ath9k/ath5k, but that feature should work with any mac80211 driver. We don't care if sta_info
> structs are allocated or custom structures are used, as long as we can receive a list of stations
> which includes peer and non-peer stations, along with their statistics.
>
> We are looking forward to your thoughts. :)
>
> Cheers,
> Simon
On Fri, Feb 22, 2013 at 09:42:43AM -0800, Adrian Chadd wrote:
> .. considering I do radiotap inspection in promiscuous mode all the
> time (on laptops and embedded MIPS boards alike), I'm kinda interested
> in why you're seeing such high CPU utilisation.
>
> Are you trying to capture the whole frame? Or are you only capturing
> the header? It sounds like you only need the header, right?
Yup, we capture the whole frame, but as far as I understand the main
problem are the syscalls to receive each frame individually.
We had a similar interesting experience in batman-adv, operating with
raw sockets and send/recv and could do more than 10 Mbit/s, and after
moving to kernel space we could do several hundred Mbit/s - on the same
platform (AMD Geode based boards).
>
> I agree with Felix - this is the kind of thing that it makes sense to
> define an API for so you can write in-kernel monitoring plugins. Then
> your monitoring plugin can decide how to aggregate and pass up the
> data.
Yeah, maybe ... I mean, I understand that our application is quite "special"
and 99% of the mac80211 users (think laptop, android users etc) won't use
it.
OTOH there are at least some who have similar interest, so it's good to
see who wants what. If many people want the same thing, we might be able
to share the maintainance overhead among them. :)
But if the goals are very different, it might be better as Felix suggested
to just keep it out of mac80211 and everyone writes their own modules, or
we create a shared base where everyone can attach code for their special
needs.
Anyway, I'll look at the rx_handler stuff what Felix suggested and see
if this could work out. :)
Cheers,
Simon
On Mon, 2013-02-18 at 15:46 +0100, Antonio Quartulli wrote:
> In my current implementation I created a "twin hash-table". It contains
> statistics for *all* the stations (peer and non-peer).
>
> I think that instead of embedding this new struct (let's call it sta_stats) into
> the sta_info one, it would be easier to let them be independent (this is why I
> created the twin hash) and then create
> a pointer from the sta_info to the related sta_stats.
I don't really see value in that, it would only make the implementation
less efficient, because either you follow another pointer (sta->stats)
or you have to look in the other hash table. That's why I prefer
embedding it, we have to do the station hash table lookup anyway.
> For the API I think we should create a new nl80211 command.
> If we simply add a
> flag to the normal "station dump" command, we would not have all the attributes
> to print (keep in mind station dump prints attributes that are in sta_info and
> that are not in sta_stats).
station dump can just print all attributes that exist, some stations
would have more/different attributes than other stations. But I don't
really think it makes a big difference either way, reusing it would be
fine if the default is to not include the stats-only entries.
johannes
Hi,
Since this is a rare special case, I think it really does not belong
into the mac80211 data path. How about creating a monitor mode device
and claiming it from within the kernel in your own module via rx_handler
(the same mechanism that the bridge code uses to hook into the data path
of a normal net_device). You can extract relevant information from the
radiotap headers.
That way you can keep your code completely separate from mac80211 and
don't have to dump the maintenance overhead of this on other people.
- Felix
On Wed, Feb 20, 2013 at 11:10:14AM -0800, Thomas Pedersen wrote:
> Hi Simon,
>
> On Wed, Feb 20, 2013 at 9:19 AM, Simon Wunderlich
> <[email protected]> wrote:
>
> > To sum from this discussion (I think it's a good idea):
> >
> > * embed the stats_entry into the sta_info
> > * update peer-stats by modifying the embedded stats_entry (we do the lookup anyway
> > * keep the non-peer stats in a seperate hash, and only keep stats_entry for them (we don't need
> > the full sta_info after all).
> >
> > We should consider some corner cases here, e.g. adding stas, then we have to
> > copy+remove the stats from the non-peer hash, or removing stas, then we have
> > to copy the so-far collected stats to the non-peer hash.
> >
> > If you are okay with it, we can use the NL80211_CMD_GET_STATION command
> > (as in iw station dump), and add a seperate flag to give info for non-peer sta.
> >
> > What about the other commands I suggested (read+reset, start, stop)? For read+reset,
> > we could just send yet another flag (RESET_STATS) with the GET_STATION command, but
> > for start/stop we would need new commands? Or would you have any better idea?
> >
> > @Thomas: Is there anything to consider for 802.11s?
>
> I can't think of anything that would be specifically useful for
> 802.11s right now, and we can always extend the statistics in the
> future. One case where this might be useful is if the driver has a
> limited number of station slots, the MPM could monitor neighbor
> stations for a more "suitable" peer candidate, but your existing stats
> should cover that.
OK, that sounds good. If there are any other requirements we should consider,
please let us know.
>
> Turning on this feature would add some sort of (internal to mac80211)
> monitor interface, or just disable all frame filters on a given PHY?
> How does it work now?
Right now we use a monitor interface to get frames from other BSS and
signal info via radiotap. I'm not sure yet if any "virtual" monitor
interface or disabling RX filters is required. If anyone has a good idea
please let me know, otherwise I will try out.
Thanks,
Simon
On Mon, 2013-02-18 at 16:38 +0100, Antonio Quartulli wrote:
> On Mon, Feb 18, 2013 at 07:29:47 -0800, Johannes Berg wrote:
> > On Mon, 2013-02-18 at 15:46 +0100, Antonio Quartulli wrote:
> >
> > > In my current implementation I created a "twin hash-table". It contains
> > > statistics for *all* the stations (peer and non-peer).
> > >
> > > I think that instead of embedding this new struct (let's call it sta_stats) into
> > > the sta_info one, it would be easier to let them be independent (this is why I
> > > created the twin hash) and then create
> > > a pointer from the sta_info to the related sta_stats.
> >
> > I don't really see value in that, it would only make the implementation
> > less efficient, because either you follow another pointer (sta->stats)
> > or you have to look in the other hash table. That's why I prefer
> > embedding it, we have to do the station hash table lookup anyway.
>
> I did not like this approach because the sta_info struct is so big that
> when we want to fill the stats substruct only we will waste a lot of bytes.
I don't understand your point.
struct sta_info {
...
struct stats stats;
};
struct stats_entry {
struct hash/list/whatever;
struct stats stats;
};
johannes
On Mon, Feb 18, 2013 at 06:33:02 -0800, Johannes Berg wrote:
> On Mon, 2013-02-18 at 15:30 +0100, Johannes Berg wrote:
> > On Fri, 2013-02-15 at 18:19 +0100, Simon Wunderlich wrote:
> >
> > > Commands we would like to propose are:
> > > * start collecting - this feature should not run by default to avoid bloating memory for users who
> > > don't even need this
> > > * stop collecting
> > > * read - dumps the data for all stations
> > > * read + reset - dump the data and reset information for all stations. This should also clean up stations,
> > > at least those which are not connected to the BSS, to not bloat the station table.
> > >
> > > I guess the right position to implement this is mac80211 receive path. Our intended platform
> > > is ath9k/ath5k, but that feature should work with any mac80211 driver. We don't care if sta_info
> > > structs are allocated or custom structures are used, as long as we can receive a list of stations
> > > which includes peer and non-peer stations, along with their statistics.
> > >
> > > We are looking forward to your thoughts. :)
> >
> > I would argue that since most of the sta_info struct is used for
> > operational stuff, you shouldn't use it, but have a separate struct and
> > maybe embed that separate struct in sta_info for its statistics.
> >
> > I'd also not use the existing nl80211 station APIs since this could be
> > an optional feature for many things, and it will likely break existing
> > expectations, e.g. that all stations listed by "iw wlan0 station dump"
> > are clients connected to an AP interface.
> >
> > It could be argued that this API then should also not even include the
> > connected stations when listing ones, i.e. explicitly be non-connected
> > stations.
>
> Or maybe use the APIs, but require including a special attribute in the
> dump/get request message in order to dump/get the/a non-connected
> station(s), and only include those attributes that are relevant.
In my current implementation I created a "twin hash-table". It contains
statistics for *all* the stations (peer and non-peer).
I think that instead of embedding this new struct (let's call it sta_stats) into
the sta_info one, it would be easier to let them be independent (this is why I
created the twin hash) and then create
a pointer from the sta_info to the related sta_stats.
For the API I think we should create a new nl80211 command.
If we simply add a
flag to the normal "station dump" command, we would not have all the attributes
to print (keep in mind station dump prints attributes that are in sta_info and
that are not in sta_stats).
Cheers,
--
Antonio Quartulli
..each of us alone is worth nothing..
Ernesto "Che" Guevara
On Mon, 2013-02-18 at 15:30 +0100, Johannes Berg wrote:
> On Fri, 2013-02-15 at 18:19 +0100, Simon Wunderlich wrote:
>
> > Commands we would like to propose are:
> > * start collecting - this feature should not run by default to avoid bloating memory for users who
> > don't even need this
> > * stop collecting
> > * read - dumps the data for all stations
> > * read + reset - dump the data and reset information for all stations. This should also clean up stations,
> > at least those which are not connected to the BSS, to not bloat the station table.
> >
> > I guess the right position to implement this is mac80211 receive path. Our intended platform
> > is ath9k/ath5k, but that feature should work with any mac80211 driver. We don't care if sta_info
> > structs are allocated or custom structures are used, as long as we can receive a list of stations
> > which includes peer and non-peer stations, along with their statistics.
> >
> > We are looking forward to your thoughts. :)
>
> I would argue that since most of the sta_info struct is used for
> operational stuff, you shouldn't use it, but have a separate struct and
> maybe embed that separate struct in sta_info for its statistics.
>
> I'd also not use the existing nl80211 station APIs since this could be
> an optional feature for many things, and it will likely break existing
> expectations, e.g. that all stations listed by "iw wlan0 station dump"
> are clients connected to an AP interface.
>
> It could be argued that this API then should also not even include the
> connected stations when listing ones, i.e. explicitly be non-connected
> stations.
Or maybe use the APIs, but require including a special attribute in the
dump/get request message in order to dump/get the/a non-connected
station(s), and only include those attributes that are relevant.
johannes
Hi Thomas, all,
please see my comments in-line...
On 02/19/2013 10:32 AM, Thomas H?hn wrote:
> Hi all,
>
> You motivate your proposal of collecting several statistics from non-peering stations in IBBS networks with:
> (1) "The statistics are then used to evaluate link quality and make some higher level decisions"
> &
> (2)"reading every packet from this monitor interface has a huge thoughput limitation"
>
> Could you go into more details about what "higher level" decisions you have in mind ?
I could think of multiple scenarios.
A concrete one is a multi-radio multi-channel network, where at some
point in time, the most suitable (i.e. 'free') channels are assigned to
associated peers.
Over time, other nodes/sources/peers might decide to choose the same
channels for their own purposes, thus potentially severely impacting the
throughput/performance of this link.
By analyzing all frames received on an interface (no matter with BSS
they belong to), one can determine (for example)
a) the number of transmitters on a channel
b) the pkt or bit rate
c) the signal level
d) MAC level errors (retransmissions, SEQ# gaps, etc.)
e) MCS/rate
Interpreting all or some of the above information, the control plane can
determine/predict possible link issues, etc (and take appropriate
counter measures).
I assume there are also other scenarios, such as load balancing between
APs, etc.
> Until now you just listed a bunch of values you like to monitor on a per packet level without any concrete
> usage idea or algorithms that could make use of it.
Our goal was to propose simple (light weight) statistics inside the
kernel, which hold all required information relevant to further
processing in user space.
> I could guess that a common goal would be to increase performance in wireless networks, but lets
> get concrete about the approach you have in mind.
> A lot of recent theoretical research in wireless goes towards interference management,
> multi-user and so the gains of using channel state information as
feedback that is worth its overhead.
> But this direction would question the packet level granularity you
described? so what about aggregated statistics ?
I'm not following here. Who would perform the aggregation for you ? The
goal of our proposal is to lay the basis for aggregated statistics (to
be computed and maintained in user space).
If people agree with the above idea/goals, our proposal would be to
discuss (and eventually agree :) on a set of values, their
representation and a standardized API (i.e. via NL) to (periodically)
query such values.
The various applications could then (in user space) use this information
for their specific purposes.
Cheers,
Mathias
>
> Cheers
> Thomas
>
>
> On 15.02.2013, at 18:19, Simon Wunderlich <[email protected]> wrote:
>
>> Hello wireless folks,
>>
>> Mathias Kretschmer and me would like to bring another new feature to the kernel:
>> Collecting information for (non-peer) stations. As far as I know, at least
>> Antonio and Thomas are interested in a similar feature as well, and it seems
>> Antonio has already prepared some code. I'd like to present our ideas and
>> requirements to this feature and would like to ask Antonio and Thomas, and everyone
>> else interested in this feature, to do the same. Then we can hopefully come up
>> with a design which is acceptable for upstream kernels and useful for everyone.
>>
>> We would like to gather statistics for all peers, both connected and not connected
>> to our current IBSS. We do the same in userspace currently using a monitor interface,
>> but reading every packet from this monitor interface has a huge thoughput limitation
>> on our routers, and is therefore not acceptable for productive use. The statistics are
>> then used to evaluate link quality and make some higher level decisions.
>>
>> The statistics we are interested in are, per station (identified by sender MAC address), for
>> both peer and non-peer stations:
>> * RSSI (*)
>> * sequence number gap size (*) - monitors the sequence number of a 80211 packet, per queue
>> * bad frame count (badFCS and such)
>> * total frame count
>> * bytes received
>> * last seen - timestamp
>>
>> Fields marked with (*) should capture min, max, avg, count, sum, sum_square. Other
>> statistics (stddev, rms) can then be calculated in userspace using floating point numbers.
>> The other fields can be normal counters. Some of the fields are already accesible for connected
>> peers (like RSSI, last seen -> iw station dump), some of them are new, and none of them are currently
>> available for non-peer stations. We probably don't need all of them integrated
>> in mainline as they might be only interesting for us, but this is just to give you an idea.
>>
>> Regarding the kernel interface, we don't have any requirements, from nl80211 to debugfs, everything
>> would be fine. We will access the data with a custom program.
>>
>> Commands we would like to propose are:
>> * start collecting - this feature should not run by default to avoid bloating memory for users who
>> don't even need this
>> * stop collecting
>> * read - dumps the data for all stations
>> * read + reset - dump the data and reset information for all stations. This should also clean up stations,
>> at least those which are not connected to the BSS, to not bloat the station table.
>>
>> I guess the right position to implement this is mac80211 receive path. Our intended platform
>> is ath9k/ath5k, but that feature should work with any mac80211 driver. We don't care if sta_info
>> structs are allocated or custom structures are used, as long as we can receive a list of stations
>> which includes peer and non-peer stations, along with their statistics.
>>
>> We are looking forward to your thoughts. :)
>>
>> Cheers,
>> Simon
>
--
Fraunhofer FOKUS - RESourCe Optimised Networks
Dr. Mathias Kretschmer
Schloss Birlinghoven; D-53754 Sankt Augustin
T +49-2241-14-3466, F +49-2241-14-1050,
E [email protected]
http://www.fokus.fraunhofer.de/en/rescon
Hi Simon,
On Wed, Feb 20, 2013 at 9:19 AM, Simon Wunderlich
<[email protected]> wrote:
> To sum from this discussion (I think it's a good idea):
>
> * embed the stats_entry into the sta_info
> * update peer-stats by modifying the embedded stats_entry (we do the lookup anyway
> * keep the non-peer stats in a seperate hash, and only keep stats_entry for them (we don't need
> the full sta_info after all).
>
> We should consider some corner cases here, e.g. adding stas, then we have to
> copy+remove the stats from the non-peer hash, or removing stas, then we have
> to copy the so-far collected stats to the non-peer hash.
>
> If you are okay with it, we can use the NL80211_CMD_GET_STATION command
> (as in iw station dump), and add a seperate flag to give info for non-peer sta.
>
> What about the other commands I suggested (read+reset, start, stop)? For read+reset,
> we could just send yet another flag (RESET_STATS) with the GET_STATION command, but
> for start/stop we would need new commands? Or would you have any better idea?
>
> @Thomas: Is there anything to consider for 802.11s?
I can't think of anything that would be specifically useful for
802.11s right now, and we can always extend the statistics in the
future. One case where this might be useful is if the driver has a
limited number of station slots, the MPM could monitor neighbor
stations for a more "suitable" peer candidate, but your existing stats
should cover that.
Turning on this feature would add some sort of (internal to mac80211)
monitor interface, or just disable all frame filters on a given PHY?
How does it work now?
--
Thomas
On Mon, 2013-02-18 at 17:07 +0100, Antonio Quartulli wrote:
> On Mon, Feb 18, 2013 at 07:58:18 -0800, Johannes Berg wrote:
> > On Mon, 2013-02-18 at 16:49 +0100, Antonio Quartulli wrote:
> > > On Mon, Feb 18, 2013 at 07:43:26 -0800, Johannes Berg wrote:
> > > > > I did not like this approach because the sta_info struct is so big that
> > > > > when we want to fill the stats substruct only we will waste a lot of bytes.
> > > >
> > > > I don't understand your point.
> > > >
> > > > struct sta_info {
> > > > ...
> > > > struct stats stats;
> > > > };
> > >
> > > My concern is about those "..." that we are allocating within the sta_info struct
> > > that we will never use for every non-peer station.
> > >
> > > While if we used the struct below (with its own hash table), we would allocate
> > > only the space needed for the stats.
> > >
> > > >
> > > > struct stats_entry {
> > > > struct hash/list/whatever;
> > > > struct stats stats;
> > > > };
> > > >
> > >
> > >
> > > no?
> > > Maybe I misunderstood your idea?
> >
> > But I'm not saying that these are mutually exclusive, I'm saying both
> > should exist.
>
> Ah ok..Sorry, but I did not take this as an option :)
>
> So, if I understood correctly, this means one table lookup for peer stations,
> while two table lookups for non peers (first in sta_hash, which will fail). Right?
>
> This would save one look up for each peer, since we have to do perform one of
> them anyway (now I fully understood your previous statement!).
Right... But the failing sta lookup has to happen anyway, so it really
adds practically no cost in the peer case, and a singe lookup in the
"non-peer already exists" case.
johannes
On Mon, Feb 18, 2013 at 07:29:47 -0800, Johannes Berg wrote:
> On Mon, 2013-02-18 at 15:46 +0100, Antonio Quartulli wrote:
>
> > In my current implementation I created a "twin hash-table". It contains
> > statistics for *all* the stations (peer and non-peer).
> >
> > I think that instead of embedding this new struct (let's call it sta_stats) into
> > the sta_info one, it would be easier to let them be independent (this is why I
> > created the twin hash) and then create
> > a pointer from the sta_info to the related sta_stats.
>
> I don't really see value in that, it would only make the implementation
> less efficient, because either you follow another pointer (sta->stats)
> or you have to look in the other hash table. That's why I prefer
> embedding it, we have to do the station hash table lookup anyway.
I did not like this approach because the sta_info struct is so big that
when we want to fill the stats substruct only we will waste a lot of bytes.
I think this is the usual space vs. time tread-off :).
However, since this struct is accessed on the critical path, it may be worth to
save time and go for the "embed" approach. Let's see what the other thinks about
it....
>
> > For the API I think we should create a new nl80211 command.
> > If we simply add a
> > flag to the normal "station dump" command, we would not have all the attributes
> > to print (keep in mind station dump prints attributes that are in sta_info and
> > that are not in sta_stats).
>
> station dump can just print all attributes that exist, some stations
> would have more/different attributes than other stations. But I don't
> really think it makes a big difference either way, reusing it would be
> fine if the default is to not include the stats-only entries.
yeah, the default behaviour should remain the same (print peer stations
only).
Cheers,
--
Antonio Quartulli
..each of us alone is worth nothing..
Ernesto "Che" Guevara
On Mon, Feb 18, 2013 at 05:51:42PM +0100, Johannes Berg wrote:
> On Mon, 2013-02-18 at 17:07 +0100, Antonio Quartulli wrote:
> > On Mon, Feb 18, 2013 at 07:58:18 -0800, Johannes Berg wrote:
> > > On Mon, 2013-02-18 at 16:49 +0100, Antonio Quartulli wrote:
> > > > On Mon, Feb 18, 2013 at 07:43:26 -0800, Johannes Berg wrote:
> > > > > > I did not like this approach because the sta_info struct is so big that
> > > > > > when we want to fill the stats substruct only we will waste a lot of bytes.
> > > > >
> > > > > I don't understand your point.
> > > > >
> > > > > struct sta_info {
> > > > > ...
> > > > > struct stats stats;
> > > > > };
> > > >
> > > > My concern is about those "..." that we are allocating within the sta_info struct
> > > > that we will never use for every non-peer station.
> > > >
> > > > While if we used the struct below (with its own hash table), we would allocate
> > > > only the space needed for the stats.
> > > >
> > > > >
> > > > > struct stats_entry {
> > > > > struct hash/list/whatever;
> > > > > struct stats stats;
> > > > > };
> > > > >
> > > >
> > > >
> > > > no?
> > > > Maybe I misunderstood your idea?
> > >
> > > But I'm not saying that these are mutually exclusive, I'm saying both
> > > should exist.
> >
> > Ah ok..Sorry, but I did not take this as an option :)
> >
> > So, if I understood correctly, this means one table lookup for peer stations,
> > while two table lookups for non peers (first in sta_hash, which will fail). Right?
> >
> > This would save one look up for each peer, since we have to do perform one of
> > them anyway (now I fully understood your previous statement!).
>
> Right... But the failing sta lookup has to happen anyway, so it really
> adds practically no cost in the peer case, and a singe lookup in the
> "non-peer already exists" case.
To sum from this discussion (I think it's a good idea):
* embed the stats_entry into the sta_info
* update peer-stats by modifying the embedded stats_entry (we do the lookup anyway
* keep the non-peer stats in a seperate hash, and only keep stats_entry for them (we don't need
the full sta_info after all).
We should consider some corner cases here, e.g. adding stas, then we have to
copy+remove the stats from the non-peer hash, or removing stas, then we have
to copy the so-far collected stats to the non-peer hash.
If you are okay with it, we can use the NL80211_CMD_GET_STATION command
(as in iw station dump), and add a seperate flag to give info for non-peer sta.
What about the other commands I suggested (read+reset, start, stop)? For read+reset,
we could just send yet another flag (RESET_STATS) with the GET_STATION command, but
for start/stop we would need new commands? Or would you have any better idea?
@Thomas: Is there anything to consider for 802.11s?
Thanks for your input,
Simon
On Mon, Feb 18, 2013 at 07:58:18 -0800, Johannes Berg wrote:
> On Mon, 2013-02-18 at 16:49 +0100, Antonio Quartulli wrote:
> > On Mon, Feb 18, 2013 at 07:43:26 -0800, Johannes Berg wrote:
> > > > I did not like this approach because the sta_info struct is so big that
> > > > when we want to fill the stats substruct only we will waste a lot of bytes.
> > >
> > > I don't understand your point.
> > >
> > > struct sta_info {
> > > ...
> > > struct stats stats;
> > > };
> >
> > My concern is about those "..." that we are allocating within the sta_info struct
> > that we will never use for every non-peer station.
> >
> > While if we used the struct below (with its own hash table), we would allocate
> > only the space needed for the stats.
> >
> > >
> > > struct stats_entry {
> > > struct hash/list/whatever;
> > > struct stats stats;
> > > };
> > >
> >
> >
> > no?
> > Maybe I misunderstood your idea?
>
> But I'm not saying that these are mutually exclusive, I'm saying both
> should exist.
Ah ok..Sorry, but I did not take this as an option :)
So, if I understood correctly, this means one table lookup for peer stations,
while two table lookups for non peers (first in sta_hash, which will fail). Right?
This would save one look up for each peer, since we have to do perform one of
them anyway (now I fully understood your previous statement!).
Cheers,
--
Antonio Quartulli
..each of us alone is worth nothing..
Ernesto "Che" Guevara
On Mon, 2013-02-18 at 16:49 +0100, Antonio Quartulli wrote:
> On Mon, Feb 18, 2013 at 07:43:26 -0800, Johannes Berg wrote:
> > > I did not like this approach because the sta_info struct is so big that
> > > when we want to fill the stats substruct only we will waste a lot of bytes.
> >
> > I don't understand your point.
> >
> > struct sta_info {
> > ...
> > struct stats stats;
> > };
>
> My concern is about those "..." that we are allocating within the sta_info struct
> that we will never use for every non-peer station.
>
> While if we used the struct below (with its own hash table), we would allocate
> only the space needed for the stats.
>
> >
> > struct stats_entry {
> > struct hash/list/whatever;
> > struct stats stats;
> > };
> >
>
>
> no?
> Maybe I misunderstood your idea?
But I'm not saying that these are mutually exclusive, I'm saying both
should exist.
johannes
On Fri, 2013-02-15 at 18:19 +0100, Simon Wunderlich wrote:
> Commands we would like to propose are:
> * start collecting - this feature should not run by default to avoid bloating memory for users who
> don't even need this
> * stop collecting
> * read - dumps the data for all stations
> * read + reset - dump the data and reset information for all stations. This should also clean up stations,
> at least those which are not connected to the BSS, to not bloat the station table.
>
> I guess the right position to implement this is mac80211 receive path. Our intended platform
> is ath9k/ath5k, but that feature should work with any mac80211 driver. We don't care if sta_info
> structs are allocated or custom structures are used, as long as we can receive a list of stations
> which includes peer and non-peer stations, along with their statistics.
>
> We are looking forward to your thoughts. :)
I would argue that since most of the sta_info struct is used for
operational stuff, you shouldn't use it, but have a separate struct and
maybe embed that separate struct in sta_info for its statistics.
I'd also not use the existing nl80211 station APIs since this could be
an optional feature for many things, and it will likely break existing
expectations, e.g. that all stations listed by "iw wlan0 station dump"
are clients connected to an AP interface.
It could be argued that this API then should also not even include the
connected stations when listing ones, i.e. explicitly be non-connected
stations.
johannes
On Fri, Feb 22, 2013 at 9:03 AM, Felix Fietkau <[email protected]> wrote:
> On 2013-02-22 5:36 PM, Antonio Quartulli wrote:
>> On Fri, Feb 22, 2013 at 08:21:14AM -0800, Felix Fietkau wrote:
>>> Hi,
>>>
>>> Since this is a rare special case, I think it really does not belong
>>> into the mac80211 data path. How about creating a monitor mode device
>>> and claiming it from within the kernel in your own module via rx_handler
>>> (the same mechanism that the bridge code uses to hook into the data path
>>> of a normal net_device). You can extract relevant information from the
>>> radiotap headers.
>>
>> I think this would increase the complexity given the fact that each and every
>> packet needs to be encapsulated into the radiotap header..
> I think it's only a slight increase in complexity.
>
>> Then, most of the information that people wants to grab does not appear in
>> such header.
> What's missing?
>
>> Other than that, by hooking into the mac80211 rx_path, in an early point, packets can
>> then be dropped as usual (all those packets not going to this device), while
>> with this monitor-like solution all of them have to be carried up to the virtual
>> interface.
> If the data path overhead here is really that important, then I suggest
> adding a new minimalistic data path hook that allows you to inspect the
> packets with the rx data in the cb from an external module. The actual
> node tracking and reporting should be outside of mac80211 as long as the
> reasons for putting it in have not been properly justified.
>
>> Imho this introduces a not negligible complexity that it is better to avoid.
>>
>> And what about 802.11s? I don't think this is a good solution for them..
>
> I have yet to see an explanation how this gathering of semi-related data
> points even helps 802.11s. I only noticed this bit here by Thomas:
>> I can't think of anything that would be specifically useful for
>> 802.11s right now, and we can always extend the statistics in the
>> future. One case where this might be useful is if the driver has a
>> limited number of station slots, the MPM could monitor neighbor
>> stations for a more "suitable" peer candidate, but your existing stats
>> should cover that.
> Is 'limited number of station slots' even a real issue or just an
> abstract hypothetical? What are other things in 802.11s that this would
> be useful for?
ath9k_htc has a real limit of number of station slots in the firmware.
@Antonio: I think you should leave the mac80211 mesh code out of your
considerations for now though. Non-peer tracking for the kernel MPM
will look more like OBSS scanning (for beacon collision avoidance and
correct HT protection mode). If any high level decisions and
statistical analysis is needed, we can do that in the userspace mesh
implementation (much like IBSS).
--
Thomas
.. considering I do radiotap inspection in promiscuous mode all the
time (on laptops and embedded MIPS boards alike), I'm kinda interested
in why you're seeing such high CPU utilisation.
Are you trying to capture the whole frame? Or are you only capturing
the header? It sounds like you only need the header, right?
I agree with Felix - this is the kind of thing that it makes sense to
define an API for so you can write in-kernel monitoring plugins. Then
your monitoring plugin can decide how to aggregate and pass up the
data.
Adrian
Hey Thomas,
On Tue, Feb 19, 2013 at 10:32:29AM +0100, Thomas Hühn wrote:
> Hi all,
>
> You motivate your proposal of collecting several statistics from non-peering stations in IBBS networks with:
> (1) "The statistics are then used to evaluate link quality and make some higher level decisions"
> &
> (2)"reading every packet from this monitor interface has a huge thoughput limitation"
>
> Could you go into more details about what "higher level" decisions you have in mind ?
> Until now you just listed a bunch of values you like to monitor on a per packet level without any concrete usage idea or algorithms that could make use of it.
> I could guess that a common goal would be to increase performance in wireless networks, but lets get concrete about the approach you have in mind.
> A lot of recent theoretical research in wireless goes towards interference management, multi-user and so the gains of using channel state information as feedback that is worth its overhead. But this direction would question the packet level granularity you described… so what about aggregated statistics ?
thanks for your questions, we did not explain this in detail and I think Mathias can
give some more background on this, but I'll try to explain:
We are currently evaluating some statistics in userspace by collecting packets via the monitor
interface. We evaluate sequence number jumps (which indicate packet loss due to interference),
signal levels and such to throw this in a model to assess the channel quality. This is used
to make routing decisions according to channel qualities within a network (assume a multihop
mesh-like network). One problem we see here is that reading and evaluating these packets in
userspace is performance, reading every packet just uses a lot of CPU cycles (because of
the transfer kernel->userspace and such). Therefore we would like to move this statistics
collection into kernelspace, and only retrieve aggregated statistics.
So the goal for us to improve performance of the channel assessment we are already doing, and
use it for this custom routing software. Maybe other users can use a similar approach on
other kinds of routing software, or even completely different tasks I'm currently not aware
of (e.g. these 802.11s use cases).
Cheers,
Simon
On Mon, Feb 18, 2013 at 07:43:26 -0800, Johannes Berg wrote:
> > I did not like this approach because the sta_info struct is so big that
> > when we want to fill the stats substruct only we will waste a lot of bytes.
>
> I don't understand your point.
>
> struct sta_info {
> ...
> struct stats stats;
> };
My concern is about those "..." that we are allocating within the sta_info struct
that we will never use for every non-peer station.
While if we used the struct below (with its own hash table), we would allocate
only the space needed for the stats.
>
> struct stats_entry {
> struct hash/list/whatever;
> struct stats stats;
> };
>
no?
Maybe I misunderstood your idea?
Cheers,
--
Antonio Quartulli
..each of us alone is worth nothing..
Ernesto "Che" Guevara
Hey,
On Mon, Feb 25, 2013 at 11:28:17AM +0100, Simon Wunderlich wrote:
> On Fri, Feb 22, 2013 at 09:42:43AM -0800, Adrian Chadd wrote:
> Anyway, I'll look at the rx_handler stuff what Felix suggested and see
> if this could work out. :)
>
We've drafted some external module - the general approach seems to work quite
nicely, the only thing which appears needless it to add a radiotap header
and remove it later in the statistics module again ...
Anyway, here is some code to digest for anyone who is interested:
https://github.com/simonwunderlich/wifi_statistics
It hooks in an rx_handler onto mon0 (yes, that's currently static), and
throws out some info via debugfs. It parses the radiotap and wifi header
of every packet on the monitor interface.
@Antonio, Zefir: I'd be interested in your opinions. Would that be useful
for you too? Should we add all the statistics we are interested in, or better
use some kind of API if we are interested in "special statistics" which no one
else is?
If there is general interest, we can propose to include it in upstream Linux.
Otherwise, we can maintain it (together ?) as seperate module. It's not so
big anyway ...
Cheers,
Simon
Hey Zefir,
On Mon, Mar 11, 2013 at 01:01:03PM +0100, Zefir Kurtisi wrote:
> Hello Simon,
>
> tested and looks good.
thanks for the feedback!
>
> I had to apply below patch to make it compile on latest
> compat-drivers (hlist_for_each_entry_* API changed in 3.9).
I've adopted your changes and added some compat code.
>
> It is definitively useful and I will start on top of it
> when I get back to work on our statistics module.
>
> I have no preferences whether to keep it separated or
> integrate it upstream. If statistics are required for
> mesh, there's maybe no choice other than integrating.
Thanks for sharing your thoughts!
As far as I understood from Thomas, we should not consider
the mesh case too much for now. Also I don't know if this
implementation is very useful for him/802.11s
I'll need to put some more effort in this module first
(its more a proof of concept for now). Maybe we propose it
for integration later. But I'm grateful for any patches like the
one you sent with this mail. ;)
>
> As for the API, I'd propose to leave it as simple as it is
> now, i.e. whoever needs more / different statistics hooks
> into ws_handle_frame() and adds his handlers after
> ws_sta_parse_ieee80211_hdr().
Yeah maybe you are right, it's the best to keep it as simple
as it is.
Thanks again,
Simon
Hello Simon,
tested and looks good.
I had to apply below patch to make it compile on latest
compat-drivers (hlist_for_each_entry_* API changed in 3.9).
It is definitively useful and I will start on top of it
when I get back to work on our statistics module.
I have no preferences whether to keep it separated or
integrate it upstream. If statistics are required for
mesh, there's maybe no choice other than integrating.
As for the API, I'd propose to leave it as simple as it is
now, i.e. whoever needs more / different statistics hooks
into ws_handle_frame() and adds his handlers after
ws_sta_parse_ieee80211_hdr().
Thanks for sharing this,
Zefir
---
diff --git a/Makefile b/Makefile
new file mode 100644
index 0000000..1d8cbd2
--- /dev/null
+++ b/Makefile
@@ -0,0 +1,3 @@
+obj-m += wifi_statistics.o
+wifi_statistics-y = debugfs.o hash.o station.o main.o
+
diff --git a/debugfs.c b/debugfs.c
index 94724ed..414ee4e 100644
--- a/debugfs.c
+++ b/debugfs.c
@@ -26,7 +26,6 @@ int ws_sta_seq_read(struct seq_file *seq, void *offset)
{
struct ws_hash *hash = &ws_hash;
struct hlist_head *head;
- struct hlist_node *node;
struct ws_sta *ws_sta;
int i;
@@ -34,7 +33,7 @@ int ws_sta_seq_read(struct seq_file *seq, void *offset)
head = &hash->table[i];
rcu_read_lock();
- hlist_for_each_entry_rcu(ws_sta, node, head, hash_entry)
+ hlist_for_each_entry_rcu(ws_sta, head, hash_entry)
ws_sta_seq_print(ws_sta, seq, offset);
rcu_read_unlock();
}
@@ -45,7 +44,7 @@ int ws_sta_seq_read_reset(struct seq_file *seq, void *offset)
{
struct ws_hash *hash = &ws_hash;
struct hlist_head *head;
- struct hlist_node *node, *node_tmp;
+ struct hlist_node *node;
spinlock_t *list_lock; /* protects write access to the hash lists */
struct ws_sta *ws_sta;
int i;
@@ -56,7 +55,7 @@ int ws_sta_seq_read_reset(struct seq_file *seq, void *offset)
/* TODO: can we write while doing spinlocks?! */
spin_lock_bh(list_lock);
- hlist_for_each_entry_safe(ws_sta, node, node_tmp, head, hash_entry) {
+ hlist_for_each_entry_safe(ws_sta, node, head, hash_entry) {
ws_sta_seq_print(ws_sta, seq, offset);
ws_sta_free_ref(ws_sta);
hlist_del_rcu(node);
diff --git a/hash.c b/hash.c
index 81e08d3..c76fc10 100644
--- a/hash.c
+++ b/hash.c
@@ -41,7 +41,6 @@ struct ws_sta *ws_hash_find(struct ws_hash *hash, u8 *mac)
struct ws_sta *res = NULL, *tmp_sta;
spinlock_t *list_lock; /* spinlock to protect write access */
struct hlist_head *head;
- struct hlist_node *node;
u32 index;
index = ws_hash_choose(mac);
@@ -49,7 +48,7 @@ struct ws_sta *ws_hash_find(struct ws_hash *hash, u8 *mac)
list_lock = &hash->list_locks[index];
rcu_read_lock();
- hlist_for_each_entry_rcu(tmp_sta, node, head, hash_entry) {
+ hlist_for_each_entry_rcu(tmp_sta, head, hash_entry) {
if (memcmp(mac, tmp_sta->mac, ETH_ALEN))
continue;
@@ -104,7 +103,7 @@ int ws_hash_free(void)
{
struct ws_hash *hash = &ws_hash; /* static for now */
struct ws_sta *ws_sta;
- struct hlist_node *node, *node_tmp;
+ struct hlist_node *node;
struct hlist_head *head;
spinlock_t *list_lock; /* protects write access to the hash lists */
int i;
@@ -114,7 +113,7 @@ int ws_hash_free(void)
list_lock = &hash->list_locks[i];
spin_lock_bh(list_lock);
- hlist_for_each_entry_safe(ws_sta, node, node_tmp, head, hash_entry) {
+ hlist_for_each_entry_safe(ws_sta, node, head, hash_entry) {
ws_sta_free_ref(ws_sta);
hlist_del_rcu(node);
}
diff --git a/wifi-statistics.mod.c b/wifi-statistics.mod.c
deleted file mode 100644
index 198264b..0000000
--- a/wifi-statistics.mod.c
+++ /dev/null
@@ -1,64 +0,0 @@
-#include <linux/module.h>
-#include <linux/vermagic.h>
-#include <linux/compiler.h>
-
-MODULE_INFO(vermagic, VERMAGIC_STRING);
-
-struct module __this_module
-__attribute__((section(".gnu.linkonce.this_module"))) = {
- .name = KBUILD_MODNAME,
- .init = init_module,
-#ifdef CONFIG_MODULE_UNLOAD
- .exit = cleanup_module,
-#endif
- .arch = MODULE_ARCH_INIT,
-};
-
-static const struct modversion_info ____versions[]
-__used
-__attribute__((section("__versions"))) = {
- { 0x568fba06, "module_layout" },
- { 0x5a5e7ea3, "simple_read_from_buffer" },
- { 0xd5f8162f, "debugfs_create_dir" },
- { 0x9b5b51b2, "single_open" },
- { 0x60a13e90, "rcu_barrier" },
- { 0xfd492070, "single_release" },
- { 0x4d884691, "malloc_sizes" },
- { 0x34ec54de, "netdev_rx_handler_register" },
- { 0xc7a4fbed, "rtnl_lock" },
- { 0x1637ff0f, "_raw_spin_lock_bh" },
- { 0xb53c5051, "skb_clone" },
- { 0xfa1d0ed8, "dev_get_by_name" },
- { 0x21a7d814, "seq_printf" },
- { 0xc63f1b81, "ieee80211_radiotap_iterator_next" },
- { 0xc2a3e73d, "debugfs_create_file" },
- { 0x60152b00, "debugfs_remove_recursive" },
- { 0xcb49c52f, "seq_read" },
- { 0x7d11c268, "jiffies" },
- { 0x760a4192, "__pskb_pull_tail" },
- { 0x8bfe7988, "default_llseek" },
- { 0x37befc70, "jiffies_to_msecs" },
- { 0x2fa5a500, "memcmp" },
- { 0x982e6b6d, "ieee80211_radiotap_iterator_init" },
- { 0xa1c76e0a, "_cond_resched" },
- { 0x85abc85f, "strncmp" },
- { 0x86856070, "skb_pull" },
- { 0x3775290c, "init_net" },
- { 0xba63339c, "_raw_spin_unlock_bh" },
- { 0xb6251ac0, "netdev_rx_handler_unregister" },
- { 0xbb0ebf03, "kfree_skb" },
- { 0x7a172b67, "kmem_cache_alloc_trace" },
- { 0xa4b3ff06, "ieee80211_get_hdrlen_from_skb" },
- { 0x10166945, "seq_lseek" },
- { 0x50f5e532, "call_rcu_sched" },
- { 0x77e2f33, "_copy_from_user" },
- { 0x6e720ff2, "rtnl_unlock" },
-};
-
-static const char __module_depends[]
-__used
-__attribute__((section(".modinfo"))) =
-"depends=cfg80211";
-
-
-MODULE_INFO(srcversion, "2D50C255DD87C856632A707");