putting ieee80211_tx_control into skb->cb?

putting ieee80211_tx_control into skb->cb?

[Johannes Berg]

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

With my patch to shrink ieee80211_tx_control, I can now put that into
skb->cb.

Do we want to use that as the interface to drivers as well? Many drivers
could benefit from that a lot since we could save all the extra copies
done here.

I've grepped all drivers, currently those using skb->cb are:
 * p54:     only needs 8 bytes
 * rt2x00:  needs almost all the space (40 bytes)
 * adm8211: puts the 802.11 header there
 * rtl8180: needs a single pointer to control
 * rtl8187: usb urb, control and hw pointers
 * zd1211:  control, hw and context pointers

Notice how four drivers here put the control information in there, which
needs to be copied by drivers. However, mac80211 only needs the control
flags later!

Additionally, mac80211 needs to copy the TX status information around a
lot when the irqsafe functions are used. That's also bad.

What we could do is this:

First, we put ieee80211_tx_control into skb->cb. The first member of
that is a 'u32 flags' which is the only thing we need later for TX
status reporting, so we require that drivers leave that in there.

Then, we merge the TX status flags into the TX flags so that now the
driver only needs to set a few flags in skb->cb. Also, we overlay this.

Hence, we have something like the following structure:

struct ieee80211_tx_information {
    u32 flags;

    union {
        struct {
            s8 tx_rate_idx, rts_cts_rate_idx,
               alt_retry_rate_idx, retry_limit;
            struct ieee80211_vif *vif;
            struct ieee80211_key_conf *hw_key;
            enum ieee80211_band band;
            u16 aid;
            u8 antenna_sel_tx;
            u8 icv_len, iv_len;
        } txctl;

        struct {
            u64 amdpu_ack_map;
            int ack_signal;
            u8 ampdu_ack_len;
            bool excessive_retries; /* should be a flag / removed */
            u8 retry_count;
        } status;

        /* can be used freely by driver after its ops->tx() accepts
         * the frame and before tx status reporting */
        u8 drv_data[40] __attribute__((__aligned__(sizeof(void *)));
    };
};

This means that the drivers are free to use everything in skb->cb for
their own use but should leave the 'u32 flags' intact, and need to make
sure to copy out all the information they need before putting in new
information. This fits with all the drivers, rt2x00 needs at most 32
bytes (40 now, but one is the txcontrol pointer) while we have 44 free.
Also, if the driver rejects the skb in ops->tx(), it must not have
modified skb->cb.

The only thing I'm not sure yet is how to handle alignment here. skb->cb
itself is 8-byte aligned so we probably need to reserve another four
bytes to get things to 8-byte alignment and then we go down to 40
available bytes, this is what I did above.

Thoughts?

johannes

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

Re: putting ieee80211_tx_control into skb->cb?

[Ivo van Doorn]

On Wednesday 30 April 2008, Johannes Berg wrote:
> With my patch to shrink ieee80211_tx_control, I can now put that into
> skb->cb.
> 
> Do we want to use that as the interface to drivers as well? Many drivers
> could benefit from that a lot since we could save all the extra copies
> done here.
> 
> I've grepped all drivers, currently those using skb->cb are:
>  * p54:     only needs 8 bytes
>  * rt2x00:  needs almost all the space (40 bytes)

Latest rt2x00.git needs _all_ space (48 bytes)
However I can trim it down to 40 bytes quite easily,
since mac80211 wasn't using it anyway I wasn't too conservative
about the usage. ;)
I could even trim it down further if I disable the TX/RX dumping
through debugfs as intermediate solution.

>  * adm8211: puts the 802.11 header there
>  * rtl8180: needs a single pointer to control
>  * rtl8187: usb urb, control and hw pointers
>  * zd1211:  control, hw and context pointers
> 
> Notice how four drivers here put the control information in there, which
> needs to be copied by drivers. However, mac80211 only needs the control
> flags later!

Very nice, removing the requirement to store the entire control data would
save a lot of memory for rt2x00. (Currently it has a per-entry copy of the
tx control data).

> First, we put ieee80211_tx_control into skb->cb. The first member of
> that is a 'u32 flags' which is the only thing we need later for TX
> status reporting, so we require that drivers leave that in there.
> 
> Then, we merge the TX status flags into the TX flags so that now the
> driver only needs to set a few flags in skb->cb. Also, we overlay this.
> 
> Hence, we have something like the following structure:
> 
> struct ieee80211_tx_information {
>     u32 flags;
> 
>     union {
>         struct {
>             s8 tx_rate_idx, rts_cts_rate_idx,
>                alt_retry_rate_idx, retry_limit;
>             struct ieee80211_vif *vif;
>             struct ieee80211_key_conf *hw_key;
>             enum ieee80211_band band;
>             u16 aid;
>             u8 antenna_sel_tx;
>             u8 icv_len, iv_len;
>         } txctl;
> 
>         struct {
>             u64 amdpu_ack_map;
>             int ack_signal;
>             u8 ampdu_ack_len;
>             bool excessive_retries; /* should be a flag / removed */
>             u8 retry_count;
>         } status;
> 
>         /* can be used freely by driver after its ops->tx() accepts
>          * the frame and before tx status reporting */
>         u8 drv_data[40] __attribute__((__aligned__(sizeof(void *)));
>     };
> };
> 
> This means that the drivers are free to use everything in skb->cb for
> their own use but should leave the 'u32 flags' intact, and need to make
> sure to copy out all the information they need before putting in new
> information. This fits with all the drivers, rt2x00 needs at most 32
> bytes (40 now, but one is the txcontrol pointer) while we have 44 free.
> Also, if the driver rejects the skb in ops->tx(), it must not have
> modified skb->cb.

For rt2x00 that won't be a problem, it won't touch the skb->cb anyway
until it has performed all checks for queue entry availability.

> The only thing I'm not sure yet is how to handle alignment here. skb->cb
> itself is 8-byte aligned so we probably need to reserve another four
> bytes to get things to 8-byte alignment and then we go down to 40
> available bytes, this is what I did above.

Ivo

Re: putting ieee80211_tx_control into skb->cb?

[Johannes Berg]

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


> Latest rt2x00.git needs _all_ space (48 bytes)
> However I can trim it down to 40 bytes quite easily,
> since mac80211 wasn't using it anyway I wasn't too conservative
> about the usage. ;)

Heh.

> I could even trim it down further if I disable the TX/RX dumping
> through debugfs as intermediate solution.

I'm working on a patch that keeps 40 bytes available for the driver.

> Very nice, removing the requirement to store the entire control data would
> save a lot of memory for rt2x00. (Currently it has a per-entry copy of the
> tx control data).

Yeah, that's one thing, another is all the copying of it. Since I wrote
the mail I have extended my idea to the internal packet_data struct
which again saves a lot of code.

> > This means that the drivers are free to use everything in skb->cb for
> > their own use but should leave the 'u32 flags' intact, and need to make
> > sure to copy out all the information they need before putting in new
> > information. This fits with all the drivers, rt2x00 needs at most 32
> > bytes (40 now, but one is the txcontrol pointer) while we have 44 free.
> > Also, if the driver rejects the skb in ops->tx(), it must not have
> > modified skb->cb.
> 
> For rt2x00 that won't be a problem, it won't touch the skb->cb anyway
> until it has performed all checks for queue entry availability.

Good :)

johannes

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

Re: putting ieee80211_tx_control into skb->cb?

[Johannes Berg]

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

On Wed, 2008-04-30 at 17:47 +0200, Johannes Berg wrote:
> With my patch to shrink ieee80211_tx_control, I can now put that into
> skb->cb.
> 
> Do we want to use that as the interface to drivers as well? Many drivers
> could benefit from that a lot since we could save all the extra copies
> done here.

Yes, we do.

Proof-of-concept code
here: http://johannes.sipsolutions.net/patches/kernel/all/2008-05-01-00:32/041-mac80211-tx-info-skb-cb.patch

If you want to give it a try, beware! I changed a number of patches this
is based on, the GSO-fragmentation one is among them, so you really want
to pull all those from there rather than using the versions I posted.

Will have to update all the other drivers now (done b43 so far)... But
I'll let this simmer in my tree for a while and test with AP mode etc.,
I think we should do one thing at a time ;)

johannes

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