| Message ID | 1425318028-26531-9-git-send-email-fw@strlen.de |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2015-03-02 at 18:40 +0100, Florian Westphal wrote: > to make it fit into (future) 44-byte sized skb->cb[]. > > This works, since flags is only used to store values > from mac80211_tx_control_flags enum, and these are just 2 bits. > We can thus move this to the padding hole inside the union. > > Also add BUILD_BUG_ON magic to make sure that the new flags > field doesn't share storage w. other members of the union. This is really ugly - what's the point of this? Mind you - we are actually acutely out of space and would rather have *more*, not less. johannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Johannes Berg <johannes@sipsolutions.net> wrote: > On Mon, 2015-03-02 at 18:40 +0100, Florian Westphal wrote: > > to make it fit into (future) 44-byte sized skb->cb[]. > > > > This works, since flags is only used to store values > > from mac80211_tx_control_flags enum, and these are just 2 bits. > > We can thus move this to the padding hole inside the union. > > > > Also add BUILD_BUG_ON magic to make sure that the new flags > > field doesn't share storage w. other members of the union. > > This is really ugly - what's the point of this? Eventually reducing skb size to make it fit into 3 cachelines again even on 64bit architectures. For that 40 bytes need to go. > Mind you - we are actually acutely out of space and would rather have > *more*, not less. :-( I'm not familiar with mac80211, aside from that it seemed to me that 40 byte cb would be doable, given enough work. Where are to main problems, exactly? I known that pushing something into ->cb is a lot easier than e.g. keeping extra state on stack, but, IMO cb should really only be used when you need to associate data strictly with an skb so that this data is still availabe even when skb gets queued somewehere. Is there a document somewhere that lists all of the per-skb data that mac80211 needs to store (or wants to store)? Thanks, Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2015-03-02 at 20:03 +0100, Florian Westphal wrote: > Eventually reducing skb size to make it fit into 3 cachelines again even > on 64bit architectures. For that 40 bytes need to go. That seems like a worthy goal I guess (I guess you should've copied a patch 0/N to us). > > Mind you - we are actually acutely out of space and would rather have > > *more*, not less. > > :-( > > I'm not familiar with mac80211, aside from that it seemed to me > that 40 byte cb would be doable, given enough work. Right now it is, mostly, yes. > Where are to main problems, exactly? Well, that depends. Right now clearly we're not really using all of it as you saw (even if this patch moving bits here and there is really ugly) but there are multiple things: 1) Of course mac80211 isn't static, it keeps getting developed! Right now for example we need to fix single TCP flow throughput over wifi, and for that we need a timestamp. That won't even fit into skb->cb any more right now; we'll probably be able to get away with (ab)using skb->tstamp or doing reshuffling similar to yours to get some space, but that's just lucky this time. 2) We're actually out of flags that are kept from TX generation to TX destruction and it's almost certain that we'll need to add more things. Also, we already do a lot of bit twiddling here, doing it even more makes the code even harder to follow. It's bad enough as is if you ask me. > I known that pushing something into ->cb is a lot easier than e.g. keeping > extra state on stack, but, IMO cb should really only be used when you > need to associate data strictly with an skb so that this data is still > availabe even when skb gets queued somewehere. Right, and we do that. We've in the past moved out data from here to elsewhere, but it's extremely tedious and error-prone, and I'm not sure we have much that we can possibly move now, since we do need to hang on to SKBs in many cases like client powersaving etc. > Is there a document somewhere that lists all of the per-skb data that > mac80211 needs to store (or wants to store)? Not really, sorry. > + enum mac80211_tx_control_flags flags:2; > + /* used for BUILD_BUG_ON validation that ->flags won't > + * overlap with jiffies below */ > + char flags_end[0]; That "jiffies" should probably say "other members (currently jiffies)" or something ... I got a bit confused here. johannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Johannes Berg <johannes@sipsolutions.net> wrote: > On Mon, 2015-03-02 at 20:03 +0100, Florian Westphal wrote: > > > Eventually reducing skb size to make it fit into 3 cachelines again even > > on 64bit architectures. For that 40 bytes need to go. > > That seems like a worthy goal I guess (I guess you should've copied a > patch 0/N to us). Indeed, sorry. archived cover letter: http://marc.info/?l=linux-netdev&m=142531804325076&w=2 > > I'm not familiar with mac80211, aside from that it seemed to me > > that 40 byte cb would be doable, given enough work. > > Right now it is, mostly, yes. > > > Where are to main problems, exactly? > > Well, that depends. Right now clearly we're not really using all of it > as you saw (even if this patch moving bits here and there is really > ugly) but there are multiple things: > 1) Of course mac80211 isn't static, it keeps getting developed! Right > now for > example we need to fix single TCP flow throughput over wifi, and for > that we > need a timestamp. That won't even fit into skb->cb any more right > now; we'll > probably be able to get away with (ab)using skb->tstamp or doing > reshuffling > similar to yours to get some space, but that's just lucky this time. > 2) We're actually out of flags that are kept from TX generation to TX > destruction and it's almost certain that we'll need to add more > things. > > Also, we already do a lot of bit twiddling here, doing it even more > makes the code even harder to follow. It's bad enough as is if you ask > me. True. > > I known that pushing something into ->cb is a lot easier than e.g. keeping > > extra state on stack, but, IMO cb should really only be used when you > > need to associate data strictly with an skb so that this data is still > > availabe even when skb gets queued somewehere. > > Right, and we do that. We've in the past moved out data from here to > elsewhere, but it's extremely tedious and error-prone, and I'm not sure > we have much that we can possibly move now, since we do need to hang on > to SKBs in many cases like client powersaving etc. I see. I cannot comment any further at the moment, I will have to dig into mac80211 more. Thanks for your comments! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 63c3708..36c2599 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -737,16 +737,21 @@ struct ieee80211_tx_info { u8 use_cts_prot:1; u8 short_preamble:1; u8 skip_table:1; - /* 2 bytes free */ + enum mac80211_tx_control_flags flags:2; + /* used for BUILD_BUG_ON validation that ->flags won't + * overlap with jiffies below */ + char flags_end[0]; }; /* only needed before rate control */ unsigned long jiffies; + + /* used for BUILD_BUG_ON validation that ->flags won't + * overlap with other members of this union. */ + char union_end[0]; }; /* NB: vif can be NULL for injected frames */ struct ieee80211_vif *vif; struct ieee80211_key_conf *hw_key; - u32 flags; - /* 4 bytes free */ } control; struct { struct ieee80211_tx_rate rates[IEEE80211_TX_MAX_RATES]; diff --git a/net/mac80211/main.c b/net/mac80211/main.c index 5e09d35..9e8c807 100644 --- a/net/mac80211/main.c +++ b/net/mac80211/main.c @@ -1218,6 +1218,9 @@ static int __init ieee80211_init(void) BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, driver_data) + IEEE80211_TX_INFO_DRIVER_DATA_SIZE > sizeof(skb->cb)); + BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, control.flags_end) < + offsetof(struct ieee80211_tx_info, control.union_end)); + ret = rc80211_minstrel_init(); if (ret) return ret;
to make it fit into (future) 44-byte sized skb->cb[]. This works, since flags is only used to store values from mac80211_tx_control_flags enum, and these are just 2 bits. We can thus move this to the padding hole inside the union. Also add BUILD_BUG_ON magic to make sure that the new flags field doesn't share storage w. other members of the union. Cc: linux-wireless@vger.kernel.org Signed-off-by: Florian Westphal <fw@strlen.de> --- include/net/mac80211.h | 11 ++++++++--- net/mac80211/main.c | 3 +++ 2 files changed, 11 insertions(+), 3 deletions(-)