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

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1: Type: text/plain, Size: 3283 bytes --]

Hi Ron,

> Johannes, i re-inspected the code to implement your solution, and it
> is not straight forward as you have seen it.

Ok.

> 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

Yeah, sounds like it. Some things like monitor may discard the frame.

> 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

Hmm, good points. Didn't really think about it but yeah, txrx data is on
the stack there.

> 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.

Sounds fine.

> 2 - __ieee80211_rx_pre_handle will be called for each frame. either
> directly or through tasklet.

Hm, ok, we should keep __ieee80211_rx() as the entry point though so we
don't have to change drivers.

> 3 - in regular state, proceed to __ieee80211_rx immediately, in
> reorder state loop over __ieee80211_rx for each already ordered frame.

Ah ok.

> 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)

I don't think it can move there because the rx handlers can potentially
be invoked multiple times. Will that work? In any case, I'm not against
totally dissolving the pre-handlers into function calls. Makes for
better code generation anyway since gcc will be able to optimise across
the functions when inlining them (since they'll all have only one user)

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

Not at all, thanks for doing that.

I think we should probably keep __iee80211_rx() as the entry point for
the tasklet or the drivers.

So I'd see the flow as:

tasklet/driver
__ieee80211_rx()
	__rx_pre_monitor()
	__rx_pre_load()
	__rx_pre_qos()

	if (agg)
		__rx_handle_reorder()
	else
		__rx_handle_packet()

and __rx_handle_reorder() calls __rx_handle_packet() which does all the
rest, I guess.

Does that sound sane to you? You're in a better position to judge. I
think this is basically what you wanted, just keeping __ieee80211_rx()
as the main entry point.

Or maybe I'm totally off. Can you explain again what we need to do for
such a reordered frame?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]