Currently whenever link AP beacon is assigned, sdata->u.ap.active flag is
set and whenever it is brought down, the flag is reset. However, with MLO,
all the links of the same MLD would use the same sdata. Hence there is no
need to set/reset for each link up/down. Also, resetting it when only one
of the links went down is not desirable.
Add changes to set the active flag only when first link is assigned
beacon. Similarly, add changes to reset that flag only when last link is
brought down.
Signed-off-by: Aditya Kumar Singh <[email protected]>
---
net/mac80211/cfg.c | 62 +++++++++++++++++++++++++---------------------
1 file changed, 34 insertions(+), 28 deletions(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index f03452dc716d..ca0a3e6ba0fb 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1092,6 +1092,30 @@ ieee80211_copy_rnr_beacon(u8 *pos, struct cfg80211_rnr_elems *dst,
return offset;
}
+static u8 ieee80211_num_beaconing_links(struct ieee80211_sub_if_data *sdata)
+{
+ struct ieee80211_link_data *link;
+ u8 link_id, num = 0;
+
+ if (sdata->vif.type != NL80211_IFTYPE_AP &&
+ sdata->vif.type != NL80211_IFTYPE_P2P_GO)
+ return num;
+
+ if (!sdata->vif.valid_links)
+ return num;
+
+ for (link_id = 0; link_id < IEEE80211_MLD_MAX_NUM_LINKS; link_id++) {
+ link = sdata_dereference(sdata->link[link_id], sdata);
+ if (!link)
+ continue;
+
+ if (sdata_dereference(link->u.ap.beacon, sdata))
+ num++;
+ }
+
+ return num;
+}
+
static int
ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
struct ieee80211_link_data *link,
@@ -1232,7 +1256,9 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
}
rcu_assign_pointer(link->u.ap.beacon, new);
- sdata->u.ap.active = true;
+
+ if (ieee80211_num_beaconing_links(sdata) <= 1)
+ sdata->u.ap.active = true;
if (old)
kfree_rcu(old, rcu_head);
@@ -1241,30 +1267,6 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
return 0;
}
-static u8 ieee80211_num_beaconing_links(struct ieee80211_sub_if_data *sdata)
-{
- struct ieee80211_link_data *link;
- u8 link_id, num = 0;
-
- if (sdata->vif.type != NL80211_IFTYPE_AP &&
- sdata->vif.type != NL80211_IFTYPE_P2P_GO)
- return num;
-
- if (!sdata->vif.valid_links)
- return num;
-
- for (link_id = 0; link_id < IEEE80211_MLD_MAX_NUM_LINKS; link_id++) {
- link = sdata_dereference(sdata->link[link_id], sdata);
- if (!link)
- continue;
-
- if (sdata_dereference(link->u.ap.beacon, sdata))
- num++;
- }
-
- return num;
-}
-
static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
struct cfg80211_ap_settings *params)
{
@@ -1486,7 +1488,10 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
if (old)
kfree_rcu(old, rcu_head);
RCU_INIT_POINTER(link->u.ap.beacon, NULL);
- sdata->u.ap.active = false;
+
+ if (!ieee80211_num_beaconing_links(sdata))
+ sdata->u.ap.active = false;
+
goto error;
}
@@ -1619,11 +1624,12 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev,
list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list)
netif_carrier_off(vlan->dev);
- if (ieee80211_num_beaconing_links(sdata) <= 1)
+ if (ieee80211_num_beaconing_links(sdata) <= 1) {
netif_carrier_off(dev);
+ sdata->u.ap.active = false;
+ }
/* remove beacon and probe response */
- sdata->u.ap.active = false;
RCU_INIT_POINTER(link->u.ap.beacon, NULL);
RCU_INIT_POINTER(link->u.ap.probe_resp, NULL);
RCU_INIT_POINTER(link->u.ap.fils_discovery, NULL);
base-commit: d69aef8084cc72df7b0f2583096d9b037c647ec8
--
2.25.1
On Tue, 2024-03-26 at 20:41 +0530, Aditya Kumar Singh wrote:
> @@ -1232,7 +1256,9 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
> }
>
> rcu_assign_pointer(link->u.ap.beacon, new);
> - sdata->u.ap.active = true;
> +
> + if (ieee80211_num_beaconing_links(sdata) <= 1)
> + sdata->u.ap.active = true;
I don't understand this change. Neither the <= 1 really, nor the fact
that you actually _make_ this change.
> @@ -1486,7 +1488,10 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
> if (old)
> kfree_rcu(old, rcu_head);
> RCU_INIT_POINTER(link->u.ap.beacon, NULL);
> - sdata->u.ap.active = false;
> +
> + if (!ieee80211_num_beaconing_links(sdata))
> + sdata->u.ap.active = false;
== 0 maybe?
Or maybe we should just save/restore the value instead?
> list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list)
> netif_carrier_off(vlan->dev);
>
> - if (ieee80211_num_beaconing_links(sdata) <= 1)
Unrelated, but it looks like the VLAN netif_carrier_off() handling above
is also wrong and should really go into this if block as well.
johannes
On 4/8/24 23:55, Johannes Berg wrote:
> On Tue, 2024-03-26 at 20:41 +0530, Aditya Kumar Singh wrote:
>
>> @@ -1232,7 +1256,9 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
>> }
>>
>> rcu_assign_pointer(link->u.ap.beacon, new);
>> - sdata->u.ap.active = true;
>> +
>> + if (ieee80211_num_beaconing_links(sdata) <= 1)
>> + sdata->u.ap.active = true;
>
> I don't understand this change. Neither the <= 1 really, nor the fact
> that you actually _make_ this change.
>
The place above where we are checking number of beaconing links, at that
point at least 1 should be active. Since before checking, we have done
rcu_assign_pointer() so at least 1 should be there. That is why that
condition.
If it is more than 1, then this is not the first link which is going to
come up and hence there is no need to set the flag again.
>> @@ -1486,7 +1488,10 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
>> if (old)
>> kfree_rcu(old, rcu_head);
>> RCU_INIT_POINTER(link->u.ap.beacon, NULL);
>> - sdata->u.ap.active = false;
>> +
>> + if (!ieee80211_num_beaconing_links(sdata))
>> + sdata->u.ap.active = false;
>
> == 0 maybe?
>
Yeah can do. I prefer "!expr" over "expr == 0". Do you have any preference?
> Or maybe we should just save/restore the value instead?
>
>> list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list)
>> netif_carrier_off(vlan->dev);
>>
>> - if (ieee80211_num_beaconing_links(sdata) <= 1)
>
> Unrelated, but it looks like the VLAN netif_carrier_off() handling above
> is also wrong and should really go into this if block as well.
>
Yeah MLO VLAN changes would do that? The previous change was focusing on
the AP mode alone and I did not want to break anything in VLAN so did
not touch it there.
On Tue, 2024-04-09 at 09:27 +0530, Aditya Kumar Singh wrote:
> On 4/8/24 23:55, Johannes Berg wrote:
> > On Tue, 2024-03-26 at 20:41 +0530, Aditya Kumar Singh wrote:
> >
> > > @@ -1232,7 +1256,9 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
> > > }
> > >
> > > rcu_assign_pointer(link->u.ap.beacon, new);
> > > - sdata->u.ap.active = true;
> > > +
> > > + if (ieee80211_num_beaconing_links(sdata) <= 1)
> > > + sdata->u.ap.active = true;
> >
> > I don't understand this change. Neither the <= 1 really, nor the fact
> > that you actually _make_ this change.
> >
>
> The place above where we are checking number of beaconing links, at that
> point at least 1 should be active. Since before checking, we have done
> rcu_assign_pointer() so at least 1 should be there. That is why that
> condition.
>
> If it is more than 1, then this is not the first link which is going to
> come up and hence there is no need to set the flag again.
Hmm, OK, I guess that makes some sense. However, it's also completely
pointless, since setting it =true when it's already =true doesn't really
do anything. Adding the code seems to imply it should avoid setting it
in some cases, which isn't actually the case.
Besides, doing the counting is almost certainly far more expensive than
simply setting it to true when it's already true. Certainly the state
should be =true after this function is called.
If you really think the extra write might be a problem (it isn't though)
then you'd still want to write it as "if (!active) active=true" since
that's actually checking the right thing. But ... that really wouldn't
matter in all but the highest-performance code meant to deal with high
(CPU/core) parallelism.
So this is just a long-winded way of saying: don't do that, just keep it
unconditionally setting active.
> > > @@ -1486,7 +1488,10 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
> > > if (old)
> > > kfree_rcu(old, rcu_head);
> > > RCU_INIT_POINTER(link->u.ap.beacon, NULL);
> > > - sdata->u.ap.active = false;
> > > +
> > > + if (!ieee80211_num_beaconing_links(sdata))
> > > + sdata->u.ap.active = false;
> >
> > == 0 maybe?
> >
>
> Yeah can do. I prefer "!expr" over "expr == 0". Do you have any preference?
I think for something that actually _counts_, like here, I'd (slightly)
prefer ==0. It's obviously equivalent, but it seems more natural to
write "is number of beaconing links equal to zero" rather than "is not
number of beaconing links".
I may be influenced too much by Dan's thoughts on the matter ;-)
https://staticthinking.wordpress.com/2024/02/20/when-to-use-0/
> > Or maybe we should just save/restore the value instead?
> >
> > > list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list)
> > > netif_carrier_off(vlan->dev);
> > >
> > > - if (ieee80211_num_beaconing_links(sdata) <= 1)
> >
> > Unrelated, but it looks like the VLAN netif_carrier_off() handling above
> > is also wrong and should really go into this if block as well.
> >
>
> Yeah MLO VLAN changes would do that? The previous change was focusing on
> the AP mode alone and I did not want to break anything in VLAN so did
> not touch it there.
Sure sure, just drive-by observation.
johannes
On 4/9/24 12:57, Johannes Berg wrote:
> On Tue, 2024-04-09 at 09:27 +0530, Aditya Kumar Singh wrote:
>> On 4/8/24 23:55, Johannes Berg wrote:
>>> On Tue, 2024-03-26 at 20:41 +0530, Aditya Kumar Singh wrote:
>>>
>>>> @@ -1232,7 +1256,9 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
>>>> }
>>>>
>>>> rcu_assign_pointer(link->u.ap.beacon, new);
>>>> - sdata->u.ap.active = true;
>>>> +
>>>> + if (ieee80211_num_beaconing_links(sdata) <= 1)
>>>> + sdata->u.ap.active = true;
>>>
>>> I don't understand this change. Neither the <= 1 really, nor the fact
>>> that you actually _make_ this change.
>>>
>>
>> The place above where we are checking number of beaconing links, at that
>> point at least 1 should be active. Since before checking, we have done
>> rcu_assign_pointer() so at least 1 should be there. That is why that
>> condition.
>>
>> If it is more than 1, then this is not the first link which is going to
>> come up and hence there is no need to set the flag again.
>
> Hmm, OK, I guess that makes some sense. However, it's also completely
> pointless, since setting it =true when it's already =true doesn't really
> do anything. Adding the code seems to imply it should avoid setting it
> in some cases, which isn't actually the case.
>
> Besides, doing the counting is almost certainly far more expensive than
> simply setting it to true when it's already true. Certainly the state
> should be =true after this function is called.
>
> If you really think the extra write might be a problem (it isn't though)
> then you'd still want to write it as "if (!active) active=true" since
> that's actually checking the right thing. But ... that really wouldn't
> matter in all but the highest-performance code meant to deal with high
> (CPU/core) parallelism.
>
> So this is just a long-winded way of saying: don't do that, just keep it
> unconditionally setting active.
>
Well, that's true. Just to have symmetry in beaconing up and tear down I
did like that but it seems costly as you said. I will instead go with
!active thing as you suggested. Thanks for your inputs. Will send v2
soon for review.
>>>> @@ -1486,7 +1488,10 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
>>>> if (old)
>>>> kfree_rcu(old, rcu_head);
>>>> RCU_INIT_POINTER(link->u.ap.beacon, NULL);
>>>> - sdata->u.ap.active = false;
>>>> +
>>>> + if (!ieee80211_num_beaconing_links(sdata))
>>>> + sdata->u.ap.active = false;
>>>
>>> == 0 maybe?
>>>
>>
>> Yeah can do. I prefer "!expr" over "expr == 0". Do you have any preference?
>
> I think for something that actually _counts_, like here, I'd (slightly)
> prefer ==0. It's obviously equivalent, but it seems more natural to
> write "is number of beaconing links equal to zero" rather than "is not
> number of beaconing links".
>
> I may be influenced too much by Dan's thoughts on the matter ;-)
>
> https://staticthinking.wordpress.com/2024/02/20/when-to-use-0/
Yeah in this case it seems reasonable to use "== 0" instead. Will do
like this.
>
>>> Or maybe we should just save/restore the value instead?
>>>
>>>> list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list)
>>>> netif_carrier_off(vlan->dev);
>>>>
>>>> - if (ieee80211_num_beaconing_links(sdata) <= 1)
>>>
>>> Unrelated, but it looks like the VLAN netif_carrier_off() handling above
>>> is also wrong and should really go into this if block as well.
>>>
>>
>> Yeah MLO VLAN changes would do that? The previous change was focusing on
>> the AP mode alone and I did not want to break anything in VLAN so did
>> not touch it there.
>
> Sure sure, just drive-by observation.
:)