| Message ID | 1362421635-28008-1-git-send-email-karl.beldan@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote: > From: Karl Beldan <karl.beldan@rivierawaves.com> > > Currently it gets it from the sdata. This uses and updates the ad-hoc > masks of the ieee80211_tx_rate_control instead of copying them. Is there any need to update them? The change for "mask" seems to make it less efficient since it could otherwise be put into a register. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 04, 2013 at 09:12:04PM +0100, Johannes Berg wrote: > On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote: > > From: Karl Beldan <karl.beldan@rivierawaves.com> > > > > Currently it gets it from the sdata. This uses and updates the ad-hoc > > masks of the ieee80211_tx_rate_control instead of copying them. > > Is there any need to update them? > > The change for "mask" seems to make it less efficient since it could > otherwise be put into a register. > Totally, this commit spares the 10bytes copy of mcs_mask but adds a less efficient indirection to mask. I thought of it but kept the symmetry with mcs_mask. Apparently you wouldn't mind the dissymmetry so I will re-send using mask by value, plus I wrote "updates .." where it is more like "lets the ad-hoc masks get overwritten". Karl -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013-03-04 9:45 PM, john wrote: > On Mon, Mar 04, 2013 at 09:12:04PM +0100, Johannes Berg wrote: >> On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote: >> > From: Karl Beldan <karl.beldan@rivierawaves.com> >> > >> > Currently it gets it from the sdata. This uses and updates the ad-hoc >> > masks of the ieee80211_tx_rate_control instead of copying them. >> >> Is there any need to update them? >> >> The change for "mask" seems to make it less efficient since it could >> otherwise be put into a register. >> > Totally, this commit spares the 10bytes copy of mcs_mask but adds a less > efficient indirection to mask. > I thought of it but kept the symmetry with mcs_mask. > Apparently you wouldn't mind the dissymmetry so I will re-send using mask > by value, plus I wrote "updates .." where it is more like "lets the > ad-hoc masks get overwritten". It seems to me that all of this could be made more efficient by default if a mcs mask pointer is only passed to rate control if the user actually configured a MCS mask. Also, filtering out rates from the mask that the sta does not support seems a bit unnecessary, since the rate control usually looks at the HT capabilities and the sta's mcs rx mask anyway. - Felix -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 05, 2013 at 02:29:11PM +0100, Felix Fietkau wrote: > On 2013-03-04 9:45 PM, john wrote: > > On Mon, Mar 04, 2013 at 09:12:04PM +0100, Johannes Berg wrote: > >> On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote: > >> > From: Karl Beldan <karl.beldan@rivierawaves.com> > >> > > >> > Currently it gets it from the sdata. This uses and updates the ad-hoc > >> > masks of the ieee80211_tx_rate_control instead of copying them. > >> > >> Is there any need to update them? > >> > >> The change for "mask" seems to make it less efficient since it could > >> otherwise be put into a register. > >> > > Totally, this commit spares the 10bytes copy of mcs_mask but adds a less > > efficient indirection to mask. > > I thought of it but kept the symmetry with mcs_mask. > > Apparently you wouldn't mind the dissymmetry so I will re-send using mask > > by value, plus I wrote "updates .." where it is more like "lets the > > ad-hoc masks get overwritten". > It seems to me that all of this could be made more efficient by default > if a mcs mask pointer is only passed to rate control if the user > actually configured a MCS mask. Also, filtering out rates from the mask > that the sta does not support seems a bit unnecessary, since the rate > control usually looks at the HT capabilities and the sta's mcs rx mask > anyway. > Yes, some things look a bit overkill in the masks logic. Karl -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-03-05 at 17:10 +0100, Karl Beldan wrote: > > It seems to me that all of this could be made more efficient by default > > if a mcs mask pointer is only passed to rate control if the user > > actually configured a MCS mask. Also, filtering out rates from the mask > > that the sta does not support seems a bit unnecessary, since the rate > > control usually looks at the HT capabilities and the sta's mcs rx mask > > anyway. > > > Yes, some things look a bit overkill in the masks logic. Are you planning to send new patches to improve this? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 05, 2013 at 07:53:39PM +0100, Johannes Berg wrote: > On Tue, 2013-03-05 at 17:10 +0100, Karl Beldan wrote: > > > > It seems to me that all of this could be made more efficient by default > > > if a mcs mask pointer is only passed to rate control if the user > > > actually configured a MCS mask. Also, filtering out rates from the mask > > > that the sta does not support seems a bit unnecessary, since the rate > > > control usually looks at the HT capabilities and the sta's mcs rx mask > > > anyway. > > > > > Yes, some things look a bit overkill in the masks logic. > > Are you planning to send new patches to improve this? > I'll see what I can come up with. Karl -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 05, 2013 at 02:29:11PM +0100, Felix Fietkau wrote: > On 2013-03-04 9:45 PM, john wrote: > > On Mon, Mar 04, 2013 at 09:12:04PM +0100, Johannes Berg wrote: > >> On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote: > >> > From: Karl Beldan <karl.beldan@rivierawaves.com> > >> > > >> > Currently it gets it from the sdata. This uses and updates the ad-hoc > >> > masks of the ieee80211_tx_rate_control instead of copying them. > >> > >> Is there any need to update them? > >> > >> The change for "mask" seems to make it less efficient since it could > >> otherwise be put into a register. > >> > > Totally, this commit spares the 10bytes copy of mcs_mask but adds a less > > efficient indirection to mask. > > I thought of it but kept the symmetry with mcs_mask. > > Apparently you wouldn't mind the dissymmetry so I will re-send using mask > > by value, plus I wrote "updates .." where it is more like "lets the > > ad-hoc masks get overwritten". > It seems to me that all of this could be made more efficient by default > if a mcs mask pointer is only passed to rate control if the user > actually configured a MCS mask. Also, filtering out rates from the mask > that the sta does not support seems a bit unnecessary, since the rate > control usually looks at the HT capabilities and the sta's mcs rx mask > anyway. > Filtering is necessary to lookup alternative downgrade/upgrade rates. Karl -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013-03-10 11:16 PM, Karl Beldan wrote: > On Tue, Mar 05, 2013 at 02:29:11PM +0100, Felix Fietkau wrote: >> On 2013-03-04 9:45 PM, john wrote: >> > On Mon, Mar 04, 2013 at 09:12:04PM +0100, Johannes Berg wrote: >> >> On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote: >> >> > From: Karl Beldan <karl.beldan@rivierawaves.com> >> >> > >> >> > Currently it gets it from the sdata. This uses and updates the ad-hoc >> >> > masks of the ieee80211_tx_rate_control instead of copying them. >> >> >> >> Is there any need to update them? >> >> >> >> The change for "mask" seems to make it less efficient since it could >> >> otherwise be put into a register. >> >> >> > Totally, this commit spares the 10bytes copy of mcs_mask but adds a less >> > efficient indirection to mask. >> > I thought of it but kept the symmetry with mcs_mask. >> > Apparently you wouldn't mind the dissymmetry so I will re-send using mask >> > by value, plus I wrote "updates .." where it is more like "lets the >> > ad-hoc masks get overwritten". >> It seems to me that all of this could be made more efficient by default >> if a mcs mask pointer is only passed to rate control if the user >> actually configured a MCS mask. Also, filtering out rates from the mask >> that the sta does not support seems a bit unnecessary, since the rate >> control usually looks at the HT capabilities and the sta's mcs rx mask >> anyway. >> > Filtering is necessary to lookup alternative downgrade/upgrade rates. Right, but the code could be changed to only do the filtering if mac80211 needs to look up an alternative downgrade/upgrade rate. - Felix -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 05, 2013 at 11:27:48PM +0100, john wrote: > On Tue, Mar 05, 2013 at 07:53:39PM +0100, Johannes Berg wrote: > > On Tue, 2013-03-05 at 17:10 +0100, Karl Beldan wrote: > > > > > > It seems to me that all of this could be made more efficient by default > > > > if a mcs mask pointer is only passed to rate control if the user > > > > actually configured a MCS mask. Also, filtering out rates from the mask > > > > that the sta does not support seems a bit unnecessary, since the rate > > > > control usually looks at the HT capabilities and the sta's mcs rx mask > > > > anyway. > > > > > > > Yes, some things look a bit overkill in the masks logic. > > > > Are you planning to send new patches to improve this? > > > I'll see what I can come up with. > Now, FWIW, I was looking at how the masks are applied - the code tries to be thorough wrt the various RC flags - 2 things at least are missing: handle basic rates with multicast, and protection when downgrading to pre-ht rates. Karl -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Mar 10, 2013 at 11:27:01PM +0100, Felix Fietkau wrote: > On 2013-03-10 11:16 PM, Karl Beldan wrote: > > On Tue, Mar 05, 2013 at 02:29:11PM +0100, Felix Fietkau wrote: > >> On 2013-03-04 9:45 PM, john wrote: > >> > On Mon, Mar 04, 2013 at 09:12:04PM +0100, Johannes Berg wrote: > >> >> On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote: > >> >> > From: Karl Beldan <karl.beldan@rivierawaves.com> > >> >> > > >> >> > Currently it gets it from the sdata. This uses and updates the ad-hoc > >> >> > masks of the ieee80211_tx_rate_control instead of copying them. > >> >> > >> >> Is there any need to update them? > >> >> > >> >> The change for "mask" seems to make it less efficient since it could > >> >> otherwise be put into a register. > >> >> > >> > Totally, this commit spares the 10bytes copy of mcs_mask but adds a less > >> > efficient indirection to mask. > >> > I thought of it but kept the symmetry with mcs_mask. > >> > Apparently you wouldn't mind the dissymmetry so I will re-send using mask > >> > by value, plus I wrote "updates .." where it is more like "lets the > >> > ad-hoc masks get overwritten". > >> It seems to me that all of this could be made more efficient by default > >> if a mcs mask pointer is only passed to rate control if the user > >> actually configured a MCS mask. Also, filtering out rates from the mask > >> that the sta does not support seems a bit unnecessary, since the rate > >> control usually looks at the HT capabilities and the sta's mcs rx mask > >> anyway. > >> > > Filtering is necessary to lookup alternative downgrade/upgrade rates. > Right, but the code could be changed to only do the filtering if > mac80211 needs to look up an alternative downgrade/upgrade rate. > With this I agree. Do you have strong opinions wrt basic rates ? The current code might tx mc/bc with non-basic rates. Karl -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013-03-11 12:06 AM, Karl Beldan wrote: > On Sun, Mar 10, 2013 at 11:27:01PM +0100, Felix Fietkau wrote: >> On 2013-03-10 11:16 PM, Karl Beldan wrote: >> > On Tue, Mar 05, 2013 at 02:29:11PM +0100, Felix Fietkau wrote: >> >> On 2013-03-04 9:45 PM, john wrote: >> >> > On Mon, Mar 04, 2013 at 09:12:04PM +0100, Johannes Berg wrote: >> >> >> On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote: >> >> >> > From: Karl Beldan <karl.beldan@rivierawaves.com> >> >> >> > >> >> >> > Currently it gets it from the sdata. This uses and updates the ad-hoc >> >> >> > masks of the ieee80211_tx_rate_control instead of copying them. >> >> >> >> >> >> Is there any need to update them? >> >> >> >> >> >> The change for "mask" seems to make it less efficient since it could >> >> >> otherwise be put into a register. >> >> >> >> >> > Totally, this commit spares the 10bytes copy of mcs_mask but adds a less >> >> > efficient indirection to mask. >> >> > I thought of it but kept the symmetry with mcs_mask. >> >> > Apparently you wouldn't mind the dissymmetry so I will re-send using mask >> >> > by value, plus I wrote "updates .." where it is more like "lets the >> >> > ad-hoc masks get overwritten". >> >> It seems to me that all of this could be made more efficient by default >> >> if a mcs mask pointer is only passed to rate control if the user >> >> actually configured a MCS mask. Also, filtering out rates from the mask >> >> that the sta does not support seems a bit unnecessary, since the rate >> >> control usually looks at the HT capabilities and the sta's mcs rx mask >> >> anyway. >> >> >> > Filtering is necessary to lookup alternative downgrade/upgrade rates. >> Right, but the code could be changed to only do the filtering if >> mac80211 needs to look up an alternative downgrade/upgrade rate. >> > With this I agree. > Do you have strong opinions wrt basic rates ? The current code might tx > mc/bc with non-basic rates. The only strong opinion I have about the rate masking code is that it shouldn't waste precious CPU cycles in a hot path ;) - Felix -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-03-11 at 00:06 +0100, Karl Beldan wrote: > > > Filtering is necessary to lookup alternative downgrade/upgrade rates. > > Right, but the code could be changed to only do the filtering if > > mac80211 needs to look up an alternative downgrade/upgrade rate. > > > With this I agree. > Do you have strong opinions wrt basic rates ? The current code might tx > mc/bc with non-basic rates. If the users want to shoot themselves in the foot I'd rather give them enough rope to hang themselves with ;-) johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" 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/net/mac80211/rate.c b/net/mac80211/rate.c index dd88381..c1e5f25 100644 --- a/net/mac80211/rate.c +++ b/net/mac80211/rate.c @@ -435,8 +435,8 @@ void rate_control_get_rate(struct ieee80211_sub_if_data *sdata, struct ieee80211_sta *ista = NULL; struct ieee80211_tx_info *info = IEEE80211_SKB_CB(txrc->skb); int i; - u32 mask; - u8 mcs_mask[IEEE80211_HT_MCS_MASK_LEN]; + u32 *mask; + u8 *mcs_mask; if (sta && test_sta_flag(sta, WLAN_STA_RATE_CONTROL)) { ista = &sta->sta; @@ -459,14 +459,13 @@ void rate_control_get_rate(struct ieee80211_sub_if_data *sdata, * default mask (allow all rates) is used to save some processing for * the common case. */ - mask = sdata->rc_rateidx_mask[info->band]; - memcpy(mcs_mask, sdata->rc_rateidx_mcs_mask[info->band], - sizeof(mcs_mask)); - if (mask != (1 << txrc->sband->n_bitrates) - 1) { + mask = &txrc->rate_idx_mask; + mcs_mask = txrc->rate_idx_mcs_mask; + if (*mask != (1 << txrc->sband->n_bitrates) - 1) { if (sta) { /* Filter out rates that the STA does not support */ - mask &= sta->sta.supp_rates[info->band]; - for (i = 0; i < sizeof(mcs_mask); i++) + *mask &= sta->sta.supp_rates[info->band]; + for (i = 0; i < sizeof(txrc->rate_idx_mcs_mask); i++) mcs_mask[i] &= sta->sta.ht_cap.mcs.rx_mask[i]; } /* @@ -479,7 +478,7 @@ void rate_control_get_rate(struct ieee80211_sub_if_data *sdata, if (info->control.rates[i].idx < 0) break; rate_idx_match_mask(&info->control.rates[i], txrc, - mask, mcs_mask); + *mask, mcs_mask); } }