| Message ID | 7d713206957ec56dc297d5645203b45341578588.1722885720.git-series.nbd@nbd.name |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: cfg80211/mac80211: improve support for multiple radios | expand |
On Mon, 2024-08-05 at 21:23 +0200, Felix Fietkau wrote: > When restricting a monitor vif to only operate on a specific set of radios, > filter out rx packets belonging to other radios. This only works if drivers > fill in radio_valid and radio_idx in the rx status. Why does the driver need to provide the radio, it already provides the frequency? But then I wonder if this doesn't go a step too far? This is pretty much pretending that monitor only exists on a specific sub-radio, but ... what for? Even userspace could filter on the frequency. I mean ... I get that you're trying to preserve a notion that you had that an interface exists on a given PHY and they're all separate, but they're not really separate any more, get used to it? johannes
On 23.08.24 12:23, Johannes Berg wrote: > On Mon, 2024-08-05 at 21:23 +0200, Felix Fietkau wrote: >> When restricting a monitor vif to only operate on a specific set of radios, >> filter out rx packets belonging to other radios. This only works if drivers >> fill in radio_valid and radio_idx in the rx status. > > Why does the driver need to provide the radio, it already provides the > frequency? > > But then I wonder if this doesn't go a step too far? This is pretty much > pretending that monitor only exists on a specific sub-radio, but ... > what for? Even userspace could filter on the frequency. > > I mean ... I get that you're trying to preserve a notion that you had > that an interface exists on a given PHY and they're all separate, but > they're not really separate any more, get used to it? Well, there's a performance aspect as well. When only monitoring specific radios (while operating normally on others), relying on filtering in user space or even BPF comes at a cost, since mac80211 still has to prepare radiotap headers and potentially clone data packets received on other radios. - Felix
On Fri, 2024-08-23 at 19:33 +0200, Felix Fietkau wrote: > On 23.08.24 12:23, Johannes Berg wrote: > > On Mon, 2024-08-05 at 21:23 +0200, Felix Fietkau wrote: > > > When restricting a monitor vif to only operate on a specific set of radios, > > > filter out rx packets belonging to other radios. This only works if drivers > > > fill in radio_valid and radio_idx in the rx status. > > > > Why does the driver need to provide the radio, it already provides the > > frequency? > > > > But then I wonder if this doesn't go a step too far? This is pretty much > > pretending that monitor only exists on a specific sub-radio, but ... > > what for? Even userspace could filter on the frequency. > > > > I mean ... I get that you're trying to preserve a notion that you had > > that an interface exists on a given PHY and they're all separate, but > > they're not really separate any more, get used to it? > > Well, there's a performance aspect as well. Which I'd firmly put into the "premature optimisation" basket at this stage though. johannes
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 7a5418713dfc..6222f4f44ac2 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -1619,6 +1619,8 @@ enum mac80211_rx_encoding { * @ampdu_reference: A-MPDU reference number, must be a different value for * each A-MPDU but the same for each subframe within one A-MPDU * @zero_length_psdu_type: radiotap type of the 0-length PSDU + * @radio_valid: if the index of the radio in radio_idx is valid. + * @radio_idx: index of the wiphy radio that the frame was received on. * @link_valid: if the link which is identified by @link_id is valid. This flag * is set only when connection is MLO. * @link_id: id of the link used to receive the packet. This is used along with @@ -1656,6 +1658,7 @@ struct ieee80211_rx_status { u8 chains; s8 chain_signal[IEEE80211_MAX_CHAINS]; u8 zero_length_psdu_type; + u8 radio_valid:1, radio_idx:5; u8 link_valid:1, link_id:4; }; diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c index 718f02f0a181..de4196fa3eb5 100644 --- a/net/mac80211/rx.c +++ b/net/mac80211/rx.c @@ -762,8 +762,8 @@ ieee80211_rx_monitor(struct ieee80211_local *local, struct sk_buff *origskb, struct ieee80211_rate *rate) { struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(origskb); - struct ieee80211_sub_if_data *sdata; - struct sk_buff *monskb = NULL; + struct ieee80211_sub_if_data *sdata, *prev_sdata = NULL; + struct sk_buff *skb, *monskb = NULL; int present_fcs_len = 0; unsigned int rtap_space = 0; struct ieee80211_sub_if_data *monitor_sdata = @@ -837,40 +837,46 @@ ieee80211_rx_monitor(struct ieee80211_local *local, struct sk_buff *origskb, ieee80211_handle_mu_mimo_mon(monitor_sdata, origskb, rtap_space); list_for_each_entry_rcu(sdata, &local->mon_list, u.mntr.list) { - bool last_monitor = list_is_last(&sdata->u.mntr.list, - &local->mon_list); + if (status->radio_valid && + !(sdata->wdev.radio_mask & BIT(status->radio_idx))) + continue; + + if (!prev_sdata) { + prev_sdata = sdata; + continue; + } if (!monskb) monskb = ieee80211_make_monitor_skb(local, &origskb, rate, rtap_space, - only_monitor && - last_monitor); + false); + if (!monskb) + continue; - if (monskb) { - struct sk_buff *skb; + skb = skb_clone(monskb, GFP_ATOMIC); + if (!skb) + continue; - if (last_monitor) { - skb = monskb; - monskb = NULL; - } else { - skb = skb_clone(monskb, GFP_ATOMIC); - } + skb->dev = prev_sdata->dev; + dev_sw_netstats_rx_add(skb->dev, skb->len); + netif_receive_skb(skb); + prev_sdata = sdata; + } - if (skb) { - skb->dev = sdata->dev; - dev_sw_netstats_rx_add(skb->dev, skb->len); - netif_receive_skb(skb); - } + if (prev_sdata) { + if (monskb) + skb = monskb; + else + skb = ieee80211_make_monitor_skb(local, &origskb, + rate, rtap_space, + only_monitor); + if (skb) { + skb->dev = prev_sdata->dev; + dev_sw_netstats_rx_add(skb->dev, skb->len); + netif_receive_skb(skb); } - - if (last_monitor) - break; } - /* this happens if last_monitor was erroneously false */ - dev_kfree_skb(monskb); - - /* ditto */ if (!origskb) return NULL;
When restricting a monitor vif to only operate on a specific set of radios, filter out rx packets belonging to other radios. This only works if drivers fill in radio_valid and radio_idx in the rx status. Signed-off-by: Felix Fietkau <nbd@nbd.name> --- include/net/mac80211.h | 3 ++- net/mac80211/rx.c | 58 +++++++++++++++++++++++-------------------- 2 files changed, 35 insertions(+), 26 deletions(-)