Re: [PATCH 5/8] mac80211: A-MPDU Rx handling aggregation reordering

["Ron Rindjunsky" <ron.rindjunsky@intel.com>]

>
> Basically, it seems to me that you're injecting frames down the same
> __ieee80211_rx() path when they have already gone through that, and then
> after the fact annotate pretty much all of that path before the actual
> rx handlers (after pre-handlers) with "skip if reordered frame". That I
> don't really like, it means we need to special-case everything we ever
> put there.
>
> Hence, I think it'd be best to first reorganise the current code:
>  (1) split __ieee80211_rx() right after the pre handler invocation and
>     call e.g. __ieee80211_rx_handlers() which contains all the code
>     from "if (sta && !(sta->flags & (WLAN_STA_WDS | WLAN_STA_ASSOC_AP"
>     to "} else dev_kfree_skb(skb);"
>     The function would have certain requirements like a filled
>     ieee80211_txrx_data and running under rcu_read_lock().
>  (2) then, you don't need status.ordered but can drop the frame right
>     away or pass it to __ieee80211_rx_handlers().
>
> That way, we guarantee that each frame only passes through the complete
> RX path once and your dropping the unordered frames is equivalent to

Johannes, i re-inspected the code to implement your solution, and it
is not straight forward as you have seen it.
what we do now in __ieee80211_rx is:
- invoke monitor
- fill in txrx_data
- invoke michael_mic_report (if needed)
- call pre-handlers of QoS that change txrx_data as well.
- call pre-handlers of statistics that change txrx_data
- call reorder buffer prehandler
    - saving sk_buff to reordering buffer.
    - if we can, send sk_buffs back to __ieee80211_rx, that will
properly fill txrx_data

now, in order to proceed according to your implementation, as i do not
call again  __ieee80211, and do not get the txrx_data restored,  i
will either:
1 - have to fill in txrx_data inside reordering function, which will
end in massive code duplication, and i don't like this solution
2 - keep the whole txrx_data and not only the sk_buff inside
reordering buffer, which will end up in allocation of the txrx_data
struct for every incoming frame, or in a big maximum possible size of
64 pre-allocated array of txrx_data per tid - this is either waste of
cpu or memory
3 - stick to my current implementation, but assure that no function
will be called for ordered frames on their second call in __ieee80211.
we already mentioned it may lead to low encapsulation of reordering
mechanism.

so, what do you think about next solution:

1 - move the rxtx_data indifferent functionality out of __ieee80211_rx
(monitor, load and reordering) to, lets say __ieee80211_rx_pre_handle.
2 - __ieee80211_rx_pre_handle will be called for each frame. either
directly or through tasklet.
3 - in regular state, proceed to __ieee80211_rx immediately, in
reorder state loop over __ieee80211_rx for each already ordered frame.
4 - I thought to eliminate __ieee80211_invoke_rx_handlers(local,
local->rx_pre_handlers), as QoS will be the only one to inhabit it
now, so it may move to ieee80211_invoke_rx_handlers(local,
local->rx_handlers)

sorry for the long mail, but i wanted to write my full opinion about this issue.